Repository: metal3-io/baremetal-operator (go)

Issue:
Retry when node in "Adopt Failed" state
Currently, we are not retrying when ironic fails to adopt and puts the node in 'adopt failed' state:  https://github.com/metal3-io/baremetal-operator/blob/master/pkg/provisioner/ironic/ironic.go#L1001

As [checked](https://docs.openstack.org/ironic/latest/_images/states.svg), we could try to retry to get a node out of "adopt failed" by:

-  sending manage command(via API) ==> manageable
- sending adopt command (via API) ==> adopting(retry adoption)




Pull request description:
Force retry when adoption fails
Currently when adoption fails, we use RegistrationError but this gets
cleared any time registration succeeds. We need a separate signal for
adoption failure, which allows us to force retry the adoption again.

Fixes #697

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index aa63aefb..b4ba38f1 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -123,6 +123,9 @@ const (
 type ErrorType string
 
 const (
+	// ProvisionedRegistrationError is an error condition occurring when the controller
+	// is unable to re-register an already provisioned host.
+	ProvisionedRegistrationError ErrorType = "provisioned registration error"
 	// RegistrationError is an error condition occurring when the
 	// controller is unable to connect to the Host's baseboard management
 	// controller.
@@ -522,7 +525,7 @@ type BareMetalHostStatus struct {
 
 	// ErrorType indicates the type of failure encountered when the
 	// OperationalStatus is OperationalStatusError
-	// +kubebuilder:validation:Enum=registration error;inspection error;provisioning error;power management error
+	// +kubebuilder:validation:Enum=provisioned registration error;registration error;inspection error;provisioning error;power management error
 	ErrorType ErrorType `json:"errorType,omitempty"`
 
 	// LastUpdated identifies when this status was last observed.
diff --git a/config/crd/bases/metal3.io_baremetalhosts.yaml b/config/crd/bases/metal3.io_baremetalhosts.yaml
index c957b003..c99bc03b 100644
--- a/config/crd/bases/metal3.io_baremetalhosts.yaml
+++ b/config/crd/bases/metal3.io_baremetalhosts.yaml
@@ -262,6 +262,7 @@ spec:
               errorType:
                 description: ErrorType indicates the type of failure encountered when the OperationalStatus is OperationalStatusError
                 enum:
+                - provisioned registration error
                 - registration error
                 - inspection error
                 - provisioning error
diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml
index 88532073..92190fca 100644
--- a/config/render/capm3.yaml
+++ b/config/render/capm3.yaml
@@ -260,6 +260,7 @@ spec:
               errorType:
                 description: ErrorType indicates the type of failure encountered when the OperationalStatus is OperationalStatusError
                 enum:
+                - provisioned registration error
                 - registration error
                 - inspection error
                 - provisioning error
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index d940abbf..d5967ce2 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -280,10 +280,11 @@ func recordActionFailure(info *reconcileInfo, errorType metal3v1alpha1.ErrorType
 	setErrorMessage(info.host, errorType, errorMessage)
 
 	eventType := map[metal3v1alpha1.ErrorType]string{
-		metal3v1alpha1.RegistrationError:    "RegistrationError",
-		metal3v1alpha1.InspectionError:      "InspectionError",
-		metal3v1alpha1.ProvisioningError:    "ProvisioningError",
-		metal3v1alpha1.PowerManagementError: "PowerManagementError",
+		metal3v1alpha1.ProvisionedRegistrationError: "ProvisionedRegistrationError",
+		metal3v1alpha1.RegistrationError:            "RegistrationError",
+		metal3v1alpha1.InspectionError:              "InspectionError",
+		metal3v1alpha1.ProvisioningError:            "ProvisioningError",
+		metal3v1alpha1.PowerManagementError:         "PowerManagementError",
 	}[errorType]
 
 	counter := actionFailureCounters.WithLabelValues(eventType)
@@ -467,7 +468,8 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 	// Reaching this point means the credentials are valid and worked,
 	// so clear any previous error and record the success in the
 	// status block.
-	if !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret) {
+	registeredNewCreds := !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret)
+	if registeredNewCreds {
 		info.log.Info("updating credentials success status fields")
 		info.host.UpdateGoodCredentials(*info.bmcCredsSecret)
 		info.publishEvent("BMCAccessValidated", "Verified access to BMC")
@@ -475,9 +477,10 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 	} else {
 		info.log.Info("verified access to the BMC")
 	}
-	if clearError(info.host) {
+
+	if info.host.Status.ErrorType == metal3v1alpha1.RegistrationError || registeredNewCreds {
 		info.log.Info("clearing previous error message")
-		dirty = true
+		dirty = clearError(info.host)
 	}
 
 	if dirty {
@@ -615,12 +618,12 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 	if info.host.Status.Provisioning.Image.URL != "" {
 		// Adopt the host in case it has been re-registered during the
 		// deprovisioning process before it completed
-		provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
+		provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.ProvisionedRegistrationError)
 		if err != nil {
 			return actionError{err}
 		}
 		if provResult.ErrorMessage != "" {
-			return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
+			return recordActionFailure(info, metal3v1alpha1.ProvisionedRegistrationError, provResult.ErrorMessage)
 		}
 		if provResult.Dirty {
 			result := actionContinue{provResult.RequeueAfter}
@@ -758,12 +761,12 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 // action. We use the Adopt() API to make sure that the provisioner is aware of
 // the provisioning details. Then we monitor its power status.
 func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
-	provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
+	provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.ProvisionedRegistrationError)
 	if err != nil {
 		return actionError{err}
 	}
 	if provResult.ErrorMessage != "" {
-		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
+		return recordActionFailure(info, metal3v1alpha1.ProvisionedRegistrationError, provResult.ErrorMessage)
 	}
 	if provResult.Dirty {
 		result := actionContinue{provResult.RequeueAfter}


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 51c1a971..58d9a3d2 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -289,6 +289,59 @@ func TestErrorCountCleared(t *testing.T) {
 	}
 }
 
+func TestErrorClean(t *testing.T) {
+
+	tests := []struct {
+		Scenario    string
+		Host        *metal3v1alpha1.BareMetalHost
+		SecretName  string
+		ExpectError bool
+	}{
+		{
+			Scenario: "clean-after-registration-error",
+			Host: host(metal3v1alpha1.StateInspecting).
+				SetStatusError(metal3v1alpha1.OperationalStatusError, metal3v1alpha1.RegistrationError, "some error", 1).
+				build(),
+		},
+		{
+			Scenario: "not-clean-after-provisioned-registration-error",
+			Host: host(metal3v1alpha1.StateInspecting).
+				SetStatusError(metal3v1alpha1.OperationalStatusError, metal3v1alpha1.ProvisionedRegistrationError, "some error", 1).
+				build(),
+			ExpectError: true,
+		},
+		{
+			Scenario: "clean-after-creds-change",
+			Host: host(metal3v1alpha1.StateReady).
+				SetStatusError(metal3v1alpha1.OperationalStatusError, metal3v1alpha1.InspectionError, "some error", 1).
+				build(),
+			SecretName: "NewCreds",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.Scenario, func(t *testing.T) {
+			prov := &mockProvisioner{}
+			hsm := newHostStateMachine(tt.Host, &BareMetalHostReconciler{}, prov, true)
+
+			info := makeDefaultReconcileInfo(tt.Host)
+			if tt.SecretName != "" {
+				info.bmcCredsSecret.Name = tt.SecretName
+			}
+
+			hsm.ReconcileState(info)
+
+			if tt.ExpectError {
+				assert.Equal(t, tt.Host.Status.ErrorType, v1alpha1.ProvisionedRegistrationError)
+				assert.NotEmpty(t, tt.Host.Status.ErrorMessage)
+			} else {
+				assert.Equal(t, tt.Host.Status.OperationalStatus, v1alpha1.OperationalStatusOK)
+				assert.Empty(t, tt.Host.Status.ErrorType)
+				assert.Empty(t, tt.Host.Status.ErrorMessage)
+			}
+		})
+	}
+}
+
 type hostBuilder struct {
 	metal3v1alpha1.BareMetalHost
 }
@@ -334,6 +387,15 @@ func (hb *hostBuilder) SetImageURL(url string) *hostBuilder {
 	return hb
 }
 
+func (hb *hostBuilder) SetStatusError(opStatus metal3v1alpha1.OperationalStatus, errType metal3v1alpha1.ErrorType, errMsg string, errCount int) *hostBuilder {
+	hb.Status.OperationalStatus = opStatus
+	hb.Status.ErrorType = errType
+	hb.Status.ErrorMessage = errMsg
+	hb.Status.ErrorCount = errCount
+
+	return hb
+}
+
 func makeDefaultReconcileInfo(host *metal3v1alpha1.BareMetalHost) *reconcileInfo {
 	return &reconcileInfo{
 		log:     logf.Log.WithName("controllers").WithName("BareMetalHost").WithName("host_state_machine"),