Merge pull request #222 from yevgeny-shnaidman/yevgeny/finalization-flow

adding finalizer flow

Author: k8s-ci-robot

internal/configmap/configmap.go

@@ -18,9 +18,12 @@ package configmap
 
 import (
 	"context"
+	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	nfdv1 "sigs.k8s.io/node-feature-discovery-operator/api/v1"
@@ -30,14 +33,17 @@ import (
 
 type ConfigMapAPI interface {
 	SetWorkerConfigMapAsDesired(ctx context.Context, nfdInstance *nfdv1.NodeFeatureDiscovery, workerCM *corev1.ConfigMap) error
+	DeleteConfigMap(ctx context.Context, namespace, name string) error
 }
 
 type configMap struct {
+	client client.Client
 	scheme *runtime.Scheme
 }
 
-func NewConfigMapAPI(scheme *runtime.Scheme) ConfigMapAPI {
+func NewConfigMapAPI(client client.Client, scheme *runtime.Scheme) ConfigMapAPI {
 	return &configMap{
+		client: client,
 		scheme: scheme,
 	}
 }
@@ -48,3 +54,17 @@ func (c *configMap) SetWorkerConfigMapAsDesired(ctx context.Context, nfdInstance
 
 	return controllerutil.SetControllerReference(nfdInstance, cm, c.scheme)
 }
+
+func (c *configMap) DeleteConfigMap(ctx context.Context, namespace, name string) error {
+	cm := corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      name,
+		},
+	}
+	err := c.client.Delete(ctx, &cm)
+	if err != nil && client.IgnoreNotFound(err) != nil {
+		return fmt.Errorf("failed to delete configmap %s/%s: %w", namespace, name, err)
+	}
+	return nil
+}

internal/configmap/configmap_test.go

@@ -18,13 +18,18 @@ package configmap
 
 import (
 	"context"
+	"fmt"
 	"os"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	nfdv1 "sigs.k8s.io/node-feature-discovery-operator/api/v1"
+	"sigs.k8s.io/node-feature-discovery-operator/internal/client"
 	"sigs.k8s.io/yaml"
 )
 
@@ -34,7 +39,7 @@ var _ = Describe("SetWorkerDaemonsetAsDesired", func() {
 	)
 
 	BeforeEach(func() {
-		configmapAPI = NewConfigMapAPI(scheme)
+		configmapAPI = NewConfigMapAPI(nil, scheme)
 	})
 
 	ctx := context.Background()
@@ -71,3 +76,48 @@ var _ = Describe("SetWorkerDaemonsetAsDesired", func() {
 		Expect(&expectedWorkerCM).To(BeComparableTo(&actualWorkerCM))
 	})
 })
+
+var _ = Describe("DeleteConfigMap", func() {
+	var (
+		ctrl  *gomock.Controller
+		clnt  *client.MockClient
+		cmAPI ConfigMapAPI
+	)
+
+	BeforeEach(func() {
+		ctrl = gomock.NewController(GinkgoT())
+		clnt = client.NewMockClient(ctrl)
+		cmAPI = NewConfigMapAPI(clnt, scheme)
+	})
+
+	ctx := context.Background()
+	name := "cm-name"
+	namespace := "cm-namespace"
+	expectedCM := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      name,
+		},
+	}
+
+	It("failure to delete configmap from the cluster", func() {
+		clnt.EXPECT().Delete(ctx, expectedCM).Return(fmt.Errorf("some error"))
+
+		err := cmAPI.DeleteConfigMap(ctx, namespace, name)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("configmap is not present in the cluster", func() {
+		clnt.EXPECT().Delete(ctx, expectedCM).Return(apierrors.NewNotFound(schema.GroupResource{}, "whatever"))
+
+		err := cmAPI.DeleteConfigMap(ctx, namespace, name)
+		Expect(err).To(BeNil())
+	})
+
+	It("configmap deleted successfully", func() {
+		clnt.EXPECT().Delete(ctx, expectedCM).Return(nil)
+
+		err := cmAPI.DeleteConfigMap(ctx, namespace, name)
+		Expect(err).To(BeNil())
+	})
+})

internal/configmap/mock_configmap.go

@@ -160,6 +160,36 @@ func newNodeFeatureDiscoveryHelperAPI(client client.Client, deploymentAPI deploy
 }
 
 func (nfdh *nodeFeatureDiscoveryHelper) finalizeComponents(ctx context.Context, nfdInstance *nfdv1.NodeFeatureDiscovery) error {
+	err := nfdh.daemonsetAPI.DeleteDaemonSet(ctx, nfdInstance.Namespace, "nfd-worker")
+	if err != nil {
+		return fmt.Errorf("failed to delete worker daemonset: %w", err)
+	}
+
+	err = nfdh.configmapAPI.DeleteConfigMap(ctx, nfdInstance.Namespace, "nfd-worker")
+	if err != nil {
+		return fmt.Errorf("failed to delete worker config map: %w", err)
+	}
+
+	if nfdInstance.Spec.TopologyUpdater {
+		err = nfdh.daemonsetAPI.DeleteDaemonSet(ctx, nfdInstance.Namespace, "nfd-topology-updater")
+		if err != nil {
+			return fmt.Errorf("failed to delete topology-updater daemonset: %w", err)
+		}
+	}
+	err = nfdh.deploymentAPI.DeleteDeployment(ctx, nfdInstance.Namespace, "nfd-master")
+	if err != nil {
+		return fmt.Errorf("failed to delete master deployment: %w", err)
+	}
+
+	err = nfdh.deploymentAPI.DeleteDeployment(ctx, nfdInstance.Namespace, "nfd-gc")
+	if err != nil {
+		return fmt.Errorf("failed to delete GC deployment: %w", err)
+	}
+
+	updated := controllerutil.RemoveFinalizer(nfdInstance, finalizerLabel)
+	if updated {
+		return nfdh.client.Update(ctx, nfdInstance)
+	}
 	return nil
 }
 

internal/controllers/nodefeaturediscovery_reconciler.go

@@ -29,6 +29,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	nfdv1 "sigs.k8s.io/node-feature-discovery-operator/api/v1"
 
@@ -546,3 +547,93 @@ var _ = Describe("setFinalizer", func() {
 		Expect(err).To(BeNil())
 	})
 })
+
+var _ = Describe("finalizeComponents", func() {
+	var (
+		ctrl           *gomock.Controller
+		clnt           *client.MockClient
+		mockDeployment *deployment.MockDeploymentAPI
+		mockDS         *daemonset.MockDaemonsetAPI
+		mockCM         *configmap.MockConfigMapAPI
+		nfdh           nodeFeatureDiscoveryHelperAPI
+	)
+
+	BeforeEach(func() {
+		ctrl = gomock.NewController(GinkgoT())
+		clnt = client.NewMockClient(ctrl)
+		mockDeployment = deployment.NewMockDeploymentAPI(ctrl)
+		mockDS = daemonset.NewMockDaemonsetAPI(ctrl)
+		mockCM = configmap.NewMockConfigMapAPI(ctrl)
+
+		nfdh = newNodeFeatureDiscoveryHelperAPI(clnt, mockDeployment, mockDS, mockCM, scheme)
+	})
+
+	ctx := context.Background()
+	namespace := "test-namespace"
+	nfdCR := nfdv1.NodeFeatureDiscovery{
+		ObjectMeta: metav1.ObjectMeta{Namespace: namespace},
+		Spec: nfdv1.NodeFeatureDiscoverySpec{
+			TopologyUpdater: true,
+		},
+	}
+
+	DescribeTable("check finalization normal and error flows", func(deleteWorkerDSError,
+		deleteWorkerCMError,
+		deleteTopologyDSError,
+		deleteMasterDeploymentError,
+		deleteGCDeploymentError,
+		updateError bool) {
+
+		controllerutil.AddFinalizer(&nfdCR, finalizerLabel)
+
+		if deleteWorkerDSError {
+			mockDS.EXPECT().DeleteDaemonSet(ctx, namespace, "nfd-worker").Return(fmt.Errorf("some error"))
+			goto executeTestFunction
+		}
+		mockDS.EXPECT().DeleteDaemonSet(ctx, namespace, "nfd-worker").Return(nil)
+		if deleteWorkerCMError {
+			mockCM.EXPECT().DeleteConfigMap(ctx, namespace, "nfd-worker").Return(fmt.Errorf("some error"))
+			goto executeTestFunction
+		}
+		mockCM.EXPECT().DeleteConfigMap(ctx, namespace, "nfd-worker").Return(nil)
+		if deleteTopologyDSError {
+			mockDS.EXPECT().DeleteDaemonSet(ctx, namespace, "nfd-topology-updater").Return(fmt.Errorf("some error"))
+			goto executeTestFunction
+		}
+		mockDS.EXPECT().DeleteDaemonSet(ctx, namespace, "nfd-topology-updater").Return(nil)
+		if deleteMasterDeploymentError {
+			mockDeployment.EXPECT().DeleteDeployment(ctx, namespace, "nfd-master").Return(fmt.Errorf("some error"))
+			goto executeTestFunction
+		}
+		mockDeployment.EXPECT().DeleteDeployment(ctx, namespace, "nfd-master").Return(nil)
+		if deleteGCDeploymentError {
+			mockDeployment.EXPECT().DeleteDeployment(ctx, namespace, "nfd-gc").Return(fmt.Errorf("some error"))
+			goto executeTestFunction
+		}
+		mockDeployment.EXPECT().DeleteDeployment(ctx, namespace, "nfd-gc").Return(nil)
+		if updateError {
+			clnt.EXPECT().Update(ctx, gomock.Any()).Return(fmt.Errorf("some error"))
+			goto executeTestFunction
+		}
+		clnt.EXPECT().Update(ctx, gomock.Any()).Return(nil)
+
+	executeTestFunction:
+
+		err := nfdh.finalizeComponents(ctx, &nfdCR)
+
+		if deleteGCDeploymentError || deleteWorkerDSError || deleteWorkerCMError ||
+			deleteTopologyDSError || deleteMasterDeploymentError || updateError {
+			Expect(err).To(HaveOccurred())
+		} else {
+			Expect(err).To(BeNil())
+		}
+	},
+		Entry("delete worker daemonset failed", true, false, false, false, false, false),
+		Entry("delete worker configmap failed", false, true, false, false, false, false),
+		Entry("delete topology daemonset failed", false, false, true, false, false, false),
+		Entry("delete master deployment failed", false, false, false, true, false, false),
+		Entry("delete gc deployment failed", false, false, false, false, true, false),
+		Entry("updating removed finalizer failed", false, false, false, false, false, true),
+		Entry("finalization flow was succesful", false, false, false, false, false, false),
+	)
+})

internal/controllers/nodefeaturediscovery_reconciler_test.go

@@ -18,12 +18,14 @@ package daemonset
 
 import (
 	"context"
+	"fmt"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	nfdv1 "sigs.k8s.io/node-feature-discovery-operator/api/v1"
@@ -34,14 +36,17 @@ import (
 type DaemonsetAPI interface {
 	SetTopologyDaemonsetAsDesired(ctx context.Context, nfdInstance *nfdv1.NodeFeatureDiscovery, topologyDS *appsv1.DaemonSet) error
 	SetWorkerDaemonsetAsDesired(ctx context.Context, nfdInstance *nfdv1.NodeFeatureDiscovery, workerDS *appsv1.DaemonSet) error
+	DeleteDaemonSet(ctx context.Context, namespace, name string) error
 }
 
 type daemonset struct {
+	client client.Client
 	scheme *runtime.Scheme
 }
 
-func NewDaemonsetAPI(scheme *runtime.Scheme) DaemonsetAPI {
+func NewDaemonsetAPI(client client.Client, scheme *runtime.Scheme) DaemonsetAPI {
 	return &daemonset{
+		client: client,
 		scheme: scheme,
 	}
 }
@@ -82,6 +87,20 @@ func (d *daemonset) SetTopologyDaemonsetAsDesired(ctx context.Context, nfdInstan
 	return controllerutil.SetControllerReference(nfdInstance, topologyDS, d.scheme)
 }
 
+func (d *daemonset) DeleteDaemonSet(ctx context.Context, namespace, name string) error {
+	ds := appsv1.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: namespace,
+			Name:      name,
+		},
+	}
+	err := d.client.Delete(ctx, &ds)
+	if err != nil && client.IgnoreNotFound(err) != nil {
+		return fmt.Errorf("failed to delete daemonset %s/%s: %w", namespace, name, err)
+	}
+	return nil
+}
+
 func getImagePullPolicy(nfdInstance *nfdv1.NodeFeatureDiscovery) corev1.PullPolicy {
 	if nfdInstance.Spec.Operand.ImagePullPolicy != "" {
 		return corev1.PullPolicy(nfdInstance.Spec.Operand.ImagePullPolicy)