Repository: openshift/assisted-service (language: python)

Issue:
MGMT-18980: BMH finalizer - remove paused annotation
In order to ensure BMH deprovision by Ironic, removing the 'paused' annotation (if exists) as part of the finalizer.

<!--
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.

Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
information on how to link a PR to an existing issue.

It's recommended to take a few extra minutes to provide more information about
how this code was tested. Here are some questions that may be worth answering:

- Should this PR be tested by the reviewer?
- Is this PR relying on CI for an e2e test run?
- Should this PR be tested in a specific environment?
- Any logs, screenshots, etc that can help with the review process?

-->

## List all the issues related to this PR

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [ ] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [x] dev-scripts environment
- [ ] Reviewer's test appreciated
- [ ] Waiting for CI to do a full test run
- [ ] Manual (Elaborate on how it was tested)
- [x] No tests needed

## Checklist

- [x] Title and description added to both, commit and PR.
- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
- [x] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?

[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md



Discussion / hints:

Fix patch:
diff --git a/internal/controller/controllers/bmh_agent_controller.go b/internal/controller/controllers/bmh_agent_controller.go
index 1a2de5972e3..43907ae95ec 100644
--- a/internal/controller/controllers/bmh_agent_controller.go
+++ b/internal/controller/controllers/bmh_agent_controller.go
@@ -334,12 +334,16 @@ func (r *BMACReconciler) Reconcile(origCtx context.Context, req ctrl.Request) (c
 
 // handleBMHFinalizer adds a finalizer to the BMH if it isn't being deleted and the finalizer isn't already present
 // If the BMH is being deleted, the associated node is drained and the agent (if any) is annotated and deleted before the BMH is allowed to be removed
-// Finalizer handling is only run if the user has annotated the BMH and converged flow is enabled
+// Finalizer handling is only run if the user has annotated the BMH and converged flow is enabled,
+// or, when the BMH is annotated with 'paused' annotation.
 func (r *BMACReconciler) handleBMHFinalizer(ctx context.Context, log logrus.FieldLogger, bmh *bmh_v1alpha1.BareMetalHost, agent *aiv1beta1.Agent) reconcileResult {
-	if _, has_annotation := bmh.GetAnnotations()[BMH_DELETE_ANNOTATION]; !has_annotation || !r.ConvergedFlowEnabled {
-		return reconcileComplete{}
+	var removeAgentOnDelete bool
+	if _, has_annotation := bmh.GetAnnotations()[BMH_DELETE_ANNOTATION]; has_annotation && r.ConvergedFlowEnabled {
+		removeAgentOnDelete = true
 	}
+	_, bmhPaused := bmh.GetAnnotations()[BMH_PAUSED_ANNOTATION]
 
+	// Register a finalizer if it's absent
 	bmhHasFinalizer := funk.ContainsString(bmh.GetFinalizers(), BMH_FINALIZER_NAME)
 	if bmh.ObjectMeta.DeletionTimestamp.IsZero() {
 		if !bmhHasFinalizer {
@@ -351,7 +355,11 @@ func (r *BMACReconciler) handleBMHFinalizer(ctx context.Context, log logrus.Fiel
 
 	// BMH is being deleted and finalizer is gone, ensure detached and paused are gone and return
 	if !bmhHasFinalizer {
-		dirty := removeBMHDetachedAndPausedAnnotations(log, bmh)
+		if removeAgentOnDelete {
+			removeBMHDetachedAnnotation(log, bmh)
+		}
+		removeBMHPausedAnnotation(log, bmh)
+		dirty := removeAgentOnDelete || bmhPaused
 		return reconcileComplete{stop: true, dirty: dirty}
 	}
 
@@ -359,32 +367,38 @@ func (r *BMACReconciler) handleBMHFinalizer(ctx context.Context, log logrus.Fiel
 	// agent could be nil here if it wasn't created yet, or if we deleted it, then failed to remove the finalizer for some reason
 	// if the agent is missing, just allow the BMH to be removed
 	if agent == nil {
-		removeBMHDetachedAndPausedAnnotations(log, bmh)
+		if removeAgentOnDelete {
+			removeBMHDetachedAnnotation(log, bmh)
+		}
+		removeBMHPausedAnnotation(log, bmh)
 		controllerutil.RemoveFinalizer(bmh, BMH_FINALIZER_NAME)
-		return reconcileComplete{stop: true, dirty: true}
+		dirty := removeAgentOnDelete || bmhPaused
+		return reconcileComplete{stop: true, dirty: dirty}
 	}
 
-	dirty := false
-	if agent.Spec.ClusterDeploymentName != nil {
-		var res reconcileResult
-		dirty, res = r.handleDrain(ctx, log, agent, bmh)
-		if res.Stop(ctx) {
-			return res
-		}
+	if removeAgentOnDelete {
+		dirty := false
+		if agent.Spec.ClusterDeploymentName != nil {
+			var res reconcileResult
+			dirty, res = r.handleDrain(ctx, log, agent, bmh)
+			if res.Stop(ctx) {
+				return res
+			}
 
-		if updated := configureBMHForCleaning(log, bmh); updated {
-			return reconcileComplete{stop: true, dirty: true}
-		}
+			if updated := configureBMHForCleaning(log, bmh); updated {
+				return reconcileComplete{stop: true, dirty: true}
+			}
 
-		if bmh.Status.Provisioning.State != bmh_v1alpha1.StateDeleting {
-			log.Info("Waiting for BMH to deprovision")
-			return reconcileRequeue{requeueAfter: defaultRequeueAfterOnError}
+			if bmh.Status.Provisioning.State != bmh_v1alpha1.StateDeleting {
+				log.Info("Waiting for BMH to deprovision")
+				return reconcileRequeue{requeueAfter: defaultRequeueAfterOnError}
+			}
 		}
-	}
 
-	if err := r.Delete(ctx, agent); err != nil {
-		log.WithError(err).Errorf("Failed to delete agent as a part of BMH removal")
-		return reconcileError{err: err, dirty: dirty}
+		if err := r.Delete(ctx, agent); err != nil {
+			log.WithError(err).Errorf("Failed to delete agent as a part of BMH removal")
+			return reconcileError{err: err, dirty: dirty}
+		}
 	}
 
 	log.Infof("Removing BMH finalizer %s", BMH_FINALIZER_NAME)
@@ -1691,12 +1705,16 @@ func (r *BMACReconciler) getClusterDeploymentAndCheckIfInstalled(ctx context.Con
 	return clusterDeployment, true, err
 }
 
-func removeBMHDetachedAndPausedAnnotations(log logrus.FieldLogger, bmh *bmh_v1alpha1.BareMetalHost) (dirty bool) {
+func removeBMHDetachedAnnotation(log logrus.FieldLogger, bmh *bmh_v1alpha1.BareMetalHost) (dirty bool) {
 	if _, ok := bmh.GetAnnotations()[BMH_DETACHED_ANNOTATION]; ok {
 		log.Info("removing BMH detached annotation")
 		delete(bmh.Annotations, BMH_DETACHED_ANNOTATION)
 		dirty = true
 	}
+	return
+}
+
+func removeBMHPausedAnnotation(log logrus.FieldLogger, bmh *bmh_v1alpha1.BareMetalHost) (dirty bool) {
 	if _, ok := bmh.GetAnnotations()[BMH_PAUSED_ANNOTATION]; ok {
 		log.Info("removing BMH paused annotation")
 		delete(bmh.Annotations, BMH_PAUSED_ANNOTATION)
@@ -1706,7 +1724,8 @@ func removeBMHDetachedAndPausedAnnotations(log logrus.FieldLogger, bmh *bmh_v1al
 }
 
 func configureBMHForCleaning(log logrus.FieldLogger, bmh *bmh_v1alpha1.BareMetalHost) (dirty bool) {
-	dirty = removeBMHDetachedAndPausedAnnotations(log, bmh)
+	dirty = removeBMHDetachedAnnotation(log, bmh)
+	dirty = removeBMHPausedAnnotation(log, bmh) || dirty
 
 	if bmh.Spec.AutomatedCleaningMode != bmh_v1alpha1.CleaningModeMetadata {
 		log.Infof("setting BMH cleaning mode to %s", bmh_v1alpha1.CleaningModeMetadata)


Test patch:
diff --git a/internal/controller/controllers/bmh_agent_controller_test.go b/internal/controller/controllers/bmh_agent_controller_test.go
index 01df00b4ad5..94e082838e4 100644
--- a/internal/controller/controllers/bmh_agent_controller_test.go
+++ b/internal/controller/controllers/bmh_agent_controller_test.go
@@ -86,19 +86,6 @@ var _ = Describe("bmac reconcile", func() {
 		Expect(host.GetFinalizers()).To(ContainElement(BMH_FINALIZER_NAME))
 	})
 
-	It("doesn't add a finalizer to the BMH when it does not have the management annotation", func() {
-		host := newBMH("bmh-reconcile", &bmh_v1alpha1.BareMetalHostSpec{})
-		Expect(c.Create(ctx, host)).To(BeNil())
-
-		result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
-		Expect(err).To(BeNil())
-		Expect(result).To(Equal(ctrl.Result{}))
-
-		key := types.NamespacedName{Name: host.Name, Namespace: host.Namespace}
-		Expect(c.Get(ctx, key, host)).To(Succeed())
-		Expect(host.GetFinalizers()).ToNot(ContainElement(BMH_FINALIZER_NAME))
-	})
-
 	Describe("queue bmh request for agent", func() {
 		var host *bmh_v1alpha1.BareMetalHost
 		var agent *v1beta1.Agent
@@ -1064,7 +1051,7 @@ var _ = Describe("bmac reconcile", func() {
 					func(ctx context.Context, bmh *bmh_v1alpha1.BareMetalHost, opts ...client.UpdateOption) error {
 						return c.Update(ctx, bmh)
 					},
-				).Times(2)
+				).Times(3)
 				mockClient.EXPECT().Update(gomock.Any(), gomock.AssignableToTypeOf(&v1beta1.Agent{})).DoAndReturn(
 					func(ctx context.Context, agent *v1beta1.Agent, opts ...client.UpdateOption) error {
 						return c.Update(ctx, agent)
@@ -2672,6 +2659,28 @@ var _ = Describe("handleBMHFinalizer", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		It("removes the finalizer and paused", func() {
+			setAnnotation(&bmh.ObjectMeta, BMH_PAUSED_ANNOTATION, "assisted-service-controller")
+
+			res := bmhr.handleBMHFinalizer(ctx, bmhr.Log, bmh, nil)
+			Expect(res.Dirty()).To(BeTrue())
+			Expect(bmh.GetFinalizers()).NotTo(ContainElement(BMH_FINALIZER_NAME))
+			Expect(bmh.GetAnnotations()).NotTo(HaveKey(BMH_PAUSED_ANNOTATION))
+			_, err := res.Result()
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("removes paused if the finalizer is already gone", func() {
+			setAnnotation(&bmh.ObjectMeta, BMH_PAUSED_ANNOTATION, "assisted-service-controller")
+			bmh.ObjectMeta.Finalizers = nil
+
+			res := bmhr.handleBMHFinalizer(ctx, bmhr.Log, bmh, nil)
+			Expect(res.Dirty()).To(BeTrue())
+			Expect(bmh.GetAnnotations()).NotTo(HaveKey(BMH_PAUSED_ANNOTATION))
+			_, err := res.Result()
+			Expect(err).NotTo(HaveOccurred())
+		})
+
 		Context("with a matching agent", func() {
 			var (
 				agent           *v1beta1.Agent