✨ Allow forced detachment of a host from Ironic#2955
✨ Allow forced detachment of a host from Ironic#2955CrystalChun wants to merge 3 commits intometal3-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Hi @CrystalChun. Thanks for your PR. I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
b67394d to
c92b26b
Compare
|
/ok-to-test |
|
@CrystalChun while I'm testing something locally, could you squash your commits and remove references to RH Jira from the messages? |
c92b26b to
33f438d
Compare
Yes, squashed my commits and removed the RH Jira mentions from the commit message and the description of this PR! |
33f438d to
86ae602
Compare
86ae602 to
3c4f7c1
Compare
3c4f7c1 to
371f113
Compare
|
@CrystalChun I've provided a draft implementation, needs testing now (also together with soon-to-be-merged Ironic change https://review.opendev.org/c/openstack/ironic/+/973279) |
dbf2a88 to
628f858
Compare
628f858 to
2cd903e
Compare
72ec11b to
e086f2b
Compare
611558f to
13bb2e2
Compare
|
/hold Despite 2 seconds sleep, one of the tests still did not reach "deploying" before forced detachment was applied. |
13bb2e2 to
52b2698
Compare
|
/hold cancel I had to drop the e2e tests for now to avoid blocking the entire feature. There are signs that they would pass, but they're timing-dependent, which is pretty bad. I'll propose them separately. |
|
I will take a look soon, for some reason this PR have not received the 2reviewer automatically. |
|
In my review queue now, I would like to merge this before 0.13 release @Sunnatillo @smoshiur1237 please keep an eye on this PR. |
There was a problem hiding this comment.
Pull request overview
Adds support for forced BareMetalHost detachment so operators can request immediate detachment even when the host is mid-operation, with Ironic-side logic attempting to abort in-progress workflows when possible.
Changes:
- Extend the
Provisionerinterface and controller detachment flow to accept aforceboolean. - Implement forced-detach behavior in the Ironic provisioner by reusing deletion logic and issuing
abort/fallback transitions depending on Ironic microversion support. - Add unit tests for Ironic forced-detach behavior and introduce an API annotation argument (
force) to trigger forced detachment.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/provisioner/provisioner.go | Updates the provisioner interface to accept a force flag for detachment. |
| pkg/provisioner/ironic/ironic.go | Refactors deletion into realDelete(..., force) and wires Detach(..., force) to use it, including abort/deprovision logic by state. |
| pkg/provisioner/ironic/delete_test.go | Adds unit test coverage for forced detachment state handling and updates existing detach calls for the new signature. |
| pkg/provisioner/ironic/clients/features.go | Adds detection for deployment-abort support and selects microversion 1.110 when available. |
| pkg/provisioner/fixture/fixture.go | Updates fixture provisioner to implement the new Detach(ctx, force) signature. |
| pkg/provisioner/demo/demo.go | Updates demo provisioner to implement the new Detach(ctx, force) signature. |
| internal/controller/metal3.io/host_state_machine_test.go | Updates mock provisioner to match the new Detach(ctx, force) signature. |
| internal/controller/metal3.io/host_state_machine.go | Enables forced detach via detached annotation JSON args and adjusts delete initiation behavior for detached hosts. |
| internal/controller/metal3.io/baremetalhost_controller.go | Plumbs the force flag through reconciler detachment handling. |
| apis/metal3.io/v1alpha1/baremetalhost_types.go | Adds force to DetachedAnnotationArguments for annotation-driven forced detachment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hasDetachedAnnotation(hsm.Host) { | ||
| // Only allow detaching hosts in Provisioned/ExternallyProvisioned/Ready/Available states | ||
| // Only allow detaching hosts in Provisioned/ExternallyProvisioned/Ready/Available states unless forced | ||
| switch info.host.Status.Provisioning.State { | ||
| case metal3api.StateProvisioned, metal3api.StateExternallyProvisioned, metal3api.StateReady, metal3api.StateAvailable: | ||
| return hsm.Reconciler.detachHost(ctx, hsm.Provisioner, info) | ||
| return hsm.Reconciler.detachHost(ctx, hsm.Provisioner, info, false) | ||
| case metal3api.StateDeleting: | ||
| // No point in detaching a host that is being deleted already | ||
| default: | ||
| if hasForceDetachAnnotation(hsm.Host) { | ||
| info.log.Info("forcing detach of host", "provisioningState", info.host.Status.Provisioning.State) | ||
| return hsm.Reconciler.detachHost(ctx, hsm.Provisioner, info, true) | ||
| } |
There was a problem hiding this comment.
The new forced-detach path (hasForceDetachAnnotation + passing force=true into detachHost) is not covered by the existing host state machine unit tests. Add a unit test that sets the detached annotation payload to JSON with {"force":true} while the host is in a non-detachable provisioning state (e.g. StateProvisioning) and asserts the reconciler calls Provisioner.Detach(..., true) and transitions OperationalStatus to Detached appropriately.
Adds Force as DetachedAnnotationArgument to API. Detach in all states if force is set to true by aborting the currently running process. Co-Authored-By: Dmitry Tantsur <dtantsur@protonmail.com> Signed-off-by: CrystalChun <cchun@redhat.com> Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
The deletion logic does not need to be aware of which states are valid for detachment. If the operationalStatus is detached, the host can be deleted immediately. Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
52b2698 to
38f73aa
Compare
What this PR does / why we need it:
Introduces the
forceargument for detaching. If set to true, it would bypass whichever status the host is currently in, abort the action in Ironic, and force the host's detachment.Note: there is a soft dependency on the new feature in Ironic that allows the
abortaction on deploying nodes. Without it, deprovisioning will be triggered first, which won't preserve the exact host's state.Finally, I have prepared e2e tests for this change, but they ended up being timing-dependent. To fix this dependency, I need direct access to Ironic Node record, which is going to greatly increase the already large size of this change. The tests will be proposed separately.
Fixes #2923
Checklist: