Skip to content

Attempt to power off BareMetalHost before delete#412

Closed
sadasu wants to merge 1 commit intometal3-io:masterfrom
sadasu:bmh-delete
Closed

Attempt to power off BareMetalHost before delete#412
sadasu wants to merge 1 commit intometal3-io:masterfrom
sadasu:bmh-delete

Conversation

@sadasu
Copy link
Copy Markdown
Member

@sadasu sadasu commented Feb 3, 2020

When a BareMetalHost is deleted, power it off before performing
the delete operation.
Fixes #410

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 3, 2020
Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
Copy link
Copy Markdown
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, reading through the provisioner code just now, it looks like PowerOff never returns an ErrorMessage. Ideally we would check for one, and if it is present just log it and increment a prometheus metric, then carry on as we're doing here. But given that it can't happen right now, I'm not too worried about it.

Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
@sadasu sadasu changed the title WIP: Attempt to power off BareMetalHost before delete Attempt to power off BareMetalHost before delete Feb 26, 2020
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 26, 2020
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Apr 15, 2020

/test-integration

Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
@zaneb
Copy link
Copy Markdown
Member

zaneb commented May 12, 2020

I opened a separate PR (#514) to clean up the existing problem with saveStatus, because it turns out it might be related to a bug so that's probably a good reason to keep it separate.

@dhellmann
Copy link
Copy Markdown
Member

@sadasu are you still working on this PR?

@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented May 22, 2020

@sadasu are you still working on this PR?

Yes, trying to understand PR (#514) and modify implementation here.

@dhellmann
Copy link
Copy Markdown
Member

@sadasu are you still working on this PR?

Yes, trying to understand PR (#514) and modify implementation here.

Sounds good. There's no particular rush, I was looking at some PRs that I might be able to help with rebasing or something to move forward. Little stuff for a Friday afternoon. :-)

@zaneb
Copy link
Copy Markdown
Member

zaneb commented May 22, 2020

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2020
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented May 26, 2020

/test-integration

@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented May 26, 2020

@dhellmann could you please take a look? I need an approve label.

Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
@zaneb
Copy link
Copy Markdown
Member

zaneb commented May 27, 2020

On closer inspection, it looks like the provisioner code doesn't actually check for a failed attempt to power off (it doesn't look at any error messages from Ironic), so I suspect we will just end up retrying forever if the credentials don't work.
In addition, if we can find the host in Ironic (possibly due to a previous call to actionDelete() removing it) then there'll be an error that sends us into an infinite retry loop.
There's no current way to distinguish this error from other errors (other than looking at the text), and in any event it might be wise to just (log +) ignore all errors from this call anyway.

/lgtm cancel

@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label May 27, 2020
@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sadasu
To complete the pull request process, please assign zaneb
You can assign the PR to them by writing /assign @zaneb in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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 metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 2, 2020
@sadasu sadasu changed the title Attempt to power off BareMetalHost before delete WIP: Attempt to power off BareMetalHost before delete Jun 2, 2020
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2020
Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
Comment thread pkg/controller/baremetalhost/baremetalhost_controller.go Outdated
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 13, 2020
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Aug 13, 2020

/test-integration

@sadasu sadasu changed the title WIP: Attempt to power off BareMetalHost before delete Attempt to power off BareMetalHost before delete Aug 13, 2020
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2020
@sadasu sadasu changed the title Attempt to power off BareMetalHost before delete WIP: Attempt to power off BareMetalHost before delete Nov 10, 2020
@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 10, 2020
@sadasu sadasu changed the title WIP: Attempt to power off BareMetalHost before delete Attempt to power off BareMetalHost before delete Nov 12, 2020
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2020
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Nov 12, 2020

/test-integration

Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@metal3-io-bot
Copy link
Copy Markdown
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@metal3-io-bot metal3-io-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 10, 2021
@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Feb 10, 2021

/remove-lifecycle stale

@metal3-io-bot metal3-io-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 10, 2021
@s3rj1k
Copy link
Copy Markdown
Member

s3rj1k commented Feb 10, 2021

Any updates on this?

Comment thread pkg/provisioner/provisioner.go Outdated
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Mar 1, 2021

/test-integration

Comment thread pkg/provisioner/ironic/ironic.go
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Mar 2, 2021

/test-integration

Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Mar 4, 2021

/test-integration

Comment thread pkg/provisioner/ironic/ironic.go Outdated
Comment thread pkg/provisioner/ironic/ironic.go Outdated
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Mar 5, 2021

/test-integration

@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Mar 5, 2021

/test-integration

Comment thread pkg/provisioner/ironic/delete_test.go
@sadasu
Copy link
Copy Markdown
Member Author

sadasu commented Mar 16, 2021

Implementation has moved to #816.

@sadasu sadasu closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Host should be powered down once deleted

6 participants