diff --git a/controllers/classifier_deployer.go b/controllers/classifier_deployer.go index 9c08304..73e30e8 100644 --- a/controllers/classifier_deployer.go +++ b/controllers/classifier_deployer.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/yaml" "github.com/projectsveltos/classifier/controllers/keymanager" @@ -781,8 +782,40 @@ func undeployClassifierFromCluster(ctx context.Context, c client.Client, return err } + return removeClassifierInstance(ctx, c, remoteClient, currentClassifier, logger) +} + +// removeClassifierInstance deletes the Classifier instance found in a managed cluster. +// +// When the cluster is the management cluster itself (registered as a managed cluster), the instance +// found is the Classifier being reconciled, not a copy. It must be left alone: removing its +// finalizer or deleting it from here would bypass the cleanup of all the other clusters. +func removeClassifierInstance(ctx context.Context, managementClient, remoteClient client.Client, + instance *libsveltosv1beta1.Classifier, logger logr.Logger) error { + + classifier := &libsveltosv1beta1.Classifier{} + err := managementClient.Get(ctx, types.NamespacedName{Name: instance.Name}, classifier) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + if err == nil && classifier.UID == instance.UID { + logger.V(logs.LogDebug).Info("classifier instance is the classifier itself. Nothing to remove") + return nil + } + + // Instances deployed by older versions of sveltos-agent carry the finalizer owned by classifier-manager. + // sveltos-agent now uses its own finalizer and ignores this one, which would block the removal of the instance. + if controllerutil.ContainsFinalizer(instance, libsveltosv1beta1.ClassifierFinalizer) { + logger.V(logs.LogDebug).Info("remove legacy finalizer from classifier instance") + patch := client.MergeFromWithOptions(instance.DeepCopy(), client.MergeFromWithOptimisticLock{}) + controllerutil.RemoveFinalizer(instance, libsveltosv1beta1.ClassifierFinalizer) + if err := remoteClient.Patch(ctx, instance, patch); err != nil { + return err + } + } + logger.V(logs.LogDebug).Info("remove classifier instance") - return remoteClient.Delete(ctx, currentClassifier) + return remoteClient.Delete(ctx, instance) } func undeployClassifierInPullMode(ctx context.Context, c client.Client, diff --git a/controllers/classifier_deployer_test.go b/controllers/classifier_deployer_test.go index 8b9de20..4b8c221 100644 --- a/controllers/classifier_deployer_test.go +++ b/controllers/classifier_deployer_test.go @@ -22,6 +22,7 @@ import ( "fmt" "reflect" "strings" + "time" "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" @@ -38,6 +39,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/projectsveltos/classifier/controllers" "github.com/projectsveltos/classifier/controllers/keymanager" @@ -405,12 +407,14 @@ var _ = Describe("Classifier Deployer", func() { Expect(dep.IsKeyInProgress(key)).To(BeTrue()) }) - It("undeployClassifierFromCluster removes Classifier from Cluster", func() { + It("undeployClassifierFromCluster does not remove the Classifier when the cluster is the management cluster", func() { dep := fakedeployer.GetClient(context.TODO(), logger, testEnv.Client) Expect(dep.RegisterFeatureID(libsveltosv1beta1.FeatureClassifier)).To(Succeed()) classifier := getClassifierInstance(randomString()) + classifier.Finalizers = []string{libsveltosv1beta1.ClassifierFinalizer} + // prepareCluster registers testEnv, the cluster the Classifier is created in, as the managed cluster cluster := prepareCluster() Expect(testEnv.Create(context.TODO(), classifier)).To(Succeed()) @@ -421,13 +425,51 @@ var _ = Describe("Classifier Deployer", func() { deployer.Options{}, logger) Expect(err).To(BeNil()) - // Eventual loop so testEnv Cache is synced + verifyClassifierIsUntouched(classifier.Name) + removeFinalizersAndDeleteClassifier(classifier.Name) + }) + + It("removeClassifierInstance leaves the Classifier alone when the instance is the Classifier itself", func() { + classifier := getClassifierInstance(randomString()) + classifier.Finalizers = []string{libsveltosv1beta1.ClassifierFinalizer} + Expect(testEnv.Create(context.TODO(), classifier)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, classifier)).To(Succeed()) + + // Management cluster and managed cluster are the same cluster, so the instance is the Classifier + Expect(controllers.RemoveClassifierInstance(context.TODO(), testEnv, testEnv, classifier, logger)).To(Succeed()) + + verifyClassifierIsUntouched(classifier.Name) + removeFinalizersAndDeleteClassifier(classifier.Name) + }) + + It("removeClassifierInstance removes the legacy finalizer and deletes the instance of a managed cluster", func() { + const agentFinalizer = "sveltos-agent.projectsveltos.io/classifier" + + instance := getClassifierInstance(randomString()) + instance.Finalizers = []string{libsveltosv1beta1.ClassifierFinalizer, agentFinalizer} + Expect(testEnv.Create(context.TODO(), instance)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, instance)).To(Succeed()) + + // The Classifier in the management cluster is a different object from the instance in the managed cluster + classifier := getClassifierInstance(instance.Name) + classifier.UID = types.UID(randomString()) + managementClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(classifier).Build() + + Expect(controllers.RemoveClassifierInstance(context.TODO(), managementClient, testEnv, instance, logger)).To(Succeed()) + + // The finalizer of sveltos-agent is still there, so the instance is terminating Eventually(func() bool { - currentClassifier := &libsveltosv1beta1.Classifier{} - err = testEnv.Get(context.TODO(), types.NamespacedName{Name: classifier.Name}, - currentClassifier) - return err != nil && apierrors.IsNotFound(err) + currentInstance := &libsveltosv1beta1.Classifier{} + err := testEnv.Get(context.TODO(), types.NamespacedName{Name: instance.Name}, currentInstance) + if err != nil { + return false + } + return !currentInstance.DeletionTimestamp.IsZero() && + !controllerutil.ContainsFinalizer(currentInstance, libsveltosv1beta1.ClassifierFinalizer) && + controllerutil.ContainsFinalizer(currentInstance, agentFinalizer) }, timeout, pollingInterval).Should(BeTrue()) + + removeFinalizersAndDeleteClassifier(instance.Name) }) It("undeployClassifier queues a job to remove classifier from cluster", func() { @@ -1478,3 +1520,25 @@ func verifyPatches(patches []libsveltosv1beta1.Patch) { } Expect(found).To(BeTrue()) } + +// verifyClassifierIsUntouched verifies the Classifier is not marked for deletion and still has its finalizer +func verifyClassifierIsUntouched(name string) { + Consistently(func() bool { + classifier := &libsveltosv1beta1.Classifier{} + err := testEnv.Get(context.TODO(), types.NamespacedName{Name: name}, classifier) + if err != nil { + return false + } + return classifier.DeletionTimestamp.IsZero() && + controllerutil.ContainsFinalizer(classifier, libsveltosv1beta1.ClassifierFinalizer) + }, 3*time.Second, time.Second).Should(BeTrue()) +} + +// removeFinalizersAndDeleteClassifier removes all finalizers and deletes the Classifier, so it does not outlive the test +func removeFinalizersAndDeleteClassifier(name string) { + classifier := &libsveltosv1beta1.Classifier{} + Expect(testEnv.Get(context.TODO(), types.NamespacedName{Name: name}, classifier)).To(Succeed()) + classifier.Finalizers = nil + Expect(testEnv.Update(context.TODO(), classifier)).To(Succeed()) + Expect(client.IgnoreNotFound(testEnv.Delete(context.TODO(), classifier))).To(Succeed()) +} diff --git a/controllers/export_test.go b/controllers/export_test.go index f24ca7d..7970545 100644 --- a/controllers/export_test.go +++ b/controllers/export_test.go @@ -54,6 +54,7 @@ var ( ClassifierHash = classifierHash DeployClassifierInCluster = deployClassifierInCluster UndeployClassifierFromCluster = undeployClassifierFromCluster + RemoveClassifierInstance = removeClassifierInstance RemoveClassifierReports = removeClassifierReports RemoveClusterClassifierReports = removeClusterClassifierReports PruneClassifierReportsForDeletedClusters = pruneClassifierReportsForDeletedClusters diff --git a/pkg/agent/sveltos-agent-in-mgmt-cluster.go b/pkg/agent/sveltos-agent-in-mgmt-cluster.go index 2cccdfa..784b09c 100644 --- a/pkg/agent/sveltos-agent-in-mgmt-cluster.go +++ b/pkg/agent/sveltos-agent-in-mgmt-cluster.go @@ -63,7 +63,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/sveltos-agent@sha256:6bcf5d20ad39145e617109509c452daa420e3e5b86636adf6a1462908789ff70 + image: docker.io/projectsveltos/sveltos-agent@sha256:59272b4f68af3cdce0d59ba221530ea7783b7e30901750474a1e805f17eab673 livenessProbe: failureThreshold: 3 httpGet: diff --git a/pkg/agent/sveltos-agent-in-mgmt-cluster.yaml b/pkg/agent/sveltos-agent-in-mgmt-cluster.yaml index e10d2df..bf1b2c5 100644 --- a/pkg/agent/sveltos-agent-in-mgmt-cluster.yaml +++ b/pkg/agent/sveltos-agent-in-mgmt-cluster.yaml @@ -45,7 +45,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/sveltos-agent@sha256:6bcf5d20ad39145e617109509c452daa420e3e5b86636adf6a1462908789ff70 + image: docker.io/projectsveltos/sveltos-agent@sha256:59272b4f68af3cdce0d59ba221530ea7783b7e30901750474a1e805f17eab673 livenessProbe: failureThreshold: 3 httpGet: diff --git a/pkg/agent/sveltos-agent.go b/pkg/agent/sveltos-agent.go index 7f7b826..ba31133 100644 --- a/pkg/agent/sveltos-agent.go +++ b/pkg/agent/sveltos-agent.go @@ -212,7 +212,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/sveltos-agent@sha256:6bcf5d20ad39145e617109509c452daa420e3e5b86636adf6a1462908789ff70 + image: docker.io/projectsveltos/sveltos-agent@sha256:59272b4f68af3cdce0d59ba221530ea7783b7e30901750474a1e805f17eab673 livenessProbe: failureThreshold: 3 httpGet: diff --git a/pkg/agent/sveltos-agent.yaml b/pkg/agent/sveltos-agent.yaml index 72e6df8..c0d7b0f 100644 --- a/pkg/agent/sveltos-agent.yaml +++ b/pkg/agent/sveltos-agent.yaml @@ -194,7 +194,7 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - image: docker.io/projectsveltos/sveltos-agent@sha256:6bcf5d20ad39145e617109509c452daa420e3e5b86636adf6a1462908789ff70 + image: docker.io/projectsveltos/sveltos-agent@sha256:59272b4f68af3cdce0d59ba221530ea7783b7e30901750474a1e805f17eab673 livenessProbe: failureThreshold: 3 httpGet: