fix(dell): merge BIOS writes into the pending job instead of failing

[mcanevet]

Problem

iDRAC keeps one pending BIOS config job. Any write to Bios/Settings, or to a resource that mirrors a BIOS attribute (such as the SecureBoot resource), seals it, and every further write is rejected with SYS011 until the job is deleted or has run. Two BIOS-affecting calls in the same boot cycle therefore fail on the second one, although both would apply together at the next reset.

Change

The Dell provider's SetBiosConfiguration reads the pending attributes before writing:

• nothing pending: a plain write, as before;
• something pending: delete the live job (only if it has not started), merge the request over the staged attributes and write once;
• everything requested already has the requested value (staged, or applied where nothing is staged) and a live job carries it: nothing is done, so repeating a call is idempotent.

The merge carries along attributes an earlier caller staged, because deleting the job discards what it staged. The deletion is logged, and if the merged write fails afterwards the staged attributes are put back. An unexpected SYS011 is returned to the caller. It is not safe against another client writing BIOS settings on the same BMC at the same time.

redfishwrapper gains ApplyBiosAttributes (typed values; SetBiosConfiguration now delegates to it) and Jobs() (one $expand request, falling back to per-job reads).

Testing

Unit tests, plus runs against a PowerEdge R6715 (iDRAC 1.20.80.51) and an R760xd2 (7.30.10.50): two writes in one boot cycle end up as one job, repeats leave the job alone, and the iDRAC is clean afterwards. Read-only checks on 29 other Dell hosts (iDRAC 5.00 to 7.30, and 1.20) confirmed the settings redirect, the JobService, the ConfigBIOS: job naming and Dell's job resource.

[joelrebel]

@mcanevet can you elaborate what problem is this PR trying to solve and why
It would help if you can keep the PR summary clear and concise, leave the details of how its being done to the code and comments.

[mcanevet]

@joelrebel I updated the PR description. Does it make more sense now? I'm not convinced on the solution, but the problem is real. Currently we can't change 2 BIOS parameters on Dell iDRAC without rebooting.

[joelrebel]

bmclib's Dell setters (SetSecureBoot, SetSecureBootKeyManagement, SetNetworkBootEnabled, SetHTTPBootURI, SetHTTPBootTLSMode) each use the first shape independently, with no visibility into whether another one is coming later in the same boot cycle. So two of them in one boot cycle fail on the second with IDRAC..SYS011, even though both would apply together fine as one job — this isn't an edge case, Tinkerbell's provisioning workflow routinely calls several in the same boot cycle.

@mcanevet based on my understanding, what this PR is trying to achieve is to clear a pending operation that is in conflict and apply the provided settings, correct me if I'm misunderstanding

By forcing all of this into a single method, we prevent the caller from being able to make a decision and it often
hides a real problem in the orchestration/provisioning system where multiple calls are made with no lock like mechanism.

bmclib methods should ideally perform a single operation (read, write) on the BMC and return quickly,
In this case - I would suggest splitting into two methods, see the firmware install methods in the Dell provider as an example*

1. BIOSConfigurationStatus() which would return the current BIOS configuration status - any active/pending jobs
2. ApplyBIOSConfiguration() with a force option, this method first checks if any jobs are currently non-terminal (pending/active) and returns an error to the caller, if the force option was specified it logs the pending operation and clears it before applying.

Also note, we could re-use some of how the dell Job and task state is being converted instead of re-implementing, see

bmclib/providers/dell/firmware.go

Line 192 in fec875d

type oem struct {

[mcanevet]

From what I observe, Dell iDRAC works like this:

• A PATCH without @Redfish.SettingsApplyTime only stages values. Several of these accumulate fine, but staged values are gone after a reboot unless a job exists.
• Creating a job (a PATCH with OnReset, or POST .../Jobs) locks Bios/Settings. Any further PATCH, with or without apply time, fails with SYS011.
• Deleting the job wipes its staged values.

So two BIOS changes in the same boot cycle can only work by either cancelling the job and resubmitting the merged set (what this PR does), or by splitting stage and commit into separate calls. This also affects force in your proposal: clearing the job drops the earlier change unless it merges what was staged.

In Tinkerbell's case an explicit commit would just be one more action before the reboot, which is fine. What I'd like to avoid is making the caller aware of Dell's quirk. So if we go that way, I'd rather have a generic stage/commit contract (commit is a no-op on vendors that already apply at reset) than a Dell-specific opt-in.

The options I can see:

1. Keep it transparent: read pending, cancel the scheduled job, resubmit the merged attributes. No caller changes, but it does cancel a job.
2. Generic stage/commit: BIOS setters stage, and a new commit call (a Dell job, a no-op elsewhere) applies them before reboot. No cancelling, but callers must commit.

Open to other approaches if you see a better one.

[joelrebel]

@mcanevet thanks for elaborating that helps clear up the reasoning, the concerns I have is based on these two points, if you can look into resolving these then we likely have a path to have this merged

1. The current SetBiosConfiguration cancels, merges and applies the merged BIOS attributes - regardless of if the new attributes are the same as the one staged (so we're not able to keep this operation idempotent), does the API provide a mechanism to determine which attributes were previously staged and have this method return if its the same as before?
2. By merging previously staged attributes, we accept the trade off that its likely - attributes that were not intended by the caller to SetBiosConfiguration could be applied.

side note: The orchestrator (Rufio) in this case should ideally batch up the BIOS changes required and create a single request to set the configuration and follow up the related job on the BMC is finalized

[mcanevet]

One alternative to the current "PATCH, then recover on SYS011": check for a pending job first and only write once nothing is blocking.

1. GET Bios/Settings.
2. If there are no pending attributes, PATCH as it does today.
3. If there are some, find the pending config job, cancel it if it hasn't started, merge the attributes and PATCH once.

From what I observe, any pending BIOS job locks Bios/Settings regardless of which attribute you write, so the pending-attributes check should catch every case. That way the SYS011 rejection wouldn't happen in the normal flow.

The cost is one extra GET on every Dell BIOS write, which is about 0.5s in my setup. Today a write with no conflict is a single PATCH. Stacked writes in a Tinkerbell workflow would save the rejected PATCH each time.

Not sure which fits bmclib better. Happy to keep it as is, or rework it if you'd prefer this.

[joelrebel]

One alternative to the current "PATCH, then recover on SYS011": check for a pending job first and only write once nothing is blocking.

1. GET `Bios/Settings`.
2. If there are no pending attributes, PATCH as it does today.
3. If there are some, find the pending config job, cancel it if it hasn't started, merge the attributes and PATCH once.

What you suggest here sounds like a fair approach, if you can include those changes, and also if determine if any of the existing dell job struct, task unpacking can be re-used

[mcanevet]

@joelrebel I reworked this along the lines you agreed to, and rewrote the description to be shorter.

• Look first. SetBiosConfiguration now reads the pending attributes before writing (two extra GETs) and only deletes and merges when something is pending. The retry on SYS011 is gone, so a SYS011 the read could not foresee reaches the caller. I went with this rather than the separate status and force methods, as agreed above.
• Idempotence. If every requested attribute already has the requested value (staged, or applied where nothing is staged for it) and a live job carries the staged ones, nothing is touched. I checked on hardware that repeating a call keeps the same job.
• Unintended attributes. This remains a trade-off I can't remove: iDRAC keeps one job and deleting it discards what it staged, so the merge carries those attributes along. The deletion is logged with the staged and requested attribute names, and if the merged write fails afterwards, the attributes of the deleted job are staged again and the error says whether that worked.
• Reuse. The "has it started" check now reads Dell's own job resource through the existing Dell struct and the endpoint that job() in firmware.go already reads (moved onto the wrapper, Conn.job delegates to it). That turned out to be needed: on iDRAC 7.30 the JobService job has no Oem block, so ActualRunningStartTime is only visible there. I did not reuse ConvertTaskState/TaskStateActive: Scheduled maps to PowerCycleHost, which TaskStateActive rejects, and paused jobs end up in Failed, which I need to treat as possibly started.
• Cost. Reading jobs one by one took up to a minute on hosts with about 120 jobs, so Jobs() asks for the collection expanded (a single request on every iDRAC I tried, with a fallback to per-job reads).

I ran two writes in one boot cycle against a PowerEdge R6715 (iDRAC 1.20.80.51) and an R760xd2 (7.30.10.50): they end up as one job, repeats leave it alone, and the iDRAC is clean afterwards. I also did read-only checks on 29 other Dell hosts (iDRAC 5.00 to 7.30, and 1.20) for the settings redirect, JobService, the ConfigBIOS: naming and Dell's job resource. The in-flight job states (Scheduled, Starting, …) are only confirmed on the two generations where I created a job; an unknown state is refused rather than deleted.