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

Issue:
MGMT-19215: add paused annotation only when BMH is detached
Before this change, the paused annotation is added when the BMH reaches `provisioned` state.
But since the detached annotation is added only afterwards, the status annotation is stored with: `operationalStatus: OK` (i.e. instead of `operationalStatus: detached`).

Thus, when deleting the BMH, the wrong operationStatus is restored according to the status annotation (upon deleting the paused annotation). So this change ensures that the paused annotation is added only if the BMH is already detached.

Note: also added an info log in the finalizer to help with debugging.

<!--
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)
- [ ] 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 4f08d38d372..b24875ff5e6 100644
--- a/internal/controller/controllers/bmh_agent_controller.go
+++ b/internal/controller/controllers/bmh_agent_controller.go
@@ -348,6 +348,7 @@ func (r *BMACReconciler) handleBMHFinalizer(ctx context.Context, log logrus.Fiel
 
 	var removeAgentOnDelete bool
 	if _, has_annotation := bmh.GetAnnotations()[BMH_DELETE_ANNOTATION]; has_annotation && r.ConvergedFlowEnabled {
+		log.Infof("'%s' annotation was added and converged flow is enabled", BMH_DELETE_ANNOTATION)
 		removeAgentOnDelete = true
 	}
 	_, bmhPaused := bmh.GetAnnotations()[BMH_PAUSED_ANNOTATION]
@@ -1895,6 +1896,12 @@ func (r *BMACReconciler) addBMHStatusAndPausedAnnotations(log logrus.FieldLogger
 		bmh.ObjectMeta.Annotations = make(map[string]string)
 	}
 
+	if bmh.Status.OperationalStatus != bmh_v1alpha1.OperationalStatusDetached {
+		// The BMH is not yet detached, thus we shouldn't pause it in this state
+		// (since 'detached' OperationalStatus won't be saved in status annotation).
+		return reconcileComplete{}
+	}
+
 	// Convert BMH status to a JSON string
 	statusJson, err := json.Marshal(bmh.Status)
 	if err != nil {


Test patch:
diff --git a/internal/controller/controllers/bmh_agent_controller_test.go b/internal/controller/controllers/bmh_agent_controller_test.go
index aa058d466c6..ec2c8a7bd27 100644
--- a/internal/controller/controllers/bmh_agent_controller_test.go
+++ b/internal/controller/controllers/bmh_agent_controller_test.go
@@ -2036,6 +2036,7 @@ var _ = Describe("bmac reconcile", func() {
 		Context("when BMH is in provisioned state", func() {
 			BeforeEach(func() {
 				bmh.Status.Provisioning.State = bmh_v1alpha1.StateProvisioned
+				bmh.Status.OperationalStatus = bmh_v1alpha1.OperationalStatusDetached
 				bmh.ObjectMeta.Annotations = make(map[string]string)
 			})
 
@@ -2090,7 +2091,7 @@ var _ = Describe("bmac reconcile", func() {
 				// Update BMH
 				err = c.Get(ctx, types.NamespacedName{Name: bmh.Name, Namespace: testNamespace}, bmh)
 				Expect(err).To(BeNil())
-				bmh.Status.OperationalStatus = "discovered"
+				bmh.Status.PoweredOn = true
 				Expect(c.Update(ctx, bmh)).To(BeNil())
 
 				// Reconcile updated BMH
@@ -2185,6 +2186,25 @@ var _ = Describe("bmac reconcile", func() {
 				// Ensure BMH is in 'provisioned' state
 				Expect(updatedBMH.Status.Provisioning.State).To(Equal(bmh_v1alpha1.StateProvisioned))
 			})
+
+			It("should not add paused annotation if the BMH is not detached", func() {
+				// Create BMH
+				Expect(c.Create(ctx, bmh)).To(Succeed())
+				bmh.Status.OperationalStatus = bmh_v1alpha1.OperationalStatusOK
+
+				// Reconcile BMH
+				result, err := bmhr.Reconcile(ctx, newBMHRequest(bmh))
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				// Get updated BMH
+				updatedBMH = &bmh_v1alpha1.BareMetalHost{}
+				err = c.Get(ctx, types.NamespacedName{Name: bmh.Name, Namespace: testNamespace}, updatedBMH)
+				Expect(err).To(BeNil())
+
+				// Ensure 'paused' annotation is not added
+				Expect(bmh.ObjectMeta.Annotations).To(Not(HaveKey(BMH_PAUSED_ANNOTATION)))
+			})
 		})
 
 		Context("when BMH's provisioning state is missing", func() {