From 854597f5715b33fee8dfbee478d61272b9f1c111 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Fri, 30 Oct 2026 12:17:13 +0100 Subject: [PATCH] fix: Do not remove the Classifier when undeploying from the management cluster When the management cluster is registered as a managed cluster, the Classifier instance found there is the Classifier itself. Undeploying it deleted the Classifier from the handler, bypassing the cleanup of the other clusters. Skip the instance when it has the same UID as the Classifier in the management cluster. For the other clusters, remove the finalizer older sveltos-agent versions share with classifier-manager before deleting the instance, as sveltos-agent now uses its own finalizer and would not release it. Co-Authored-By: Claude Sonnet 5.5 --- controllers/classifier_deployer.go | 35 ++++++++- controllers/classifier_deployer_test.go | 76 ++++++++++++++++++-- controllers/export_test.go | 1 + pkg/agent/sveltos-agent-in-mgmt-cluster.go | 2 +- pkg/agent/sveltos-agent-in-mgmt-cluster.yaml | 2 +- pkg/agent/sveltos-agent.go | 2 +- pkg/agent/sveltos-agent.yaml | 2 +- 7 files changed, 109 insertions(+), 11 deletions(-) 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: