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..593518c5b2 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/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{} diff --git a/controllers/metal3.io/host_state_machine.go b/controllers/metal3.io/host_state_machine.go index fd5b6aa14f..b316eabb83 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 dbad4f9fe6..fda0051fa0 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,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 } } @@ -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 diff --git a/controllers/metal3.io/hostfirmwarecomponents_test.go b/controllers/metal3.io/hostfirmwarecomponents_test.go index fc7c332dfc..0faac06260 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 = ¤tTime actual.Status.LastUpdated = ¤tTime 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..a59a75ea6d 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) } } diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go index ccc242f29b..fe8834a5c4 100644 --- a/pkg/provisioner/ironic/provision_test.go +++ b/pkg/provisioner/ironic/provision_test.go @@ -691,3 +691,121 @@ func TestBuildCleanSteps(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) + } + }) + } +}