From bef2aa4da0049aae70788e972723e343b693b84a Mon Sep 17 00:00:00 2001 From: Iury Gregory Melo Ferreira Date: Mon, 24 Jun 2024 21:14:15 -0300 Subject: [PATCH] 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 --- .../v1alpha1/hostfirmwarecomponents_types.go | 2 +- .../metal3.io/baremetalhost_controller.go | 40 ++++++ controllers/metal3.io/host_state_machine.go | 8 ++ .../hostfirmwarecomponents_controller.go | 21 ++-- .../metal3.io/hostfirmwarecomponents_test.go | 65 +++------- main.go | 1 + pkg/provisioner/fixture/fixture.go | 9 +- pkg/provisioner/ironic/ironic.go | 10 +- pkg/provisioner/ironic/provision_test.go | 118 ++++++++++++++++++ 9 files changed, 213 insertions(+), 61 deletions(-) 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..dbf66160ef 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/controllers/metal3.io/baremetalhost_controller.go @@ -1164,6 +1164,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 +1754,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{} 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) + } + }) + } +}