fix: Record per cluster Classifier removal in the ClassifierReport

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,39 +951,41 @@ 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.
 	// Otherwise, if we undeploy feature while same feature is still being deployed, if two workers process those request in
 	// 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)
 	status := r.convertResultStatus(result)
 
 	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.

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
+}