From 4fada9bae59b3c6ebaf326288f20c33cc1d3b205 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Fri, 30 Oct 2026 13:51:10 +0100 Subject: [PATCH] fix: Record per cluster Classifier removal in the ClassifierReport undeployClassifier needed the cleanup of every cluster to complete within a single reconcile pass. Deployer results are consumed on read and, in pull mode, the ConfigurationGroup is deleted once the removal completes (a missing ConfigurationGroup is then treated as an error and a new Remove one is created), so with many Classifiers deleted at once the cleanup never converged. When a Classifier is removed from a cluster, set the DeploymentStatus of the ClassifierReport to Removed. Reports already in that state are skipped on the next pass. removeClassifier now returns whether the Classifier was removed. Co-Authored-By: Claude Sonnet 5.5 --- controllers/classifier_deployer.go | 42 ++++++-- controllers/classifier_deployer_test.go | 130 +++++++++++++++++++++++- 2 files changed, 161 insertions(+), 11 deletions(-) diff --git a/controllers/classifier_deployer.go b/controllers/classifier_deployer.go index 73e30e8..b1af03c 100644 --- a/controllers/classifier_deployer.go +++ b/controllers/classifier_deployer.go @@ -248,6 +248,14 @@ func (r *ClassifierReconciler) undeployClassifier(ctx context.Context, classifie if report.Spec.ClusterNamespace == "" { continue } + // Skip clusters whose removal has already completed. The result of a removal is consumed when read, so + // the ClassifierReport is what remembers the removal is done: without it, a cluster done in an earlier + // pass would be removed again, and the removal would never complete in all the clusters at once. + if report.Status.DeploymentStatus != nil && + *report.Status.DeploymentStatus == libsveltosv1beta1.SveltosStatusRemoved { + + continue + } c := getClusterRefFromClassifierReport(report) r.Deployer.CleanupEntries(c.Namespace, c.Name, classifier.Name, f.id, clusterproxy.GetClusterType(c), false) @@ -272,7 +280,7 @@ func (r *ClassifierReconciler) undeployClassifier(ctx context.Context, classifie var failedCount int for i := range clusters { c := clusters[i] - err := r.removeClassifier(ctx, classifierScope, c, f, logger) + removed, err := r.removeClassifier(ctx, classifierScope, c, f, logger) if err != nil { failureMessage := err.Error() if updateErr := updateClassifierReportDeploymentStatus(ctx, r.Client, classifier.Name, c, @@ -284,6 +292,18 @@ func (r *ClassifierReconciler) undeployClassifier(ctx context.Context, classifie logger.V(logs.LogInfo).Error(updateErr, "failed to update ClassifierReport undeploy status") } failedCount++ + continue + } + + if removed { + if updateErr := updateClassifierReportDeploymentStatus(ctx, r.Client, classifier.Name, c, + &libsveltosv1beta1.ClusterInfo{ + Cluster: *c, + Status: libsveltosv1beta1.SveltosStatusRemoved, + }); updateErr != nil { + logger.V(logs.LogInfo).Error(updateErr, "failed to record Classifier removal in ClassifierReport") + failedCount++ + } } } @@ -931,20 +951,22 @@ func (r *ClassifierReconciler) isPaused(ctx context.Context, cluster *corev1.Obj return annotations.HasPaused(classifier), nil } -// removeClassifier removes Classifier instance from cluster +// removeClassifier removes Classifier instance from cluster. +// It returns true when the instance has been removed. It returns false with no error when there is nothing +// to wait for but nothing was removed either (the cluster is paused). func (r *ClassifierReconciler) removeClassifier(ctx context.Context, classifierScope *scope.ClassifierScope, cluster *corev1.ObjectReference, f feature, logger logr.Logger, -) error { +) (bool, error) { classifier := classifierScope.Classifier paused, err := r.isPaused(ctx, cluster, classifierScope.Classifier) if err != nil { - return err + return false, err } if paused { logger.V(logs.LogDebug).Info("cluster is paused. Do nothing.") - return nil + return false, nil } // If deploying feature is in progress, wait for it to complete. @@ -952,7 +974,7 @@ func (r *ClassifierReconciler) removeClassifier(ctx context.Context, classifierS // parallel some resources might end stale. if r.Deployer.IsInProgress(cluster.Namespace, cluster.Name, classifier.Name, f.id, clusterproxy.GetClusterType(cluster), false) { logger.V(logs.LogDebug).Info("deploy is in progress") - return fmt.Errorf("deploy of %s in cluster still in progress. Wait before redeploying", f.id) + return false, fmt.Errorf("deploy of %s in cluster still in progress. Wait before redeploying", f.id) } result := r.Deployer.GetResult(ctx, cluster.Namespace, cluster.Name, classifier.Name, f.id, clusterproxy.GetClusterType(cluster), true) @@ -960,10 +982,10 @@ func (r *ClassifierReconciler) removeClassifier(ctx context.Context, classifierS if status != nil { if *status == libsveltosv1beta1.SveltosStatusProvisioning { - return fmt.Errorf("feature is still being removed") + return false, fmt.Errorf("feature is still being removed") } if *status == libsveltosv1beta1.SveltosStatusRemoved { - return nil + return true, nil } } else { logger.V(logs.LogDebug).Info("no result is available") @@ -972,10 +994,10 @@ func (r *ClassifierReconciler) removeClassifier(ctx context.Context, classifierS logger.V(logs.LogDebug).Info("queueing request to un-deploy") if err := r.Deployer.Deploy(ctx, cluster.Namespace, cluster.Name, classifier.Name, f.id, clusterproxy.GetClusterType(cluster), true, undeployClassifierFromCluster, programDuration, deployer.Options{}); err != nil { - return err + return false, err } - return fmt.Errorf("cleanup request is queued") + return false, fmt.Errorf("cleanup request is queued") } // canProceed returns true if cluster is ready to be programmed and it is not paused. diff --git a/controllers/classifier_deployer_test.go b/controllers/classifier_deployer_test.go index 4b8c221..2ee4a19 100644 --- a/controllers/classifier_deployer_test.go +++ b/controllers/classifier_deployer_test.go @@ -397,8 +397,9 @@ var _ = Describe("Classifier Deployer", func() { Expect(waitForObject(context.TODO(), testEnv.Client, classifier)).To(Succeed()) f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureClassifier) - err := controllers.RemoveClassifier(classifierReconciler, context.TODO(), classifierScope, + removed, err := controllers.RemoveClassifier(classifierReconciler, context.TODO(), classifierScope, getClusterRef(cluster), f, logger) + Expect(removed).To(BeFalse()) Expect(err).ToNot(BeNil()) Expect(err.Error()).To(Equal("cleanup request is queued")) @@ -521,6 +522,74 @@ var _ = Describe("Classifier Deployer", func() { Expect(dep.IsKeyInProgress(key)).To(BeTrue()) }) + It("undeployClassifier does not queue a job for a cluster whose ClassifierReport says the Classifier was removed", func() { + dep := fakedeployer.GetClient(context.TODO(), logger, testEnv.Client) + Expect(dep.RegisterFeatureID(libsveltosv1beta1.FeatureClassifier)).To(Succeed()) + classifierReconciler := getClassifierReconciler(testEnv.Client, dep) + + classifier := createClassifier(getClassifierInstance(randomString())) + cluster := prepareCluster() + removed := libsveltosv1beta1.SveltosStatusRemoved + report := createClassifierReport(classifier, cluster, &removed) + + classifierScope := getClassifierScope(testEnv.Client, logger, classifier) + f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureClassifier) + Expect(controllers.UndeployClassifier(classifierReconciler, context.TODO(), classifierScope, f, logger)).To(Succeed()) + + key := deployer.GetKey(cluster.Namespace, cluster.Name, + classifier.Name, libsveltosv1beta1.FeatureClassifier, libsveltosv1beta1.ClusterTypeCapi, true) + Expect(dep.IsKeyInProgress(key)).To(BeFalse()) + + // The report is left alone: removeClassifierReports deletes it once all the clusters are done + currentReport := &libsveltosv1beta1.ClassifierReport{} + Expect(testEnv.Get(context.TODO(), client.ObjectKeyFromObject(report), currentReport)).To(Succeed()) + }) + + It("undeployClassifier records in the ClassifierReport the Classifier was removed from the cluster", func() { + dep := &removedDeployer{DeployerInterface: fakedeployer.GetClient(context.TODO(), logger, testEnv.Client)} + classifierReconciler := getClassifierReconciler(testEnv.Client, dep) + + classifier := createClassifier(getClassifierInstance(randomString())) + cluster := prepareCluster() + report := createClassifierReport(classifier, cluster, nil) + + classifierScope := getClassifierScope(testEnv.Client, logger, classifier) + f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureClassifier) + Expect(controllers.UndeployClassifier(classifierReconciler, context.TODO(), classifierScope, f, logger)).To(Succeed()) + + // Eventual loop so testEnv Cache is synced + Eventually(func() bool { + currentReport := &libsveltosv1beta1.ClassifierReport{} + err := testEnv.Get(context.TODO(), client.ObjectKeyFromObject(report), currentReport) + if err != nil { + return false + } + return currentReport.Status.DeploymentStatus != nil && + *currentReport.Status.DeploymentStatus == libsveltosv1beta1.SveltosStatusRemoved + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("undeployClassifier does not record the Classifier as removed from a paused cluster", func() { + dep := &removedDeployer{DeployerInterface: fakedeployer.GetClient(context.TODO(), logger, testEnv.Client)} + classifierReconciler := getClassifierReconciler(testEnv.Client, dep) + + classifier := getClassifierInstance(randomString()) + classifier.Annotations = map[string]string{clusterv1.PausedAnnotation: ""} + classifier = createClassifier(classifier) + cluster := prepareCluster() + report := createClassifierReport(classifier, cluster, nil) + + classifierScope := getClassifierScope(testEnv.Client, logger, classifier) + f := controllers.GetHandlersForFeature(libsveltosv1beta1.FeatureClassifier) + Expect(controllers.UndeployClassifier(classifierReconciler, context.TODO(), classifierScope, f, logger)).To(Succeed()) + + Consistently(func() bool { + currentReport := &libsveltosv1beta1.ClassifierReport{} + Expect(testEnv.Get(context.TODO(), client.ObjectKeyFromObject(report), currentReport)).To(Succeed()) + return currentReport.Status.DeploymentStatus == nil + }, 3*time.Second, time.Second).Should(BeTrue()) + }) + It("deploySveltosAgent deploys sveltos agent", func() { watchNamespaces := []string{watchNamespaceNs1, watchNamespaceNs2} Expect(controllers.DeploySveltosAgentInManagedCluster(ctx, testEnv.Config, randomString(), randomString(), @@ -1542,3 +1611,62 @@ func removeFinalizersAndDeleteClassifier(name string) { Expect(testEnv.Update(context.TODO(), classifier)).To(Succeed()) Expect(client.IgnoreNotFound(testEnv.Delete(context.TODO(), classifier))).To(Succeed()) } + +// removedDeployer is a fake deployer which reports every cleanup request as completed +type removedDeployer struct { + deployer.DeployerInterface +} + +func (d *removedDeployer) GetResult(_ context.Context, _, _, _, _ string, _ libsveltosv1beta1.ClusterType, + _ bool) deployer.Result { + + return deployer.Result{ResultStatus: deployer.Removed} +} + +// createClassifier creates the Classifier and returns it as the controller reads it +func createClassifier(classifier *libsveltosv1beta1.Classifier) *libsveltosv1beta1.Classifier { + Expect(testEnv.Create(context.TODO(), classifier)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, classifier)).To(Succeed()) + + currentClassifier := &libsveltosv1beta1.Classifier{} + Expect(testEnv.Get(context.TODO(), types.NamespacedName{Name: classifier.Name}, currentClassifier)).To(Succeed()) + return currentClassifier +} + +// createClassifierReport creates the ClassifierReport of the Classifier for the cluster, with the deployment +// status given, and waits for it to be in the cache +func createClassifierReport(classifier *libsveltosv1beta1.Classifier, cluster *clusterv1.Cluster, + deploymentStatus *libsveltosv1beta1.SveltosFeatureStatus) *libsveltosv1beta1.ClassifierReport { + + clusterType := libsveltosv1beta1.ClusterTypeCapi + report := &libsveltosv1beta1.ClassifierReport{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: cluster.Namespace, + Name: libsveltosv1beta1.GetClassifierReportName(classifier.Name, cluster.Name, &clusterType), + Labels: libsveltosv1beta1.GetClassifierReportLabels(classifier.Name, cluster.Name, &clusterType), + }, + Spec: libsveltosv1beta1.ClassifierReportSpec{ + ClusterNamespace: cluster.Namespace, + ClusterName: cluster.Name, + ClusterType: clusterType, + ClassifierName: classifier.Name, + }, + } + Expect(testEnv.Create(context.TODO(), report)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, report)).To(Succeed()) + + if deploymentStatus != nil { + Expect(testEnv.Get(context.TODO(), client.ObjectKeyFromObject(report), report)).To(Succeed()) + report.Status.DeploymentStatus = deploymentStatus + Expect(testEnv.Status().Update(context.TODO(), report)).To(Succeed()) + + // Eventual loop so testEnv Cache is synced + Eventually(func() bool { + currentReport := &libsveltosv1beta1.ClassifierReport{} + err := testEnv.Get(context.TODO(), client.ObjectKeyFromObject(report), currentReport) + return err == nil && currentReport.Status.DeploymentStatus != nil + }, timeout, pollingInterval).Should(BeTrue()) + } + + return report +}