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

Issue:
MGMT-18560: Fix AutomatedCleaningMode behavior
AutomatedCleaningMode should not be used when the
converged flow is disabled since it requires IPA
which is only enabled when the converged flow is enabled.

Caused by regression in PR
https://github.com/openshift/assisted-service/pull/5319 which relies on users to set the automatedCleaningMode spec, but did not take into account the converged flow being disabled.

<!--
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
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [x] 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

/cc @carbonin @eranco74 


Discussion / hints:

Fix patch:
diff --git a/internal/controller/controllers/bmh_agent_controller.go b/internal/controller/controllers/bmh_agent_controller.go
index 7380b364ef4..ec423ea14e2 100644
--- a/internal/controller/controllers/bmh_agent_controller.go
+++ b/internal/controller/controllers/bmh_agent_controller.go
@@ -922,6 +922,14 @@ func (r *BMACReconciler) reconcileBMH(ctx context.Context, log logrus.FieldLogge
 		dirty = true
 	}
 
+	if bmh.Spec.AutomatedCleaningMode != bmh_v1alpha1.CleaningModeDisabled {
+		// Disable AutomatedCleaningMode if the converged flow is not enabled
+		// since AutomatedCleaning requires IPA, but disabling the converged flow
+		// disables IPA.
+		bmh.Spec.AutomatedCleaningMode = bmh_v1alpha1.CleaningModeDisabled
+		dirty = true
+	}
+
 	proceed, stopReconcileLoop, requeuePeriod, reason := shouldReconcileBMH(bmh, infraEnv)
 
 	if !proceed {


Test patch:
diff --git a/internal/controller/controllers/bmh_agent_controller_test.go b/internal/controller/controllers/bmh_agent_controller_test.go
index 387241f485e..f6a7b88217c 100644
--- a/internal/controller/controllers/bmh_agent_controller_test.go
+++ b/internal/controller/controllers/bmh_agent_controller_test.go
@@ -348,22 +348,22 @@ var _ = Describe("bmac reconcile", func() {
 				Expect(host.ObjectMeta.Annotations[BMH_INSPECT_ANNOTATION]).To(Equal("disabled"))
 				Expect(host.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeDisabled))
 
-				// Test that cleaning mode stays the same
+				// Test that cleaning mode is set to disabled
 				host.Spec.AutomatedCleaningMode = bmh_v1alpha1.CleaningModeMetadata
 				host.Status.Provisioning.State = bmh_v1alpha1.StateProvisioned
 
 				result = bmhr.reconcileBMH(ctx, bmhr.Log, host, nil, infraEnv)
-				Expect(result).To(Equal(reconcileComplete{dirty: false, stop: true}))
+				Expect(result).To(Equal(reconcileComplete{dirty: true, stop: true}))
 				Expect(host.ObjectMeta.Annotations).To(HaveKey(BMH_INSPECT_ANNOTATION))
 				Expect(host.ObjectMeta.Annotations[BMH_INSPECT_ANNOTATION]).To(Equal("disabled"))
-				Expect(host.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeMetadata))
+				Expect(host.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeDisabled))
 
 				// This should not return a dirty result because label is already set
 				result = bmhr.reconcileBMH(ctx, bmhr.Log, host, nil, infraEnv)
 				Expect(result).To(Equal(reconcileComplete{dirty: false, stop: true}))
 				Expect(host.ObjectMeta.Annotations).To(HaveKey(BMH_INSPECT_ANNOTATION))
 				Expect(host.ObjectMeta.Annotations[BMH_INSPECT_ANNOTATION]).To(Equal("disabled"))
-				Expect(host.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeMetadata))
+				Expect(host.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeDisabled))
 			})
 
 			It("should set the ISODownloadURL in the BMH", func() {
@@ -377,7 +377,20 @@ var _ = Describe("bmac reconcile", func() {
 				Expect(updatedHost.Spec.Image.URL).To(Equal(isoImageURL))
 			})
 
-			It("should not disable cleaning and set online true in the BMH", func() {
+			It("should disable cleaning and set online true in the BMH", func() {
+				result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				updatedHost := &bmh_v1alpha1.BareMetalHost{}
+				err = c.Get(ctx, types.NamespacedName{Name: "bmh-reconcile", Namespace: testNamespace}, updatedHost)
+				Expect(err).To(BeNil())
+				Expect(updatedHost.Spec.Online).To(Equal(true))
+				Expect(updatedHost.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeDisabled))
+			})
+
+			It("should disable cleaning when set to metadata by user in the BMH", func() {
+				host.Spec.AutomatedCleaningMode = bmh_v1alpha1.CleaningModeMetadata
 				result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
 				Expect(err).To(BeNil())
 				Expect(result).To(Equal(ctrl.Result{}))
@@ -386,8 +399,9 @@ var _ = Describe("bmac reconcile", func() {
 				err = c.Get(ctx, types.NamespacedName{Name: "bmh-reconcile", Namespace: testNamespace}, updatedHost)
 				Expect(err).To(BeNil())
 				Expect(updatedHost.Spec.Online).To(Equal(true))
-				Expect(updatedHost.Spec.AutomatedCleaningMode).NotTo(Equal(bmh_v1alpha1.CleaningModeDisabled))
+				Expect(updatedHost.Spec.AutomatedCleaningMode).To(Equal(bmh_v1alpha1.CleaningModeDisabled))
 			})
+
 			It("should not reconcile BMH if the updated image has not been around longer than the grace period", func() {
 				// Reconcile with the original ISO
 				_ = bmhr.reconcileBMH(ctx, bmhr.Log, host, nil, infraEnv)