Fix Status logic for HostFirmwareComponents

Make Updates and Components optional in HostFirmwareComponentsStatus,
so we don't get errors when they are empty.

The UpdatedAt field for FirmwareComponentStatus can be nil, this
caused runtime error in the BMO.

Change logic in GetFirmwareComponents, so we fail earlier in case
is not supported, now we raise a specific error
ErrFirmwareUpdateUnsupported.

Simplify the logic in the Controller, so we handle conditions in only
one function

Signed-off-by: Iury Gregory Melo Ferreira <imelofer@redhat.com>

Author: iurygregory

apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go

@@ -56,10 +56,11 @@ type HostFirmwareComponentsSpec struct {
 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"`
 
 	// Components is the list of all available firmware components and their information.
-	Components []FirmwareComponentStatus `json:"components"`
+	Components []FirmwareComponentStatus `json:"components,omitempty"`
 
 	// Time that the status was last updated
 	// +optional

config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml

@@ -170,9 +170,6 @@ spec:
                   - url
                   type: object
                 type: array
-            required:
-            - components
-            - updates
             type: object
         type: object
     served: true

config/render/capm3.yaml

@@ -1770,9 +1770,6 @@ spec:
                   - url
                   type: object
                 type: array
-            required:
-            - components
-            - updates
             type: object
         type: object
     served: true

controllers/metal3.io/hostfirmwarecomponents_controller.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 
@@ -151,23 +152,21 @@ func (r *HostFirmwareComponentsReconciler) Reconcile(ctx context.Context, req ct
 		return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, nil
 	}
 
-	newStatus, err := r.updateHostFirmware(info)
-	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("could not update hostfirmwarecomponents: %w", err)
-	}
-
+	info.log.Info("retrieving firmware components and saving to resource", "Node", bmh.Status.Provisioning.ID)
 	// Check ironic for the components information if possible
 	components, err := prov.GetFirmwareComponents()
-	info.log.Info("retrieving firmware components and saving to resource", "Node", bmh.Status.Provisioning.ID)
 
 	if err != nil {
-		reqLogger.Error(err, "provisioner returns error", "RequeueAfter", provisionerRetryDelay)
-		setUpdatesCondition(info.hfc.GetGeneration(), &newStatus, info, metal3api.HostFirmwareComponentsValid, metav1.ConditionFalse, reasonInvalidComponent, err.Error())
+		if errors.Is(err, provisioner.ErrFirmwareUpdateUnsupported) {
+			return ctrl.Result{Requeue: false}, err
+		}
+		reqLogger.Info("provisioner returns error", "Error", err.Error(), "RequeueAfter", provisionerRetryDelay)
 		return ctrl.Result{Requeue: true, RequeueAfter: provisionerRetryDelay}, err
 	}
 
-	if err = r.updateHostFirmwareComponents(newStatus, components, info); err != nil {
-		return ctrl.Result{Requeue: false}, err
+	if err = r.updateHostFirmware(info, components); err != nil {
+		info.log.Info("updateHostFirmware returned error")
+		return ctrl.Result{}, fmt.Errorf("could not update hostfirmwarecomponents: %w", err)
 	}
 
 	for _, e := range info.events {
@@ -181,14 +180,19 @@ func (r *HostFirmwareComponentsReconciler) Reconcile(ctx context.Context, req ct
 }
 
 // Update the HostFirmwareComponents resource using the components from provisioner.
-func (r *HostFirmwareComponentsReconciler) updateHostFirmware(info *rhfcInfo) (newStatus metal3api.HostFirmwareComponentsStatus, err error) {
+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)
+	if len(info.hfc.Spec.Updates) == 0 && len(info.hfc.Status.Updates) == 0 {
+		updatesMismatch = false
+	}
 
 	reason := reasonValidComponent
 	generation := info.hfc.GetGeneration()
@@ -222,32 +226,6 @@ func (r *HostFirmwareComponentsReconciler) updateHostFirmware(info *rhfcInfo) (n
 		info.log.Info("Status for HostFirmwareComponents changed")
 		info.hfc.Status = *newStatus.DeepCopy()
 
-		t := metav1.Now()
-		info.hfc.Status.LastUpdated = &t
-		return newStatus, r.Status().Update(info.ctx, info.hfc)
-	}
-	return newStatus, nil
-}
-
-// Update the HostFirmwareComponents resource using the components from provisioner.
-func (r *HostFirmwareComponentsReconciler) updateHostFirmwareComponents(newStatus metal3api.HostFirmwareComponentsStatus, components []metal3api.FirmwareComponentStatus, info *rhfcInfo) (err error) {
-	dirty := false
-	// change the Components in Status
-	newStatus.Components = components
-	// Check if the components information we retrieved is different from the one in Status
-	componentsInfoMismatch := !reflect.DeepEqual(components, info.hfc.Status.Components)
-	reason := reasonValidComponent
-	generation := info.hfc.GetGeneration()
-	// Log the components we have
-	info.log.Info("firmware components for node", "components", components, "bmh", info.bmh.Name)
-	if componentsInfoMismatch {
-		setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionTrue, reason, "")
-		dirty = true
-	}
-	if dirty {
-		info.log.Info("Components Status for HostFirmwareComponents changed")
-		info.hfc.Status = *newStatus.DeepCopy()
-
 		t := metav1.Now()
 		info.hfc.Status.LastUpdated = &t
 		return r.Status().Update(info.ctx, info.hfc)

controllers/metal3.io/hostfirmwarecomponents_test.go

@@ -425,12 +425,9 @@ func TestStoreHostFirmwareComponents(t *testing.T) {
 				bmh: bmh,
 			}
 
-			currentStatus, err := r.updateHostFirmware(info)
-			assert.NoError(t, err)
-
 			components, err := prov.GetFirmwareComponents()
 			assert.NoError(t, err)
-			err = r.updateHostFirmwareComponents(currentStatus, components, info)
+			err = r.updateHostFirmware(info, components)
 			assert.NoError(t, err)
 
 			// Check that resources get created or updated

pkg/provisioner/ironic/ironic.go

@@ -1162,21 +1162,20 @@ func (p *ironicProvisioner) GetFirmwareComponents() ([]metal3api.FirmwareCompone
 	if !p.availableFeatures.HasFirmwareUpdates() {
 		return nil, fmt.Errorf("current ironic version does not support firmware updates")
 	}
+
+	// Setting to 2 since we only support bmc and bios
+	componentsInfo := make([]metal3api.FirmwareComponentStatus, 0, 2)
+
+	if ironicNode.FirmwareInterface == "no-firmware" {
+		return componentsInfo, provisioner.ErrFirmwareUpdateUnsupported
+	}
 	// Get the components from Ironic via Gophercloud
 	componentList, componentListErr := nodes.ListFirmware(p.ctx, p.client, ironicNode.UUID).Extract()
 
-	if componentListErr != nil || len(componentList) == 0 {
-		bmcAccess, _ := p.bmcAccess()
-		if ironicNode.FirmwareInterface == "no-firmware" {
-			return nil, fmt.Errorf("driver %s does not support firmware updates", bmcAccess.Driver())
-		}
-
+	if componentListErr != nil {
 		return nil, fmt.Errorf("could not get firmware components for node %s: %w", ironicNode.UUID, componentListErr)
 	}
 
-	// Setting to 2 since we only support bmc and bios
-	componentsInfo := make([]metal3api.FirmwareComponentStatus, 0, 2)
-
 	// Iterate over the list of components to extract their information and update the list.
 	for _, fwc := range componentList {
 		if fwc.Component != "bios" && fwc.Component != "bmc" {
@@ -1188,9 +1187,12 @@ func (p *ironicProvisioner) GetFirmwareComponents() ([]metal3api.FirmwareCompone
 			InitialVersion:     fwc.InitialVersion,
 			CurrentVersion:     fwc.CurrentVersion,
 			LastVersionFlashed: fwc.LastVersionFlashed,
-			UpdatedAt: metav1.Time{
+		}
+		// Check if UpdatedAt is nil before adding it.
+		if fwc.UpdatedAt != nil {
+			component.UpdatedAt = metav1.Time{
 				Time: *fwc.UpdatedAt,
-			},
+			}
 		}
 		componentsInfo = append(componentsInfo, component)
 		p.log.Info("firmware component added for node", "component", fwc.Component, "node", ironicNode.UUID)

pkg/provisioner/provisioner.go

@@ -231,3 +231,6 @@ var ErrNeedsRegistration = errors.New("host not registered")
 // ErrNeedsPreprovisioningImage is returned if a preprovisioning image is
 // required.
 var ErrNeedsPreprovisioningImage = errors.New("no suitable Preprovisioning image available")
+
+// ErrFirmwareUpdateUnsupported is returned if the host can't execute firmware updates.
+var ErrFirmwareUpdateUnsupported = errors.New("host does not support Firmware Updates")