Repository: metal3-io/baremetal-operator
Pull request #772 (2021-01-20 20:41:38)

Ironic: Don't adopt after clean failure during deprovisioning
During deprovisioning of a Host, if 'deleting' (i.e. deprovisioning) the
node succeeds (i.e. it doesn't go to the Error state) but the automated
cleaning that follows fails, the only way to recover is to return the
node to the manageable state.

Previously, once in the manageable state we would attempt adoption on
the node so that we could deprovision again. However, in the course of
'deleting' the node, the image information is cleared from it so it
cannot be adopted again. (Adoption continues to be the right thing to do
if the node has just been re-registered due to the Ironic database being
recreated, and in that case the image information is present since it
gets added during the initial registration.)

To work around this, don't attempt to adopt during the Deprovisioning
state if the node is manageable and the image data is not present.
Handle the manageable state in Deprovision() by declaring the
deprovisioning complete.

A node in the manageable state cannot be re-provisioned without first
being cleaned - it must go through cleaning to reach the available state
before it can be provisioned. Provisioning already handles nodes in the
manageable state, as this is how they begin after the initial inspection
of the host before the first provisioning (which does the initial
cleaning).

In the long term it might be better to declare Deprovisioning complete as soon as cleaning _begins_, and move the Host to a new Preparing state which will retry the cleaning until the node can be moved to `available`. In the meantime though, this should resolve the bug.


Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
index 8865d6f7..aa63aefb 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_types.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -123,9 +123,6 @@ const (
 type ErrorType string
 
 const (
-	// ProvisionedRegistrationError is an error condition occurring when the controller
-	// is unable to re-register an already provisioned host.
-	ProvisionedRegistrationError ErrorType = "provisioned registration error"
 	// RegistrationError is an error condition occurring when the
 	// controller is unable to connect to the Host's baseboard management
 	// controller.
@@ -300,18 +297,15 @@ type Image struct {
 	URL string `json:"url"`
 
 	// Checksum is the checksum for the image.
-	Checksum string `json:"checksum,omitempty"`
+	Checksum string `json:"checksum"`
 
 	// ChecksumType is the checksum algorithm for the image.
 	// e.g md5, sha256, sha512
 	ChecksumType ChecksumType `json:"checksumType,omitempty"`
 
-	// DiskFormat contains the format of the image (raw, qcow2, ...).
-	// Needs to be set to raw for raw images streaming.
-	// Note live-iso means an iso referenced by the url will be live-booted
-	// and not deployed to disk, and in this case the checksum options
-	// are not required and if specified will be ignored.
-	// +kubebuilder:validation:Enum=raw;qcow2;vdi;vmdk;live-iso
+	// DiskFormat contains the format of the image (raw, qcow2, ...)
+	// Needs to be set to raw for raw images streaming
+	// +kubebuilder:validation:Enum=raw;qcow2;vdi;vmdk
 	DiskFormat *string `json:"format,omitempty"`
 }
 
@@ -528,7 +522,7 @@ type BareMetalHostStatus struct {
 
 	// ErrorType indicates the type of failure encountered when the
 	// OperationalStatus is OperationalStatusError
-	// +kubebuilder:validation:Enum=provisioned registration error;registration error;inspection error;provisioning error;power management error
+	// +kubebuilder:validation:Enum=registration error;inspection error;provisioning error;power management error
 	ErrorType ErrorType `json:"errorType,omitempty"`
 
 	// LastUpdated identifies when this status was last observed.
@@ -818,12 +812,6 @@ func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
 		return
 	}
 
-	if image.DiskFormat != nil && *image.DiskFormat == "live-iso" {
-		// Checksum is not required for live-iso
-		ok = true
-		return
-	}
-
 	if image.Checksum == "" {
 		// Return empty if checksum is not provided
 		return
diff --git a/config/crd/bases/metal3.io_baremetalhosts.yaml b/config/crd/bases/metal3.io_baremetalhosts.yaml
index 2ef56a59..c957b003 100644
--- a/config/crd/bases/metal3.io_baremetalhosts.yaml
+++ b/config/crd/bases/metal3.io_baremetalhosts.yaml
@@ -141,18 +141,18 @@ spec:
                     - sha512
                     type: string
                   format:
-                    description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
+                    description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
                     enum:
                     - raw
                     - qcow2
                     - vdi
                     - vmdk
-                    - live-iso
                     type: string
                   url:
                     description: URL is a location of an image to deploy.
                     type: string
                 required:
+                - checksum
                 - url
                 type: object
               metaData:
@@ -262,7 +262,6 @@ spec:
               errorType:
                 description: ErrorType indicates the type of failure encountered when the OperationalStatus is OperationalStatusError
                 enum:
-                - provisioned registration error
                 - registration error
                 - inspection error
                 - provisioning error
@@ -555,18 +554,18 @@ spec:
                         - sha512
                         type: string
                       format:
-                        description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
+                        description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
                         enum:
                         - raw
                         - qcow2
                         - vdi
                         - vmdk
-                        - live-iso
                         type: string
                       url:
                         description: URL is a location of an image to deploy.
                         type: string
                     required:
+                    - checksum
                     - url
                     type: object
                   rootDeviceHints:
diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml
index 20ceb5dc..88532073 100644
--- a/config/render/capm3.yaml
+++ b/config/render/capm3.yaml
@@ -139,18 +139,18 @@ spec:
                     - sha512
                     type: string
                   format:
-                    description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
+                    description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
                     enum:
                     - raw
                     - qcow2
                     - vdi
                     - vmdk
-                    - live-iso
                     type: string
                   url:
                     description: URL is a location of an image to deploy.
                     type: string
                 required:
+                - checksum
                 - url
                 type: object
               metaData:
@@ -260,7 +260,6 @@ spec:
               errorType:
                 description: ErrorType indicates the type of failure encountered when the OperationalStatus is OperationalStatusError
                 enum:
-                - provisioned registration error
                 - registration error
                 - inspection error
                 - provisioning error
@@ -553,18 +552,18 @@ spec:
                         - sha512
                         type: string
                       format:
-                        description: DiskFormat contains the format of the image (raw, qcow2, ...). Needs to be set to raw for raw images streaming. Note live-iso means an iso referenced by the url will be live-booted and not deployed to disk, and in this case the checksum options are not required and if specified will be ignored.
+                        description: DiskFormat contains the format of the image (raw, qcow2, ...) Needs to be set to raw for raw images streaming
                         enum:
                         - raw
                         - qcow2
                         - vdi
                         - vmdk
-                        - live-iso
                         type: string
                       url:
                         description: URL is a location of an image to deploy.
                         type: string
                     required:
+                    - checksum
                     - url
                     type: object
                   rootDeviceHints:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index d5967ce2..d940abbf 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -280,11 +280,10 @@ func recordActionFailure(info *reconcileInfo, errorType metal3v1alpha1.ErrorType
 	setErrorMessage(info.host, errorType, errorMessage)
 
 	eventType := map[metal3v1alpha1.ErrorType]string{
-		metal3v1alpha1.ProvisionedRegistrationError: "ProvisionedRegistrationError",
-		metal3v1alpha1.RegistrationError:            "RegistrationError",
-		metal3v1alpha1.InspectionError:              "InspectionError",
-		metal3v1alpha1.ProvisioningError:            "ProvisioningError",
-		metal3v1alpha1.PowerManagementError:         "PowerManagementError",
+		metal3v1alpha1.RegistrationError:    "RegistrationError",
+		metal3v1alpha1.InspectionError:      "InspectionError",
+		metal3v1alpha1.ProvisioningError:    "ProvisioningError",
+		metal3v1alpha1.PowerManagementError: "PowerManagementError",
 	}[errorType]
 
 	counter := actionFailureCounters.WithLabelValues(eventType)
@@ -468,8 +467,7 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 	// Reaching this point means the credentials are valid and worked,
 	// so clear any previous error and record the success in the
 	// status block.
-	registeredNewCreds := !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret)
-	if registeredNewCreds {
+	if !info.host.Status.GoodCredentials.Match(*info.bmcCredsSecret) {
 		info.log.Info("updating credentials success status fields")
 		info.host.UpdateGoodCredentials(*info.bmcCredsSecret)
 		info.publishEvent("BMCAccessValidated", "Verified access to BMC")
@@ -477,10 +475,9 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
 	} else {
 		info.log.Info("verified access to the BMC")
 	}
-
-	if info.host.Status.ErrorType == metal3v1alpha1.RegistrationError || registeredNewCreds {
+	if clearError(info.host) {
 		info.log.Info("clearing previous error message")
-		dirty = clearError(info.host)
+		dirty = true
 	}
 
 	if dirty {
@@ -618,12 +615,12 @@ func (r *BareMetalHostReconciler) actionDeprovisioning(prov provisioner.Provisio
 	if info.host.Status.Provisioning.Image.URL != "" {
 		// Adopt the host in case it has been re-registered during the
 		// deprovisioning process before it completed
-		provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.ProvisionedRegistrationError)
+		provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
 		if err != nil {
 			return actionError{err}
 		}
 		if provResult.ErrorMessage != "" {
-			return recordActionFailure(info, metal3v1alpha1.ProvisionedRegistrationError, provResult.ErrorMessage)
+			return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
 		}
 		if provResult.Dirty {
 			result := actionContinue{provResult.RequeueAfter}
@@ -761,12 +758,12 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 // action. We use the Adopt() API to make sure that the provisioner is aware of
 // the provisioning details. Then we monitor its power status.
 func (r *BareMetalHostReconciler) actionManageSteadyState(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
-	provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.ProvisionedRegistrationError)
+	provResult, err := prov.Adopt(info.host.Status.ErrorType == metal3v1alpha1.RegistrationError)
 	if err != nil {
 		return actionError{err}
 	}
 	if provResult.ErrorMessage != "" {
-		return recordActionFailure(info, metal3v1alpha1.ProvisionedRegistrationError, provResult.ErrorMessage)
+		return recordActionFailure(info, metal3v1alpha1.RegistrationError, provResult.ErrorMessage)
 	}
 	if provResult.Dirty {
 		result := actionContinue{provResult.RequeueAfter}
diff --git a/docs/api.md b/docs/api.md
index f5011ff5..3a16ea43 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -92,10 +92,8 @@ The sub-fields are
   only `md5`, `sha256`, `sha512` are recognized. If nothing is specified
   `md5` is assumed.
 * *format* -- This is the disk format of the image. It can be one of `raw`,
-  `qcow2`, `vdi`, `vmdk`, `live-iso` or be left unset.
-  Setting it to raw enables raw image streaming in Ironic agent for that image.
-  Setting it to live-iso enables iso images to live boot without deploying
-  to disk, in this case the checksum fields are ignored.
+  `qcow2`, `vdi`, `vmdk`, or be left unset. Setting it to raw enables raw
+  image streaming in Ironic agent for that image.
 
 Even though the image sub-fields are required by Ironic,
 when the host provisioning is managed externally via `externallyProvisioned: true`,
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 9e364fcb..baaca4d8 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -372,7 +372,6 @@ func (p *ironicProvisioner) ValidateManagementAccess(credentialsChanged, force b
 				BootInterface:       p.bmcAccess.BootInterface(),
 				Name:                p.host.Name,
 				DriverInfo:          driverInfo,
-				DeployInterface:     p.deployInterface(),
 				InspectInterface:    "inspector",
 				ManagementInterface: p.bmcAccess.ManagementInterface(),
 				PowerInterface:      p.bmcAccess.PowerInterface(),
@@ -722,7 +721,7 @@ func (p *ironicProvisioner) UpdateHardwareState() (hwState provisioner.HardwareS
 }
 
 func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, imageData *metal3v1alpha1.Image) (updates nodes.UpdateOpts, err error) {
-	checksum, checksumType, ok := imageData.GetChecksum()
+	checksum, checksumType, ok := p.host.GetImageChecksum()
 	if !ok {
 		p.log.Info("image/checksum not found for host")
 		return
@@ -737,72 +736,8 @@ func (p *ironicProvisioner) getImageUpdateOptsForNode(ironicNode *nodes.Node, im
 			Value: string(p.host.ObjectMeta.UID),
 		},
 	)
-	// live-iso format
-	var op nodes.UpdateOp
-	if imageData.DiskFormat != nil && *imageData.DiskFormat == "live-iso" {
-		if _, ok := ironicNode.InstanceInfo["boot_iso"]; !ok {
-			op = nodes.AddOp
-			p.log.Info("adding boot_iso")
-		} else {
-			op = nodes.ReplaceOp
-			p.log.Info("updating boot_iso")
-		}
-		updates = append(
-			updates,
-			nodes.UpdateOperation{
-				Op:    op,
-				Path:  "/instance_info/boot_iso",
-				Value: imageData.URL,
-			},
-		)
-		updates = append(
-			updates,
-			nodes.UpdateOperation{
-				Op:    nodes.ReplaceOp,
-				Path:  "/deploy_interface",
-				Value: "ramdisk",
-			},
-		)
-		// remove any image_source or checksum options
-		removals := []string{
-			"image_source", "image_os_hash_value", "image_os_hash_algo", "image_checksum"}
-		op = nodes.RemoveOp
-		for _, item := range removals {
-			if _, ok := ironicNode.InstanceInfo[item]; ok {
-				p.log.Info("removing " + item)
-				updates = append(
-					updates,
-					nodes.UpdateOperation{
-						Op:   op,
-						Path: "/instance_info/" + item,
-					},
-				)
-			}
-		}
-		return updates, nil
-	}
-
-	// Set deploy_interface direct when not booting a live-iso
-	updates = append(
-		updates,
-		nodes.UpdateOperation{
-			Op:    nodes.ReplaceOp,
-			Path:  "/deploy_interface",
-			Value: "direct",
-		},
-	)
-	// Remove any boot_iso field
-	if _, ok := ironicNode.InstanceInfo["boot_iso"]; ok {
-		p.log.Info("removing boot_iso")
-		updates = append(
-			updates,
-			nodes.UpdateOperation{
-				Op:   nodes.RemoveOp,
-				Path: "/instance_info/boot_iso",
-			},
-		)
-	}
 	// image_source
+	var op nodes.UpdateOp
 	if _, ok := ironicNode.InstanceInfo["image_source"]; !ok {
 		op = nodes.AddOp
 		p.log.Info("adding image_source")
@@ -905,13 +840,34 @@ func (p *ironicProvisioner) getUpdateOptsForNode(ironicNode *nodes.Node) (update
 	}
 	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")
+	} else {
+		op = nodes.ReplaceOp
+		p.log.Info("updating root_gb")
+	}
+	updates = append(
+		updates,
+		nodes.UpdateOperation{
+			Op:    op,
+			Path:  "/instance_info/root_gb",
+			Value: hwProf.RootGB,
+		},
+	)
+
 	// root_device
 	//
 	// FIXME(dhellmann): We need to specify the root device to receive
 	// the image. That should come from some combination of inspecting
 	// the host to see what is available and the hardware profile to
 	// give us instructions.
-	var op nodes.UpdateOp
 	if _, ok := ironicNode.Properties["root_device"]; !ok {
 		op = nodes.AddOp
 		p.log.Info("adding root_device")
@@ -1081,14 +1037,6 @@ func (p *ironicProvisioner) setUpForProvisioning(ironicNode *nodes.Node, hostCon
 	return
 }
 
-func (p *ironicProvisioner) deployInterface() (result string) {
-	result = "direct"
-	if p.host.Spec.Image != nil && p.host.Spec.Image.DiskFormat != nil && *p.host.Spec.Image.DiskFormat == "live-iso" {
-		result = "ramdisk"
-	}
-	return result
-}
-
 // Adopt allows an externally-provisioned server to be adopted by Ironic.
 func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err error) {
 	var ironicNode *nodes.Node
@@ -1105,6 +1053,19 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er
 		return transientError(fmt.Errorf("Invalid state for adopt: %s",
 			ironicNode.ProvisionState))
 	case nodes.Manageable:
+		_, hasImageSource := ironicNode.InstanceInfo["image_source"]
+		_, hasBootISO := ironicNode.InstanceInfo["boot_iso"]
+		if p.status.State == metal3v1alpha1.StateDeprovisioning &&
+			!(hasImageSource || hasBootISO) {
+			// If we got here after a fresh registration and image data is
+			// available, it should have been added to the node during
+			// registration. If it isn't present then we got here due to a
+			// failed cleaning on deprovision. The node will be cleaned again
+			// before the next provisioning, so just allow the controller to
+			// continue without adopting.
+			p.log.Info("no image info; not adopting", "state", ironicNode.ProvisionState)
+			return operationComplete()
+		}
 		return p.changeNodeProvisionState(
 			ironicNode,
 			nodes.ProvisionStateOpts{
@@ -1131,30 +1092,6 @@ func (p *ironicProvisioner) Adopt(force bool) (result provisioner.Result, err er
 	return operationComplete()
 }
 
-func (p *ironicProvisioner) ironicHasSameImage(ironicNode *nodes.Node) (sameImage bool) {
-	// To make it easier to test if ironic is configured with
-	// the same image we are trying to provision to the host.
-	if p.host.Spec.Image != nil && p.host.Spec.Image.DiskFormat != nil && *p.host.Spec.Image.DiskFormat == "live-iso" {
-		sameImage = (ironicNode.InstanceInfo["boot_iso"] == p.host.Spec.Image.URL)
-		p.log.Info("checking image settings",
-			"boot_iso", ironicNode.InstanceInfo["boot_iso"],
-			"same", sameImage,
-			"provisionState", ironicNode.ProvisionState)
-	} else {
-		checksum, checksumType, _ := p.host.GetImageChecksum()
-		sameImage = (ironicNode.InstanceInfo["image_source"] == p.host.Spec.Image.URL &&
-			ironicNode.InstanceInfo["image_os_hash_algo"] == checksumType &&
-			ironicNode.InstanceInfo["image_os_hash_value"] == checksum)
-		p.log.Info("checking image settings",
-			"source", ironicNode.InstanceInfo["image_source"],
-			"image_os_hash_algo", checksumType,
-			"image_os_has_value", checksum,
-			"same", sameImage,
-			"provisionState", ironicNode.ProvisionState)
-	}
-	return sameImage
-}
-
 // Provision writes the image from the host spec to the host. It may
 // be called multiple times, and should return true for its dirty flag
 // until the deprovisioning operation is completed.
@@ -1170,7 +1107,20 @@ func (p *ironicProvisioner) Provision(hostConf provisioner.HostConfigData) (resu
 
 	p.log.Info("provisioning image to host", "state", ironicNode.ProvisionState)
 
-	ironicHasSameImage := p.ironicHasSameImage(ironicNode)
+	checksum, checksumType, _ := p.host.GetImageChecksum()
+
+	// Local variable to make it easier to test if ironic is
+	// configured with the same image we are trying to provision to
+	// the host.
+	ironicHasSameImage := (ironicNode.InstanceInfo["image_source"] == p.host.Spec.Image.URL &&
+		ironicNode.InstanceInfo["image_os_hash_algo"] == checksumType &&
+		ironicNode.InstanceInfo["image_os_hash_value"] == checksum)
+	p.log.Info("checking image settings",
+		"source", ironicNode.InstanceInfo["image_source"],
+		"image_os_hash_algo", checksumType,
+		"image_os_has_value", checksum,
+		"same", ironicHasSameImage,
+		"provisionState", ironicNode.ProvisionState)
 
 	// Ironic has the settings it needs, see if it finds any issues
 	// with them.
@@ -1361,6 +1311,7 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result,
 		)
 
 	case nodes.CleanFail:
+		p.log.Info("cleaning failed")
 		if ironicNode.Maintenance {
 			p.log.Info("clearing maintenance flag")
 			return p.setMaintenanceFlag(ironicNode, false)
@@ -1374,6 +1325,14 @@ func (p *ironicProvisioner) Deprovision(force bool) (result provisioner.Result,
 			nodes.ProvisionStateOpts{Target: nodes.TargetManage},
 		)
 
+	case nodes.Manageable:
+		// We end up here after CleanFail. Because cleaning happens in the
+		// process of moving from manageable to available, the node will still
+		// get cleaned before we provision it again. Therefore, just declare
+		// deprovisioning complete.
+		p.log.Info("deprovisioning node is in manageable state")
+		return operationComplete()
+
 	case nodes.Available:
 		p.publisher("DeprovisioningComplete", "Image deprovisioning completed")
 		return operationComplete()


Test patch:
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 58d9a3d2..51c1a971 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -289,59 +289,6 @@ func TestErrorCountCleared(t *testing.T) {
 	}
 }
 
-func TestErrorClean(t *testing.T) {
-
-	tests := []struct {
-		Scenario    string
-		Host        *metal3v1alpha1.BareMetalHost
-		SecretName  string
-		ExpectError bool
-	}{
-		{
-			Scenario: "clean-after-registration-error",
-			Host: host(metal3v1alpha1.StateInspecting).
-				SetStatusError(metal3v1alpha1.OperationalStatusError, metal3v1alpha1.RegistrationError, "some error", 1).
-				build(),
-		},
-		{
-			Scenario: "not-clean-after-provisioned-registration-error",
-			Host: host(metal3v1alpha1.StateInspecting).
-				SetStatusError(metal3v1alpha1.OperationalStatusError, metal3v1alpha1.ProvisionedRegistrationError, "some error", 1).
-				build(),
-			ExpectError: true,
-		},
-		{
-			Scenario: "clean-after-creds-change",
-			Host: host(metal3v1alpha1.StateReady).
-				SetStatusError(metal3v1alpha1.OperationalStatusError, metal3v1alpha1.InspectionError, "some error", 1).
-				build(),
-			SecretName: "NewCreds",
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.Scenario, func(t *testing.T) {
-			prov := &mockProvisioner{}
-			hsm := newHostStateMachine(tt.Host, &BareMetalHostReconciler{}, prov, true)
-
-			info := makeDefaultReconcileInfo(tt.Host)
-			if tt.SecretName != "" {
-				info.bmcCredsSecret.Name = tt.SecretName
-			}
-
-			hsm.ReconcileState(info)
-
-			if tt.ExpectError {
-				assert.Equal(t, tt.Host.Status.ErrorType, v1alpha1.ProvisionedRegistrationError)
-				assert.NotEmpty(t, tt.Host.Status.ErrorMessage)
-			} else {
-				assert.Equal(t, tt.Host.Status.OperationalStatus, v1alpha1.OperationalStatusOK)
-				assert.Empty(t, tt.Host.Status.ErrorType)
-				assert.Empty(t, tt.Host.Status.ErrorMessage)
-			}
-		})
-	}
-}
-
 type hostBuilder struct {
 	metal3v1alpha1.BareMetalHost
 }
@@ -387,15 +334,6 @@ func (hb *hostBuilder) SetImageURL(url string) *hostBuilder {
 	return hb
 }
 
-func (hb *hostBuilder) SetStatusError(opStatus metal3v1alpha1.OperationalStatus, errType metal3v1alpha1.ErrorType, errMsg string, errCount int) *hostBuilder {
-	hb.Status.OperationalStatus = opStatus
-	hb.Status.ErrorType = errType
-	hb.Status.ErrorMessage = errMsg
-	hb.Status.ErrorCount = errCount
-
-	return hb
-}
-
 func makeDefaultReconcileInfo(host *metal3v1alpha1.BareMetalHost) *reconcileInfo {
 	return &reconcileInfo{
 		log:     logf.Log.WithName("controllers").WithName("BareMetalHost").WithName("host_state_machine"),
diff --git a/pkg/provisioner/ironic/ironic_test.go b/pkg/provisioner/ironic/ironic_test.go
index f4d7bb7c..864609a3 100644
--- a/pkg/provisioner/ironic/ironic_test.go
+++ b/pkg/provisioner/ironic/ironic_test.go
@@ -71,12 +71,5 @@ func makeHost() metal3v1alpha1.BareMetalHost {
 	}
 }
 
-func makeHostLiveIso() (host metal3v1alpha1.BareMetalHost) {
-	host = makeHost()
-	format := "live-iso"
-	host.Spec.Image.DiskFormat = &format
-	return host
-}
-
 // Implements provisioner.EventPublisher to swallow events for tests.
 func nullEventPublisher(reason, message string) {}
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index 7f578915..66cc778c 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -8,7 +8,6 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
 	"github.com/stretchr/testify/assert"
 
-	"github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
@@ -199,6 +198,23 @@ func TestDeprovision(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
+		{
+			name: "clean fail state",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.CleanFail),
+				UUID:           nodeUUID,
+			}),
+			expectedRequestAfter: 10,
+			expectedDirty:        true,
+		},
+		{
+			name: "manageable state",
+			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
+				ProvisionState: string(nodes.Manageable),
+				UUID:           nodeUUID,
+			}),
+			expectedDirty: false,
+		},
 	}
 
 	for _, tc := range cases {
@@ -238,136 +254,3 @@ func TestDeprovision(t *testing.T) {
 		})
 	}
 }
-
-func TestIronicHasSameImage(t *testing.T) {
-	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
-	cases := []struct {
-		name             string
-		expected         bool
-		node             nodes.Node
-		liveImage        bool
-		hostImage        string
-		hostChecksum     string
-		hostChecksumType v1alpha1.ChecksumType
-	}{
-		{
-			name:      "image same",
-			expected:  true,
-			liveImage: false,
-			node: nodes.Node{
-				InstanceInfo: map[string]interface{}{
-					"image_source":        "theimage",
-					"image_os_hash_value": "thechecksum",
-					"image_os_hash_algo":  "md5",
-				},
-			},
-			hostImage:        "theimage",
-			hostChecksum:     "thechecksum",
-			hostChecksumType: v1alpha1.MD5,
-		},
-		{
-			name:      "image different",
-			expected:  false,
-			liveImage: false,
-			node: nodes.Node{
-				InstanceInfo: map[string]interface{}{
-					"image_source":        "theimage",
-					"image_os_hash_value": "thechecksum",
-					"image_os_hash_algo":  "md5",
-				},
-			},
-			hostImage:        "different",
-			hostChecksum:     "thechecksum",
-			hostChecksumType: v1alpha1.MD5,
-		},
-		{
-			name:      "image checksum different",
-			expected:  false,
-			liveImage: false,
-			node: nodes.Node{
-				InstanceInfo: map[string]interface{}{
-					"image_source":        "theimage",
-					"image_os_hash_value": "thechecksum",
-					"image_os_hash_algo":  "md5",
-				},
-			},
-			hostImage:        "theimage",
-			hostChecksum:     "different",
-			hostChecksumType: v1alpha1.MD5,
-		},
-		{
-			name:      "image checksum type different",
-			expected:  false,
-			liveImage: false,
-			node: nodes.Node{
-				InstanceInfo: map[string]interface{}{
-					"image_source":        "theimage",
-					"image_os_hash_value": "thechecksum",
-					"image_os_hash_algo":  "md5",
-				},
-			},
-			hostImage:        "theimage",
-			hostChecksum:     "thechecksum",
-			hostChecksumType: v1alpha1.SHA512,
-		},
-		{
-			name:      "live image same",
-			liveImage: true,
-			expected:  true,
-			node: nodes.Node{
-				InstanceInfo: map[string]interface{}{
-					"boot_iso": "theimage",
-				},
-			},
-			hostImage: "theimage",
-		},
-		{
-			name:      "live image different",
-			liveImage: true,
-			expected:  false,
-			node: nodes.Node{
-				InstanceInfo: map[string]interface{}{
-					"boot_iso": "theimage",
-				},
-			},
-			hostImage: "different",
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			ironic := testserver.NewIronic(t).WithDefaultResponses().Node(tc.node)
-			ironic.Start()
-			defer ironic.Stop()
-
-			inspector := testserver.NewInspector(t).Ready().WithIntrospection(nodeUUID, introspection.Introspection{
-				Finished: false,
-			})
-			inspector.Start()
-			defer inspector.Stop()
-
-			var host v1alpha1.BareMetalHost
-			if tc.liveImage {
-				host = makeHostLiveIso()
-				host.Spec.Image.URL = tc.hostImage
-			} else {
-				host = makeHost()
-				host.Spec.Image.URL = tc.hostImage
-				host.Spec.Image.Checksum = tc.hostChecksum
-				host.Spec.Image.ChecksumType = tc.hostChecksumType
-			}
-			publisher := func(reason, message string) {}
-			auth := clients.AuthConfig{Type: clients.NoAuth}
-			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
-				ironic.Endpoint(), auth, inspector.Endpoint(), auth,
-			)
-			if err != nil {
-				t.Fatalf("could not create provisioner: %s", err)
-			}
-
-			prov.status.ID = nodeUUID
-			sameImage := prov.ironicHasSameImage(&tc.node)
-			assert.Equal(t, tc.expected, sameImage)
-		})
-	}
-}
diff --git a/pkg/provisioner/ironic/testserver/ironic.go b/pkg/provisioner/ironic/testserver/ironic.go
index e5e825a2..6a3dea91 100644
--- a/pkg/provisioner/ironic/testserver/ironic.go
+++ b/pkg/provisioner/ironic/testserver/ironic.go
@@ -151,7 +151,7 @@ func (m *IronicMock) WithNodeStatesProvision(nodeUUID string) *IronicMock {
 	return m.withNodeStatesProvision(nodeUUID, http.MethodGet)
 }
 
-// WithNodeStatesProvisionUpdate configures the server with a valid response for [PATCH] /v1/nodes/<node>/states/provision
+// WithNodeStatesProvision configures the server with a valid response for [PATCH] /v1/nodes/<node>/states/provision
 func (m *IronicMock) WithNodeStatesProvisionUpdate(nodeUUID string) *IronicMock {
 	return m.withNodeStatesProvision(nodeUUID, http.MethodPut)
 }
diff --git a/pkg/provisioner/ironic/updateopts_test.go b/pkg/provisioner/ironic/updateopts_test.go
index 92fce010..d712064f 100644
--- a/pkg/provisioner/ironic/updateopts_test.go
+++ b/pkg/provisioner/ironic/updateopts_test.go
@@ -153,6 +153,10 @@ func TestGetUpdateOptsForNodeVirtual(t *testing.T) {
 			Path:  "/instance_uuid",
 			Value: "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
 		},
+		{
+			Path:  "/instance_info/root_gb",
+			Value: 10,
+		},
 		{
 			Path:  "/properties/cpu_arch",
 			Value: "x86_64",
@@ -249,6 +253,10 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 			Path:  "/instance_uuid",
 			Value: "27720611-e5d1-45d3-ba3a-222dcfaa4ca2",
 		},
+		{
+			Path:  "/instance_info/root_gb",
+			Value: 10,
+		},
 		{
 			Path:  "/properties/cpu_arch",
 			Value: "x86_64",
@@ -278,216 +286,3 @@ func TestGetUpdateOptsForNodeDell(t *testing.T) {
 		})
 	}
 }
-
-func TestGetUpdateOptsForNodeLiveIso(t *testing.T) {
-	eventPublisher := func(reason, message string) {}
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-
-	prov, err := newProvisionerWithSettings(makeHostLiveIso(), bmc.Credentials{}, eventPublisher,
-		"https://ironic.test", auth, "https://ironic.test", auth,
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	ironicNode := &nodes.Node{}
-
-	patches, err := prov.getUpdateOptsForNode(ironicNode)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Logf("patches: %v", patches)
-
-	expected := []struct {
-		Path  string         // the node property path
-		Key   string         // if value is a map, the key we care about
-		Value interface{}    // the value being passed to ironic (or value associated with the key)
-		Op    nodes.UpdateOp // The operation add/replace/remove
-	}{
-		{
-			Path:  "/instance_info/boot_iso",
-			Value: "not-empty",
-			Op:    nodes.AddOp,
-		},
-		{
-			Path:  "/deploy_interface",
-			Value: "ramdisk",
-			Op:    nodes.ReplaceOp,
-		},
-	}
-
-	for _, e := range expected {
-		t.Run(e.Path, func(t *testing.T) {
-			t.Logf("expected: %v", e)
-			var update nodes.UpdateOperation
-			for _, patch := range patches {
-				update = patch.(nodes.UpdateOperation)
-				if update.Path == e.Path {
-					break
-				}
-			}
-			if update.Path != e.Path {
-				t.Errorf("did not find %q in updates", e.Path)
-				return
-			}
-			t.Logf("update: %v", update)
-			assert.Equal(t, e.Value, update.Value, fmt.Sprintf("%s does not match", e.Path))
-		})
-	}
-}
-
-func TestGetUpdateOptsForNodeImageToLiveIso(t *testing.T) {
-	eventPublisher := func(reason, message string) {}
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-
-	prov, err := newProvisionerWithSettings(makeHostLiveIso(), bmc.Credentials{}, eventPublisher,
-		"https://ironic.test", auth, "https://ironic.test", auth,
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	ironicNode := &nodes.Node{
-		InstanceInfo: map[string]interface{}{
-			"image_source":        "oldimage",
-			"image_os_hash_value": "thechecksum",
-			"image_os_hash_algo":  "md5",
-		},
-	}
-
-	patches, err := prov.getUpdateOptsForNode(ironicNode)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Logf("patches: %v", patches)
-
-	expected := []struct {
-		Path  string         // the node property path
-		Key   string         // if value is a map, the key we care about
-		Value interface{}    // the value being passed to ironic (or value associated with the key)
-		Op    nodes.UpdateOp // The operation add/replace/remove
-	}{
-		{
-			Path:  "/instance_info/boot_iso",
-			Value: "not-empty",
-			Op:    nodes.AddOp,
-		},
-		{
-			Path:  "/deploy_interface",
-			Value: "ramdisk",
-			Op:    nodes.ReplaceOp,
-		},
-		{
-			Path: "/instance_info/image_source",
-			Op:   nodes.RemoveOp,
-		},
-		{
-			Path: "/instance_info/image_os_hash_algo",
-			Op:   nodes.RemoveOp,
-		},
-		{
-			Path: "/instance_info/image_os_hash_value",
-			Op:   nodes.RemoveOp,
-		},
-	}
-
-	for _, e := range expected {
-		t.Run(e.Path, func(t *testing.T) {
-			t.Logf("expected: %v", e)
-			var update nodes.UpdateOperation
-			for _, patch := range patches {
-				update = patch.(nodes.UpdateOperation)
-				if update.Path == e.Path {
-					break
-				}
-			}
-			if update.Path != e.Path {
-				t.Errorf("did not find %q in updates", e.Path)
-				return
-			}
-			t.Logf("update: %v", update)
-			assert.Equal(t, e.Value, update.Value, fmt.Sprintf("%s value does not match", e.Path))
-			assert.Equal(t, e.Op, update.Op, fmt.Sprintf("%s operation does not match", e.Path))
-		})
-	}
-}
-
-func TestGetUpdateOptsForNodeLiveIsoToImage(t *testing.T) {
-	eventPublisher := func(reason, message string) {}
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-
-	host := makeHost()
-	host.Spec.Image.URL = "newimage"
-	host.Spec.Image.Checksum = "thechecksum"
-	host.Spec.Image.ChecksumType = metal3v1alpha1.MD5
-	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, eventPublisher,
-		"https://ironic.test", auth, "https://ironic.test", auth,
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	ironicNode := &nodes.Node{
-		InstanceInfo: map[string]interface{}{
-			"boot_iso": "oldimage",
-		},
-	}
-
-	patches, err := prov.getUpdateOptsForNode(ironicNode)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	t.Logf("patches: %v", patches)
-
-	expected := []struct {
-		Path  string         // the node property path
-		Key   string         // if value is a map, the key we care about
-		Value interface{}    // the value being passed to ironic (or value associated with the key)
-		Op    nodes.UpdateOp // The operation add/replace/remove
-	}{
-		{
-			Path: "/instance_info/boot_iso",
-			Op:   nodes.RemoveOp,
-		},
-		{
-			Path:  "/deploy_interface",
-			Value: "direct",
-			Op:    nodes.ReplaceOp,
-		},
-		{
-			Path:  "/instance_info/image_source",
-			Value: "newimage",
-			Op:    nodes.AddOp,
-		},
-		{
-			Path:  "/instance_info/image_os_hash_algo",
-			Value: "md5",
-			Op:    nodes.AddOp,
-		},
-		{
-			Path:  "/instance_info/image_os_hash_value",
-			Value: "thechecksum",
-			Op:    nodes.AddOp,
-		},
-	}
-
-	for _, e := range expected {
-		t.Run(e.Path, func(t *testing.T) {
-			t.Logf("expected: %v", e)
-			var update nodes.UpdateOperation
-			for _, patch := range patches {
-				update = patch.(nodes.UpdateOperation)
-				if update.Path == e.Path {
-					break
-				}
-			}
-			if update.Path != e.Path {
-				t.Errorf("did not find %q in updates", e.Path)
-				return
-			}
-			t.Logf("update: %v", update)
-			assert.Equal(t, e.Value, update.Value, fmt.Sprintf("%s value does not match", e.Path))
-			assert.Equal(t, e.Op, update.Op, fmt.Sprintf("%s operation does not match", e.Path))
-		})
-	}
-}
diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
index 40624a4d..378d62ff 100644
--- a/pkg/provisioner/ironic/validatemanagementaccess_test.go
+++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -102,83 +102,6 @@ func TestValidateManagementAccessCreateNode(t *testing.T) {
 	assert.Equal(t, "", result.ErrorMessage)
 	assert.NotEqual(t, "", createdNode.UUID)
 	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, createdNode.DeployInterface, "direct")
-}
-
-func TestValidateManagementAccessCreateWithImage(t *testing.T) {
-	// Create a host with Image specified in the Spec
-	host := makeHost()
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-	host.Spec.Image.URL = "theimagefoo"
-	host.Spec.Image.Checksum = "thechecksumxyz"
-
-	var createdNode *nodes.Node
-
-	createCallback := func(node nodes.Node) {
-		createdNode = &node
-	}
-
-	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).NoNode(host.Name)
-	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
-	ironic.Start()
-	defer ironic.Stop()
-
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher,
-		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
-	)
-	if err != nil {
-		t.Fatalf("could not create provisioner: %s", err)
-	}
-
-	result, provID, err := prov.ValidateManagementAccess(false, false)
-	if err != nil {
-		t.Fatalf("error from ValidateManagementAccess: %s", err)
-	}
-	assert.Equal(t, "", result.ErrorMessage)
-	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, createdNode.DeployInterface, "direct")
-	updates, _ := ironic.GetLastRequestFor("/v1/nodes/node-0", http.MethodPatch)
-	assert.Contains(t, updates, "/instance_info/image_source")
-	assert.Contains(t, updates, host.Spec.Image.URL)
-	assert.Contains(t, updates, "/instance_info/image_checksum")
-	assert.Contains(t, updates, host.Spec.Image.Checksum)
-}
-
-func TestValidateManagementAccessCreateWithLiveIso(t *testing.T) {
-	// Create a host with Image specified in the Spec
-	host := makeHostLiveIso()
-	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
-
-	var createdNode *nodes.Node
-
-	createCallback := func(node nodes.Node) {
-		createdNode = &node
-	}
-
-	ironic := testserver.NewIronic(t).Ready().CreateNodes(createCallback).NoNode(host.Name)
-	ironic.AddDefaultResponse("/v1/nodes/node-0", "PATCH", http.StatusOK, "{}")
-	ironic.Start()
-	defer ironic.Stop()
-
-	auth := clients.AuthConfig{Type: clients.NoAuth}
-	prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, nullEventPublisher,
-		ironic.Endpoint(), auth, testserver.NewInspector(t).Endpoint(), auth,
-	)
-	if err != nil {
-		t.Fatalf("could not create provisioner: %s", err)
-	}
-
-	result, provID, err := prov.ValidateManagementAccess(false, false)
-	if err != nil {
-		t.Fatalf("error from ValidateManagementAccess: %s", err)
-	}
-	assert.Equal(t, "", result.ErrorMessage)
-	assert.Equal(t, createdNode.UUID, provID)
-	assert.Equal(t, createdNode.DeployInterface, "ramdisk")
-	updates, _ := ironic.GetLastRequestFor("/v1/nodes/node-0", http.MethodPatch)
-	assert.Contains(t, updates, "/instance_info/boot_iso")
-	assert.Contains(t, updates, host.Spec.Image.URL)
 }
 
 func TestValidateManagementAccessExistingNode(t *testing.T) {