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

Issue:
MGMT-16706: Allow in-progress clusters to be deleted in more cases
Before this commit the only in-progress clusters that could be successfully deleted were ones in states "installing" or "preparing-for-installation".

This means that if a cluster failed in, for example, "installing-pending-user-action" and a user deleted the cluster deployment the delete would hang on the finalizer handling because the controller wouldn't cancel the install and the hosts would fail to be unbound when install was in progress.

This commit aligns the states we call cancel for a cluster in the agent cluster install finalizer handling with the states that the state machine allows a cluster to be canceled in.

## List all the issues related to this PR

Resolves https://issues.redhat.com/browse/MGMT-16706

- [ ] 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?

1. Deployed a dev-scripts cluster
2. Created an infraenv with RHCOS version 4.15
3. Booted 3 hosts
4. Bound agents to a cluster install for 4.12
5. Waited for hosts to start failing due to https://issues.redhat.com/browse/MGMT-16705
6. Waited for cluster to enter `installing-pending-user-action` state
7. Delete cluster deployment

Before the fix the delete stalls on finalizer handling with logs like:

```
time="2024-02-22T21:02:39Z" level=info msg="ClusterDeployment Reconcile started" func="github.com/openshift/assisted-service/internal/controller/controllers.(*ClusterDeploymentsReconciler).Reconcile" file="/remote-source/assisted-service/app/internal/controller/controllers/clusterdeployments_controller.go:138" cluster_deployment=multi-node cluster_deployment_namespace=multi-node go-id=396 request_id=a622437a-6b03-47aa-b262-77ed127c6d7f
time="2024-02-22T21:02:39Z" level=info msg="unbind agent 30bed84e-359d-446c-9fd5-e3892326cfd2 namespace multi-node" func="github.com/openshift/assisted-service/internal/controller/controllers.(*ClusterDeploymentsReconciler).unbindAgents" file="/remote-source/assisted-service/app/internal/controller/controllers/clusterdeployments_controller.go:1573" agent_cluster_install=multi-node agent_cluster_install_namespace=multi-node clusterDeployment=multi-node cluster_deployment=multi-node cluster_deployment_namespace=multi-node go-id=396 namespace=multi-node request_id=a622437a-6b03-47aa-b262-77ed127c6d7f
time="2024-02-22T21:02:39Z" level=error msg="failed to add unbind resource 30bed84e-359d-446c-9fd5-e3892326cfd2 multi-node" func="github.com/openshift/assisted-service/internal/controller/controllers.(*ClusterDeploymentsReconciler).unbindAgents" file="/remote-source/assisted-service/app/internal/controller/controllers/clusterdeployments_controller.go:1576" agent_cluster_install=multi-node agent_cluster_install_namespace=multi-node clusterDeployment=multi-node cluster_deployment=multi-node cluster_deployment_namespace=multi-node error="admission webhook \"agentvalidators.admission.agentinstall.openshift.io\" denied the request: Attempted to change Spec.ClusterDeploymentName which is immutable during Agent installation." go-id=396 namespace=multi-node request_id=a622437a-6b03-47aa-b262-77ed127c6d7f
time="2024-02-22T21:02:39Z" level=info msg="ClusterDeployment Reconcile ended" func="github.com/openshift/assisted-service/internal/controller/controllers.(*ClusterDeploymentsReconciler).Reconcile.func1" file="/remote-source/assisted-service/app/internal/controller/controllers/clusterdeployments_controller.go:135" agent_cluster_install=multi-node agent_cluster_install_namespace=multi-node cluster_deployment=multi-node cluster_deployment_namespace=multi-node go-id=396 request_id=a622437a-6b03-47aa-b262-77ed127c6d7f
```

After the fix the delete happens without issue.

<!-- Please, select one or more if needed: -->

- [ ] 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)
- [ ] 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?



Discussion / hints:

Fix patch:
diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go
index a7ca697ab2d..33fc3d05fcc 100644
--- a/internal/controller/controllers/clusterdeployments_controller.go
+++ b/internal/controller/controllers/clusterdeployments_controller.go
@@ -283,14 +283,20 @@ func (r *ClusterDeploymentsReconciler) agentClusterInstallFinalizer(ctx context.
 			if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
 				return &ctrl.Result{Requeue: true}, err
 			}
-			if err == nil {
-				if swag.StringValue(cluster.Status) == models.ClusterStatusInstalling || swag.StringValue(cluster.Status) == models.ClusterStatusPreparingForInstallation {
-					log.Infof("ClusterInstall is being deleted, cancel installation for cluster %s", *cluster.ID)
-					if _, err = r.Installer.CancelInstallationInternal(ctx, installer.V2CancelInstallationParams{
-						ClusterID: *cluster.ID,
-					}); err != nil {
-						return &ctrl.Result{Requeue: true}, err
-					}
+			// these should match the valid source states for the cancel installation transition
+			cancelStates := []string{
+				models.ClusterStatusInstalling,
+				models.ClusterStatusPreparingForInstallation,
+				models.ClusterStatusInstallingPendingUserAction,
+				models.ClusterStatusFinalizing,
+				models.ClusterStatusError,
+			}
+			if err == nil && funk.Contains(cancelStates, swag.StringValue(cluster.Status)) {
+				log.Infof("ClusterInstall is being deleted, cancel installation for cluster %s", *cluster.ID)
+				if _, err = r.Installer.CancelInstallationInternal(ctx, installer.V2CancelInstallationParams{
+					ClusterID: *cluster.ID,
+				}); err != nil {
+					return &ctrl.Result{Requeue: true}, err
 				}
 			}
 			//Unbind agents


Test patch:
diff --git a/internal/controller/controllers/clusterdeployments_controller_test.go b/internal/controller/controllers/clusterdeployments_controller_test.go
index 551e7aa93d2..07aab004640 100644
--- a/internal/controller/controllers/clusterdeployments_controller_test.go
+++ b/internal/controller/controllers/clusterdeployments_controller_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/google/uuid"
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	common_api "github.com/openshift/assisted-service/api/common"
 	hiveext "github.com/openshift/assisted-service/api/hiveextension/v1beta1"
@@ -1224,6 +1225,31 @@ var _ = Describe("cluster reconcile", func() {
 			Expect(result).Should(Equal(ctrl.Result{}))
 		})
 
+		DescribeTable("deleting AgentClusterInstall resource cancels the installation",
+			func(clusterStatus string) {
+				backEndCluster := &common.Cluster{
+					Cluster: models.Cluster{
+						ID:     &sId,
+						Status: swag.String(models.ClusterStatusInstalling),
+					},
+				}
+				mockInstallerInternal.EXPECT().GetClusterByKubeKey(gomock.Any()).Return(backEndCluster, nil).Times(2)
+				mockInstallerInternal.EXPECT().DeregisterClusterInternal(gomock.Any(), gomock.Any()).Return(nil)
+				mockInstallerInternal.EXPECT().CancelInstallationInternal(gomock.Any(), gomock.Any()).Return(backEndCluster, nil).Times(1)
+
+				simulateACIDeletionWithFinalizer(ctx, c, aci)
+				request := newClusterDeploymentRequest(cd)
+				result, err := cr.Reconcile(ctx, request)
+				Expect(err).ShouldNot(HaveOccurred())
+				Expect(result).Should(Equal(ctrl.Result{}))
+			},
+			Entry("in installing status", models.ClusterStatusInstalling),
+			Entry("in preparing-for-installation status", models.ClusterStatusPreparingForInstallation),
+			Entry("in installing-pending-user-action status", models.ClusterStatusInstallingPendingUserAction),
+			Entry("in finalizing status", models.ClusterStatusFinalizing),
+			Entry("in error status", models.ClusterStatusError),
+		)
+
 		It("agentClusterInstall resource deleted - verify call to cancel installation", func() {
 			backEndCluster := &common.Cluster{
 				Cluster: models.Cluster{