🐛 Fix HFC to execute updates #1793

iurygregory · 2024-06-25T02:57:21Z

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.

What this PR does / why we need it: metal3-io/metal3-docs#364

tuminoid · 2024-06-25T07:15:10Z

We probably want to to cherry-pick this to release-0.6 when done, if HFC doesn't work?

iurygregory · 2024-06-25T11:29:06Z

@tuminoid correct

tuminoid · 2024-06-25T11:36:50Z

@tuminoid correct

Let's schedule the bot then to do the pick:
/cherry-pick release-0.6

metal3-io-bot · 2024-06-25T11:36:54Z

@tuminoid: once the present PR merges, I will cherry-pick it on top of release-0.6 in a new PR and assign it to you.

In response to this:

@tuminoid correct

Let's schedule the bot then to do the pick:
/cherry-pick release-0.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

controllers/metal3.io/baremetalhost_controller.go

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: - BMH stuck in preparing and executing the same firmware update multiple times - not being able to define the HostFirmwareComponent manually and have the update applied when the BMH is in preparing. Signed-off-by: Iury Gregory Melo Ferreira <imelofer@redhat.com>

iurygregory · 2024-07-02T18:17:28Z

Done, I've tested again in my setup before pushing the changes here.

zaneb

I left some comments, but clearly this is a huge improvement so it may be worth merging and following up on those afterwards.

zaneb · 2024-07-04T04:40:54Z

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)


This saves the new status when we begin the manual cleaning, but I think I'm right in saying clearHostProvisioningSettings() does not clear them? So if there's a failure in actually applying this change, we won't retry.

if there is a failure we shouldn't retry I would say, it can be a bad firmware (Dell for example has firmware separate for each model, if I use the firmware of an R750 in R640 it complains and fails)

Or it could be a dropped connection or a failed write.
We shouldn't report that we've done something if we haven't done it.
If the user provides the wrong firmware we should keep trying until they realise and stop doing that.

Yeah, unless we invent a way for Ironic to say "this will never work, don't try again", the current trend is to retry.

if provResult.ErrorMessage != "" { if bmhDirty { info.log.Info("handling cleaning error in controller") clearHostProvisioningSettings(info.host) } if hfcDitry { clearHostFirmwareComponentsUpdates(hfc) } return recordActionFailure(info, metal3api.PreparationError, provResult.ErrorMessage) }

Something like this? a new function clearHostFirmwareComponentsUpdates to be used, since I don't think we should change clearHostProvisioningSettings

zaneb · 2024-07-04T04:49:09Z

controllers/metal3.io/baremetalhost_controller.go

+	}
+
+	// Retrieve new information about the firmware components stored in ironic
+	components, err := prov.GetFirmwareComponents()


This function is only going to get called once, at the beginning of manual cleaning.
Aren't the Components expected to change in ironic only after manual cleaning is complete?

yeah, this is the problem I'm planning to fix in a separate PR, still trying to figure out how

Zane is right. But I'm also wondering why the HFC controller does not update the components afterwards.

Hmm, good point. I thought at one point we deleted that from the HFC controller, but indeed it's still there.

Yup, I've waited some hours to see if the HFC controller would attemp to update, but it didn't..,my best guess is that I need to change the conditions when calling updateHostFirmware in the Reconcile for HostFirmwareComponentsReconciler

controllers/metal3.io/baremetalhost_controller.go

zaneb · 2024-07-04T05:14:13Z

controllers/metal3.io/baremetalhost_controller.go

@@ -1851,15 +1904,6 @@ func (r *BareMetalHostReconciler) getHostFirmwareComponents(info *reconcileInfo)

 	// Check if there are Updates in the Spec that are different than the Status
 	if meta.IsStatusConditionTrue(hfc.Status.Conditions, string(metal3api.HostFirmwareComponentsChangeDetected)) {


In theory we should check that the condition matches the current Generation (example) so we know the data is not out of date.
Not a huge deal though. (Bigger worry is actually that we miss that the Spec has been copied to the Status - which won't bump the generation - which would result in us doing the update again.)

I will do some tests in the follow-up I'm working on.

zaneb · 2024-07-05T12:07:32Z

/approve
This is strictly better than what we have. Should probably hold off on the backport though.

dtantsur

I'm fine with a follow-up. Thank you Iury.

/lgtm

metal3-io-bot · 2024-07-05T14:05:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dtantsur,zaneb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

metal3-io-bot · 2024-07-05T14:09:36Z

@tuminoid: #1793 failed to apply on top of branch "release-0.6":

Applying: Fix HFC to execute updates
Using index info to reconstruct a base tree...
M	controllers/metal3.io/baremetalhost_controller.go
M	controllers/metal3.io/host_state_machine.go
M	controllers/metal3.io/hostfirmwarecomponents_controller.go
M	controllers/metal3.io/hostfirmwarecomponents_test.go
M	main.go
M	pkg/provisioner/fixture/fixture.go
M	pkg/provisioner/ironic/ironic.go
M	pkg/provisioner/ironic/provision_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/provisioner/ironic/provision_test.go
CONFLICT (content): Merge conflict in pkg/provisioner/ironic/provision_test.go
Auto-merging pkg/provisioner/ironic/ironic.go
Auto-merging pkg/provisioner/fixture/fixture.go
CONFLICT (content): Merge conflict in pkg/provisioner/fixture/fixture.go
Auto-merging main.go
CONFLICT (content): Merge conflict in main.go
Auto-merging controllers/metal3.io/hostfirmwarecomponents_test.go
CONFLICT (content): Merge conflict in controllers/metal3.io/hostfirmwarecomponents_test.go
Auto-merging controllers/metal3.io/hostfirmwarecomponents_controller.go
Auto-merging controllers/metal3.io/host_state_machine.go
Auto-merging controllers/metal3.io/baremetalhost_controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix HFC to execute updates
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

@tuminoid correct

Let's schedule the bot then to do the pick:
/cherry-pick release-0.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

metal3-io-bot requested review from honza and zaneb June 25, 2024 02:57

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2024

iurygregory force-pushed the hfc-update branch from ffcfa6f to aa59a80 Compare June 25, 2024 02:58

dtantsur requested changes Jun 25, 2024

View reviewed changes

iurygregory force-pushed the hfc-update branch from aa59a80 to bef2aa4 Compare June 25, 2024 16:42

iurygregory requested a review from dtantsur June 25, 2024 16:43

iurygregory force-pushed the hfc-update branch from bef2aa4 to 5f8119c Compare June 27, 2024 19:38

Rozzii added this to the BMO - v0.7.0 milestone Jun 28, 2024

iurygregory force-pushed the hfc-update branch from 5f8119c to a17d865 Compare June 28, 2024 22:16

dtantsur reviewed Jul 1, 2024

View reviewed changes

controllers/metal3.io/baremetalhost_controller.go Outdated Show resolved Hide resolved

iurygregory force-pushed the hfc-update branch from a17d865 to 223b766 Compare July 1, 2024 16:36

dtantsur reviewed Jul 2, 2024

View reviewed changes

controllers/metal3.io/baremetalhost_controller.go Outdated Show resolved Hide resolved

controllers/metal3.io/baremetalhost_controller.go Show resolved Hide resolved

iurygregory force-pushed the hfc-update branch from 223b766 to dc50efe Compare July 2, 2024 18:16

Rozzii modified the milestones: BMO - v0.6.2, BMO - v0.7.0 Jul 3, 2024

zaneb reviewed Jul 4, 2024

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2024

dtantsur approved these changes Jul 5, 2024

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2024

metal3-io-bot merged commit f8d5da6 into metal3-io:main Jul 5, 2024
17 checks passed

This was referenced Jul 5, 2024

OCPBUGS-35559: Bmo sync jul5 openshift/baremetal-operator#362

Merged

🌱 Use OwnerReference for HostFirmwareComponents #1821

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix HFC to execute updates #1793

🐛 Fix HFC to execute updates #1793

iurygregory commented Jun 25, 2024

tuminoid commented Jun 25, 2024

iurygregory commented Jun 25, 2024

tuminoid commented Jun 25, 2024

metal3-io-bot commented Jun 25, 2024

iurygregory commented Jul 2, 2024

zaneb left a comment

zaneb Jul 4, 2024

iurygregory Jul 5, 2024

zaneb Jul 5, 2024

dtantsur Jul 5, 2024

iurygregory Jul 9, 2024 •

edited

Loading

zaneb Jul 4, 2024

iurygregory Jul 5, 2024

dtantsur Jul 5, 2024

zaneb Jul 9, 2024

iurygregory Jul 9, 2024

zaneb Jul 4, 2024

iurygregory Jul 5, 2024

zaneb commented Jul 5, 2024

dtantsur left a comment

metal3-io-bot commented Jul 5, 2024

metal3-io-bot commented Jul 5, 2024

		@@ -1851,15 +1904,6 @@ func (r BareMetalHostReconciler) getHostFirmwareComponents(info reconcileInfo)

		// Check if there are Updates in the Spec that are different than the Status
		if meta.IsStatusConditionTrue(hfc.Status.Conditions, string(metal3api.HostFirmwareComponentsChangeDetected)) {

🐛 Fix HFC to execute updates #1793

🐛 Fix HFC to execute updates #1793

Conversation

iurygregory commented Jun 25, 2024

tuminoid commented Jun 25, 2024

iurygregory commented Jun 25, 2024

tuminoid commented Jun 25, 2024

metal3-io-bot commented Jun 25, 2024

iurygregory commented Jul 2, 2024

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iurygregory Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb commented Jul 5, 2024

dtantsur left a comment

Choose a reason for hiding this comment

metal3-io-bot commented Jul 5, 2024

metal3-io-bot commented Jul 5, 2024

iurygregory Jul 9, 2024 •

edited

Loading