Skip to content

Implement an Unmanaged (discovered) state (2)#569

Merged
metal3-io-bot merged 6 commits intometal3-io:masterfrom
dhellmann:unmanaged-state
Jul 13, 2020
Merged

Implement an Unmanaged (discovered) state (2)#569
metal3-io-bot merged 6 commits intometal3-io:masterfrom
dhellmann:unmanaged-state

Conversation

@dhellmann
Copy link
Copy Markdown
Member

If no BMC credentials are provided, move the host to a new Unmanaged
state and change the operational status to discovered, rather than
remaining without a provisioning state and in an operational error.

This has always been envisaged in the state machine diagram (with the name
Discovered rather than Unmanaged), but was not previously implemented.

This is a rebased version of #563

@metal3-io-bot metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2020
@dhellmann
Copy link
Copy Markdown
Member Author

/test-integration

@dhellmann
Copy link
Copy Markdown
Member Author

/cc @n1r1 @kirankt @honza @stbenjam

@metal3-io-bot
Copy link
Copy Markdown
Contributor

@dhellmann: GitHub didn't allow me to request PR reviews from the following users: n1r1.

Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @n1r1 @kirankt @honza @stbenjam

Instructions 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/test-infra repository.

@n1r1
Copy link
Copy Markdown
Member

n1r1 commented Jun 30, 2020

copying the comments from previous PR:

We also need to consider the consequences on other components that already have some assumptions on the existing API

@n1r1 are you thinking of something specific? The cluster-api-provider-* clients?

Yes. CAPx clients and downstream in OCP the UI might need some changes as well (e.g. gray out the reboot button if host has no BMC details)

Comment thread pkg/apis/metal3/v1alpha1/baremetalhost_types.go
@dhellmann
Copy link
Copy Markdown
Member Author

/test-integration

@dhellmann
Copy link
Copy Markdown
Member Author

yum mirror access failed

/test-integration

@dhellmann
Copy link
Copy Markdown
Member Author

Since this is technically an API change and I keep asking people to write up docs for those, I've put metal3-io/metal3-docs#120 up for review.

dhellmann pushed a commit to dhellmann/baremetal-operator that referenced this pull request Jul 2, 2020
Define a new state for newly-created Hosts with no BMC details.

We need the state definition so we can add it to CAPM3 so that does
not break when metal3-io#569 merges.

See metal3-io/metal3-docs#120 for more
details.
@dhellmann
Copy link
Copy Markdown
Member Author

This will need to be rebased on top of #571

@dhellmann dhellmann force-pushed the unmanaged-state branch 2 times, most recently from d76a168 to ecf6f93 Compare July 2, 2020 17:26
@dhellmann dhellmann requested a review from hardys July 6, 2020 20:48
zaneb and others added 6 commits July 7, 2020 14:32
This state isn't actually implemented in the code, so before we
implement it rename it to "Unmanaged". This reflects the fact that what
this state really means is that the Host has been created without
attempting to provide credentials. That may have transpired through a
discovery process or some other mechanism.
Bring the code into line with the state machine documentation, which
says that once the Host is reconciled it will immediately move into one
of the states (registering or unmanaged) and not remain with an empty
provisioning state. Since the Unmanaged state has not been implemented
yet, move unconditionally to Registering.

Previously, we waited until the the BMC credentials had at least once
been deemed worthy of trying (present, correctly formatted) before the
Host would move to the Registering state.
Newly-created Hosts enter the Unmanaged state if no BMC details are
provided in the resource Spec.

As soon as details are provided - regardless of whether they are
complete or correctly formatter - the Host will return to the
Registering state. Even if all of the details are later removed, the
Host will not return to the Unmanaged state.

This is in line with how this state is documented in the state machine
diagram, but it was never previously implemented.
When a host is discovered or otherwise created without any BMC data, it now
goes into an Unmanaged state, in which buildAndValidateBMCCredentials()
is not called.

Once a host leaves the unmanaged state (i.e. the BMC address and/or
credentials have been supplied at least once), any time the BMC address
and/or credentials are missing it is due to a configuration error, not
to having newly-discovered a host.

Since the unmanaged state handles newly-discovered hosts, in the error
handling for buildAndValidateBMCCredentials we can now flag an error if
any of the data is missing. This prevents transitions like a Host going
back into a "discovered" operational status despite having been
provisioned already before the credentials were removed. That should be,
and now is, treated as an error.
Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann
Copy link
Copy Markdown
Member Author

/test-integration

@dhellmann
Copy link
Copy Markdown
Member Author

Rebased after #571 merged

@dhellmann
Copy link
Copy Markdown
Member Author

/hold we need to merge metal3-io/cluster-api-provider-metal3#108 and metal3-io/cluster-api-provider-metal3#109 before this

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2020
@maelk
Copy link
Copy Markdown
Member

maelk commented Jul 9, 2020

/test-integration
Even if this is not yet on CAPM3 side, the integration tests should pass

@dhellmann
Copy link
Copy Markdown
Member Author

/hold clear
/cc @zaneb

This is ready for review and merging.

@metal3-io-bot metal3-io-bot requested a review from zaneb July 13, 2020 17:43
@zaneb
Copy link
Copy Markdown
Member

zaneb commented Jul 13, 2020

/approve

@metal3-io-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, 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:

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

@stbenjam
Copy link
Copy Markdown
Member

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2020
@stbenjam
Copy link
Copy Markdown
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2020
@metal3-io-bot metal3-io-bot merged commit 9aba030 into metal3-io:master Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants