Merge pull request #1749 from metal3-io-bot/cherry-pick-1695-to-release-0.6

🐛 Fix Status logic for HostFirmwareComponents

Author: metal3-io-bot

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