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

Issue:
MGMT-17893: Don't destroy cluster on detach
Currently the procedure to detach a cluster that was created using hosts from a late binding pool is to first delete the `ManagedCluster` object, then add the `preserveOnDelete: true` field to the `ClusterDeployment` and then delete that `ClusterDeployment`. But the cluster deployment controller doesn't look at the `preserveOnDelete` field at all. As a result the hosts of the cluster are returned to the pool and they are provisioned again, which effectively destroys the cluster.

This patch changes the deployment manager controller so that in that case it will check the `preserveOnDelete` field and if it is `true` it will delete the corresponding `Agent` objects. The result of that is that the hosts will go back to the pool, but marked the will still be marked as provisioned and they will not be provisioned again, thus avoiding the destruction of the cluster.

Note that the `BareMetalHosts` will not be removed, but they will stay detached.

Related: https://issues.redhat.com/browse/MGMT-17893


## 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?

- [x] Automation (CI, tools, etc)
- [ ] Cloud
- [x] Operator Managed Deployments
- [x] None

## How was this code tested?

- [ ] assisted-test-infra environment
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [ ] Waiting for CI to do a full test run
- [x] Manual (Elaborate on how it was tested)
- [x] No tests needed

Tested manually with MCE 2.5.4, replacing the assisted service image with a modified one containing this change.

## 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:
@jhernand: This pull request references [MGMT-17893](https://issues.redhat.com//browse/MGMT-17893) which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.17.0" version, but no target version was set.

<details>

In response to [this](https://github.com/openshift/assisted-service/pull/6532):

>Currently the procedure to detach a cluster that was created using hosts from a late binding pool is to first delete the `ManagedCluster` object, then add the `preserveOnDelete: true` field to the `ClusterDeployment` and then delete that `ClusterDeployment`. But the cluster deployment controller doesn't look at the `preserveOnDelete` field at all. As a result the hosts of the cluster are returned to the pool and they are provisioned again, which effectively destroys the cluster.
>
>This patch changes the deployment manager controller so that in that case it will check the `preserveOnDelete` field and if it is `true` it will delete the corresponding `Agent` objects. The result of that is that the hosts will go back to the pool, but marked the will still be marked as provisioned and they will not be provisioned again, thus avoiding the destruction of the cluster.
>
>Note that the `BareMetalHosts` will not be removed, but they will stay detached.
>
>Related: https://issues.redhat.com/browse/MGMT-17893
>
>
>## 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?
>
>- [x] Automation (CI, tools, etc)
>- [ ] Cloud
>- [x] Operator Managed Deployments
>- [x] None
>
>## How was this code tested?
>
>- [ ] assisted-test-infra environment
>- [ ] dev-scripts environment
>- [ ] Reviewer's test appreciated
>- [ ] Waiting for CI to do a full test run
>- [x] Manual (Elaborate on how it was tested)
>- [x] No tests needed
>
>Tested manually with MCE 2.5.4, replacing the assisted service image with a modified one containing this change.
>
>## 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
>


Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service).  If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
</details>
## [Codecov](https://app.codecov.io/gh/openshift/assisted-service/pull/6532?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) Report
Attention: Patch coverage is `53.33333%` with `7 lines` in your changes missing coverage. Please review.
> Project coverage is 68.42%. Comparing base [(`b7116f3`)](https://app.codecov.io/gh/openshift/assisted-service/commit/b7116f3a023014bbc5290a071aeb2326c00711e1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) to head [(`3ee8313`)](https://app.codecov.io/gh/openshift/assisted-service/commit/3ee831347c7962472066e115836505ae6a021bf9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift).
> Report is 3 commits behind head on master.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/openshift/assisted-service/pull/6532/graphs/tree.svg?width=650&height=150&src=pr&token=YOR4NSSOXQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)](https://app.codecov.io/gh/openshift/assisted-service/pull/6532?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)

```diff
@@            Coverage Diff             @@
##           master    #6532      +/-   ##
==========================================
- Coverage   68.43%   68.42%   -0.01%     
==========================================
  Files         247      247              
  Lines       36416    36425       +9     
==========================================
+ Hits        24920    24923       +3     
- Misses       9290     9294       +4     
- Partials     2206     2208       +2     
```

| [Files](https://app.codecov.io/gh/openshift/assisted-service/pull/6532?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift) | Coverage Δ | |
|---|---|---|
| [...oller/controllers/clusterdeployments\_controller.go](https://app.codecov.io/gh/openshift/assisted-service/pull/6532?src=pr&el=tree&filepath=internal%2Fcontroller%2Fcontrollers%2Fclusterdeployments_controller.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift#diff-aW50ZXJuYWwvY29udHJvbGxlci9jb250cm9sbGVycy9jbHVzdGVyZGVwbG95bWVudHNfY29udHJvbGxlci5nbw==) | `71.86% <53.33%> (-0.17%)` | :arrow_down: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/openshift/assisted-service/pull/6532/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=openshift)

</details>
@jhernand: This pull request references [MGMT-17893](https://issues.redhat.com//browse/MGMT-17893) which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.17.0" version, but no target version was set.

<details>

In response to [this](https://github.com/openshift/assisted-service/pull/6532):

>Currently the procedure to detach a cluster that was created using hosts from a late binding pool is to first delete the `ManagedCluster` object, then add the `preserveOnDelete: true` field to the `ClusterDeployment` and then delete that `ClusterDeployment`. But the cluster deployment controller doesn't look at the `preserveOnDelete` field at all. As a result the hosts of the cluster are returned to the pool and they are provisioned again, which effectively destroys the cluster.
>
>This patch changes the deployment manager controller so that in that case it will check the `preserveOnDelete` field and if it is `true` it will delete the corresponding `Agent` objects. The result of that is that the hosts will go back to the pool, but marked the will still be marked as provisioned and they will not be provisioned again, thus avoiding the destruction of the cluster.
>
>Note that the `BareMetalHosts` will not be removed, but they will stay detached.
>
>Related: https://issues.redhat.com/browse/MGMT-17893
>
>
>## 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?
>
>- [x] Automation (CI, tools, etc)
>- [ ] Cloud
>- [x] Operator Managed Deployments
>- [x] None
>
>## How was this code tested?
>
>- [ ] assisted-test-infra environment
>- [ ] dev-scripts environment
>- [ ] Reviewer's test appreciated
>- [ ] Waiting for CI to do a full test run
>- [x] Manual (Elaborate on how it was tested)
>- [x] No tests needed
>
>Tested manually with MCE 2.5.4, replacing the assisted service image with a modified one containing this change.
>
>## 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
>


Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fassisted-service).  If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
</details>
/retest
[APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/openshift/assisted-service/pull/6532#pullrequestreview-2167206971" title="Approved">carbonin</a>*, *<a href="https://github.com/openshift/assisted-service/pull/6532#" title="Author self-approved">jhernand</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=openshift%2Fassisted-service).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/openshift/assisted-service/blob/master/OWNERS)~~ [carbonin,jhernand]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
@jhernand: all tests **passed!**


[Full PR test history](https://prow.ci.openshift.org/pr-history?org=openshift&repo=assisted-service&pr=6532). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:jhernand).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Fix patch:
diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go
index 33fc3d05fcc..66a57f7aac5 100644
--- a/internal/controller/controllers/clusterdeployments_controller.go
+++ b/internal/controller/controllers/clusterdeployments_controller.go
@@ -1531,7 +1531,7 @@ func (r *ClusterDeploymentsReconciler) deleteClusterInstall(ctx context.Context,
 	return buildReply(err)
 }
 
-func (r *ClusterDeploymentsReconciler) shouldDeleteAgentOnUnbind(ctx context.Context, agent aiv1beta1.Agent, clusterDeployment types.NamespacedName) bool {
+func (r *ClusterDeploymentsReconciler) shouldDeleteAgentOnUnbind(ctx context.Context, agent aiv1beta1.Agent, clusterDeployment types.NamespacedName) (bool, error) {
 	log := logutil.FromContext(ctx, r.Log).WithFields(logrus.Fields{
 		"cluster_deployment":           clusterDeployment.Name,
 		"cluster_deployment_namespace": clusterDeployment.Namespace,
@@ -1539,23 +1539,36 @@ func (r *ClusterDeploymentsReconciler) shouldDeleteAgentOnUnbind(ctx context.Con
 	infraEnvName, ok := agent.Labels[aiv1beta1.InfraEnvNameLabel]
 	if !ok {
 		log.Errorf("Failed to find infraEnv name for agent %s in namespace %s", agent.Name, agent.Namespace)
-		return false
+		return false, nil
 	}
 
 	infraEnv := &aiv1beta1.InfraEnv{}
 	if err := r.Get(ctx, types.NamespacedName{Name: infraEnvName, Namespace: agent.Namespace}, infraEnv); err != nil {
 		log.Errorf("Failed to get infraEnv %s in namespace %s", infraEnvName, agent.Namespace)
-		return false
+		return false, nil
 	}
 
 	if infraEnv.Spec.ClusterRef != nil &&
 		infraEnv.Spec.ClusterRef.Name == clusterDeployment.Name &&
 		infraEnv.Spec.ClusterRef.Namespace == clusterDeployment.Namespace {
 
-		return true
+		return true, nil
 	}
 
-	return false
+	// If the agent came from a late binding pool (the infrastructure environment has no cluster
+	// reference) then we want to delete it only if the cluster deployment is marked to preserve
+	// on delete. This is necessary because otherwise the host will go back to the pool, and it
+	// will be provisioned again, effectively destroying the cluster.
+	if infraEnv.Spec.ClusterRef == nil {
+		clusterDeploymentObject := &hivev1.ClusterDeployment{}
+		err := r.Get(ctx, clusterDeployment, clusterDeploymentObject)
+		if err != nil {
+			return false, err
+		}
+		return clusterDeploymentObject.Spec.PreserveOnDelete, nil
+	}
+
+	return false, nil
 }
 
 func (r *ClusterDeploymentsReconciler) unbindAgents(ctx context.Context, log logrus.FieldLogger, clusterDeployment types.NamespacedName) error {
@@ -1568,7 +1581,11 @@ func (r *ClusterDeploymentsReconciler) unbindAgents(ctx context.Context, log log
 		if clusterAgent.Spec.ClusterDeploymentName != nil &&
 			clusterAgent.Spec.ClusterDeploymentName.Name == clusterDeployment.Name &&
 			clusterAgent.Spec.ClusterDeploymentName.Namespace == clusterDeployment.Namespace {
-			if r.shouldDeleteAgentOnUnbind(ctx, clusterAgent, clusterDeployment) {
+			shouldDeleteAgent, err := r.shouldDeleteAgentOnUnbind(ctx, clusterAgent, clusterDeployment)
+			if err != nil {
+				return err
+			}
+			if shouldDeleteAgent {
 				log.Infof("deleting agent %s in namespace %s", clusterAgent.Name, clusterAgent.Namespace)
 				if err := r.Delete(ctx, &agents.Items[i]); err != nil {
 					log.WithError(err).Errorf("failed to delete agent %s in namespace %s", clusterAgent.Name, clusterAgent.Namespace)


Test patch:
diff --git a/internal/controller/controllers/clusterdeployments_controller_test.go b/internal/controller/controllers/clusterdeployments_controller_test.go
index 07aab004640..d3b7508dd94 100644
--- a/internal/controller/controllers/clusterdeployments_controller_test.go
+++ b/internal/controller/controllers/clusterdeployments_controller_test.go
@@ -4039,6 +4039,15 @@ var _ = Describe("unbindAgents", func() {
 		}
 		Expect(c.Create(ctx, infraEnv)).To(Succeed())
 
+		clusterDeployment := &hivev1.ClusterDeployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: clusterReference.Namespace,
+				Name:      clusterReference.Name,
+			},
+			Spec: hivev1.ClusterDeploymentSpec{},
+		}
+		Expect(c.Create(ctx, clusterDeployment)).To(Succeed())
+
 		cdName := types.NamespacedName{
 			Name:      clusterReference.Name,
 			Namespace: clusterReference.Namespace,
@@ -4080,6 +4089,41 @@ var _ = Describe("unbindAgents", func() {
 		err = c.Get(ctx, types.NamespacedName{Name: "test-agent2", Namespace: testNamespace}, agent)
 		Expect(k8serrors.IsNotFound(err)).To(BeTrue())
 	})
+
+	It("deletes late bound agents when cluster deployment is preserved", func() {
+		infraEnv := &aiv1beta1.InfraEnv{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      infraEnvName,
+				Namespace: testNamespace,
+			},
+			Spec: aiv1beta1.InfraEnvSpec{ClusterRef: nil},
+		}
+		Expect(c.Create(ctx, infraEnv)).To(Succeed())
+
+		clusterDeployment := &hivev1.ClusterDeployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: clusterReference.Namespace,
+				Name:      clusterReference.Name,
+			},
+			Spec: hivev1.ClusterDeploymentSpec{
+				PreserveOnDelete: true,
+			},
+		}
+		Expect(c.Create(ctx, clusterDeployment)).To(Succeed())
+
+		cdName := types.NamespacedName{
+			Name:      clusterReference.Name,
+			Namespace: clusterReference.Namespace,
+		}
+		Expect(cr.unbindAgents(ctx, common.GetTestLog(), cdName)).To(Succeed())
+
+		agent := &aiv1beta1.Agent{}
+		err := c.Get(ctx, types.NamespacedName{Name: "test-agent1", Namespace: testNamespace}, agent)
+		Expect(k8serrors.IsNotFound(err)).To(BeTrue())
+		agent = &aiv1beta1.Agent{}
+		err = c.Get(ctx, types.NamespacedName{Name: "test-agent2", Namespace: testNamespace}, agent)
+		Expect(k8serrors.IsNotFound(err)).To(BeTrue())
+	})
 })
 
 var _ = Describe("day2 cluster", func() {