Repository: metal3-io/baremetal-operator
Pull request #749 (2020-12-11 04:34:29)

Do retries with backoff in ValidateManagementAccess, Inspect, and Deprovision
As [discussed](https://github.com/metal3-io/baremetal-operator/issues/739#issuecomment-742218022) in #739, in order to decide whether an error we see from Ironic is one that we have seen and dealt with before (in which case we should retry the present operation) or not (in which case we should deal with the error) we need to store some sort of state.

This PR uses a differential between the ErrorType/ErrorMessage and the ErrorCount to encode this differential. The actual error is cleared whenever we successfully start or continue a new operation, but the error count is preserved until the operation is fully complete. This allows us to both determine when an error is new (when no error is currently recorded in the Status) and yet still do exponential backoff when multiple consecutive errors occur.

This fixes issues with registering, adopting, inspecting, and deprovisioning. The issues were different in each case due to a patchwork of implementations, which are now more consistent.

Some deprovisioning errors that were previously unrecoverable can now be retried. On deletion of the Host, we will retry deprovisioning up to 3 times before giving up and allowing the Host to disappear (previously there was no way to delete it in some states, other than manually removing the finalizer).

One known issue with this is that if a request to change state in Ironic results in a 409 Conflict response, this just sets the Dirty flag and is indistinguishable from success when viewed outside the Ironic provisioner. If such a conflict occurs, we will effectively skip a retry attempt, increment the ErrorCount again, and sleep for even longer before the next attempt.

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

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/749#" title="Author self-approved">zaneb</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->
zaneb: /test-integration
metal3-io-bot: @zaneb: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | f892927d8d1b610f09c4e01e5b711966e508a62c | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/749/golint/1337254436078096391) | `/test golint`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @zaneb: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | f892927d8d1b610f09c4e01e5b711966e508a62c | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/749/golint/1337254436078096391) | `/test golint`
govet | f892927d8d1b610f09c4e01e5b711966e508a62c | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/749/govet/1337254436078096385) | `/test govet`



<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zaneb: /test-integration
furkatgofurov7: @zaneb thanks a lot for the patch. I will test the patch with the scenario I was simulating with vbmc problem and come back report it here. 
andfasano: > @zaneb thanks a lot for the patch. I will test the patch with the scenario I was simulating with vbmc problem and come back report it here.

hey @furkatgofurov7 could you please share some additional details about the tested scenario? I'd like also to try reproducing it if possible
furkatgofurov7: Hi @andfasano! sure thing, I have explained it in the [issue](https://github.com/metal3-io/baremetal-operator/issues/739) itself in short, but I will add detailed steps for reproducing it as well.  
furkatgofurov7: > @zaneb thanks a lot for the patch. I will test the patch with the scenario I was simulating with vbmc problem and come back report it here.

Nice work, I have tested the patch locally to test the scenario described in the issue, and it works flawlessly!  
andfasano: Looks also good to me
furkatgofurov7: This needs a rebase though. 

/cc @zaneb 
metal3-io-bot: @furkatgofurov7: GitHub didn't allow me to request PR reviews from the following users: zaneb.

Note that only [metal3-io members](https://github.com/orgs/metal3-io/people) and repo collaborators can review this PR, and authors cannot review their own PRs.

<details>

In response to [this](https://github.com/metal3-io/baremetal-operator/pull/749#issuecomment-757814995):

>This needs a rebase though. 
>
>/cc @zaneb 


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
andfasano: @zaneb @furkatgofurov7 I think it could be useful in this scope also to land #757 as it contains a fix for ErrorCount

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 73fd61b9..b15a9ba9 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -370,10 +370,6 @@ func clearError(host *metal3v1alpha1.BareMetalHost) (dirty bool) {
 		host.Status.ErrorMessage = ""
 		dirty = true
 	}
-	if host.Status.ErrorCount != 0 {
-		host.Status.ErrorCount = 0
-		dirty = true
-	}
 	return dirty
 }
 
@@ -438,7 +434,7 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner
 		info.postSaveCallbacks = append(info.postSaveCallbacks, updatedCredentials.Inc)
 	}
 
-	provResult, err := prov.ValidateManagementAccess(credsChanged)
+	provResult, err := prov.ValidateManagementAccess(credsChanged, info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
 	if err != nil {
 		noManagementAccess.Inc()
 		return actionError{errors.Wrap(err, "failed to validate BMC access")}
@@ -476,7 +472,7 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner
 func (r *BareMetalHostReconciler) actionInspecting(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	info.log.Info("inspecting hardware")
 
-	provResult, details, err := prov.InspectHardware()
+	provResult, details, err := prov.InspectHardware(info.host.Status.ErrorType == metal3v1alpha1.InspectionError)
 	if err != nil {
 		return actionError{errors.Wrap(err, "hardware inspection failed")}
 	}
@@ -608,7 +604,7 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 
 	info.log.Info("deprovisioning")
 
-	provResult, err = prov.Deprovision()
+	provResult, err = prov.Deprovision(info.host.Status.ErrorType == metal3v1alpha1.ProvisioningError)
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to deprovision")}
 	}
@@ -720,6 +716,7 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 	// state and there were no errors, so reflect the new state in the
 	// host status field.
 	info.host.Status.PoweredOn = info.host.Spec.Online
+	info.host.Status.ErrorCount = 0
 	return steadyStateResult
 }
 
diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index 9954a38b..449782ce 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -240,6 +240,7 @@ func (hsm *hostStateMachine) handleRegistering(info *reconcileInfo) actionResult
 	} else {
 		hsm.NextState = metal3v1alpha1.StateInspecting
 	}
+	hsm.Host.Status.ErrorCount = 0
 	return actionComplete{}
 }
 
@@ -247,6 +248,7 @@ func (hsm *hostStateMachine) handleInspecting(info *reconcileInfo) actionResult
 	actResult := hsm.Reconciler.actionInspecting(hsm.Provisioner, info)
 	if _, complete := actResult.(actionComplete); complete {
 		hsm.NextState = metal3v1alpha1.StateMatchProfile
+		hsm.Host.Status.ErrorCount = 0
 	}
 	return actResult
 }
@@ -255,12 +257,14 @@ func (hsm *hostStateMachine) handleMatchProfile(info *reconcileInfo) actionResul
 	actResult := hsm.Reconciler.actionMatchProfile(hsm.Provisioner, info)
 	if _, complete := actResult.(actionComplete); complete {
 		hsm.NextState = metal3v1alpha1.StateReady
+		hsm.Host.Status.ErrorCount = 0
 	}
 	return actResult
 }
 
 func (hsm *hostStateMachine) handleExternallyProvisioned(info *reconcileInfo) actionResult {
 	if hsm.Host.Spec.ExternallyProvisioned {
+		// ErrorCount is cleared when appropriate inside actionManageSteadyState
 		return hsm.Reconciler.actionManageSteadyState(hsm.Provisioner, info)
 	}
 
@@ -281,8 +285,8 @@ func (hsm *hostStateMachine) handleReady(info *reconcileInfo) actionResult {
 		return actionComplete{}
 	}
 
+	// ErrorCount is cleared when appropriate inside actionManageReady
 	actResult := hsm.Reconciler.actionManageReady(hsm.Provisioner, info)
-
 	if _, complete := actResult.(actionComplete); complete {
 		hsm.NextState = metal3v1alpha1.StateProvisioning
 	}
@@ -317,6 +321,7 @@ func (hsm *hostStateMachine) handleProvisioning(info *reconcileInfo) actionResul
 	actResult := hsm.Reconciler.actionProvisioning(hsm.Provisioner, info)
 	if _, complete := actResult.(actionComplete); complete {
 		hsm.NextState = metal3v1alpha1.StateProvisioned
+		hsm.Host.Status.ErrorCount = 0
 	}
 	return actResult
 }
@@ -327,6 +332,7 @@ func (hsm *hostStateMachine) handleProvisioned(info *reconcileInfo) actionResult
 		return actionComplete{}
 	}
 
+	// ErrorCount is cleared when appropriate inside actionManageSteadyState
 	return hsm.Reconciler.actionManageSteadyState(hsm.Provisioner, info)
 }
 
@@ -336,6 +342,7 @@ func (hsm *hostStateMachine) handleDeprovisioning(info *reconcileInfo) actionRes
 	if hsm.Host.DeletionTimestamp.IsZero() {
 		if _, complete := actResult.(actionComplete); complete {
 			hsm.NextState = metal3v1alpha1.StateReady
+			hsm.Host.Status.ErrorCount = 0
 		}
 	} else {
 		skipToDelete := func() actionResult {
@@ -347,13 +354,13 @@ func (hsm *hostStateMachine) handleDeprovisioning(info *reconcileInfo) actionRes
 		switch r := actResult.(type) {
 		case actionComplete:
 			hsm.NextState = metal3v1alpha1.StateDeleting
+			hsm.Host.Status.ErrorCount = 0
 		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.
-			return skipToDelete()
+			if hsm.Host.Status.ErrorCount > 3 {
+				return skipToDelete()
+			}
 		case actionError:
 			if r.NeedsRegistration() && !hsm.haveCreds {
 				// If the host is not registered as a node in Ironic and we
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index f118dc27..9fc1bc81 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -68,7 +68,7 @@ func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher
 
 // ValidateManagementAccess tests the connection information for the
 // host to verify that the location and credentials work.
-func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged bool) (result provisioner.Result, err error) {
+func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) {
 	p.log.Info("testing management access")
 
 	hostName := p.host.ObjectMeta.Name
@@ -102,7 +102,7 @@ func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged bool) (res
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *demoProvisioner) InspectHardware() (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *demoProvisioner) InspectHardware(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
 	p.log.Info("inspecting hardware", "status", p.host.OperationalStatus())
 
 	hostName := p.host.ObjectMeta.Name
@@ -218,7 +218,7 @@ func (p *demoProvisioner) Provision(hostConf provisioner.HostConfigData) (result
 // Deprovision removes the host from the image. It may be called
 // multiple times, and should return true for its dirty flag until the
 // deprovisioning operation is completed.
-func (p *demoProvisioner) Deprovision() (result provisioner.Result, err error) {
+func (p *demoProvisioner) Deprovision(force bool) (result provisioner.Result, err error) {
 
 	hostName := p.host.ObjectMeta.Name
 	switch hostName {
diff --git a/pkg/provisioner/empty/empty.go b/pkg/provisioner/empty/empty.go
index 3bfd226e..a50af7fc 100644
--- a/pkg/provisioner/empty/empty.go
+++ b/pkg/provisioner/empty/empty.go
@@ -21,7 +21,7 @@ func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher
 
 // ValidateManagementAccess tests the connection information for the
 // host to verify that the location and credentials work.
-func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged bool) (provisioner.Result, error) {
+func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (provisioner.Result, error) {
 	return provisioner.Result{}, nil
 }
 
@@ -29,7 +29,7 @@ func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged bool) (pr
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *emptyProvisioner) InspectHardware() (provisioner.Result, *metal3v1alpha1.HardwareDetails, error) {
+func (p *emptyProvisioner) InspectHardware(force bool) (provisioner.Result, *metal3v1alpha1.HardwareDetails, error) {
 	return provisioner.Result{}, nil, nil
 }
 
@@ -57,7 +57,7 @@ func (p *emptyProvisioner) Provision(hostConf provisioner.HostConfigData) (provi
 // Deprovision removes the host from the image. It may be called
 // multiple times, and should return true for its dirty flag until the
 // deprovisioning operation is completed.
-func (p *emptyProvisioner) Deprovision() (provisioner.Result, error) {
+func (p *emptyProvisioner) Deprovision(force bool) (provisioner.Result, error) {
 	return provisioner.Result{}, nil
 }
 
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index bcd44458..dcb365b3 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -77,7 +77,7 @@ func NewMock(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publi
 
 // ValidateManagementAccess tests the connection information for the
 // host to verify that the location and credentials work.
-func (p *fixtureProvisioner) ValidateManagementAccess(credentialsChanged bool) (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) {
 	p.log.Info("testing management access")
 
 	// Fill in the ID of the host in the provisioning system
@@ -98,7 +98,7 @@ func (p *fixtureProvisioner) ValidateManagementAccess(credentialsChanged bool) (
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *fixtureProvisioner) InspectHardware() (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *fixtureProvisioner) InspectHardware(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
 	p.log.Info("inspecting hardware", "status", p.host.OperationalStatus())
 
 	// The inspection is ongoing. We'll need to check the fixture
@@ -201,7 +201,7 @@ func (p *fixtureProvisioner) Provision(hostConf provisioner.HostConfigData) (res
 // Deprovision removes the host from the image. It may be called
 // multiple times, and should return true for its dirty flag until the
 // deprovisioning operation is completed.
-func (p *fixtureProvisioner) Deprovision() (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) Deprovision(force bool) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is deprovisioned")
 
 	result.RequeueAfter = deprovisionRequeueDelay
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 95be5287..ade5a59a 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -333,7 +333,7 @@ func (p *ironicProvisioner) findExistingHost() (ironicNode *nodes.Node, err erro
 //
 // FIXME(dhellmann): We should rename this method to describe what it
 // actually does.
-func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) {
 	var ironicNode *nodes.Node
 
 	p.log.Info("validating management access")
@@ -572,7 +572,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (r
 	case nodes.Enroll:
 
 		// If ironic is reporting an error, stop working on the node.
-		if ironicNode.LastError != "" && !credentialsChanged {
+		if ironicNode.LastError != "" && !(credentialsChanged || force) {
 			result.ErrorMessage = ironicNode.LastError
 			return result, nil
 		}
@@ -651,7 +651,7 @@ func (p *ironicProvisioner) changeNodeProvisionState(ironicNode *nodes.Node, opt
 // details of devices discovered on the hardware. It may be called
 // multiple times, and should return true for its dirty flag until the
 // inspection is completed.
-func (p *ironicProvisioner) InspectHardware() (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
 	p.log.Info("inspecting hardware", "status", p.host.OperationalStatus())
 
 	ironicNode, err := p.findExistingHost()
@@ -674,6 +674,15 @@ func (p *ironicProvisioner) InspectHardware() (result provisioner.Result, detail
 				err = nil
 				return
 			default:
+				if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectFail && !force {
+					p.log.Info("starting inspection failed", "error", status.Error)
+					if ironicNode.LastError == "" {
+						result.ErrorMessage = "Inspection failed"
+					} else {
+						result.ErrorMessage = ironicNode.LastError
+					}
+					err = nil
+				}
 				p.log.Info("updating boot mode before hardware inspection")
 				op, value := buildCapabilitiesValue(ironicNode, p.host.Status.Provisioning.BootMode)
 				updates := nodes.UpdateOpts{
@@ -1314,7 +1323,7 @@ func (p *ironicProvisioner) setMaintenanceFlag(ironicNode *nodes.Node, value boo
 // Deprovision removes the host from the image. It may be called
 // multiple times, and should return true for its dirty flag until the
 // deprovisioning operation is completed.
-func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error) {
+func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result, err error) {
 	p.log.Info("deprovisioning")
 
 	ironicNode, err := p.findExistingHost()
@@ -1336,9 +1345,20 @@ func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error)
 
 	switch nodes.ProvisionState(ironicNode.ProvisionState) {
 	case nodes.Error:
+		if !force {
+			p.log.Info("deprovisioning failed")
+			if ironicNode.LastError == "" {
+				result.ErrorMessage = "Deprovisioning failed"
+			} else {
+				result.ErrorMessage = ironicNode.LastError
+			}
+			return result, nil
+		}
+		p.log.Info("retrying deprovisioning")
+		p.publisher("DeprovisioningStarted", "Image deprovisioning restarted")
 		return p.changeNodeProvisionState(
 			ironicNode,
-			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
+			nodes.ProvisionStateOpts{Target: nodes.TargetDeleted},
 		)
 
 	case nodes.CleanFail:
@@ -1346,6 +1366,10 @@ func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error)
 			p.log.Info("clearing maintenance flag")
 			return p.setMaintenanceFlag(ironicNode, false)
 		}
+		// This will return us to the manageable state without completing
+		// cleaning. Because cleaning happens in the process of moving from
+		// manageable to available, the node will still get cleaned before
+		// we provision it again.
 		return p.changeNodeProvisionState(
 			ironicNode,
 			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index 8fe30327..e5e01f2c 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -43,13 +43,13 @@ type Provisioner interface {
 	// of credentials it has are different from the credentials it has
 	// previously been using, without implying that either set of
 	// credentials is correct.
-	ValidateManagementAccess(credentialsChanged bool) (result Result, err error)
+	ValidateManagementAccess(credentialsChanged, force bool) (result Result, err error)
 
 	// InspectHardware updates the HardwareDetails field of the host with
 	// details of devices discovered on the hardware. It may be called
 	// multiple times, and should return true for its dirty flag until the
 	// inspection is completed.
-	InspectHardware() (result Result, details *metal3v1alpha1.HardwareDetails, err error)
+	InspectHardware(force bool) (result Result, details *metal3v1alpha1.HardwareDetails, err error)
 
 	// UpdateHardwareState fetches the latest hardware state of the
 	// server and updates the HardwareDetails field of the host with
@@ -70,11 +70,11 @@ type Provisioner interface {
 	// Deprovision removes the host from the image. It may be called
 	// multiple times, and should return true for its dirty flag until
 	// the deprovisioning operation is completed.
-	Deprovision() (result Result, err error)
+	Deprovision(force bool) (result Result, err error)
 
 	// Delete removes the host from the provisioning system. It may be
 	// called multiple times, and should return true for its dirty
-	// flag until the deprovisioning operation is completed.
+	// flag until the deletion operation is completed.
 	Delete() (result Result, err error)
 
 	// PowerOn ensures the server is powered on independently of any image


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 03df5bbc..3c02a2b9 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -1310,27 +1310,3 @@ func TestErrorCountIncrementsAlways(t *testing.T) {
 	setErrorMessage(b, metal3v1alpha1.InspectionError, "Another error message")
 	assert.Equal(t, b.Status.ErrorCount, 2)
 }
-
-func TestClearErrorCount(t *testing.T) {
-
-	b := &metal3v1alpha1.BareMetalHost{
-		Status: metal3v1alpha1.BareMetalHostStatus{
-			ErrorCount: 5,
-		},
-	}
-
-	assert.True(t, clearError(b))
-	assert.Equal(t, 0, b.Status.ErrorCount)
-}
-
-func TestClearErrorCountOnlyIfNotZero(t *testing.T) {
-
-	b := &metal3v1alpha1.BareMetalHost{
-		Status: metal3v1alpha1.BareMetalHostStatus{
-			ErrorCount: 5,
-		},
-	}
-
-	assert.True(t, clearError(b))
-	assert.False(t, clearError(b))
-}
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 7f717658..96834305 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -234,8 +234,9 @@ func TestErrorCountIncreasedWhenRegistrationFails(t *testing.T) {
 func TestErrorCountCleared(t *testing.T) {
 
 	tests := []struct {
-		Scenario string
-		Host     *metal3v1alpha1.BareMetalHost
+		Scenario                     string
+		Host                         *metal3v1alpha1.BareMetalHost
+		PreserveErrorCountOnComplete bool
 	}{
 		{
 			Scenario: "registering",
@@ -246,8 +247,9 @@ func TestErrorCountCleared(t *testing.T) {
 			Host:     host(metal3v1alpha1.StateInspecting).build(),
 		},
 		{
-			Scenario: "ready",
-			Host:     host(metal3v1alpha1.StateReady).build(),
+			Scenario:                     "ready",
+			Host:                         host(metal3v1alpha1.StateReady).build(),
+			PreserveErrorCountOnComplete: true,
 		},
 		{
 			Scenario: "deprovisioning",
@@ -258,8 +260,9 @@ func TestErrorCountCleared(t *testing.T) {
 			Host:     host(metal3v1alpha1.StateProvisioning).SetImageURL("imageSpecUrl").build(),
 		},
 		{
-			Scenario: "externallyProvisioned",
-			Host:     host(metal3v1alpha1.StateExternallyProvisioned).SetExternallyProvisioned().build(),
+			Scenario:                     "externallyProvisioned",
+			Host:                         host(metal3v1alpha1.StateExternallyProvisioned).SetExternallyProvisioned().build(),
+			PreserveErrorCountOnComplete: true,
 		},
 	}
 	for _, tt := range tests {
@@ -272,8 +275,16 @@ func TestErrorCountCleared(t *testing.T) {
 			prov.setNextResult(true)
 			result := hsm.ReconcileState(info)
 
-			assert.Equal(t, tt.Host.Status.ErrorCount, 0)
+			assert.Equal(t, 1, tt.Host.Status.ErrorCount)
 			assert.True(t, result.Dirty())
+
+			prov.setNextResult(false)
+			hsm.ReconcileState(info)
+			if tt.PreserveErrorCountOnComplete {
+				assert.Equal(t, 1, tt.Host.Status.ErrorCount)
+			} else {
+				assert.Equal(t, 0, tt.Host.Status.ErrorCount)
+			}
 		})
 	}
 }
@@ -355,11 +366,12 @@ func (m *mockProvisioner) setNextResult(dirty bool) {
 	}
 }
 
-func (m *mockProvisioner) ValidateManagementAccess(credentialsChanged bool) (result provisioner.Result, err error) {
+func (m *mockProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) {
 	return m.nextResult, err
 }
 
-func (m *mockProvisioner) InspectHardware() (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (m *mockProvisioner) InspectHardware(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+	details = &metal3v1alpha1.HardwareDetails{}
 	return m.nextResult, details, err
 }
 
@@ -375,7 +387,7 @@ func (m *mockProvisioner) Provision(configData provisioner.HostConfigData) (resu
 	return m.nextResult, err
 }
 
-func (m *mockProvisioner) Deprovision() (result provisioner.Result, err error) {
+func (m *mockProvisioner) Deprovision(force bool) (result provisioner.Result, err error) {
 	return m.nextResult, err
 }
 
diff --git a/pkg/provisioner/ironic/inspecthardware_test.go b/pkg/provisioner/ironic/inspecthardware_test.go
index b094dffe..7118cb7c 100644
--- a/pkg/provisioner/ironic/inspecthardware_test.go
+++ b/pkg/provisioner/ironic/inspecthardware_test.go
@@ -152,7 +152,7 @@ func TestInspectHardware(t *testing.T) {
 			}
 
 			prov.status.ID = nodeUUID
-			result, details, err := prov.InspectHardware()
+			result, details, err := prov.InspectHardware(false)
 
 			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 6b6038ff..8c463ffa 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -125,6 +125,7 @@ func TestDeprovision(t *testing.T) {
 		ironic               *testserver.IronicMock
 		expectedDirty        bool
 		expectedError        bool
+		expectedErrorMessage bool
 		expectedRequestAfter int
 	}{
 		{
@@ -142,8 +143,7 @@ func TestDeprovision(t *testing.T) {
 				ProvisionState: string(nodes.Error),
 				UUID:           nodeUUID,
 			}),
-			expectedRequestAfter: 10,
-			expectedDirty:        true,
+			expectedErrorMessage: true,
 		},
 		{
 			name: "available state",
@@ -207,9 +207,10 @@ func TestDeprovision(t *testing.T) {
 			}
 
 			prov.status.ID = nodeUUID
-			result, err := prov.Deprovision()
+			result, err := prov.Deprovision(false)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
+			assert.Equal(t, tc.expectedErrorMessage, result.ErrorMessage != "")
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
 			if !tc.expectedError {
 				assert.NoError(t, err)
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 522b0bec..13f49777 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -33,7 +33,7 @@ func TestValidateManagementAccessNoMAC(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(false)
+	result, err := prov.ValidateManagementAccess(false, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}
@@ -63,7 +63,7 @@ func TestValidateManagementAccessMACOptional(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(false)
+	result, err := prov.ValidateManagementAccess(false, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}
@@ -95,7 +95,7 @@ func TestValidateManagementAccessCreateNode(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(false)
+	result, err := prov.ValidateManagementAccess(false, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}
@@ -130,7 +130,7 @@ func TestValidateManagementAccessExistingNode(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(false)
+	result, err := prov.ValidateManagementAccess(false, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}
@@ -193,7 +193,7 @@ func TestValidateManagementAccessExistingNodeContinue(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
-			result, err := prov.ValidateManagementAccess(false)
+			result, err := prov.ValidateManagementAccess(false, false)
 			if err != nil {
 				t.Fatalf("error from ValidateManagementAccess: %s", err)
 			}
@@ -238,7 +238,7 @@ func TestValidateManagementAccessExistingNodeWaiting(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
-			result, err := prov.ValidateManagementAccess(false)
+			result, err := prov.ValidateManagementAccess(false, false)
 			if err != nil {
 				t.Fatalf("error from ValidateManagementAccess: %s", err)
 			}
@@ -280,7 +280,7 @@ func TestValidateManagementAccessNewCredentials(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(true)
+	result, err := prov.ValidateManagementAccess(true, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}
@@ -327,7 +327,7 @@ func TestValidateManagementAccessLinkExistingIronicNodeByMAC(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(false)
+	result, err := prov.ValidateManagementAccess(false, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}
@@ -370,7 +370,7 @@ func TestValidateManagementAccessExistingPortWithWrongUUID(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	_, err = prov.ValidateManagementAccess(false)
+	_, err = prov.ValidateManagementAccess(false, false)
 	assert.EqualError(t, err, "failed to find existing host: port exists but linked node doesn't random-wrong-id: Resource not found")
 }
 
@@ -412,7 +412,7 @@ func TestValidateManagementAccessExistingPortButHasName(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	_, err = prov.ValidateManagementAccess(false)
+	_, err = prov.ValidateManagementAccess(false, false)
 	assert.EqualError(t, err, "failed to find existing host: node found by MAC but has a name: wrong-name")
 }
 
@@ -450,7 +450,7 @@ func TestValidateManagementAccessAddTwoHostsWithSameMAC(t *testing.T) {
 		t.Fatalf("could not create provisioner: %s", err)
 	}
 
-	result, err := prov.ValidateManagementAccess(false)
+	result, err := prov.ValidateManagementAccess(false, false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}