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.

Signed-off-by: Iury Gregory Melo Ferreira <[email protected]>

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"`

controllers/metal3.io/baremetalhost_controller.go

@@ -1164,6 +1164,21 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner,
 		return recordActionFailure(info, metal3api.PreparationError, provResult.ErrorMessage)
 	}
 
+	if hfcDirty && started {
+		info.log.Info("saving host firmware components")
+		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 +1755,34 @@ 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("HostFirmwareComponents 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])
+		}
+
+		info.log.Info("re assigned status updates")
+		info.log.Info("New HostFirmwareComponents Status", "updates", hfc.Status.Updates)
+
+		// Retrieve new information about the firmware components stored in ironic
+		components, err := prov.GetFirmwareComponents()
+		if err != nil {
+			info.log.Info("Failed to get new information for firmware components in ironic")
+			return dirty, err
+		}
+		hfc.Status.Components = components
+		t := metav1.Now()
+		hfc.Status.LastUpdated = &t
+		info.log.Info("Updates for HostFirmwareComponents have changed")
+		dirty = true
+	}
+	return dirty, nil
+}
+
 func (r *BareMetalHostReconciler) createHostFirmwareComponents(info *reconcileInfo) error {
 	// Check if HostFirmwareComponents already exists
 	hfc := &metal3api.HostFirmwareComponents{}

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 {

controllers/metal3.io/hostfirmwarecomponents_controller.go

@@ -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,26 +191,29 @@ 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
 		}
 
 		err := r.validateHostFirmwareComponents(info)
 		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
 		}
 	}
@@ -295,8 +293,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

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,18 +94,15 @@ 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{
 		Client:             c,
 		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,30 +377,31 @@ 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
 			r := getTestHFCReconciler(hfc)
 			// 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,
 			}
 
 			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

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{

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) {

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)
 		}
 	}