Add support for detached annotation by hardys · Pull Request #827 · metal3-io/baremetal-operator

hardys · 2021-03-23T15:48:53Z

This allows a new annotation as discussed in metal3-io/metal3-docs#168

baremetalhost.metal3.io/unmanaged

EDIT1 this has now been renamed to "deregistered" for both the annotation and OperationalStatus - I'll update the docs PR and this description when we've reached consensus on that new name.

EDIT2 ...then renamed again to "detached"

When set this puts the BMH OperationalStatus into a "unmanaged",
which removes the host from the underlying provisioner (without
deprovisioning)

When the annotation removed the BMH can resume management without
any disruptive actions (the previous Provisioning State is maintained)

hardys · 2021-03-23T15:49:37Z

/cc @zaneb @dhellmann @andfasano

hardys · 2021-03-23T15:52:33Z

WIP as I'd like to confirm we're happy with having both an Unmanaged ProvisioningState, and an unmanaged OperationalStatus - initially I was thinking this may be confusing, but the behavior is quite similar (in both cases we skip management by the provisioner), the main difference is the OperationalStatus is independent of the provisioning state, which I think is what we want?

Also needs tests/docs, and I'll refactor the new code into a function from the Reconcile()

hardys · 2021-03-23T16:13:23Z

/cc @maelk @fmuyassarov - this leaves the existing paused annotation behavior intact, and allows for a pivot where the BMH resources remain as inventory on the parent cluster.

In the community meeting we discussed the CAPI pivot process, can you confirm if this approach will be acceptable from that perspective since it's not impacting any existing APIs?

fmuyassarov · 2021-03-24T12:02:48Z

/cc @maelk @fmuyassarov - this leaves the existing paused annotation behavior intact, and allows for a pivot where the BMH resources remain as inventory on the parent cluster.

In the community meeting we discussed the CAPI pivot process, can you confirm if this approach will be acceptable from that perspective since it's not impacting any existing APIs?

I believe it is okay, since this patch is not touching the pause annotation and logic in Reconcile().
/cc @kashifest

kashifest · 2021-03-24T13:00:19Z

/cc @maelk @fmuyassarov - this leaves the existing paused annotation behavior intact, and allows for a pivot where the BMH resources remain as inventory on the parent cluster.

In the community meeting we discussed the CAPI pivot process, can you confirm if this approach will be acceptable from that perspective since it's not impacting any existing APIs?

As long as paused and status Annotation behavior is intact, CAPI pivot process should be good IMO.

hardys · 2021-03-25T20:17:13Z

@zaneb @andfasano I reworked this to the state machine as suggested, the new provisioner API (and docs/tests) are still TODO.

PTAL, if you're happy with this revised approach I'll proceed with the remaining pieces - thanks!

zaneb · 2021-03-25T21:38:42Z


+	// Deregistered is the annotation which stops provisioner management of the host
+	// unlike in the paused case, the host status may be updated
+	DeregisteredAnnotation = "baremetalhost.metal3.io/deregistered"


I got out the thesaurus and here is some more suggestions in order of my preference:

disengage

dissociate

detach

To put this in perspective, here are some other words I am not suggesting (you're welcome):

sunder

unloose

disjoin

uncouple

dissever

Argh! If only Unmanaged and paused weren't already in use! 😆

After thinking about the name I reached the conclusion that deregistered reflects the internal state quite well (and it was definitely confusing having a State and OperationalStatus both called unmanaged after I moved the code into the same file).

Then it seemed weird to dislocate that naming convention at the annotation level, so I just renamed it, and personally I think deregistered is an OK name - given that the two most obvious alternatives are already in use?

With the benefit of hindsight we should perhaps have called the paused annotation frozen or something, but ultimately we just have to pick a name then document what it does.

I don't want to burn a bunch more cycles on the naming, but does anyone else have strong opinions on this?

Not a strong one, my inclination would be more on detach rather then deregister - just because we already have a Registering state, and to me sounds as something related. detach gives me more the idea of a BMH still alive but "detached" from an Ironic node.

BTW I intended "disengage" in the sense of a gearbox. We're basically putting it in neutral so you can attach another engine for a bit. It's quite a nice analogy, at least if you get a chance to explain it to the user ;D
My only concern with "detach", and why I listed it in 3rd place, is that it is more prone to conflicts with other things like networks or volumes (i.e. it risks repeating the same mistake we have made with "paused" and "unmanaged"). I still prefer it to "deregister" though.
"Deregister" makes sense if you understand the inside baseball (i.e. that there's a separate volatile data store, that existence of an entry in this datastore means that the power state will be continuously monitored, and that when this data store is cleared we silently re-register existing hosts in it.) However, to a user looking in from the outside and seeing only the API, I think it makes little sense at best, and at worst is actively misleading by implying that it resets the state back to the time before "registration".

Ugh. Naming is hard 😞

@dhellmann @maelk @fmuyassarov - any preference before I rename this again?

FWIW my vote is for detached - I take the point about potential overlap with networking/storage things @zaneb but in the context of the BMH resource that's probably not very likely, right?

Changed to detached in the latest revision

that's probably not very likely, right?

🤣

metal3-io/metal3-docs#132

that's probably not very likely, right?

rofl

metal3-io/metal3-docs#132

Thanks I missed that - and it adds a ports API to the BMH spec - I'm not sure that's a good idea, I was thinking any such additions would have new CRDs, and thus not overlap with BMH things, but it seems I've been wildly optimistic there.

This currently does the same as Delete but as mentioned in review of metal3-io#827 semantically this is a different action and should have a separate API call in the Provisioner interface

hardys · 2021-03-31T16:31:56Z

/retitle Add support for detached annotation

hardys · 2021-03-31T16:34:10Z

/test-integration

hardys · 2021-04-06T15:47:24Z

/test-integration

zaneb · 2021-04-07T01:12:37Z

+	// Note this doesn't change the current state, only the OperationalStatus
+	annotations := hsm.Host.GetAnnotations()
+	if annotations != nil {
+		if _, ok := annotations[metal3v1alpha1.DetachedAnnotation]; ok {


nit: a hasDetachedAnnotation() convenience function wouldn't hurt.

Done, I added a local function for now, but this seems to be a recurring pattern, so perhaps we can refactor to add a shared host.HasAnnotation() in future (I didn't attempt it here because it means moving some annotation constants which feels outside the scope of this series).

This currently does the same as Delete but as mentioned in review of metal3-io#827 semantically this is a different action and should have a separate API call in the Provisioner interface

hardys · 2021-04-07T11:13:09Z

/test-integration

andfasano · 2021-04-07T13:06:58Z

/lgtm
/hold
to allow @zaneb a final pass

zaneb

I have a couple of comments inline about the error handling. It would be nice if we could address those (particularly since it means we can undo the refactoring of clearError(), but given that both the Detach errors and Power Management errors are an entirely theoretical concept today, I also don't see any obstacle to merging this and fixing it in a follow-up.
/approve

zaneb · 2021-04-07T15:49:11Z

+	if hasDetachedAnnotation(hsm.Host) {
+		// Only allow detaching hosts in Provisioned/ExternallyProvisioned states
+		switch info.host.Status.Provisioning.State {
+		case metal3v1alpha1.StateProvisioned, metal3v1alpha1.StateExternallyProvisioned:


There's probably a case to be made for allowing this in the Ready/Available state, but I'm fine with leaving it until someone actually has a requirement for it.

metal3-io-bot · 2021-04-07T16:04:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hardys,zaneb]

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

Define a new annotation and operational status to describe the case where a BMH is detached

This currently does the same as Delete but as mentioned in review of metal3-io#827 semantically this is a different action and should have a separate API call in the Provisioner interface

zaneb · 2021-04-07T16:32:24Z

/lgtm
/hold cancel

zaneb · 2021-04-07T16:33:30Z

/test-integration

metal3-io-bot requested review from andfasano, dhellmann and zaneb March 23, 2021 15:49

hardys mentioned this pull request Mar 23, 2021

Add detached annotation proposal metal3-io/metal3-docs#168

Merged

andfasano reviewed Mar 23, 2021

View reviewed changes

Comment thread controllers/metal3.io/baremetalhost_controller.go Outdated

metal3-io-bot requested a review from kashifest March 24, 2021 12:02

zaneb reviewed Mar 24, 2021

View reviewed changes

hardys force-pushed the unmanaged_annotation branch from 41c1ba9 to 686e263 Compare March 25, 2021 20:09

hardys changed the title ~~WIP: Add support for unmanaged annotation~~ WIP: Add support for deregistered annotation Mar 25, 2021

zaneb reviewed Mar 26, 2021

View reviewed changes

andfasano reviewed Mar 26, 2021

View reviewed changes

Comment thread controllers/metal3.io/host_state_machine.go Outdated

hardys force-pushed the unmanaged_annotation branch from 686e263 to bc4f7b5 Compare March 29, 2021 16:47

hardys force-pushed the unmanaged_annotation branch from bc4f7b5 to 8de5130 Compare March 30, 2021 15:21

metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 30, 2021

hardys changed the title ~~WIP: Add support for deregistered annotation~~ WIP: Add support for detached annotation Mar 31, 2021

metal3-io-bot changed the title ~~WIP: Add support for detached annotation~~ Add support for detached annotation Mar 31, 2021

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 Mar 31, 2021

hardys force-pushed the unmanaged_annotation branch 2 times, most recently from bc54963 to 26dd0b1 Compare April 6, 2021 09:27

hardys commented Apr 6, 2021

View reviewed changes

Comment thread controllers/metal3.io/host_state_machine.go

sadasu reviewed Apr 6, 2021

View reviewed changes

Comment thread controllers/metal3.io/host_state_machine.go

zaneb reviewed Apr 7, 2021

View reviewed changes

Comment thread controllers/metal3.io/baremetalhost_controller.go

Comment thread controllers/metal3.io/host_state_machine.go

zaneb reviewed Apr 7, 2021

View reviewed changes

andfasano reviewed Apr 7, 2021

View reviewed changes

hardys force-pushed the unmanaged_annotation branch 2 times, most recently from 1fdd305 to 582d8ad Compare April 7, 2021 11:10

metal3-io-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 7, 2021

zaneb reviewed Apr 7, 2021

View reviewed changes

Steven Hardy added 2 commits April 7, 2021 17:07

Add detached annotation to BMH resource

ec8c219

Define a new annotation and operational status to describe the case where a BMH is detached

Add Detach provisioner API

64239c4

This currently does the same as Delete but as mentioned in review of metal3-io#827 semantically this is a different action and should have a separate API call in the Provisioner interface

hardys force-pushed the unmanaged_annotation branch from 582d8ad to a21f536 Compare April 7, 2021 16:15

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

hardys force-pushed the unmanaged_annotation branch from a21f536 to 349fa01 Compare April 7, 2021 16:19

Steven Hardy added 2 commits April 7, 2021 17:21

Handle detached annotation in host state machine

e978594

Add docs for detached annotation

569c094

hardys force-pushed the unmanaged_annotation branch from 349fa01 to 569c094 Compare April 7, 2021 16:21

metal3-io-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 7, 2021

metal3-io-bot merged commit adbd5df into metal3-io:master Apr 7, 2021

Conversation

hardys commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hardys commented Mar 23, 2021

Uh oh!

hardys commented Mar 23, 2021

Uh oh!

hardys commented Mar 23, 2021

Uh oh!

Uh oh!

fmuyassarov commented Mar 24, 2021

Uh oh!

kashifest commented Mar 24, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardys commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardys Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardys commented Mar 31, 2021

Uh oh!

hardys commented Mar 31, 2021

Uh oh!

Uh oh!

hardys commented Apr 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardys commented Apr 7, 2021

Uh oh!

andfasano commented Apr 7, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hardys commented Mar 23, 2021 •

edited

Loading

hardys Mar 26, 2021 •

edited

Loading