Repository: metal3-io/baremetal-operator
Pull request #716 (2020-11-12 20:09:46)

Handle re-registration in all states
Ensure that regardless of the current provisioning state, we always re-register the Node in ironic if the ironic database has been lost.
Ensure that if this occurs during deprovisioning, the Node is re-adopted so that we commence deprovisioning again, so that the Host is always left in a clean state.

This also changes the steady state that we use for Ready hosts in Ironic. Previously we would try to maintain the ironic state as `manageable`. Now we maintain it in `available` if possible. This means that we don't end up doing a second automated cleaning on provisioning if we have just done an automated cleaning on deprovisioning.


Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index f6296924..df6ff693 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -849,27 +849,31 @@ func (host *BareMetalHost) OperationMetricForState(operation ProvisioningState)
 
 // GetImageChecksum returns the hash value and its algo.
 func (host *BareMetalHost) GetImageChecksum() (string, string, bool) {
-	if host.Spec.Image == nil {
-		return "", "", false
-	}
+	return host.Spec.Image.GetChecksum()
+}
 
-	checksum := host.Spec.Image.Checksum
-	checksumType := host.Spec.Image.ChecksumType
+func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
+	if image == nil {
+		return
+	}
 
-	if checksum == "" {
+	if image.Checksum == "" {
 		// Return empty if checksum is not provided
-		return "", "", false
+		return
 	}
-	if checksumType == "" {
-		// If only checksum is specified. Assume type is md5
-		return checksum, string(MD5), true
-	}
-	switch checksumType {
+
+	switch image.ChecksumType {
+	case "":
+		checksumType = string(MD5)
 	case MD5, SHA256, SHA512:
-		return checksum, string(checksumType), true
+		checksumType = string(image.ChecksumType)
 	default:
-		return "", "", false
+		return
 	}
+
+	checksum = image.Checksum
+	ok = true
+	return
 }
 
 // +kubebuilder:object:root=true
diff --git a/controllers/metal3.io/action_result.go b/controllers/metal3.io/action_result.go
index 11cde63f..5f3a9717 100644
--- a/controllers/metal3.io/action_result.go
+++ b/controllers/metal3.io/action_result.go
@@ -1,6 +1,7 @@
 package controllers
 
 import (
+	"errors"
 	"math"
 	"math/rand"
 	"time"
@@ -8,6 +9,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metal3 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
 )
 
 const maxBackOffCount = 10
@@ -96,6 +98,10 @@ func (r actionError) Dirty() bool {
 	return false
 }
 
+func (r actionError) NeedsRegistration() bool {
+	return errors.Is(r.err, provisioner.NeedsRegistration)
+}
+
 // actionFailed is a result indicating that the current action has failed,
 // and that the resource should be marked as in error.
 type actionFailed struct {
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 5aab69c3..dfc24360 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -181,6 +181,7 @@ func (r *BareMetalHostReconciler) Reconcile(request ctrl.Request) (result ctrl.R
 	// management controller.
 	var bmcCreds *bmc.Credentials
 	var bmcCredsSecret *corev1.Secret
+	haveCreds := false
 	switch host.Status.Provisioning.State {
 	case metal3v1alpha1.StateNone, metal3v1alpha1.StateUnmanaged:
 		bmcCreds = &bmc.Credentials{}
@@ -194,6 +195,8 @@ func (r *BareMetalHostReconciler) Reconcile(request ctrl.Request) (result ctrl.R
 			} else {
 				return r.credentialsErrorResult(err, request, host)
 			}
+		} else {
+			haveCreds = true
 		}
 	}
 
@@ -219,7 +222,7 @@ func (r *BareMetalHostReconciler) Reconcile(request ctrl.Request) (result ctrl.R
 		return ctrl.Result{Requeue: true, RequeueAfter: provisionerNotReadyRetryDelay}, nil
 	}
 
-	stateMachine := newHostStateMachine(host, r, prov)
+	stateMachine := newHostStateMachine(host, r, prov, haveCreds)
 	actResult := stateMachine.ReconcileState(info)
 	result, err = actResult.Result()
 
@@ -428,15 +431,13 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner
 	// so clear any previous error and record the success in the
 	// status block.
 	info.log.Info("updating credentials success status fields")
+	registeredNewCreds := !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret)
 	info.host.UpdateGoodCredentials(*info.bmcCredsSecret)
 	info.log.Info("clearing previous error message")
 	info.host.ClearError()
 
-	info.publishEvent("BMCAccessValidated", "Verified access to BMC")
-
-	if info.host.Spec.ExternallyProvisioned {
-		info.publishEvent("ExternallyProvisioned",
-			"Registered host that was externally provisioned")
+	if registeredNewCreds {
+		info.publishEvent("BMCAccessValidated", "Verified access to BMC")
 	}
 
 	return actionComplete{}
@@ -562,9 +563,23 @@ func clearHostProvisioningSettings(host *metal3v1alpha1.BareMetalHost) {
 }
 
 func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
+	// 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)
+	if err != nil {
+		return actionError{err}
+	}
+	if provResult.ErrorMessage != "" {
+		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
+	}
+	if provResult.Dirty {
+		info.host.ClearError()
+		return actionContinue{provResult.RequeueAfter}
+	}
+
 	info.log.Info("deprovisioning")
 
-	provResult, err := prov.Deprovision()
+	provResult, err = prov.Deprovision()
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to deprovision")}
 	}
@@ -679,14 +694,12 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	return steadyStateResult
 }
 
-// A host reaching this action handler should be provisioned or
-// externally provisioned -- a state that it will stay in until the
-// user takes further action. Both of those states mean that it has
-// been registered with the provisioner once, so we use the Adopt()
-// API to ensure that is still true. Then we monitor its power status.
+// A host reaching this action handler should be provisioned or externally
+// provisioned -- a state that it will stay in until the user takes further
+// 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()
+	provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
 	if err != nil {
 		return actionError{err}
 	}
@@ -702,28 +715,10 @@ func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provi
 }
 
 // A host reaching this action handler should be ready -- a state that
-// it will stay in until the user takes further action. It has been
-// registered with the provisioner once, so we use
-// ValidateManagementAccess() to ensure that is still true. We don't
+// it will stay in until the user takes further action. We don't
 // use Adopt() because we don't want Ironic to treat the host as
 // having been provisioned. Then we monitor its power status.
 func (r *BareMetalHostReconciler) actionManageReady(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
-
-	// We always pass false for credentialsChanged because if they had
-	// changed we would have ended up in actionRegister() instead of
-	// here.
-	provResult, err := prov.ValidateManagementAccess(false)
-	if err != nil {
-		return actionError{err}
-	}
-	if provResult.ErrorMessage != "" {
-		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
-	}
-	if provResult.Dirty {
-		info.host.ClearError()
-		return actionContinue{provResult.RequeueAfter}
-	}
-
 	if info.host.NeedsProvisioning() {
 		// Ensure the provisioning settings we're going to use are stored.
 		dirty, err := saveHostProvisioningSettings(info.host)
diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index 3427fa47..1835ca47 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -16,17 +16,20 @@ type hostStateMachine struct {
 	NextState   metal3v1alpha1.ProvisioningState
 	Reconciler  *BareMetalHostReconciler
 	Provisioner provisioner.Provisioner
+	haveCreds   bool
 }
 
 func newHostStateMachine(host *metal3v1alpha1.BareMetalHost,
 	reconciler *BareMetalHostReconciler,
-	provisioner provisioner.Provisioner) *hostStateMachine {
+	provisioner provisioner.Provisioner,
+	haveCreds bool) *hostStateMachine {
 	currentState := host.Status.Provisioning.State
 	r := hostStateMachine{
 		Host:        host,
 		NextState:   currentState, // Remain in current state by default
 		Reconciler:  reconciler,
 		Provisioner: provisioner,
+		haveCreds:   haveCreds,
 	}
 	return &r
 }
@@ -62,7 +65,7 @@ func recordStateBegin(host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.P
 
 func recordStateEnd(info *reconcileInfo, host *metal3v1alpha1.BareMetalHost, state metal3v1alpha1.ProvisioningState, time metav1.Time) {
 	if prevMetric := host.OperationMetricForState(state); prevMetric != nil {
-		if !prevMetric.Start.IsZero() {
+		if !prevMetric.Start.IsZero() && prevMetric.End.IsZero() {
 			prevMetric.End = time
 			info.postSaveCallbacks = append(info.postSaveCallbacks, func() {
 				observer := stateTime[state].With(hostMetricLabels(info.request))
@@ -160,40 +163,48 @@ func (hsm *hostStateMachine) checkInitiateDelete() bool {
 }
 
 func (hsm *hostStateMachine) ensureRegistered(info *reconcileInfo) (result actionResult) {
+	if !hsm.haveCreds {
+		// If we are in the process of deletion (which may start with
+		// deprovisioning) and we have been unable to obtain any credentials,
+		// don't attempt to re-register the Host as this will always fail.
+		return
+	}
+
+	needsReregister := false
+
 	switch hsm.NextState {
 	case metal3v1alpha1.StateNone, metal3v1alpha1.StateUnmanaged:
 		// We haven't yet reached the Registration state, so don't attempt
 		// to register the Host.
 		return
-	}
-	if !hsm.Host.DeletionTimestamp.IsZero() {
-		// BUG(zaneb) We currently don't attempt to re-register the Host
-		// if we find it missing once a delete has been requested (in
-		// part because we are also willing to pass empty credentials
-		// after this point), but this means that we may not be able to
-		// deprovision a Host if the Ironic database pod is rescheduled
-		// during deprovisioning, or if the credentials need to be
-		// modified.
-		return
-	}
-
-	if hsm.Host.Status.GoodCredentials.Match(*info.bmcCredsSecret) {
-		// Credentials are unchanged since we verified them.
+	case metal3v1alpha1.StateDeleting:
+		// In the deleting state the whole idea is to de-register the host
 		return
-	}
-
-	recordStateBegin(hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
-	if hsm.Host.Status.ErrorType == metal3v1alpha1.RegistrationError {
-		if hsm.Host.Status.TriedCredentials.Match(*info.bmcCredsSecret) {
-			// Already tried with these credentials; no point retrying
-			info.log.Info("Unmodified credentials; not retrying")
-			return actionFailed{ErrorType: metal3v1alpha1.RegistrationError}
+	case metal3v1alpha1.StateRegistering:
+	default:
+		needsReregister = (hsm.Host.Status.ErrorType == metal3v1alpha1.RegistrationError ||
+			!hsm.Host.Status.GoodCredentials.Match(*info.bmcCredsSecret))
+		if needsReregister {
+			info.log.Info("Retrying registration")
+			recordStateBegin(hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
 		}
-		info.log.Info("Modified credentials detected; will retry registration")
 	}
+
 	result = hsm.Reconciler.actionRegistering(hsm.Provisioner, info)
-	if _, complete := result.(actionComplete); complete && hsm.Host.Status.Provisioning.State != metal3v1alpha1.StateRegistering {
-		recordStateEnd(info, hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
+	if _, complete := result.(actionComplete); complete {
+		if hsm.NextState != metal3v1alpha1.StateRegistering {
+			recordStateEnd(info, hsm.Host, metal3v1alpha1.StateRegistering, metav1.Now())
+		}
+		if needsReregister {
+			// Host was re-registered, so requeue and run the state machine on
+			// the next reconcile
+			result = actionContinue{}
+		} else {
+			// Allow the state machine to run, either because we were just
+			// reconfirming an existing registration, or because we are in the
+			// Registering state
+			result = nil
+		}
 	}
 	return
 }
@@ -224,7 +235,6 @@ func (hsm *hostStateMachine) handleRegistering(info *reconcileInfo) actionResult
 	// registered using the current BMC credentials, so we can move to the
 	// next state. We will not return to the Registering state, even
 	// if the credentials change and the Host must be re-registered.
-	hsm.Host.ClearError()
 	if hsm.Host.Spec.ExternallyProvisioned {
 		hsm.NextState = metal3v1alpha1.StateExternallyProvisioned
 	} else {
@@ -323,23 +333,34 @@ func (hsm *hostStateMachine) handleProvisioned(info *reconcileInfo) actionResult
 func (hsm *hostStateMachine) handleDeprovisioning(info *reconcileInfo) actionResult {
 	actResult := hsm.Reconciler.actionDeprovisioning(hsm.Provisioner, info)
 
-	switch actResult.(type) {
-	case actionComplete:
-		if !hsm.Host.DeletionTimestamp.IsZero() {
-			hsm.NextState = metal3v1alpha1.StateDeleting
-		} else {
+	if hsm.Host.DeletionTimestamp.IsZero() {
+		if _, complete := actResult.(actionComplete); complete {
 			hsm.NextState = metal3v1alpha1.StateReady
 		}
-	case actionFailed:
-		if !hsm.Host.DeletionTimestamp.IsZero() {
+	} else {
+		skipToDelete := func() actionResult {
+			hsm.NextState = metal3v1alpha1.StateDeleting
+			info.postSaveCallbacks = append(info.postSaveCallbacks, deleteWithoutDeprov.Inc)
+			return actionComplete{}
+		}
+
+		switch r := actResult.(type) {
+		case actionComplete:
+			hsm.NextState = metal3v1alpha1.StateDeleting
+		case actionFailed:
 			// If the provisioner gives up deprovisioning and
 			// deletion has been requested, continue to delete.
 			// Note that this is entirely theoretical, as the
 			// Ironic provisioner currently never gives up
 			// trying to deprovision.
-			hsm.NextState = metal3v1alpha1.StateDeleting
-			info.postSaveCallbacks = append(info.postSaveCallbacks, deleteWithoutDeprov.Inc)
-			actResult = actionComplete{}
+			return skipToDelete()
+		case actionError:
+			if r.NeedsRegistration() && !hsm.haveCreds {
+				// If the host is not registered as a node in Ironic and we
+				// lack the credentials to deprovision it, just continue to
+				// delete.
+				return skipToDelete()
+			}
 		}
 	}
 	return actResult
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index 137413b0..f118dc27 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -183,7 +183,7 @@ func (p *demoProvisioner) UpdateHardwareState() (result provisioner.Result, err
 }
 
 // Adopt allows an externally-provisioned server to be adopted.
-func (p *demoProvisioner) Adopt() (result provisioner.Result, err error) {
+func (p *demoProvisioner) Adopt(force bool) (result provisioner.Result, err error) {
 	p.log.Info("adopting host")
 	result.Dirty = false
 	return
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 86bc04b7..bcd44458 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -170,7 +170,7 @@ func (p *fixtureProvisioner) UpdateHardwareState() (result provisioner.Result, e
 }
 
 // Adopt allows an externally-provisioned server to be adopted.
-func (p *fixtureProvisioner) Adopt() (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) Adopt(force bool) (result provisioner.Result, err error) {
 	p.log.Info("adopting host")
 	if p.host.Spec.ExternallyProvisioned && !p.adopted {
 		p.adopted = true
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 2446675a..14a2a159 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -346,11 +346,23 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 			}
 		}
 
-		checksum, checksumType, ok := p.host.GetImageChecksum()
+		// If there is an image to be provisioned, or an image has
+		// previously been provisioned, include those details. Either
+		// case may mean we are re-adopting a host that was already
+		// known but removed/lost because the pod restarted.
+		var imageData *metal3v1alpha1.Image
+		switch {
+		case p.host.Status.Provisioning.Image.URL != "":
+			imageData = &p.host.Status.Provisioning.Image
+		case p.host.Spec.Image != nil && p.host.Spec.Image.URL != "":
+			imageData = p.host.Spec.Image
+		}
+
+		checksum, checksumType, ok := imageData.GetChecksum()
 
 		if ok {
 			p.log.Info("setting instance info",
-				"image_source", p.host.Spec.Image.URL,
+				"image_source", imageData.URL,
 				"image_os_hash_value", checksum,
 				"image_os_hash_algo", checksumType,
 			)
@@ -359,7 +371,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 				nodes.UpdateOperation{
 					Op:    nodes.AddOp,
 					Path:  "/instance_info/image_source",
-					Value: p.host.Spec.Image.URL,
+					Value: imageData.URL,
 				},
 				nodes.UpdateOperation{
 					Op:    nodes.AddOp,
@@ -398,11 +410,11 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 				)
 			}
 
-			if p.host.Spec.Image.DiskFormat != nil {
+			if imageData.DiskFormat != nil {
 				updates = append(updates, nodes.UpdateOperation{
 					Op:    nodes.AddOp,
 					Path:  "/instance_info/image_disk_format",
-					Value: *p.host.Spec.Image.DiskFormat,
+					Value: *imageData.DiskFormat,
 				})
 			}
 			_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
@@ -560,7 +572,7 @@ func (p *ironicProvisioner) InspectHardware() (result provisioner.Result, detail
 		return
 	}
 	if ironicNode == nil {
-		return result, nil, fmt.Errorf("no ironic node for host")
+		return result, nil, provisioner.NeedsRegistration
 	}
 
 	status, err := introspection.GetIntrospectionStatus(p.inspector, ironicNode.UUID).Extract()
@@ -650,7 +662,7 @@ func (p *ironicProvisioner) UpdateHardwareState() (result provisioner.Result, er
 		return result, errors.Wrap(err, "failed to find existing host")
 	}
 	if ironicNode == nil {
-		return result, fmt.Errorf("no ironic node for host")
+		return result, provisioner.NeedsRegistration
 	}
 
 	var discoveredVal bool
@@ -981,18 +993,11 @@ func (p *ironicProvisioner) startProvisioning(ironicNode *nodes.Node, hostConf p
 	)
 	p.publisher("ProvisioningStarted",
 		fmt.Sprintf("Image provisioning started for %s", p.host.Spec.Image.URL))
-
-	var opts nodes.ProvisionStateOpts
-	if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.DeployFail {
-		opts = nodes.ProvisionStateOpts{Target: nodes.TargetActive}
-	} else {
-		opts = nodes.ProvisionStateOpts{Target: nodes.TargetProvide}
-	}
-	return p.changeNodeProvisionState(ironicNode, opts)
+	return
 }
 
 // Adopt allows an externally-provisioned server to be adopted by Ironic.
-func (p *ironicProvisioner) Adopt() (result provisioner.Result, err error) {
+func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err error) {
 	var ironicNode *nodes.Node
 
 	if ironicNode, err = p.findExistingHost(); err != nil {
@@ -1000,18 +1005,12 @@ func (p *ironicProvisioner) Adopt() (result provisioner.Result, err error) {
 		return
 	}
 	if ironicNode == nil {
-		// The node does not exist, but we were called so the
-		// controller thinks that the node existed at one time. That
-		// likely means data loss from restarting the database, so
-		// pass through the validation process to register the node
-		// again. Pass true to indicate that we need to re-test the
-		// credentials, just in case.
-		p.log.Info("re-registering host")
-		return p.ValidateManagementAccess(true)
+		err = provisioner.NeedsRegistration
+		return
 	}
 
 	switch nodes.ProvisionState(ironicNode.ProvisionState) {
-	case nodes.Enroll:
+	case nodes.Enroll, nodes.Verifying:
 		err = fmt.Errorf("Invalid state for adopt: %s",
 			ironicNode.ProvisionState)
 	case nodes.Manageable:
@@ -1021,12 +1020,21 @@ func (p *ironicProvisioner) Adopt() (result provisioner.Result, err error) {
 				Target: nodes.TargetAdopt,
 			},
 		)
-	case nodes.Adopting, nodes.Verifying:
+	case nodes.Adopting:
 		result.RequeueAfter = provisionRequeueDelay
 		result.Dirty = true
 	case nodes.AdoptFail:
-		result.ErrorMessage = fmt.Sprintf("Host adoption failed: %s",
-			ironicNode.LastError)
+		if force {
+			return p.changeNodeProvisionState(
+				ironicNode,
+				nodes.ProvisionStateOpts{
+					Target: nodes.TargetAdopt,
+				},
+			)
+		} else {
+			result.ErrorMessage = fmt.Sprintf("Host adoption failed: %s",
+				ironicNode.LastError)
+		}
 	case nodes.Active:
 	default:
 	}
@@ -1043,7 +1051,7 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu
 		return result, errors.Wrap(err, "could not find host to receive image")
 	}
 	if ironicNode == nil {
-		return result, fmt.Errorf("no ironic node for host")
+		return result, provisioner.NeedsRegistration
 	}
 
 	p.log.Info("provisioning image to host", "state", ironicNode.ProvisionState)
@@ -1088,12 +1096,22 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu
 			return result, nil
 		}
 		p.log.Info("recovering from previous failure")
-		return p.startProvisioning(ironicNode, hostConf)
+		if provResult, err := p.startProvisioning(ironicNode, hostConf); err != nil || provResult.Dirty || provResult.ErrorMessage != "" {
+			return provResult, err
+		}
+
+		return p.changeNodeProvisionState(ironicNode,
+			nodes.ProvisionStateOpts{Target: nodes.TargetActive})
 
 	case nodes.Manageable:
-		return p.startProvisioning(ironicNode, hostConf)
+		return p.changeNodeProvisionState(ironicNode,
+			nodes.ProvisionStateOpts{Target: nodes.TargetProvide})
 
 	case nodes.Available:
+		if provResult, err := p.startProvisioning(ironicNode, hostConf); err != nil || provResult.Dirty || provResult.ErrorMessage != "" {
+			return provResult, err
+		}
+
 		// After it is available, we need to start provisioning by
 		// setting the state to "active".
 		p.log.Info("making host active")
@@ -1206,8 +1224,7 @@ func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error)
 		return result, errors.Wrap(err, "failed to find existing host")
 	}
 	if ironicNode == nil {
-		p.log.Info("no node found, already deleted")
-		return result, nil
+		return result, provisioner.NeedsRegistration
 	}
 
 	p.log.Info("deprovisioning host",
@@ -1216,56 +1233,40 @@ func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error)
 		"current", ironicNode.ProvisionState,
 		"target", ironicNode.TargetProvisionState,
 		"deploy step", ironicNode.DeployStep,
+		"instance_info", ironicNode.InstanceInfo,
 	)
 
 	switch nodes.ProvisionState(ironicNode.ProvisionState) {
-
-	case nodes.Error, nodes.CleanFail:
-		if !ironicNode.Maintenance {
-			p.log.Info("setting host maintenance flag to force image delete")
-			return p.setMaintenanceFlag(ironicNode, true)
-		}
-		// Once it's in maintenance, we can start the delete process.
+	case nodes.Error:
 		return p.changeNodeProvisionState(
 			ironicNode,
-			nodes.ProvisionStateOpts{Target: nodes.TargetDeleted},
+			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
 		)
 
-	case nodes.Available:
-		// Move back to manageable
+	case nodes.CleanFail:
+		if ironicNode.Maintenance {
+			p.log.Info("clearing maintenance flag")
+			return p.setMaintenanceFlag(ironicNode, false)
+		}
 		return p.changeNodeProvisionState(
 			ironicNode,
 			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
 		)
 
-	case nodes.Inspecting:
-		p.log.Info("waiting for inspection to complete")
-		result.Dirty = true
-		result.RequeueAfter = introspectionRequeueDelay
+	case nodes.Available:
+		p.publisher("DeprovisioningComplete", "Image deprovisioning completed")
 		return result, nil
 
-	case nodes.InspectWait:
-		p.log.Info("cancelling inspection")
-		return p.changeNodeProvisionState(
-			ironicNode,
-			nodes.ProvisionStateOpts{Target: nodes.TargetAbort},
-		)
-
-	case nodes.InspectFail:
-		p.log.Info("inspection failed or cancelled")
-		return p.changeNodeProvisionState(
-			ironicNode,
-			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
-		)
-
 	case nodes.Deleting:
 		p.log.Info("deleting")
+		// Transitions to Cleaning upon completion
 		result.Dirty = true
 		result.RequeueAfter = deprovisionRequeueDelay
 		return result, nil
 
 	case nodes.Cleaning:
 		p.log.Info("cleaning")
+		// Transitions to Available upon completion
 		result.Dirty = true
 		result.RequeueAfter = deprovisionRequeueDelay
 		return result, nil
@@ -1276,17 +1277,16 @@ func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error)
 		result.RequeueAfter = deprovisionRequeueDelay
 		return result, nil
 
-	case nodes.Manageable, nodes.Enroll, nodes.Verifying:
-		p.publisher("DeprovisioningComplete", "Image deprovisioning completed")
-		return result, nil
-
-	default:
+	case nodes.Active:
 		p.log.Info("starting deprovisioning")
 		p.publisher("DeprovisioningStarted", "Image deprovisioning started")
 		return p.changeNodeProvisionState(
 			ironicNode,
 			nodes.ProvisionStateOpts{Target: nodes.TargetDeleted},
 		)
+
+	default:
+		return result, fmt.Errorf("Unhandled ironic state %s", ironicNode.ProvisionState)
 	}
 }
 
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index da040f75..8fe30327 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -1,6 +1,7 @@
 package provisioner
 
 import (
+	"errors"
 	"time"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
@@ -59,7 +60,7 @@ type Provisioner interface {
 
 	// Adopt brings an externally-provisioned host under management by
 	// the provisioner.
-	Adopt() (result Result, err error)
+	Adopt(force bool) (result Result, err error)
 
 	// Provision writes the image from the host spec to the host. It
 	// may be called multiple times, and should return true for its
@@ -100,3 +101,5 @@ type Result struct {
 	// Any error message produced by the provisioner.
 	ErrorMessage string
 }
+
+var NeedsRegistration = errors.New("Host not registered")


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 8df390c0..7f717658 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -20,7 +20,7 @@ func testStateMachine(host *metal3v1alpha1.BareMetalHost) *hostStateMachine {
 	r := newTestReconciler()
 	p, _ := r.ProvisionerFactory(host, bmc.Credentials{},
 		func(reason, message string) {})
-	return newHostStateMachine(host, r, p)
+	return newHostStateMachine(host, r, p, true)
 }
 
 func TestProvisioningCancelled(t *testing.T) {
@@ -205,7 +205,7 @@ func TestErrorCountIncreasedWhenProvisionerFails(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.Scenario, func(t *testing.T) {
 			prov := &mockProvisioner{}
-			hsm := newHostStateMachine(tt.Host, &BareMetalHostReconciler{}, prov)
+			hsm := newHostStateMachine(tt.Host, &BareMetalHostReconciler{}, prov, true)
 			info := makeDefaultReconcileInfo(tt.Host)
 
 			prov.setNextError("some error")
@@ -220,7 +220,7 @@ func TestErrorCountIncreasedWhenProvisionerFails(t *testing.T) {
 func TestErrorCountIncreasedWhenRegistrationFails(t *testing.T) {
 	bmh := host(metal3v1alpha1.StateRegistering).build()
 	prov := &mockProvisioner{}
-	hsm := newHostStateMachine(bmh, &BareMetalHostReconciler{}, prov)
+	hsm := newHostStateMachine(bmh, &BareMetalHostReconciler{}, prov, true)
 	info := makeDefaultReconcileInfo(bmh)
 	bmh.Status.GoodCredentials = metal3v1alpha1.CredentialsStatus{}
 
@@ -265,7 +265,7 @@ func TestErrorCountCleared(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.Scenario, func(t *testing.T) {
 			prov := &mockProvisioner{}
-			hsm := newHostStateMachine(tt.Host, &BareMetalHostReconciler{}, prov)
+			hsm := newHostStateMachine(tt.Host, &BareMetalHostReconciler{}, prov, true)
 			info := makeDefaultReconcileInfo(tt.Host)
 
 			info.host.Status.ErrorCount = 1
@@ -367,7 +367,7 @@ func (m *mockProvisioner) UpdateHardwareState() (result provisioner.Result, err
 	return m.nextResult, err
 }
 
-func (m *mockProvisioner) Adopt() (result provisioner.Result, err error) {
+func (m *mockProvisioner) Adopt(force bool) (result provisioner.Result, err error) {
 	return m.nextResult, err
 }
 
diff --git a/pkg/provisioner/ironic/adopt_test.go b/pkg/provisioner/ironic/adopt_test.go
index 68610dbb..e47fb9fd 100644
--- a/pkg/provisioner/ironic/adopt_test.go
+++ b/pkg/provisioner/ironic/adopt_test.go
@@ -23,6 +23,7 @@ func TestAdopt(t *testing.T) {
 		expectedDirty        bool
 		expectedError        bool
 		expectedRequestAfter int
+		force                bool
 	}{
 		{
 			name: "node-in-enroll",
@@ -61,8 +62,8 @@ func TestAdopt(t *testing.T) {
 				UUID:           nodeUUID,
 			}),
 
-			expectedDirty:        true,
-			expectedRequestAfter: 10,
+			expectedDirty: false,
+			expectedError: true,
 		},
 		{
 			name: "node-in-AdoptFail",
@@ -74,6 +75,17 @@ func TestAdopt(t *testing.T) {
 			expectedDirty:        false,
 			expectedRequestAfter: 0,
 		},
+		{
+			name: "node-in-AdoptFail force retry",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.AdoptFail),
+				UUID:           nodeUUID,
+			}),
+
+			expectedDirty:        true,
+			expectedRequestAfter: 10,
+			force:                true,
+		},
 	}
 
 	for _, tc := range cases {
@@ -100,7 +112,7 @@ func TestAdopt(t *testing.T) {
 			}
 
 			prov.status.ID = nodeUUID
-			result, err := prov.Adopt()
+			result, err := prov.Adopt(tc.force)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index a682adaa..d924b9b7 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -39,7 +39,7 @@ func TestProvision(t *testing.T) {
 				ProvisionState: string(nodes.Manageable),
 				UUID:           nodeUUID,
 			}),
-			expectedRequestAfter: 0,
+			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
 		{
@@ -48,7 +48,7 @@ func TestProvision(t *testing.T) {
 				ProvisionState: string(nodes.Available),
 				UUID:           nodeUUID,
 			}),
-			expectedRequestAfter: 10,
+			expectedRequestAfter: 0,
 			expectedDirty:        true,
 		},
 		{
@@ -119,40 +119,31 @@ func TestDeprovision(t *testing.T) {
 		expectedRequestAfter int
 	}{
 		{
-			name: "error state",
+			name: "active state",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				ProvisionState: string(nodes.Error),
+				ProvisionState: string(nodes.Active),
 				UUID:           nodeUUID,
 			}),
-			expectedRequestAfter: 0,
+			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
 		{
-			name: "available state",
+			name: "error state",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				ProvisionState: string(nodes.Available),
+				ProvisionState: string(nodes.Error),
 				UUID:           nodeUUID,
 			}),
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
 		{
-			name: "inspecting state",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				ProvisionState: string(nodes.Inspecting),
-				UUID:           nodeUUID,
-			}),
-			expectedRequestAfter: 15,
-			expectedDirty:        true,
-		},
-		{
-			name: "inspectWait state",
+			name: "available state",
 			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				ProvisionState: string(nodes.InspectWait),
+				ProvisionState: string(nodes.Available),
 				UUID:           nodeUUID,
 			}),
-			expectedRequestAfter: 10,
-			expectedDirty:        true,
+			expectedRequestAfter: 0,
+			expectedDirty:        false,
 		},
 		{
 			name: "deleting state",
@@ -181,34 +172,6 @@ func TestDeprovision(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
-
-		{
-			name: "Manageable state",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				ProvisionState: string(nodes.Manageable),
-				UUID:           nodeUUID,
-			}),
-			expectedRequestAfter: 0,
-			expectedDirty:        false,
-		},
-		{
-			name: "Enroll state",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				ProvisionState: string(nodes.Enroll),
-				UUID:           nodeUUID,
-			}),
-			expectedRequestAfter: 0,
-			expectedDirty:        false,
-		},
-		{
-			name: "Verifying state",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				ProvisionState: string(nodes.Verifying),
-				UUID:           nodeUUID,
-			}),
-			expectedRequestAfter: 0,
-			expectedDirty:        false,
-		},
 	}
 
 	for _, tc := range cases {
diff --git a/pkg/provisioner/ironic/updatehardwarestate_test.go b/pkg/provisioner/ironic/updatehardwarestate_test.go
index 5988064d..63944b81 100644
--- a/pkg/provisioner/ironic/updatehardwarestate_test.go
+++ b/pkg/provisioner/ironic/updatehardwarestate_test.go
@@ -101,7 +101,7 @@ func TestUpdateHardwareState(t *testing.T) {
 			name:   "not-ironic-node",
 			ironic: testserver.NewIronic(t).Ready().NoNode(nodeUUID).NoNode("myhost"),
 
-			expectedError: "no ironic node for host",
+			expectedError: "Host not registered",
 		},
 	}