From 22d3bd35ecdf302fa3b1d669fd19f1c251c29548 Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 16 Feb 2023 09:08:55 -0500 Subject: [PATCH] Remove the unused Deployment finalizer Previous code added a finalizer on the Deployment running the controller when pruneObjectBehavior was used. This commit removes that unnecessary finalizer if present. Relates: https://issues.redhat.com/browse/ACM-2923 Signed-off-by: mprahl --- controllers/configurationpolicy_controller.go | 51 ++++++++++++++++- test/e2e/case29_trigger_uninstall_test.go | 56 +++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 1f9fc54f..221735b7 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -166,8 +166,18 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies( const waiting = 10 * time.Minute exiting := false + deploymentFinalizerRemoved := false for !exiting { + if !deploymentFinalizerRemoved { + err := r.removeLegacyDeploymentFinalizer() + if err == nil { + deploymentFinalizerRemoved = true + } else { + log.Error(err, "Failed to remove the legacy Deployment finalizer. Will try again.") + } + } + start := time.Now() policiesList := policyv1.ConfigurationPolicyList{} @@ -2777,25 +2787,60 @@ func convertPolicyStatusToString(plc *policyv1.ConfigurationPolicy) (results str return result } -func (r *ConfigurationPolicyReconciler) isBeingUninstalled() (bool, error) { +// getDeployment gets the Deployment object associated with this controller. If the controller is running outside of +// a cluster, no Deployment object or error will be returned. +func (r *ConfigurationPolicyReconciler) getDeployment() (*appsv1.Deployment, error) { key, err := common.GetOperatorNamespacedName() if err != nil { // Running locally if errors.Is(err, common.ErrNoNamespace) || errors.Is(err, common.ErrRunLocal) { - return false, nil + return nil, nil } - return false, err + return nil, err } deployment := appsv1.Deployment{} if err := r.Client.Get(context.TODO(), key, &deployment); err != nil { + return nil, err + } + + return &deployment, nil +} + +func (r *ConfigurationPolicyReconciler) isBeingUninstalled() (bool, error) { + deployment, err := r.getDeployment() + if deployment == nil || err != nil { return false, err } return deployment.Annotations[common.UninstallingAnnotation] == "true", nil } +// removeLegacyDeploymentFinalizer removes the policy.open-cluster-management.io/delete-related-objects on the +// Deployment object. This finalizer is no longer needed on the Deployment object, so it is removed. +func (r *ConfigurationPolicyReconciler) removeLegacyDeploymentFinalizer() error { + deployment, err := r.getDeployment() + if deployment == nil || err != nil { + return err + } + + for i, finalizer := range deployment.Finalizers { + if finalizer == pruneObjectFinalizer { + newFinalizers := append(deployment.Finalizers[:i], deployment.Finalizers[i+1:]...) + deployment.SetFinalizers(newFinalizers) + + log.Info("Removing the legacy finalizer on the controller Deployment") + + err := r.Client.Update(context.TODO(), deployment) + + return err + } + } + + return nil +} + func recoverFlow() { if r := recover(); r != nil { // V(-2) is the error level diff --git a/test/e2e/case29_trigger_uninstall_test.go b/test/e2e/case29_trigger_uninstall_test.go index 617a0946..779e8a1a 100644 --- a/test/e2e/case29_trigger_uninstall_test.go +++ b/test/e2e/case29_trigger_uninstall_test.go @@ -119,3 +119,59 @@ var _ = Describe("Clean up during uninstalls", Label("running-in-cluster"), Orde }, defaultTimeoutSeconds, 1).Should(Succeed()) }) }) + +// This test only works when the controller is running in the cluster. +var _ = Describe("Clean up the finalizer on the Deployment", Label("running-in-cluster"), Ordered, func() { + const ( + deploymentName string = "config-policy-controller" + deploymentNamespace string = "open-cluster-management-agent-addon" + pruneObjectFinalizer string = "policy.open-cluster-management.io/delete-related-objects" + ) + + It("verifies that the Deployment finalizer is removed", func() { + deploymentRsrc := clientManaged.AppsV1().Deployments(deploymentNamespace) + + By("Adding a finalizer to the Deployment") + Eventually(func(g Gomega) { + deployment, err := deploymentRsrc.Get(context.TODO(), deploymentName, metav1.GetOptions{}) + g.Expect(err).To(BeNil()) + + deployment.SetFinalizers(append(deployment.Finalizers, pruneObjectFinalizer)) + _, err = deploymentRsrc.Update(context.TODO(), deployment, metav1.UpdateOptions{}) + g.Expect(err).To(BeNil()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) + + // Trigger a restart so that the finalizer removal logic is executed. + utils.Kubectl("-n", deploymentNamespace, "rollout", "restart", "deployment/"+deploymentName) + + By("Waiting for the finalizer on the Deployment to be removed") + Eventually(func(g Gomega) { + deployment, err := deploymentRsrc.Get(context.TODO(), deploymentName, metav1.GetOptions{}) + g.Expect(err).To(BeNil()) + + g.Expect(deployment.Finalizers).ToNot(ContainElement(pruneObjectFinalizer)) + }, defaultTimeoutSeconds*2, 1).Should(Succeed()) + }) + + AfterAll(func() { + deploymentRsrc := clientManaged.AppsV1().Deployments(deploymentNamespace) + + // Use an eventually in case there are update conflicts and there needs to be a retry + Eventually(func(g Gomega) { + deployment, err := deploymentRsrc.Get(context.TODO(), deploymentName, metav1.GetOptions{}) + g.Expect(err).To(BeNil()) + + for i, finalizer := range deployment.Finalizers { + if finalizer == pruneObjectFinalizer { + newFinalizers := append(deployment.Finalizers[:i], deployment.Finalizers[i+1:]...) + deployment.SetFinalizers(newFinalizers) + + _, err := deploymentRsrc.Update(context.TODO(), deployment, metav1.UpdateOptions{}) + g.Expect(err).To(BeNil()) + + break + } + } + }, defaultTimeoutSeconds, 1).Should(Succeed()) + }) +})