[DNM] Initial attempt at using wait step to back out of failed servicing.#452
[DNM] Initial attempt at using wait step to back out of failed servicing.#452jacob-anders wants to merge 4 commits intoopenshift:mainfrom
Conversation
|
PR meant for cluster-bot to allow rapid testing of an important workaround |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jacob-anders The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
||
| // Track if HFS spec has actual settings - check independently since getHostFirmwareSettings | ||
| // returns nil when no changes even if object exists | ||
| hfsExists := &metal3api.HostFirmwareSettings{} |
There was a problem hiding this comment.
You probably also do this Get inside of getHostFirmwareSettings, let's try to avoid another request
There was a problem hiding this comment.
Actually, I feel we discussed this already, probably it's an issue of digging out an old unmerged patch. Will sort it out.
| // Track if HFC spec has actual updates - check independently since getHostFirmwareComponents | ||
| // returns nil when no changes even if object exists | ||
| hfcExists := &metal3api.HostFirmwareComponents{} | ||
| hfcExistsErr := r.Get(info.ctx, info.request.NamespacedName, hfcExists) |
| // let the provisioner handle the transition back to active | ||
| if info.host.Status.ErrorType == metal3api.ServicingError && !hasChanges { | ||
| info.log.Info("updates removed from spec while in servicing error state, attempting recovery") | ||
| provResult, _, err := prov.Service(servicingData, false, false) |
There was a problem hiding this comment.
Thinking aloud: as we grow capabilities to abort processes, maybe we need a separate Abort call in the provisioner. Then you won't need to pass all these Has*Spec variables, and the Ironic-level code will be cleaner (if not necessarily shorter).
pkg/provisioner/ironic/servicing.go
Outdated
| ironicNode, | ||
| nodes.ProvisionStateOpts{Target: nodes.TargetAbort}, | ||
| ) | ||
| p.log.Info("janders_debug: abort result", "started", started, "result", result, "error", err) |
pkg/provisioner/ironic/servicing.go
Outdated
| result, err = operationComplete() | ||
| case nodes.Servicing, nodes.ServiceWait: | ||
| // If user actually removed spec.updates/spec.settings while servicing is in progress, abort immediately | ||
| if !data.HasFirmwareSettingsSpec && !data.HasFirmwareComponentsSpec { |
There was a problem hiding this comment.
This cannot be done in Servicing, only in ServiceWait
There was a problem hiding this comment.
Yeah fair point will drop nodes.Servicing.
openshift-merge-robot
added
the
needs-rebase
Generated-By: Claude Code Sonnet 4 Signed-off-by: Jacob Anders <janders@redhat.com> (cherry picked from commit c40d0ad)
jacob-anders
force-pushed
the
bmo-service-abort-initial
branch
from
ab976fa to
05396cd
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/test e2e-metal-ipi-ovn-ipv6 |
|
@jacob-anders: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Generated-By: Claude Code Sonnet 4
(cherry picked from commit c40d0ad)