WIP: host discovery design by dhellmann · Pull Request #98 · metal3-io/metal3-docs

dhellmann · 2020-06-03T17:21:15Z

Addresses metal3-io/baremetal-operator#41

metal3-io-bot · 2020-06-03T17:21:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann

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~~ [dhellmann]

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

maelk

That would be a great feature to have. I am wondering how usable the provisioning without BMC credentials would actually be, considering that Ironic tries to do operations with the BMC in between.

ohadlevy · 2020-06-04T18:37:43Z

Back in the days I defaulted to always boot over pxe, and in case the host wasn't up to provisioning it would have a pxe configuration to boot from disk... This way it's not required to change boot order at all but change tftp / ipxe output

…

On Thu, 4 Jun 2020, 21:20 Doug Hellmann, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In design/host-discovery.md <#98 (comment)>: > +9. Add a method to `BareMetalHost` to report whether power control + operations are supported. For now, this will only look at whether + there are BMC credentials. +10. Update the bare metal controller to skip trying to apply power + state management operations to hosts that do not support them. It used to be that if you configured a host to boot from disk then PXE, if there was no image on the disk the host would skip that step and just PXE. I assume that's still the case, but maybe things changed? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#98 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE5R5H3ZMQYA3XIWXN3XTRU7QWLANCNFSM4NR3WQIA> .

dhellmann · 2020-06-04T18:48:52Z

Back in the days I defaulted to always boot over pxe, and in case the host wasn't up to provisioning it would have a pxe configuration to boot from disk... This way it's not required to change boot order at all but change tftp / ipxe output

That would also be a good option. We could investigate adding support for managing the dnsmasq settings for individual hosts to Ironic or the BMO, if it proves necessary.

We're also trying to support non-PXE cases using manually attached ISOs, so if we rely entirely on configuring the PXE response we'll have trouble with those cases and have to provide instructions for the user anyway.

maelk · 2020-06-05T06:01:37Z

There was an interesting discussion yesterday in the airship PTG related to this IPAM configuration. The idea was to look into whether Ironic could decouple from Neutron and Dnsmasq to provide this exact feature. so maybe that could be taken care of by Ironic instead of BMO in that case

dhellmann · 2020-06-10T13:53:22Z

As discussed in the community meeting on 10 June, this proposal will be merged based on lazy consensus approval on (or soon after) 19 June. If anyone has concerns or objections, please leave review comments so we can resolve them or hold off.

Addresses metal3-io/baremetal-operator#41 Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Rather than having the baremetal-operator try to figure out which ironic node a host matches, we could have the new controller create the host with enough of the status filled in via the annotation to ensure that the match works. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-06-29T18:13:51Z

Based on similar discussions about keeping the annotation up to date for pivoting, I updated this design to simplify the workflow a bit. Rather than having the baremetal-operator use the MAC to find an ironic node and update the host, we can have the new discovery controller create the host with the status annotation set so the baremetal-operator only needs to copy the data from one field to the other in the resource, as it already does. This will result in fewer changes in the baremetal-operator, and we won't need a new state in the state machine.

dhellmann · 2020-06-29T18:14:33Z

/cc @kirankt @honza

dhellmann · 2020-06-30T18:50:57Z

/hold

kirankt · 2020-06-30T18:51:56Z

Based on similar discussions about keeping the annotation up to date for pivoting, I updated this design to simplify the workflow a bit. Rather than having the baremetal-operator use the MAC to find an ironic node and update the host, we can have the new discovery controller create the host with the status annotation set so the baremetal-operator only needs to copy the data from one field to the other in the resource, as it already does. This will result in fewer changes in the baremetal-operator, and we won't need a new state in the state machine.

I think this doc is very well written and many things are clear. Few questions:

Is BMO still responsible for deploying all ironic services, including ironic-inspector?
Which entity will be responsible to launch the host discovery controller? SLO?

maelk · 2020-07-01T07:04:30Z

Hi @kirankt ! We have reworked a bit the deployment process. BMO is now separated from ironic, we have kustomizations to deploy ironic and all related containers. BMO is deployed standalone or as part of CAPM3, with clusterctl. I would expect new controllers related to BMO to also be deployed as part of CAPM3 when using clusterctl, or if not using CAPI, be deployed as part of BMO, included in the BMO kustomization. What do you think about this @dhellmann ?

dhellmann · 2020-07-02T15:50:17Z

@kirankt wrote:

Is BMO still responsible for deploying all ironic services, including ironic-inspector?

The baremetal-operator doesn't actually manage the deployment at all. Upstream, kustomize is used to generate the deployment instructions. As @maelk mentioned adding a new controller will mean adding that controller to the deployment. Downstream in OpenShift, when we adopt the feature, we will also need to update the operator we have that generates our deployment settings.

Which entity will be responsible to launch the host discovery controller? SLO?

Downstream in OpenShift, yes, I don't see us adopting this before the new operator for managing metal3 is done.

@maelk wrote:
I would expect new controllers related to BMO to also be deployed as part of CAPM3 when using clusterctl, or if not using CAPI, be deployed as part of BMO, included in the BMO kustomization. What do you think about this @dhellmann ?

I intend for this to be a completely new stand-alone controller, to let us use kubebuilder instead of operator-sdk. That may complicate deployment a bit, but the feature is optional so maybe that's not a problem.

I see some stuff in the CAPM3 repo under config that looks like it brings in the BMO when CAPM3 is deployed, so it would make sense to do something similar with the new controller. Does that seem reasonable?

maelk · 2020-07-03T07:22:16Z

I intend for this to be a completely new stand-alone controller, to let us use kubebuilder instead of operator-sdk. That may complicate deployment a bit, but the feature is optional so maybe that's not a problem.

I see some stuff in the CAPM3 repo under config that looks like it brings in the BMO when CAPM3 is deployed, so it would make sense to do something similar with the new controller. Does that seem reasonable?

For CAPM3 definitely, we'll pull it in as part of CAPM3 deployment. Then it should have its own deployment to be independent of BMO, and be included in CAPM3, the same way we do it for ip-address-manager (with releases) or BMO (without releases).

zaneb

Some of the state stuff is still unclear/contradictory. I had a go at converting my suggestions inline into an updated state machine diagram, and the diff looked like this:

--- docs/BaremetalHost_ProvisioningState.dot
+++ docs/BaremetalHost_ProvisioningState.dot
@@ -1,17 +1,24 @@
 digraph BaremetalHost {
     Created [shape=house]
-    Created -> Unmanaged [label="BMC.* == \"\""]
+    Created -> Unmanaged [label="BMC.* == \"\" && NeedsHardwareInspection()"]
+    Created -> Discovered [label="BMC.* == \"\" && !NeedsHardwareInspection()"]
     Created -> Registering [label="BMC.* != \"\""]
 
     Unmanaged [shape=doublecircle]
     Unmanaged -> Registering [label="BMC.* != \"\""]
+    Unmanaged -> Provisioned [label="BMC.* != \"\" && Status.Provisioning.Image"]
     Unmanaged -> Deleting1 [label="!DeletionTimestamp.IsZero()"]
 
+    Discovered [shape=doublecircle]
+    Discovered -> Registering [label="BMC.* != \"\""]
+    Discovered -> Provisioning [label="NeedsProvisioning()"]
+
     Deleting1 [shape=point]
 
     ExternallyProvisioned [label="Externally\nProvisioned"]
 
     Registering -> Inspecting [label="!externallyProvisioned && NeedsHardwareInspection()"]
+    Registering -> MatchProfile [label="!externallyProvisioned && !NeedsHardwareInspection()"]
     Registering -> ExternallyProvisioned [label="externallyProvisioned"]
     Registering -> Deleting2 [label="!DeletionTimestamp.IsZero()"]
 
@@ -41,6 +48,7 @@ digraph BaremetalHost {
     Deleting6 [shape=point]
 
     Provisioning -> Provisioned [label=done]
+    Provisioning -> Unmanaged [label="TriedCredentials.*=\"\""]
     Provisioning -> Deprovisioning [label="failed || !DeletionTimestamp.IsZero()"]
 
     Provisioned [shape=doublecircle]

zaneb · 2020-07-23T13:42:44Z

+template values and with an annotation containing the hardware details
+and provisioner ID that should be added to the status fields.
+
+When a user adds BMC credentials to a host in the `Discovered` state


Did you mean Unmanaged? There is no Discovered state today.

Yeah, the original draft of this predates the unmanaged work, I think.

zaneb · 2020-07-23T13:51:11Z

+
+If multiple `BareMetalHostDiscovery` resources exist, there may be
+multiple attempts to create a host from discovered information by
+different goroutines in the `host-discovery-controller`. If the name


We could configure the operator to only reconcile one HostDiscovery resource at a time; that would reduce races.
Not sure why we would need goroutines as well as the reconcile loop?

zaneb · 2020-07-23T14:01:48Z

+`baremetal-operator` to be invoked when a resource is created and to
+block duplicate hosts. See [the v1alpha2
+migration](https://github.com/metal3-io/metal3-docs/pull/101) plans
+for details related to allowing webhooks. In the future, we may add


Also relevant is metal3-io/baremetal-operator#581

zaneb · 2020-07-23T14:28:08Z

+today it moves to `Registering` and the `Provisioner` is responsible
+for configuring the host in Ironic. That logic may need to be modified
+to support partial updates of existing nodes, to set the values that
+were not not set when the node was discovered.


How do we distinguish between hosts that have been discovered and those added manually without credentials? (Presence of HardwareDetails is the only obvious answer.) Will we have a separate Discovered state for those resources?
Will we do provisioning of discovered hosts by allowing them to move directly from the Discovered to the Provisioning state? What state will we move them to from there, given that there is no power control? What you can actually do at that point is consistent with the Unmanaged state, but if you do add credentials then we'll want to to move to Provisioned rather than Ready after registering.

We talked today about possibly adding an UnmanagedProvisioned state for hosts that we have provisioned using fast-track but that we can't do power control of.

zaneb · 2020-07-23T14:37:30Z

+### Work Items
+
+1. Create the new git repository for the controller and API and set up
+   CI jobs.


Any reason not to just put in the the baremetal-operator repo? Given that it also interacts with Ironic, I think it's pretty closely related.

zaneb · 2020-07-23T14:39:24Z

+   adjusted, as needed.
+5. Update the state machine to add `Importing` state, and make the
+   related changes to the `IronicProvisioner` API to allow it to look
+   for inspection data without triggering the inspection process.


This can be dropped now; the discovery controller will add the HardwareDetails at creation time.

dhellmann · 2020-07-23T17:02:59Z

This definitely needs a lot of rework, given other work that has happened since it was originally written.

dtantsur

I'd be curious to see this revived.

dtantsur · 2020-09-02T09:10:21Z

+advance. Especially in large data centers, it may be desirable to
+build a small cluster and then attach additional hosts to the
+provisioning network and deploy them by simply powering them on and
+using PXE to boot an agent that knows how to reach back to the


You don't have to use PXE, it could be an ISO image with IPA just as well.

dtantsur · 2020-09-02T09:11:19Z

+details registered with `ironic-inspector` and have a node created in
+the Ironic database. Typically this happens via the PXE boot
+configuration, but in the future it could happen in other ways such as
+having a bootable ISO image with the agent on it. How the agent ends


It can right now, we just need to prepare an ISO with the right parameters (ironic URL, inspector URL, inspection parameters).

dtantsur · 2020-09-02T09:11:54Z

+  replaced by `-`.
+- `serial-number` -- Using the hardware serial number for the host.
+- `boot-mac` -- Using the boot MAC address of the host with `:`
+  replaced by `-`.


Just as a note: the boot MAC can only be detected during PXE (not ISO boot).

dtantsur · 2020-09-02T09:13:11Z

+and provisioner ID that should be added to the status fields.
+
+When a user adds BMC credentials to a host in the `Discovered` state
+today it moves to `Registering` and the `Provisioner` is responsible


Ironic can now be configured to provision a node without having credentials, see my demo https://youtu.be/A9Q8qrEhyh0. I don't suggest we do it by default, but it can be a handy option.

dtantsur · 2020-09-02T09:15:00Z

+1. Create the new git repository for the controller and API and set up
+   CI jobs.
+2. Write the new controller
+3. Update ironic-inspector-image to add unknown hosts using the `fake`


Should be manual-management and we'll need to make a few updates to ironic-image to enable this mode (add agent power interface, add noop management interface, enable fast track and netboot fallback - see my demo linked above).

dtantsur · 2020-09-02T09:15:37Z

+Instead of polling the Ironic API looking for new hosts, we could
+watch the `dnsmasq` logs. However, in some configurations PXE may be
+disabled or hosts may be booted in other ways even with PXE enabled,
+so we would not see all discovered hosts.


Strictly speaking DHCP does not imply PXE, but somebody could be using their own DHCP + ISO boot.

dtantsur · 2020-09-02T09:16:21Z

+to invoke a callback when a new host is discovered. This would be less
+compute intensive, but offers an opportunity to "miss" the
+notification of a new host, and does not provide an opportunity to
+configure the names for the new host resources.


Not sure I get the part about configuring names, but we could probably do both polling (infrequent) and notifications.

dtantsur · 2020-09-02T09:17:04Z

+- [Ironic node discovery
+  docs](https://specs.openstack.org/openstack/ironic-inspector-specs/specs/ironic-node-auto-discovery.html)
+- [WIP changes for
+  baremetal-operator](https://github.com/metal3-io/baremetal-operator/pull/545)


Docs on credential-less deployment: https://docs.openstack.org/ironic/latest/admin/agent-power.html

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 3, 2020

This was referenced Jun 3, 2020

Integrate Ironic discovery workflow metal3-io/baremetal-operator#41

Closed

WIP: implement host discovery metal3-io/baremetal-operator#545

Closed

maelk reviewed Jun 4, 2020

View reviewed changes

Comment thread design/host-discovery.md Outdated

dhellmann force-pushed the host-discovery branch from 5575049 to 65cf71a Compare June 4, 2020 18:18

dtantsur reviewed Jun 8, 2020

View reviewed changes

Comment thread design/host-discovery.md

Comment thread design/host-discovery.md

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

ohadlevy reviewed Jun 10, 2020

View reviewed changes

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md

Comment thread design/host-discovery.md

Comment thread design/host-discovery.md Outdated

dhellmann force-pushed the host-discovery branch from 65cf71a to 3ba1cf4 Compare June 10, 2020 16:56

zaneb reviewed Jun 24, 2020

View reviewed changes

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

Comment thread design/host-discovery.md Outdated

dhellmann force-pushed the host-discovery branch 2 times, most recently from 73d7284 to fb40b26 Compare June 26, 2020 21:20

host discovery design

e57ae1b

Addresses metal3-io/baremetal-operator#41 Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the host-discovery branch from fb40b26 to e57ae1b Compare June 26, 2020 21:24

dhellmann requested review from hardys, maelk and zaneb June 29, 2020 18:13

metal3-io-bot requested review from honza and kirankt June 29, 2020 18:14

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2020

dhellmann mentioned this pull request Jun 30, 2020

Implement an Unmanaged (discovered) state metal3-io/baremetal-operator#563

Closed

zaneb reviewed Jul 23, 2020

View reviewed changes

dhellmann changed the title ~~host discovery design~~ WIP: host discovery design Jul 23, 2020

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 Jul 23, 2020

dtantsur reviewed Sep 2, 2020

View reviewed changes

hardys mentioned this pull request Nov 16, 2020

Disable node discovery by default metal3-io/ironic-inspector-image#67

Merged

dhellmann closed this Nov 19, 2020

Conversation

dhellmann commented Jun 3, 2020

Uh oh!

metal3-io-bot commented Jun 3, 2020

Uh oh!

maelk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ohadlevy commented Jun 4, 2020 via email

Uh oh!

dhellmann commented Jun 4, 2020

Uh oh!

maelk commented Jun 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhellmann commented Jun 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhellmann commented Jun 29, 2020

Uh oh!

dhellmann commented Jun 29, 2020

Uh oh!

dhellmann commented Jun 30, 2020

Uh oh!

kirankt commented Jun 30, 2020

Uh oh!

maelk commented Jul 1, 2020

Uh oh!

dhellmann commented Jul 2, 2020

Uh oh!

maelk commented Jul 3, 2020

Uh oh!

zaneb left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann commented Jul 23, 2020

Uh oh!

dtantsur left a comment

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!