Repository: metal3-io/baremetal-operator
Pull request #1236 (2023-03-16 16:09:25)

:sparkles: Delay delete of detached hosts based on annotation metadata
**What this PR does / why we need it**:

This adds optional metadata to the detached annotation which allows a user to delay the deletion of a host.

This is useful in a case where another controller wants the BMH detached to avoid extra operations on the host, but also wants to take advantage of deprovisioning. Such a controller would set the detached annotation deprovisioning action to "delay" and also set its own finalizer on the BMH. Then, when the BMH is marked for deletion BMO would wait until the detached annotation is removed giving the other controller time to take whatever actions are needed (setting cleaning, changing images, etc)

This replaces the previous logic which watched finalizers directly. Having a controller wait on other finalizers is not desirable as it is not backward compatible in that other controllers could be setting finalizers today and not expect this new logic. It could also lead to deadlock-like behavior if this pattern were repeated in multiple controllers watching the same resource.

cc @zaneb @dtantsur 


Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index d53586ec..e3987554 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -660,6 +660,18 @@ type RebootAnnotationArguments struct {
 	Mode RebootMode `json:"mode"`
 }
 
+type DetachedDeleteAction string
+
+const (
+	DetachedDeleteActionDelay  = "delay"
+	DetachedDeleteActionDelete = "delete"
+)
+
+type DetachedAnnotationArguments struct {
+	// DeleteAction indicates the desired delete logic when the detached annotation is present
+	DeleteAction DetachedDeleteAction `json:"deleteAction,omitempty"`
+}
+
 // Match compares the saved status information with the name and
 // content of a secret object.
 func (cs CredentialsStatus) Match(secret corev1.Secret) bool {
diff --git a/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go b/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
index 031d6294..0dcd05ba 100644
--- a/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/metal3.io/v1alpha1/zz_generated.deepcopy.go
@@ -382,6 +382,21 @@ func (in DesiredSettingsMap) DeepCopy() DesiredSettingsMap {
 	return *out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *DetachedAnnotationArguments) DeepCopyInto(out *DetachedAnnotationArguments) {
+	*out = *in
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DetachedAnnotationArguments.
+func (in *DetachedAnnotationArguments) DeepCopy() *DetachedAnnotationArguments {
+	if in == nil {
+		return nil
+	}
+	out := new(DetachedAnnotationArguments)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *Firmware) DeepCopyInto(out *Firmware) {
 	*out = *in
diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index e97317ba..518c017e 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -1,11 +1,13 @@
 package controllers
 
 import (
+	"encoding/json"
 	"fmt"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
 
+	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -181,7 +183,7 @@ func (hsm *hostStateMachine) ReconcileState(info *reconcileInfo) (actionRes acti
 		return delayedResult
 	}
 
-	if hsm.checkInitiateDelete() {
+	if hsm.checkInitiateDelete(info.log) {
 		info.log.Info("Initiating host deletion")
 		return actionComplete{}
 	}
@@ -213,7 +215,7 @@ func updateBootModeStatus(host *metal3v1alpha1.BareMetalHost) bool {
 	return true
 }
 
-func (hsm *hostStateMachine) checkInitiateDelete() bool {
+func (hsm *hostStateMachine) checkInitiateDelete(log logr.Logger) bool {
 	if hsm.Host.DeletionTimestamp.IsZero() {
 		// Delete not requested
 		return false
@@ -223,11 +225,12 @@ func (hsm *hostStateMachine) checkInitiateDelete() bool {
 	default:
 		hsm.NextState = metal3v1alpha1.StateDeleting
 	case metal3v1alpha1.StateProvisioning, metal3v1alpha1.StateProvisioned:
-		if len(hsm.Host.Finalizers) > 1 {
-			// Allow other finalizers to run before changing state
-			return false
-		}
 		if hsm.Host.OperationalStatus() == metal3v1alpha1.OperationalStatusDetached {
+			if delayDeleteForDetachedHost(hsm.Host) {
+				log.Info("Delaying detached host deletion")
+				deleteDelayedForDetached.Inc()
+				return false
+			}
 			hsm.NextState = metal3v1alpha1.StateDeleting
 		} else {
 			hsm.NextState = metal3v1alpha1.StateDeprovisioning
@@ -253,6 +256,27 @@ func hasDetachedAnnotation(host *metal3v1alpha1.BareMetalHost) bool {
 	return false
 }
 
+func delayDeleteForDetachedHost(host *metal3v1alpha1.BareMetalHost) bool {
+	annotations := host.GetAnnotations()
+	args := metal3v1alpha1.DetachedAnnotationArguments{}
+	val, present := annotations[metal3v1alpha1.DetachedAnnotation]
+
+	// if the host is detached, but missing the annotation, also delay delete
+	// to allow for the host to be re-attached
+	if !present && host.OperationalStatus() == metal3v1alpha1.OperationalStatusDetached {
+		return true
+	}
+
+	if present {
+		if err := json.Unmarshal([]byte(val), &args); err != nil {
+			// default behavior if these are missing or not json is to not delay
+			return false
+		}
+	}
+
+	return args.DeleteAction == metal3v1alpha1.DetachedDeleteActionDelay
+}
+
 func (hsm *hostStateMachine) checkDetachedHost(info *reconcileInfo) (result actionResult) {
 	// If the detached annotation is set we remove any host from the
 	// provisioner and take no further action
diff --git a/controllers/metal3.io/hostfirmwaresettings_controller.go b/controllers/metal3.io/hostfirmwaresettings_controller.go
index 70be3aa4..fcbd76ff 100644
--- a/controllers/metal3.io/hostfirmwaresettings_controller.go
+++ b/controllers/metal3.io/hostfirmwaresettings_controller.go
@@ -126,11 +126,6 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl
 		return ctrl.Result{Requeue: true, RequeueAfter: resourceNotAvailableRetryDelay}, nil
 	}
 
-	if hasDetachedAnnotation(bmh) {
-		reqLogger.Info("the host is detached, not running reconciler")
-		return ctrl.Result{Requeue: true, RequeueAfter: unmanagedRetryDelay}, nil
-	}
-
 	// Fetch the HostFirmwareSettings
 	hfs := &metal3v1alpha1.HostFirmwareSettings{}
 	info := &rInfo{log: reqLogger, hfs: hfs, bmh: bmh}
@@ -152,13 +147,7 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl
 
 	ready, err := prov.IsReady()
 	if err != nil || !ready {
-		var msg string
-		if err == nil {
-			msg = "not ready"
-		} else {
-			msg = err.Error()
-		}
-		reqLogger.Info("provisioner is not ready", "Error", msg, "RequeueAfter", provisionerRetryDelay)
+		reqLogger.Info("provisioner is not ready", "RequeueAfter:", provisionerRetryDelay)
 		return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil
 	}
 
@@ -167,7 +156,7 @@ func (r *HostFirmwareSettingsReconciler) Reconcile(ctx context.Context, req ctrl
 	// Get the current settings and schema, retry if provisioner returns error
 	currentSettings, schema, err := prov.GetFirmwareSettings(true)
 	if err != nil {
-		reqLogger.Info("provisioner returns error", "Error", err.Error(), "RequeueAfter", provisionerRetryDelay)
+		reqLogger.Info("provisioner returns error", "RequeueAfter:", provisionerRetryDelay)
 		return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil
 	}
 
diff --git a/controllers/metal3.io/metrics.go b/controllers/metal3.io/metrics.go
index b9d02197..66611d7f 100644
--- a/controllers/metal3.io/metrics.go
+++ b/controllers/metal3.io/metrics.go
@@ -123,6 +123,11 @@ var provisionerNotReady = prometheus.NewCounter(prometheus.CounterOpts{
 	Help: "Number of times a host is not provision ready",
 })
 
+var deleteDelayedForDetached = prometheus.NewCounter(prometheus.CounterOpts{
+	Name: "metal3_delete_delayed_for_detached_total",
+	Help: "Number of times a host delete action was delayed due to the detached annotation",
+})
+
 func init() {
 	metrics.Registry.MustRegister(
 		reconcileCounters,
@@ -149,7 +154,8 @@ func init() {
 		hostRegistrationRequired,
 		hostUnmanaged,
 		deleteWithoutDeprov,
-		provisionerNotReady)
+		provisionerNotReady,
+		deleteDelayedForDetached)
 }
 
 func hostMetricLabels(request ctrl.Request) prometheus.Labels {


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 1038bd9c..9a700a21 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -1075,43 +1075,59 @@ func TestErrorClean(t *testing.T) {
 	}
 }
 
-func TestDeleteWaitsForFinalizers(t *testing.T) {
+func TestDeleteWaitsForDetach(t *testing.T) {
 	tests := []struct {
-		Scenario      string
-		Host          *metal3v1alpha1.BareMetalHost
-		ExpectedState metal3v1alpha1.ProvisioningState
+		Scenario                  string
+		Host                      *metal3v1alpha1.BareMetalHost
+		ExpectedState             metal3v1alpha1.ProvisioningState
+		ExpectedOperationalStatus metal3v1alpha1.OperationalStatus
 	}{
 		{
-			Scenario: "provisioning-2-finalizers",
-			Host: host(metal3v1alpha1.StateProvisioning).
+			Scenario: "detached-delay",
+			Host: host(metal3v1alpha1.StateProvisioned).
+				SetOperationalStatus(metal3v1alpha1.OperationalStatusDetached).
 				setDeletion().
-				setFinalizers([]string{metal3v1alpha1.BareMetalHostFinalizer, "other-finalizer"}).
+				setDetached("{\"deleteAction\": \"delay\"}").
 				build(),
-			ExpectedState: metal3v1alpha1.StateProvisioned,
+			ExpectedState:             metal3v1alpha1.StateProvisioned,
+			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusDetached,
 		},
 		{
-			Scenario: "provisioned-2-finalizers",
+			Scenario: "detached-delete",
 			Host: host(metal3v1alpha1.StateProvisioned).
+				SetOperationalStatus(metal3v1alpha1.OperationalStatusDetached).
 				setDeletion().
-				setFinalizers([]string{metal3v1alpha1.BareMetalHostFinalizer, "other-finalizer"}).
+				setDetached("{\"deleteAction\": \"delete\"}").
 				build(),
-			ExpectedState: metal3v1alpha1.StateProvisioned,
+			ExpectedState:             metal3v1alpha1.StateDeleting,
+			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusDetached,
 		},
 		{
-			Scenario: "provisioning-1-finalizer",
-			Host: host(metal3v1alpha1.StateProvisioning).
+			Scenario: "detached-not-json",
+			Host: host(metal3v1alpha1.StateProvisioned).
+				SetOperationalStatus(metal3v1alpha1.OperationalStatusDetached).
 				setDeletion().
-				setFinalizers([]string{metal3v1alpha1.BareMetalHostFinalizer}).
+				setDetached("true").
 				build(),
-			ExpectedState: metal3v1alpha1.StateDeprovisioning,
+			ExpectedState:             metal3v1alpha1.StateDeleting,
+			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusDetached,
 		},
 		{
-			Scenario: "provisioned-1-finalizer",
+			Scenario: "detached-no-annotation",
 			Host: host(metal3v1alpha1.StateProvisioned).
+				SetOperationalStatus(metal3v1alpha1.OperationalStatusDetached).
 				setDeletion().
-				setFinalizers([]string{metal3v1alpha1.BareMetalHostFinalizer}).
 				build(),
-			ExpectedState: metal3v1alpha1.StateDeprovisioning,
+			ExpectedState:             metal3v1alpha1.StateProvisioned,
+			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusOK,
+		},
+		{
+			Scenario: "attached",
+			Host: host(metal3v1alpha1.StateProvisioned).
+				setDeletion().
+				build(),
+			ExpectedState:             metal3v1alpha1.StateDeprovisioning,
+			ExpectedOperationalStatus: metal3v1alpha1.OperationalStatusOK,
 		},
 	}
 	for _, tt := range tests {
@@ -1125,6 +1141,7 @@ func TestDeleteWaitsForFinalizers(t *testing.T) {
 			hsm.ReconcileState(info)
 
 			assert.Equal(t, tt.ExpectedState, tt.Host.Status.Provisioning.State)
+			assert.Equal(t, tt.ExpectedOperationalStatus, tt.Host.Status.OperationalStatus)
 		})
 	}
 }
@@ -1248,8 +1265,11 @@ func (hb *hostBuilder) setDeletion() *hostBuilder {
 	return hb
 }
 
-func (hb *hostBuilder) setFinalizers(finalizers []string) *hostBuilder {
-	hb.Finalizers = finalizers
+func (hb *hostBuilder) setDetached(val string) *hostBuilder {
+	if hb.Annotations == nil {
+		hb.Annotations = make(map[string]string, 1)
+	}
+	hb.Annotations[metal3v1alpha1.DetachedAnnotation] = val
 	return hb
 }