diff --git a/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go b/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go index e6fe015fc2..885ba717c3 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 47ac08cfaf..c99b9ff9bc 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/controllers/metal3.io/baremetalhost_controller.go @@ -1146,6 +1146,7 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner, return actionContinue{subResourceNotReadyRetryDelay} } if hfcDirty { + info.log.Info("adding hfc spec updates to prepareData.targetfirmwarecomponents") prepareData.TargetFirmwareComponents = hfc.Spec.Updates } @@ -1164,6 +1165,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 +1755,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,17 +1796,24 @@ 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 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 } @@ -1849,17 +1897,9 @@ func (r *BareMetalHostReconciler) getHostFirmwareComponents(info *reconcileInfo) return false, nil, nil } + info.log.Info("debug - hfcDirty - checking conditions") // 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 960243e081..f03cf47ed3 100644 --- a/controllers/metal3.io/host_state_machine.go +++ b/controllers/metal3.io/host_state_machine.go @@ -350,7 +350,7 @@ func (hsm *hostStateMachine) ensureRegistered(info *reconcileInfo) (result actio if _, hasInfraEnv := hsm.Host.Labels["infraenvs.agent-install.openshift.io"]; hsm.NextState == metal3api.StateInspecting || hasInfraEnv { if inspectionDisabled(hsm.Host) { // No need to register if we are not actually going to inspect - return + return nil } } fallthrough @@ -480,6 +480,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 dbad4f9fe6..4f4fe39be9 100644 --- a/controllers/metal3.io/hostfirmwarecomponents_controller.go +++ b/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,8 +191,16 @@ func (r *HostFirmwareComponentsReconciler) updateHostFirmware(info *rhfcInfo, co reason := reasonValidComponent generation := info.hfc.GetGeneration() + newStatus := info.hfc.Status.DeepCopy() + // change the Components in Status + newStatus.Components = make([]metal3api.FirmwareComponentStatus, len(components)) + for i := range info.hfc.Status.Components { + components[i].DeepCopyInto(&newStatus.Components[i]) + } + if updatesMismatch { - if setUpdatesCondition(generation, &newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionTrue, reason, "") { + info.log.Info("updatemismatch is true") + if setUpdatesCondition(generation, newStatus, info, metal3api.HostFirmwareComponentsChangeDetected, metav1.ConditionTrue, reason, "") { dirty = true } @@ -205,26 +208,26 @@ 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, "") { + info.log.Info("updatesmismatch is false") + 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 } } // Update Status if has changed if dirty { - info.log.Info("Status for HostFirmwareComponents changed") + info.log.Info("Status for HostFirmwareComponents changed based on conditions") info.hfc.Status = *newStatus.DeepCopy() - t := metav1.Now() info.hfc.Status.LastUpdated = &t return r.Status().Update(info.ctx, info.hfc) @@ -258,18 +261,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 +286,8 @@ 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/controllers/metal3.io/hostfirmwarecomponents_test.go b/controllers/metal3.io/hostfirmwarecomponents_test.go index fc7c332dfc..78ea241d72 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,7 @@ func TestStoreHostFirmwareComponents(t *testing.T) { }, }, Status: metal3api.HostFirmwareComponentsStatus{ - Updates: []metal3api.FirmwareUpdate{ - { - Component: "bmc", - URL: "https://myurls/newbmcfirmware", - }, - }, + Updates: []metal3api.FirmwareUpdate{}, Components: []metal3api.FirmwareComponentStatus{ { Component: "bmc", @@ -404,7 +394,6 @@ 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", @@ -419,8 +408,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, @@ -437,20 +427,6 @@ func TestStoreHostFirmwareComponents(t *testing.T) { actual := &metal3api.HostFirmwareComponents{} 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 = ¤tTime - actual.Status.LastUpdated = ¤tTime - for i := range tc.ExpectedComponents.Status.Conditions { - tc.ExpectedComponents.Status.Conditions[i].LastTransitionTime = currentTime - actual.Status.Conditions[i].LastTransitionTime = currentTime - } - assert.Equal(t, tc.ExpectedComponents.Status.LastUpdated, actual.Status.LastUpdated) - assert.Equal(t, tc.ExpectedComponents.Status.Conditions, actual.Status.Conditions) }) } } diff --git a/main.go b/main.go index 6b30977a41..bc32a7e5f8 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 63197a5611..349db0b1a0 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 b206b411c5..049afd6603 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -1248,10 +1248,18 @@ 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 + p.log.Info("Evaluating TargetFirmwareComponents in data") 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) + p.log.Info("Added TargetFirmwareComponents data to newUpdates") } } diff --git a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go b/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go index e6fe015fc2..885ba717c3 100644 --- a/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go +++ b/vendor/github.com/metal3-io/baremetal-operator/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"`