Repository: metal3-io/baremetal-operator
Pull request #750 (2020-12-14 20:29:56)

ironic provisioner de-duplicate image options
This aims to de-duplicate the code where ironic image settings are managed, so that new code planned for the LiveImage enhancement don't need to be added in two places 

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/750#" title="Author self-approved">hardys</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)~~ [hardys]

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

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 474054d858d3c55dd281fdd1fb6fcf47e9e9defa | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/750/golint/1338582066861707268) | `/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: @hardys: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 474054d858d3c55dd281fdd1fb6fcf47e9e9defa | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/750/golint/1338582066861707268) | `/test golint`
gofmt | 474054d858d3c55dd281fdd1fb6fcf47e9e9defa | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/750/gofmt/1338582066861707265) | `/test gofmt`



<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: @hardys: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
golint | 474054d858d3c55dd281fdd1fb6fcf47e9e9defa | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/750/golint/1338582066861707268) | `/test golint`
gofmt | 474054d858d3c55dd281fdd1fb6fcf47e9e9defa | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/750/gofmt/1338582066861707265) | `/test gofmt`
govet | 474054d858d3c55dd281fdd1fb6fcf47e9e9defa | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/750/govet/1338582066861707270) | `/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 -->
hardys: > This looks right to me. I don't know what sort of unit test coverage there is for `getUpdateOptsForNode()` that might need to be changed or expanded, but these changes look fine.

Thanks for the quick review - the [test coverage](https://github.com/metal3-io/baremetal-operator/blob/master/pkg/provisioner/ironic/updateopts_test.go) looks OK to me, I could change that to refer to the new function explicitly - but I was thinking the fact that the existing tests all pass was a reasonable way to show that I hadn't broken things with the refactoring :)
hardys: /test-integration
hardys: /retitle ironic provisioner de-duplicate image options
hardys: @dhellmann any more updates needed before we can merge this one?

/cc @zaneb 
dhellmann: I'm happy with these changes. I'll give @zaneb a chance to review them, too.

/approve

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/750#issuecomment-754052783" title="Approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/750#" title="Author self-approved">hardys</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)~~ [dhellmann,hardys]

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":[]} -->
hardys: Ok I fixed the `instance_uuid` issue - from comments here and on #759 there are two potential further de-duplications:

* Avoid duplicate GetChecksum() call
* Potentially just use AddOp for both Add and Replace operations ref https://github.com/metal3-io/baremetal-operator/pull/759#discussion_r555865833 comment from @dtantsur 

Unless folks feel strongly otherwise I'll push follow-up PRs for those, since it's not strictly related to the image instance_info de-duplication I wanted to do before adding the live-iso code?
hardys: /test-integration
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/750#issuecomment-754052783" title="Approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/750#" title="Author self-approved">hardys</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/750#pullrequestreview-566695911" title="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)~~ [dhellmann,hardys,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":[]} -->

Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index b15a9ba9..73fd61b9 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -370,6 +370,10 @@ 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
 }
 
@@ -434,7 +438,7 @@ func (r *BareMetalHostReconciler) actionRegistering(prov provisioner.Provisioner
 		info.postSaveCallbacks = append(info.postSaveCallbacks, updatedCredentials.Inc)
 	}
 
-	provResult, err := prov.ValidateManagementAccess(credsChanged, info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
+	provResult, err := prov.ValidateManagementAccess(credsChanged)
 	if err != nil {
 		noManagementAccess.Inc()
 		return actionError{errors.Wrap(err, "failed to validate BMC access")}
@@ -472,7 +476,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(info.host.Status.ErrorType == metal3v1alpha1.InspectionError)
+	provResult, details, err := prov.InspectHardware()
 	if err != nil {
 		return actionError{errors.Wrap(err, "hardware inspection failed")}
 	}
@@ -604,7 +608,7 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 
 	info.log.Info("deprovisioning")
 
-	provResult, err = prov.Deprovision(info.host.Status.ErrorType == metal3v1alpha1.ProvisioningError)
+	provResult, err = prov.Deprovision()
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to deprovision")}
 	}
@@ -716,7 +720,6 @@ 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 449782ce..9954a38b 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -240,7 +240,6 @@ func (hsm *hostStateMachine) handleRegistering(info *reconcileInfo) actionResult
 	} else {
 		hsm.NextState = metal3v1alpha1.StateInspecting
 	}
-	hsm.Host.Status.ErrorCount = 0
 	return actionComplete{}
 }
 
@@ -248,7 +247,6 @@ 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
 }
@@ -257,14 +255,12 @@ 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)
 	}
 
@@ -285,8 +281,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
 	}
@@ -321,7 +317,6 @@ 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
 }
@@ -332,7 +327,6 @@ func (hsm *hostStateMachine) handleProvisioned(info *reconcileInfo) actionResult
 		return actionComplete{}
 	}
 
-	// ErrorCount is cleared when appropriate inside actionManageSteadyState
 	return hsm.Reconciler.actionManageSteadyState(hsm.Provisioner, info)
 }
 
@@ -342,7 +336,6 @@ 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 {
@@ -354,13 +347,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.
-			if hsm.Host.Status.ErrorCount > 3 {
-				return skipToDelete()
-			}
+			// Note that this is entirely theoretical, as the
+			// Ironic provisioner currently never gives up
+			// trying to deprovision.
+			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 9fc1bc81..f118dc27 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, force bool) (result provisioner.Result, err error) {
+func (p *demoProvisioner) ValidateManagementAccess(credentialsChanged 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, force boo
 // 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(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *demoProvisioner) InspectHardware() (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(force bool) (result provisioner.Result, err error) {
+func (p *demoProvisioner) Deprovision() (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 a50af7fc..3bfd226e 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, force bool) (provisioner.Result, error) {
+func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged bool) (provisioner.Result, error) {
 	return provisioner.Result{}, nil
 }
 
@@ -29,7 +29,7 @@ func (p *emptyProvisioner) ValidateManagementAccess(credentialsChanged, force bo
 // 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(force bool) (provisioner.Result, *metal3v1alpha1.HardwareDetails, error) {
+func (p *emptyProvisioner) InspectHardware() (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(force bool) (provisioner.Result, error) {
+func (p *emptyProvisioner) Deprovision() (provisioner.Result, error) {
 	return provisioner.Result{}, nil
 }
 
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index dcb365b3..bcd44458 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, force bool) (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) ValidateManagementAccess(credentialsChanged 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, force
 // 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(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *fixtureProvisioner) InspectHardware() (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(force bool) (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) Deprovision() (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 ade5a59a..68009ea1 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, force bool) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged bool) (result provisioner.Result, err error) {
 	var ironicNode *nodes.Node
 
 	p.log.Info("validating management access")
@@ -420,75 +420,23 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b
 			imageData = p.host.Spec.Image
 		}
 
-		checksum, checksumType, ok := imageData.GetChecksum()
-
-		if ok {
-			p.log.Info("setting instance info",
-				"image_source", imageData.URL,
-				"image_os_hash_value", checksum,
-				"image_os_hash_algo", checksumType,
-			)
-
-			updates := nodes.UpdateOpts{
-				nodes.UpdateOperation{
-					Op:    nodes.AddOp,
-					Path:  "/instance_info/image_source",
-					Value: imageData.URL,
-				},
-				nodes.UpdateOperation{
-					Op:    nodes.AddOp,
-					Path:  "/instance_info/image_os_hash_value",
-					Value: checksum,
-				},
-				nodes.UpdateOperation{
-					Op:    nodes.AddOp,
-					Path:  "/instance_info/image_os_hash_algo",
-					Value: checksumType,
-				},
-				nodes.UpdateOperation{
-					Op:    nodes.ReplaceOp,
-					Path:  "/instance_uuid",
-					Value: string(p.host.ObjectMeta.UID),
-				},
-			}
-
-			// image_checksum
-			//
-			// FIXME: For older versions of ironic that do not have
-			// https://review.opendev.org/#/c/711816/ failing to
-			// include the 'image_checksum' causes ironic to refuse to
-			// provision the image, even if the other hash value
-			// parameters are given. We only want to do that for MD5,
-			// however, because those versions of ironic only support
-			// MD5 checksums.
-			if checksumType == string(metal3v1alpha1.MD5) {
-				updates = append(
-					updates,
-					nodes.UpdateOperation{
-						Op:    nodes.AddOp,
-						Path:  "/instance_info/image_checksum",
-						Value: checksum,
-					},
-				)
-			}
-
-			if imageData.DiskFormat != nil {
-				updates = append(updates, nodes.UpdateOperation{
-					Op:    nodes.AddOp,
-					Path:  "/instance_info/image_disk_format",
-					Value: *imageData.DiskFormat,
-				})
+		if imageData != nil {
+			updates, err := p.getImageUpdateOptsForNode(ironicNode, imageData)
+			if err != nil {
+				return result, errors.Wrap(err, "Could not get Image options for node")
 			}
-			_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
-			switch err.(type) {
-			case nil:
-			case gophercloud.ErrDefault409:
-				p.log.Info("could not update host settings in ironic, busy")
-				result.Dirty = true
-				result.RequeueAfter = provisionRequeueDelay
-				return result, nil
-			default:
-				return result, errors.Wrap(err, "failed to update host settings in ironic")
+			if len(updates) != 0 {
+				_, err = nodes.Update(p.client, ironicNode.UUID, updates).Extract()
+				switch err.(type) {
+				case nil:
+				case gophercloud.ErrDefault409:
+					p.log.Info("could not update host settings in ironic, busy")
+					result.Dirty = true
+					result.RequeueAfter = provisionRequeueDelay
+					return result, nil
+				default:
+					return result, errors.Wrap(err, "failed to update host settings in ironic")
+				}
 			}
 		}
 	} else {
@@ -572,7 +520,7 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b
 	case nodes.Enroll:
 
 		// If ironic is reporting an error, stop working on the node.
-		if ironicNode.LastError != "" && !(credentialsChanged || force) {
+		if ironicNode.LastError != "" && !credentialsChanged {
 			result.ErrorMessage = ironicNode.LastError
 			return result, nil
 		}
@@ -651,7 +599,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(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
+func (p *ironicProvisioner) InspectHardware() (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
 	p.log.Info("inspecting hardware", "status", p.host.OperationalStatus())
 
 	ironicNode, err := p.findExistingHost()
@@ -674,15 +622,6 @@ func (p *ironicProvisioner) InspectHardware(force bool) (result provisioner.Resu
 				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{
@@ -784,16 +723,22 @@ func (p *ironicProvisioner) UpdateHardwareState() (result provisioner.Result, er
 	return result, nil
 }
 
-func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (updates nodes.UpdateOpts, err error) {
-
-	hwProf, err := hardware.GetProfile(p.host.HardwareProfile())
-
-	if err != nil {
-		return updates, errors.Wrap(err,
-			fmt.Sprintf("Could not start provisioning with bad hardware profile %s",
-				p.host.HardwareProfile()))
+func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image) (updates nodes.UpdateOpts, err error) {
+	checksum, checksumType, ok := p.host.GetImageChecksum()
+	if !ok {
+		p.log.Info("image/checksum not found for host")
+		return
 	}
-
+	// instance_uuid
+	p.log.Info("setting instance_uuid")
+	updates = append(
+		updates,
+		nodes.UpdateOperation{
+			Op:    nodes.ReplaceOp,
+			Path:  "/instance_uuid",
+			Value: string(p.host.ObjectMeta.UID),
+		},
+	)
 	// image_source
 	var op nodes.UpdateOp
 	if _, ok := ironicNode.InstanceInfo["image_source"]; !ok {
@@ -808,12 +753,10 @@ func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (update
 		nodes.UpdateOperation{
 			Op:    op,
 			Path:  "/instance_info/image_source",
-			Value: p.host.Spec.Image.URL,
+			Value: imageData.URL,
 		},
 	)
 
-	checksum, checksumType, _ := p.host.GetImageChecksum()
-
 	// image_os_hash_algo
 	if _, ok := ironicNode.InstanceInfo["image_os_hash_algo"]; !ok {
 		op = nodes.AddOp
@@ -874,30 +817,38 @@ func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (update
 		)
 	}
 
-	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,
 		})
 	}
+	return updates, nil
+}
 
-	// instance_uuid
-	p.log.Info("setting instance_uuid")
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    nodes.ReplaceOp,
-			Path:  "/instance_uuid",
-			Value: string(p.host.ObjectMeta.UID),
-		},
-	)
+func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (updates nodes.UpdateOpts, err error) {
+
+	hwProf, err := hardware.GetProfile(p.host.HardwareProfile())
+
+	if err != nil {
+		return updates, errors.Wrap(err,
+			fmt.Sprintf("Could not start provisioning with bad hardware profile %s",
+				p.host.HardwareProfile()))
+	}
+
+	imageOpts, err := p.getImageUpdateOptsForNode(ironicNode, p.host.Spec.Image)
+	if err != nil {
+		return updates, errors.Wrap(err, "Could not get Image options for node")
+	}
+	updates = append(updates, imageOpts...)
 
 	// root_gb
 	//
 	// FIXME(dhellmann): We have to provide something for the disk
 	// size until https://storyboard.openstack.org/#!/story/2005165 is
 	// fixed in ironic.
+	var op nodes.UpdateOp
 	if _, ok := ironicNode.InstanceInfo["root_gb"]; !ok {
 		op = nodes.AddOp
 		p.log.Info("adding root_gb")
@@ -1323,7 +1274,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(force bool) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) Deprovision() (result provisioner.Result, err error) {
 	p.log.Info("deprovisioning")
 
 	ironicNode, err := p.findExistingHost()
@@ -1345,20 +1296,9 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result,
 
 	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.TargetDeleted},
+			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
 		)
 
 	case nodes.CleanFail:
@@ -1366,10 +1306,6 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result,
 			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 e5e01f2c..8fe30327 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, force bool) (result Result, err error)
+	ValidateManagementAccess(credentialsChanged 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(force bool) (result Result, details *metal3v1alpha1.HardwareDetails, err error)
+	InspectHardware() (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(force bool) (result Result, err error)
+	Deprovision() (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 deletion operation is completed.
+	// flag until the deprovisioning 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 3c02a2b9..03df5bbc 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -1310,3 +1310,27 @@ 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 96834305..7f717658 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -234,9 +234,8 @@ func TestErrorCountIncreasedWhenRegistrationFails(t *testing.T) {
 func TestErrorCountCleared(t *testing.T) {
 
 	tests := []struct {
-		Scenario                     string
-		Host                         *metal3v1alpha1.BareMetalHost
-		PreserveErrorCountOnComplete bool
+		Scenario string
+		Host     *metal3v1alpha1.BareMetalHost
 	}{
 		{
 			Scenario: "registering",
@@ -247,9 +246,8 @@ func TestErrorCountCleared(t *testing.T) {
 			Host:     host(metal3v1alpha1.StateInspecting).build(),
 		},
 		{
-			Scenario:                     "ready",
-			Host:                         host(metal3v1alpha1.StateReady).build(),
-			PreserveErrorCountOnComplete: true,
+			Scenario: "ready",
+			Host:     host(metal3v1alpha1.StateReady).build(),
 		},
 		{
 			Scenario: "deprovisioning",
@@ -260,9 +258,8 @@ func TestErrorCountCleared(t *testing.T) {
 			Host:     host(metal3v1alpha1.StateProvisioning).SetImageURL("imageSpecUrl").build(),
 		},
 		{
-			Scenario:                     "externallyProvisioned",
-			Host:                         host(metal3v1alpha1.StateExternallyProvisioned).SetExternallyProvisioned().build(),
-			PreserveErrorCountOnComplete: true,
+			Scenario: "externallyProvisioned",
+			Host:     host(metal3v1alpha1.StateExternallyProvisioned).SetExternallyProvisioned().build(),
 		},
 	}
 	for _, tt := range tests {
@@ -275,16 +272,8 @@ func TestErrorCountCleared(t *testing.T) {
 			prov.setNextResult(true)
 			result := hsm.ReconcileState(info)
 
-			assert.Equal(t, 1, tt.Host.Status.ErrorCount)
+			assert.Equal(t, tt.Host.Status.ErrorCount, 0)
 			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)
-			}
 		})
 	}
 }
@@ -366,12 +355,11 @@ func (m *mockProvisioner) setNextResult(dirty bool) {
 	}
 }
 
-func (m *mockProvisioner) ValidateManagementAccess(credentialsChanged, force bool) (result provisioner.Result, err error) {
+func (m *mockProvisioner) ValidateManagementAccess(credentialsChanged bool) (result provisioner.Result, err error) {
 	return m.nextResult, err
 }
 
-func (m *mockProvisioner) InspectHardware(force bool) (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
-	details = &metal3v1alpha1.HardwareDetails{}
+func (m *mockProvisioner) InspectHardware() (result provisioner.Result, details *metal3v1alpha1.HardwareDetails, err error) {
 	return m.nextResult, details, err
 }
 
@@ -387,7 +375,7 @@ func (m *mockProvisioner) Provision(configData provisioner.HostConfigData) (resu
 	return m.nextResult, err
 }
 
-func (m *mockProvisioner) Deprovision(force bool) (result provisioner.Result, err error) {
+func (m *mockProvisioner) Deprovision() (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 7118cb7c..b094dffe 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(false)
+			result, details, err := prov.InspectHardware()
 
 			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 8c463ffa..6b6038ff 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -125,7 +125,6 @@ func TestDeprovision(t *testing.T) {
 		ironic               *testserver.IronicMock
 		expectedDirty        bool
 		expectedError        bool
-		expectedErrorMessage bool
 		expectedRequestAfter int
 	}{
 		{
@@ -143,7 +142,8 @@ func TestDeprovision(t *testing.T) {
 				ProvisionState: string(nodes.Error),
 				UUID:           nodeUUID,
 			}),
-			expectedErrorMessage: true,
+			expectedRequestAfter: 10,
+			expectedDirty:        true,
 		},
 		{
 			name: "available state",
@@ -207,10 +207,9 @@ func TestDeprovision(t *testing.T) {
 			}
 
 			prov.status.ID = nodeUUID
-			result, err := prov.Deprovision(false)
+			result, err := prov.Deprovision()
 
 			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 13f49777..522b0bec 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, false)
+	result, err := prov.ValidateManagementAccess(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, false)
+	result, err := prov.ValidateManagementAccess(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, false)
+	result, err := prov.ValidateManagementAccess(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, false)
+	result, err := prov.ValidateManagementAccess(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, false)
+			result, err := prov.ValidateManagementAccess(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, false)
+			result, err := prov.ValidateManagementAccess(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, false)
+	result, err := prov.ValidateManagementAccess(true)
 	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, false)
+	result, err := prov.ValidateManagementAccess(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, false)
+	_, err = prov.ValidateManagementAccess(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, false)
+	_, err = prov.ValidateManagementAccess(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, false)
+	result, err := prov.ValidateManagementAccess(false)
 	if err != nil {
 		t.Fatalf("error from ValidateManagementAccess: %s", err)
 	}