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