Repository: metal3-io/baremetal-operator
Pull request #1054 (2021-12-14 11:50:01)

Do not add secret finalizer while deleting a host
Fixes https://github.com/metal3-io/baremetal-operator/issues/1053

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **andfasano** after the PR has been reviewed.

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

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

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

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":["andfasano"]} -->
s3rj1k: /lgtm
s3rj1k: @andfasano nice catch, thanks!
andfasano: > @andfasano nice catch, thanks!

Thanks. Unfortunately it happens intermittently, so it took some time to nail it down. I also tested it functionally (manually) and it seems ok.
andfasano: /test-v1b1-centos-integration
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/1054#pullrequestreview-831795077" title="Approved">bfournie</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **andfasano** after the PR has been reviewed.

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

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

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

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":["andfasano"]} -->

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 8eb5b399..2cbd9163 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -1482,7 +1482,7 @@ func (r *BareMetalHostReconciler) getBMCSecretAndSetOwner(request ctrl.Request,
 	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
 	secretManager := r.secretManager(reqLogger)
 
-	bmcCredsSecret, err := secretManager.AcquireSecret(host.CredentialsKey(), host, true, true)
+	bmcCredsSecret, err := secretManager.AcquireSecret(host.CredentialsKey(), host, true, host.Status.Provisioning.State != metal3v1alpha1.StateDeleting)
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
 			return nil, &ResolveBMCSecretRefError{message: fmt.Sprintf("The BMC secret %s does not exist", host.CredentialsKey())}


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index f1021066..e5633a90 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -455,6 +455,48 @@ func TestAddFinalizers(t *testing.T) {
 	)
 }
 
+// TestDoNotAddSecretFinalizersDuringDelete verifies that during a host deletion,
+// in case the removal of a secret finalizer triggers an immediate reconcile loop,
+// then the secret finalizer is not added again
+func TestDoNotAddSecretFinalizersDuringDelete(t *testing.T) {
+	host := newDefaultHost(t)
+	r := newTestReconciler(host)
+
+	// Let the host reach the available state before deleting it
+	waitForProvisioningState(t, r, host, metal3v1alpha1.StateAvailable)
+	doDeleteHost(host, r)
+	waitForProvisioningState(t, r, host, metal3v1alpha1.StateDeleting)
+
+	// The next reconcile loop will start the delete process,
+	// and as a first step the Ironic node will be removed
+	request := newRequest(host)
+	_, err := r.Reconcile(context.Background(), request)
+	assert.NoError(t, err)
+
+	// The next reconcile loop remove the finalizers from
+	// both the host and the secret.
+	// The fake client will immediately remove the host
+	// from its cache, so let's keep the latest updated
+	// host
+	r.Get(goctx.TODO(), request.NamespacedName, host)
+	_, err = r.Reconcile(context.Background(), request)
+	assert.NoError(t, err)
+
+	// To simulate an immediate reconciliation loop due the
+	// secret update (and a slow host deletion), let's push
+	// back the host in the client cache.
+	host.ResourceVersion = ""
+	r.Client.Create(context.TODO(), host)
+	previousSecret := getHostSecret(t, r, host)
+	_, err = r.Reconcile(context.Background(), request)
+	assert.NoError(t, err)
+
+	// Secret must remain unchanged
+	actualSecret := getHostSecret(t, r, host)
+	assert.Empty(t, actualSecret.Finalizers)
+	assert.Equal(t, previousSecret.ResourceVersion, actualSecret.ResourceVersion)
+}
+
 // TestSetLastUpdated ensures that the lastUpdated timestamp in the
 // status is set to a non-zero value during reconciliation.
 func TestSetLastUpdated(t *testing.T) {