Repository: metal3-io/baremetal-operator
Pull request #1793 (2024-06-25 02:57:21)

🐛  Fix HFC to execute updates
Currently any changes in HostFirmwareComponents won't trigger the firmware update on it.
This commit aims to fix the issues we have observed while testing with real hardware.

**What this PR does / why we need it**: metal3-io/metal3-docs#364


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

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [lentzi90](https://github.com/lentzi90) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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

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

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

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":["lentzi90"]} -->
tuminoid: We probably want to to cherry-pick this to release-0.6 when done, if HFC doesn't work?

iurygregory: @tuminoid correct
metal3-io-bot: @tuminoid: once the present PR merges, I will cherry-pick it on top of release-0.6 in a new PR and assign it to you.

<details>

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

>> @tuminoid correct
>
>Let's schedule the bot then to do the pick:
>/cherry-pick release-0.6
>


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>

Patch:
diff --git a/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go b/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go
index e6fe015f..885ba717 100644
--- a/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go
+++ b/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go
@@ -57,7 +57,7 @@ type HostFirmwareComponentsStatus struct {
 	// Updates is the list of all firmware components that should be updated
 	// they are specified via name and url fields.
 	// +optional
-	Updates []FirmwareUpdate `json:"updates"`
+	Updates []FirmwareUpdate `json:"updates,omitempty"`
 
 	// Components is the list of all available firmware components and their information.
 	Components []FirmwareComponentStatus `json:"components,omitempty"`
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 47ac08cf..a779aa77 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -1164,6 +1164,20 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner,
 		return recordActionFailure(info, metal3api.PreparationError, provResult.ErrorMessage)
 	}
 
+	if hfcDirty && started {
+		hfcStillDirty, err := r.saveHostFirmwareComponents(prov, info, hfc)
+		if err != nil {
+			return actionError{errors.Wrap(err, "could not save the host firmware components")}
+		}
+
+		if hfcStillDirty {
+			info.log.Info("going to update the host firmware components")
+			if err := r.Status().Update(info.ctx, hfc); err != nil {
+				return actionError{errors.Wrap(err, "failed to update hostfirmwarecomponents status")}
+			}
+		}
+	}
+
 	if bmhDirty && started {
 		info.log.Info("saving host provisioning settings")
 		_, err := saveHostProvisioningSettings(info.host, info)
@@ -1740,6 +1754,32 @@ func saveHostProvisioningSettings(host *metal3api.BareMetalHost, info *reconcile
 	return dirty, nil
 }
 
+func (r *BareMetalHostReconciler) saveHostFirmwareComponents(prov provisioner.Provisioner, info *reconcileInfo, hfc *metal3api.HostFirmwareComponents) (dirty bool, err error) {
+	dirty = false
+	if reflect.DeepEqual(hfc.Status.Updates, hfc.Spec.Updates) {
+		info.log.Info("not saving hostFirmwareComponents information since is not necessary")
+		return dirty, nil
+	}
+
+	info.log.Info("saving hostFirmwareComponents information", "spec updates", hfc.Spec.Updates, "status updates", hfc.Status.Updates)
+
+	hfc.Status.Updates = make([]metal3api.FirmwareUpdate, len(hfc.Spec.Updates))
+	for i := range hfc.Spec.Updates {
+		hfc.Spec.Updates[i].DeepCopyInto(&hfc.Status.Updates[i])
+	}
+
+	// Retrieve new information about the firmware components stored in ironic
+	components, err := prov.GetFirmwareComponents()
+	if err != nil {
+		info.log.Error(err, "failed to get new information for firmware components in ironic")
+		return dirty, err
+	}
+	hfc.Status.Components = components
+	dirty = true
+
+	return dirty, nil
+}
+
 func (r *BareMetalHostReconciler) createHostFirmwareComponents(info *reconcileInfo) error {
 	// Check if HostFirmwareComponents already exists
 	hfc := &metal3api.HostFirmwareComponents{}
@@ -1755,18 +1795,31 @@ func (r *BareMetalHostReconciler) createHostFirmwareComponents(info *reconcileIn
 
 			// Set bmh as owner, this makes sure the resource is deleted when bmh is deleted
 			if err = controllerutil.SetControllerReference(info.host, hfc, r.Scheme()); err != nil {
-				return errors.Wrap(err, "could not set bmh as controller")
+				return errors.Wrap(err, "could not set bmh as controller for hostFirmwareComponents")
 			}
 			if err = r.Create(info.ctx, hfc); err != nil {
 				return errors.Wrap(err, "failure creating hostFirmwareComponents resource")
 			}
 
 			info.log.Info("created new hostFirmwareComponents resource")
-		} else {
-			// Error reading the object
-			return errors.Wrap(err, "could not load hostFirmwareComponents resource")
+			return nil
 		}
+		// Error reading the object
+		return errors.Wrap(err, "could not load hostFirmwareComponents resource")
 	}
+	// Necessary in case the CRD is created manually.
+
+	if !ownerReferenceExists(info.host, hfc) {
+		if err := controllerutil.SetControllerReference(info.host, hfc, r.Scheme()); err != nil {
+			return errors.Wrap(err, "could not set bmh as controller for hostFirmwareComponents")
+		}
+		if err := r.Update(info.ctx, hfc); err != nil {
+			return errors.Wrap(err, "failure updating hostFirmwareComponents resource")
+		}
+
+		return nil
+	}
+
 	return nil
 }
 
@@ -1851,15 +1904,6 @@ func (r *BareMetalHostReconciler) getHostFirmwareComponents(info *reconcileInfo)
 
 	// Check if there are Updates in the Spec that are different than the Status
 	if meta.IsStatusConditionTrue(hfc.Status.Conditions, string(metal3api.HostFirmwareComponentsChangeDetected)) {
-		// Check if the status have been populated
-		if len(hfc.Status.Updates) == 0 {
-			return false, nil, errors.New("host firmware status updates not available")
-		}
-
-		if len(hfc.Status.Components) == 0 {
-			return false, nil, errors.New("host firmware status components not available")
-		}
-
 		if meta.IsStatusConditionTrue(hfc.Status.Conditions, string(metal3api.HostFirmwareComponentsValid)) {
 			info.log.Info("hostFirmwareComponents indicating ChangeDetected", "namespacename", info.request.NamespacedName)
 			return true, hfc, nil
diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go
index fd5b6aa1..b316eabb 100644
--- a/controllers/metal3.io/host_state_machine.go
+++ b/controllers/metal3.io/host_state_machine.go
@@ -465,6 +465,14 @@ func (hsm *hostStateMachine) handleAvailable(info *reconcileInfo) actionResult {
 		return actionComplete{}
 	}
 
+	// Check if hostFirmwareComponents have changed
+	if dirty, _, err := hsm.Reconciler.getHostFirmwareComponents(info); err != nil {
+		return actionError{err}
+	} else if dirty {
+		hsm.NextState = metal3api.StatePreparing
+		return actionComplete{}
+	}
+
 	// ErrorCount is cleared when appropriate inside actionManageAvailable
 	actResult := hsm.Reconciler.actionManageAvailable(hsm.Provisioner, info)
 	if _, complete := actResult.(actionComplete); complete {
diff --git a/controllers/metal3.io/hostfirmwarecomponents_controller.go b/controllers/metal3.io/hostfirmwarecomponents_controller.go
index dbad4f9f..6ef4c72f 100644
--- a/controllers/metal3.io/hostfirmwarecomponents_controller.go
+++ b/controllers/metal3.io/hostfirmwarecomponents_controller.go
@@ -122,7 +122,6 @@ func (r *HostFirmwareComponentsReconciler) Reconcile(ctx context.Context, req ct
 
 	// Fetch the HostFirmwareComponents
 	hfc := &metal3api.HostFirmwareComponents{}
-	info := &rhfcInfo{ctx: ctx, log: reqLogger, hfc: hfc, bmh: bmh}
 	if err = r.Get(ctx, req.NamespacedName, hfc); err != nil {
 		// The HFC resource may have been deleted
 		if k8serrors.IsNotFound(err) {
@@ -134,6 +133,7 @@ func (r *HostFirmwareComponentsReconciler) Reconcile(ctx context.Context, req ct
 	}
 
 	// Create a provisioner to access Ironic API
+	info := &rhfcInfo{ctx: ctx, log: reqLogger, hfc: hfc, bmh: bmh}
 	prov, err := r.ProvisionerFactory.NewProvisioner(ctx, provisioner.BuildHostDataNoBMC(*bmh), info.publishEvent)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to create provisioner: %w", err)
@@ -181,11 +181,6 @@ func (r *HostFirmwareComponentsReconciler) Reconcile(ctx context.Context, req ct
 // Update the HostFirmwareComponents resource using the components from provisioner.
 func (r *HostFirmwareComponentsReconciler) updateHostFirmware(info *rhfcInfo, components []metal3api.FirmwareComponentStatus) (err error) {
 	dirty := false
-	var newStatus metal3api.HostFirmwareComponentsStatus
-	// change the Updates in Status
-	newStatus.Updates = info.hfc.Spec.Updates
-	// change the Components in Status
-	newStatus.Components = components
 
 	// Check if the updates in the Spec are different than Status
 	updatesMismatch := !reflect.DeepEqual(info.hfc.Status.Updates, info.hfc.Spec.Updates)
@@ -196,8 +191,11 @@ func (r *HostFirmwareComponentsReconciler) updateHostFirmware(info *rhfcInfo, co
 	reason := reasonValidComponent
 	generation := info.hfc.GetGeneration()
 
+	newStatus := info.hfc.Status.DeepCopy()
+	newStatus.Components = components
+
 	if updatesMismatch {
-		if setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionTrue, reason, "") {
+		if setUpdatesCondition(generation, newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionTrue, reason, "") {
 			dirty = true
 		}
 
@@ -205,17 +203,17 @@ func (r *HostFirmwareComponentsReconciler) updateHostFirmware(info *rhfcInfo, co
 		if err != nil {
 			info.publishEvent("ValidationFailed", fmt.Sprintf("Invalid Firmware Components: %s", err))
 			reason = reasonInvalidComponent
-			if setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionFalse, reason, fmt.Sprintf("Invalid Firmware Components: %s", err)) {
+			if setUpdatesCondition(generation, newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionFalse, reason, fmt.Sprintf("Invalid Firmware Components: %s", err)) {
 				dirty = true
 			}
-		} else if setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionTrue, reason, "") {
+		} else if setUpdatesCondition(generation, newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionTrue, reason, "") {
 			dirty = true
 		}
 	} else {
-		if setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionTrue, reason, "") {
+		if setUpdatesCondition(generation, newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionTrue, reason, "") {
 			dirty = true
 		}
-		if setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionFalse, reason, "") {
+		if setUpdatesCondition(generation, newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionFalse, reason, "") {
 			dirty = true
 		}
 	}
@@ -258,18 +256,6 @@ func (r *HostFirmwareComponentsReconciler) validateHostFirmwareComponents(info *
 		if _, ok := allowedNames[componentName]; !ok {
 			errors = append(errors, fmt.Errorf("component %s is invalid, only 'bmc' or 'bios' are allowed as update names", componentName))
 		}
-		if len(errors) == 0 {
-			componentInStatus := false
-			for _, componentStatus := range info.hfc.Status.Components {
-				if componentName == componentStatus.Component {
-					componentInStatus = true
-					break
-				}
-			}
-			if !componentInStatus {
-				errors = append(errors, fmt.Errorf("component %s is invalid because is not present in status", componentName))
-			}
-		}
 	}
 
 	return errors
@@ -295,8 +281,9 @@ func setUpdatesCondition(generation int64, status *metal3api.HostFirmwareCompone
 		Reason:             string(reason),
 		Message:            message,
 	}
-	currCond := meta.FindStatusCondition(info.hfc.Status.Conditions, string(cond))
+
 	meta.SetStatusCondition(&status.Conditions, newCondition)
+	currCond := meta.FindStatusCondition(info.hfc.Status.Conditions, string(cond))
 
 	if currCond == nil || currCond.Status != newStatus {
 		return true
diff --git a/main.go b/main.go
index 6b30977a..bc32a7e5 100644
--- a/main.go
+++ b/main.go
@@ -337,6 +337,7 @@ func main() {
 		ProvisionerFactory: provisionerFactory,
 	}).SetupWithManager(mgr, maxConcurrency); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "HostFirmwareComponents")
+		os.Exit(1)
 	}
 
 	if err = (&metal3iocontroller.DataImageReconciler{
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index 63197a56..349db0b1 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -62,6 +62,10 @@ type HostFirmwareSettingsMock struct {
 	Schema   map[string]metal3api.SettingSchema
 }
 
+type HostFirmwareComponentsMock struct {
+	Components []metal3api.FirmwareComponentStatus
+}
+
 // Fixture contains persistent state for a particular host.
 type Fixture struct {
 	// counter to set the provisioner as ready
@@ -80,6 +84,8 @@ type Fixture struct {
 	customDeploy *metal3api.CustomDeploy
 
 	HostFirmwareSettings HostFirmwareSettingsMock
+
+	HostFirmwareComponents HostFirmwareComponentsMock
 }
 
 // NewProvisioner returns a new Fixture Provisioner.
@@ -357,7 +363,8 @@ func (p *fixtureProvisioner) RemoveBMCEventSubscriptionForNode(_ metal3api.BMCEv
 }
 
 func (p *fixtureProvisioner) GetFirmwareComponents() (components []metal3api.FirmwareComponentStatus, err error) {
-	return components, nil
+	p.log.Info("getting Firmware components")
+	return p.state.HostFirmwareComponents.Components, nil
 }
 
 func (p *fixtureProvisioner) IsDataImageReady() (isNodeBusy bool, nodeError error) {
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index bb4741dd..80b07be5 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1248,10 +1248,16 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails
 	}
 
 	// extract to generate the updates that will trigger a clean step
-	newUpdates := make(map[string]string)
+	// the format we send to ironic is:
+	// [{"component":"...", "url":"..."}, {"component":"...","url":".."}]
+	var newUpdates []map[string]string
 	if data.TargetFirmwareComponents != nil {
 		for _, update := range data.TargetFirmwareComponents {
-			newUpdates[update.Component] = update.URL
+			newComponentUpdate := map[string]string{
+				"component": update.Component,
+				"url":       update.URL,
+			}
+			newUpdates = append(newUpdates, newComponentUpdate)
 		}
 	}
 


Test patch:
diff --git a/controllers/metal3.io/hostfirmwarecomponents_test.go b/controllers/metal3.io/hostfirmwarecomponents_test.go
index fc7c332d..0faac062 100644
--- a/controllers/metal3.io/hostfirmwarecomponents_test.go
+++ b/controllers/metal3.io/hostfirmwarecomponents_test.go
@@ -5,6 +5,9 @@ import (
 	"testing"
 
 	metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
+	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -25,20 +28,15 @@ func getTestHFCReconciler(host *metal3api.HostFirmwareComponents) *HostFirmwareC
 	return reconciler
 }
 
-func getMockHFCProvisioner(components []metal3api.FirmwareComponentStatus) *hfcMockProvisioner {
-	return &hfcMockProvisioner{
-		Components: components,
-		Error:      nil,
+func getMockHFCProvisioner(host *metal3api.BareMetalHost, components []metal3api.FirmwareComponentStatus) provisioner.Provisioner {
+	state := fixture.Fixture{
+		HostFirmwareComponents: fixture.HostFirmwareComponentsMock{
+			Components: components,
+		},
 	}
-}
-
-type hfcMockProvisioner struct {
-	Components []metal3api.FirmwareComponentStatus
-	Error      error
-}
-
-func (m *hfcMockProvisioner) GetFirmwareComponents() (components []metal3api.FirmwareComponentStatus, err error) {
-	return m.Components, m.Error
+	p, _ := state.NewProvisioner(context.TODO(), provisioner.BuildHostData(*host, bmc.Credentials{}),
+		func(reason, message string) {})
+	return p
 }
 
 // Mock components to return from provisioner.
@@ -96,7 +94,7 @@ func getCurrentComponents(updatedComponents string) []metal3api.FirmwareComponen
 // Create the baremetalhost reconciler and use that to create bmh in same namespace.
 func createBaremetalHostHFC() *metal3api.BareMetalHost {
 	bmh := &metal3api.BareMetalHost{}
-	bmh.ObjectMeta = metav1.ObjectMeta{Name: "hostName", Namespace: "hostNamespace"}
+	bmh.ObjectMeta = metav1.ObjectMeta{Name: hostName, Namespace: hostNamespace}
 	c := fakeclient.NewFakeClient(bmh)
 
 	reconciler := &BareMetalHostReconciler{
@@ -104,10 +102,7 @@ func createBaremetalHostHFC() *metal3api.BareMetalHost {
 		ProvisionerFactory: nil,
 		Log:                ctrl.Log.WithName("bmh_reconciler").WithName("BareMetalHost"),
 	}
-	err := reconciler.Create(context.TODO(), bmh)
-	if err != nil {
-		return nil
-	}
+	_ = reconciler.Create(context.TODO(), bmh)
 
 	return bmh
 }
@@ -170,12 +165,6 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 					},
 				},
 				Status: metal3api.HostFirmwareComponentsStatus{
-					Updates: []metal3api.FirmwareUpdate{
-						{
-							Component: "bmc",
-							URL:       "https://myurls/newbmcfirmware",
-						},
-					},
 					Components: []metal3api.FirmwareComponentStatus{
 						{
 							Component:          "bmc",
@@ -247,12 +236,6 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 					},
 				},
 				Status: metal3api.HostFirmwareComponentsStatus{
-					Updates: []metal3api.FirmwareUpdate{
-						{
-							Component: "bios",
-							URL:       "https://myurls/newbiosfirmware",
-						},
-					},
 					Components: []metal3api.FirmwareComponentStatus{
 						{
 							Component:      "bmc",
@@ -328,16 +311,6 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 					},
 				},
 				Status: metal3api.HostFirmwareComponentsStatus{
-					Updates: []metal3api.FirmwareUpdate{
-						{
-							Component: "bmc",
-							URL:       "https://myurl/newbmcfirmware",
-						},
-						{
-							Component: "bios",
-							URL:       "https://myurls/newbiosfirmware",
-						},
-					},
 					Components: []metal3api.FirmwareComponentStatus{
 						{
 							Component:          "bmc",
@@ -404,14 +377,13 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.Scenario, func(t *testing.T) {
 			ctx := context.TODO()
-			prov := getMockHFCProvisioner(getCurrentComponents(tc.UpdatedComponents))
 
 			tc.ExpectedComponents.TypeMeta = metav1.TypeMeta{
 				Kind:       "HostFirmwareComponents",
 				APIVersion: "metal3.io/v1alpha1"}
 			tc.ExpectedComponents.ObjectMeta = metav1.ObjectMeta{
-				Name:            "hostName",
-				Namespace:       "hostNamespace",
+				Name:            hostName,
+				Namespace:       hostNamespace,
 				ResourceVersion: "2"}
 
 			hfc := tc.CurrentHFCResource
@@ -419,8 +391,9 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 			// Create a bmh resource needed by hfc reconciler
 			bmh := createBaremetalHostHFC()
 
+			prov := getMockHFCProvisioner(bmh, getCurrentComponents(tc.UpdatedComponents))
+
 			info := &rhfcInfo{
-				ctx: ctx,
 				log: logf.Log.WithName("controllers").WithName("HostFirmwareComponents"),
 				hfc: tc.CurrentHFCResource,
 				bmh: bmh,
@@ -428,6 +401,7 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 
 			components, err := prov.GetFirmwareComponents()
 			assert.NoError(t, err)
+
 			err = r.updateHostFirmware(info, components)
 			assert.NoError(t, err)
 
@@ -438,10 +412,9 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 			err = r.Client.Get(ctx, key, actual)
 			assert.Equal(t, nil, err)
 
-			// Ensure ExpectedComponents matches actual
 			assert.Equal(t, tc.ExpectedComponents.Spec.Updates, actual.Spec.Updates)
 			assert.Equal(t, tc.ExpectedComponents.Status.Components, actual.Status.Components)
-			assert.Equal(t, tc.ExpectedComponents.Status.Updates, actual.Status.Updates)
+
 			currentTime := metav1.Now()
 			tc.ExpectedComponents.Status.LastUpdated = &currentTime
 			actual.Status.LastUpdated = &currentTime
diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go
index a85e28fa..2e3098db 100644
--- a/pkg/provisioner/ironic/provision_test.go
+++ b/pkg/provisioner/ironic/provision_test.go
@@ -1444,3 +1444,121 @@ func TestGetUpdateOptsForNodeSecureBoot(t *testing.T) {
 		})
 	}
 }
+
+func TestBuildCleanStepsForUpdateFirmware(t *testing.T) {
+	nodeUUID := "eec38659-4c68-7431-9535-d10766f07a58"
+	cases := []struct {
+		name                     string
+		ironic                   *testserver.IronicMock
+		targetFirmwareComponents []metal3api.FirmwareUpdate
+		expectedFirmwareUpdates  []map[string]string
+	}{
+		{
+			name: "no updates",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.DeployFail),
+				UUID:           nodeUUID,
+			}),
+			targetFirmwareComponents: nil,
+			expectedFirmwareUpdates:  nil,
+		},
+		{
+			name: "bmc update",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.DeployFail),
+				UUID:           nodeUUID,
+			}),
+			targetFirmwareComponents: []metal3api.FirmwareUpdate{
+				{
+					Component: "bmc",
+					URL:       "https://mybmc.newfirmware",
+				},
+			},
+			expectedFirmwareUpdates: []map[string]string{
+				{
+					"component": "bmc",
+					"url":       "https://mybmc.newfirmware",
+				},
+			},
+		},
+		{
+			name: "bios update",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.DeployFail),
+				UUID:           nodeUUID,
+			}),
+			targetFirmwareComponents: []metal3api.FirmwareUpdate{
+				{
+					Component: "bios",
+					URL:       "https://mybios.newfirmware",
+				},
+			},
+			expectedFirmwareUpdates: []map[string]string{
+				{
+					"component": "bios",
+					"url":       "https://mybios.newfirmware",
+				},
+			},
+		},
+		{
+			name: "bmc and bios update",
+			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
+				ProvisionState: string(nodes.DeployFail),
+				UUID:           nodeUUID,
+			}),
+			targetFirmwareComponents: []metal3api.FirmwareUpdate{
+				{
+					Component: "bmc",
+					URL:       "https://mybmc.newfirmware",
+				},
+				{
+					Component: "bios",
+					URL:       "https://mybios.newfirmware",
+				},
+			},
+			expectedFirmwareUpdates: []map[string]string{
+				{
+					"component": "bmc",
+					"url":       "https://mybmc.newfirmware",
+				},
+				{
+					"component": "bios",
+					"url":       "https://mybios.newfirmware",
+				},
+			},
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.ironic != nil {
+				tc.ironic.Start()
+				defer tc.ironic.Stop()
+			}
+
+			host := makeHost()
+			host.Status.Provisioning.ID = nodeUUID
+			publisher := func(reason, message string) {}
+			auth := clients.AuthConfig{Type: clients.NoAuth}
+			prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher, tc.ironic.Endpoint(), auth)
+			if err != nil {
+				t.Fatalf("could not create provisioner: %s", err)
+			}
+
+			parsedURL := &url.URL{Scheme: "redfish", Host: "10.1.1.1"}
+
+			testBMC, _ := testbmc.NewTestBMCAccessDetails(parsedURL, false)
+
+			cleanSteps, err := prov.buildManualCleaningSteps(testBMC, provisioner.PrepareData{
+				TargetFirmwareComponents: tc.targetFirmwareComponents,
+			})
+
+			assert.Equal(t, nil, err)
+			if tc.targetFirmwareComponents == nil {
+				assert.Equal(t, tc.expectedFirmwareUpdates, []map[string]string(nil))
+			} else {
+				settings := cleanSteps[0].Args["settings"]
+				assert.ElementsMatch(t, tc.expectedFirmwareUpdates, settings)
+			}
+		})
+	}
+}