root device hints #495

dhellmann · 2020-04-29T20:25:31Z

This supercedes #442

Add a new field spec.rootDeviceHints to the API with sub-fields for different types of hints based on what Ironic supports today.

Add a new field status.rootDeviceHints to record the values used when provisioning the host.

Change the Provisioner API to return the new public struct with the more detailed device hints so that the values in the hard-coded profiles can be stored directly in the new status field.

If the user does not provide explicit hints, use the values from the profile.

Implements https://github.com/metal3-io/metal3-docs/blob/master/design/user-defined-root-device-hints.md

metal3-io-bot · 2020-04-29T20:25:35Z

[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

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

mikkosest · 2020-05-04T15:53:01Z

Looks good, I guess unit test updates in the same PR?

dhellmann · 2020-05-04T16:25:01Z

Looks good, I guess unit test updates in the same PR?

Yes, we'll want that.

Given some of the other work I did last week on #434 I think we're going to want to add these new features to the next API version (v1alpha2 or v1beta1) after we have the CI fixes like #504 in place.

maelk · 2020-05-05T07:14:37Z

This should be backward compatible, no ? we should be able to add it without waiting for the next version. I am not sure I get the reason for postponing it.

dhellmann · 2020-05-05T13:17:29Z

This should be backward compatible, no ? we should be able to add it without waiting for the next version. I am not sure I get the reason for postponing it.

The validation for updating the CRD is broken. I think we should prioritize fixing that over extending the API further so that when we do work on the API changes we can trust that we're making all of the necessary changes. That doesn't necessarily assume waiting for the next API version, but since I plan to do that right away as part of fixing the current issue (see #434 for the todo list) I don't think the wait will be very long.

maelk · 2020-05-06T06:09:11Z

ok, that's good! thank you

derekhiggins · 2020-05-11T11:41:54Z

I posted this in the other PR but not sure if anybody saw it

I've pushed up a related PR for masters to the installer repository
openshift/installer#3348

It suggests a simpler roothint pass through approach, would it be worth considering and allowing ironic to do the validation?

I'm worried this functionality will need to be duplicated(in the installer and in ironic itself) for provisioning master nodes is that the case? and if so could we just passthrough of the json to ironic?

dhellmann · 2020-05-11T15:18:15Z

I posted this in the other PR but not sure if anybody saw it

I've pushed up a related PR for masters to the installer repository
openshift/installer#3348
It suggests a simpler roothint pass through approach, would it be worth considering and allowing ironic to do the validation?

I'm worried this functionality will need to be duplicated(in the installer and in ironic itself) for provisioning master nodes is that the case? and if so could we just passthrough of the json to ironic?

Yes, the OpenShift installer is going to need to do its own thing. We should talk about that over on that PR, since it's not really relevant to metal3 upstream.

dhellmann · 2020-05-29T15:59:51Z

/test-integration

pkg/provisioner/ironic/ironic.go

pkg/controller/baremetalhost/baremetalhost_controller.go

pkg/apis/metal3/v1alpha1/baremetalhost_types.go

pkg/provisioner/ironic/ironic.go

zaneb

Looks basically good

pkg/controller/baremetalhost/baremetalhost_controller.go

Add a new field spec.rootDeviceHints to the API with sub-fields for different types of hints based on what Ironic supports today. Add a new field status.rootDeviceHints to record the values used when provisioning the host. Change the Provisioner API to return the new public struct with the more detailed device hints so that the values in the hard-coded profiles can be stored directly in the new status field. If the user does not provide explicit hints, use the values from the profile. Implements https://github.com/metal3-io/metal3-docs/blob/master/design/user-defined-root-device-hints.md Co-Authored-By: Mika Koskimaki <mika.koskimaki@est.tech> Co-Authored-By: Doug Hellmann <dhellmann@redhat.com> Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

maelk · 2020-06-01T06:54:57Z

/test-integration

maelk · 2020-06-01T06:55:11Z

that looks good to me

dhellmann · 2020-06-01T11:50:45Z

I need to update the API docs.

/hold

dhellmann · 2020-06-01T12:12:51Z

/hold cancel

dhellmann · 2020-06-01T12:13:02Z

/test-integration

Add a missing reference to the hardwareProfile in the spec section so that we can include a deprecation warning. Add missing fields in the provisioning status section so that we can extend it with the root device hints field. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-06-01T12:21:25Z

/test-integration

dtantsur · 2020-06-01T17:04:23Z

deploy/crds/metal3.io_baremetalhosts_crd.yaml

+                  type: string
+                sizeGigabytes:
+                  description: Size of the device in Gigabytes
+                  type: integer


In ironic this may include an operator, like ">= 4". I think it makes sense to support it here as well, wdyt?

What is the default behavior if only a number is provided?

Maybe we should have minSizeGigabytes and maxSizeGigabytes.
I agree it's a bit hard on the user forcing them to guess the exact integral number of gigabytes that the disk size will be rounded to.

Strict equality (yeah, not exactly convenient).

Having 2 fields makes sense. That's more in line with what we've done in other similar APIs where we don't want to embed expressions in fields.

It's not clear that we can support both min and max at the same time. I have renamed the field to MinSizeGigabytes and updated the behavior to always include >= in the value sent to ironic. If we can have an expression with both >= and <= then we can add a MaxSizeGigabytes field as well.

This also raises the question of what operators we should be assuming for the other fields. And whether we need to allow variations.

The latest version sets an explicit operator for each value we pass to ironic and does not allow the operators to be modified.

zaneb

I agree with Dmitry's comment, but other than that this looks ready to go.

zaneb · 2020-06-01T22:01:09Z

deploy/crds/metal3.io_baremetalhosts_crd.yaml

+                  type: string
+                sizeGigabytes:
+                  description: Size of the device in Gigabytes
+                  type: integer


Maybe we should have minSizeGigabytes and maxSizeGigabytes.
I agree it's a bit hard on the user forcing them to guess the exact integral number of gigabytes that the disk size will be rounded to.

The default for ironic is to treat the value as an exact match, but we want to use it as a minimum value. Rename the field and update the code that produces the map to send to the ironic API to always add the '>=' operator. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-06-02T14:32:51Z

/test-integration

Instead of relying on ironic's default behavior, pass explicit operators with each hint. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-06-02T16:24:07Z

/test-integration

zaneb

/lgtm

zaneb · 2020-06-02T17:18:58Z

pkg/apis/metal3/v1alpha1/baremetalhost_types.go

+	SerialNumber string `json:"serialNumber,omitempty"`
+
+	// The minimum size of the device in Gigabytes.
+	MinSizeGigabytes int `json:"minSizeGigabytes,omitempty"`


I'm not sure that omitempty does anything unless this is a pointer.
Because it's MinSize it doesn't matter that the default is 0 though, so maybe we don't care.

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 Apr 29, 2020

metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 29, 2020

dhellmann force-pushed the root-device-hints branch from 9a3c4df to ede242b Compare April 29, 2020 20:26

dhellmann requested a review from mikkosest April 29, 2020 20:26

dhellmann force-pushed the root-device-hints branch from ede242b to 5612e35 Compare May 29, 2020 15:56

dhellmann changed the title ~~WIP: root device hints~~ root device hints May 29, 2020

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 May 29, 2020

dhellmann requested review from zaneb, stbenjam, dtantsur and juliakreger May 29, 2020 15:57

dhellmann requested a review from yrobla May 29, 2020 16:06

zaneb reviewed May 29, 2020

View reviewed changes

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved

pkg/controller/baremetalhost/baremetalhost_controller.go Outdated Show resolved Hide resolved

pkg/apis/metal3/v1alpha1/baremetalhost_types.go Outdated Show resolved Hide resolved

dhellmann force-pushed the root-device-hints branch from 5612e35 to 74c613d Compare May 29, 2020 18:42

stbenjam reviewed May 29, 2020

View reviewed changes

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved

dhellmann force-pushed the root-device-hints branch 2 times, most recently from 530c040 to df37555 Compare May 29, 2020 18:57

zaneb reviewed May 29, 2020

View reviewed changes

pkg/controller/baremetalhost/baremetalhost_controller.go Outdated Show resolved Hide resolved

pkg/controller/baremetalhost/baremetalhost_controller.go Outdated Show resolved Hide resolved

pkg/controller/baremetalhost/baremetalhost_controller.go Outdated Show resolved Hide resolved

dhellmann force-pushed the root-device-hints branch from df37555 to 86ffcc8 Compare May 29, 2020 22:21

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

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

dhellmann force-pushed the root-device-hints branch from c74e342 to 592d826 Compare June 1, 2020 12:20

dtantsur reviewed Jun 1, 2020

View reviewed changes

zaneb reviewed Jun 2, 2020

View reviewed changes

use explicit operators when passing device hints to ironic

c2513a6

Instead of relying on ironic's default behavior, pass explicit operators with each hint. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the root-device-hints branch from f3ae7c4 to c2513a6 Compare June 2, 2020 15:34

zaneb reviewed Jun 2, 2020

View reviewed changes

metal3-io-bot assigned zaneb Jun 2, 2020

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2020

metal3-io-bot merged commit 007211a into metal3-io:master Jun 2, 2020

This was referenced Jun 2, 2020

Extend BaremetalHost CRD to support BIOS configuration in baremetal server #302

Merged

Support RAID configuration for baremetal server #292

Merged

maelk mentioned this pull request Jun 3, 2020

🐛 Update BMO dependency for root device hints metal3-io/cluster-api-provider-metal3#87

Merged

dhellmann mentioned this pull request Sep 3, 2020

Support complete set of root device hints #400

Closed

zaneb mentioned this pull request Aug 19, 2022

Remove spec matcher string operation in deviceHint #1160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

root device hints #495

root device hints #495

dhellmann commented Apr 29, 2020 •

edited

Loading

metal3-io-bot commented Apr 29, 2020

mikkosest commented May 4, 2020

dhellmann commented May 4, 2020

maelk commented May 5, 2020

dhellmann commented May 5, 2020

maelk commented May 6, 2020

derekhiggins commented May 11, 2020

dhellmann commented May 11, 2020

dhellmann commented May 29, 2020

zaneb left a comment

maelk commented Jun 1, 2020

maelk commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dtantsur Jun 1, 2020

dhellmann Jun 1, 2020

zaneb Jun 1, 2020

dtantsur Jun 2, 2020

dhellmann Jun 2, 2020

dhellmann Jun 2, 2020

dhellmann Jun 2, 2020

dhellmann Jun 2, 2020

zaneb left a comment

zaneb Jun 1, 2020

dhellmann commented Jun 2, 2020

dhellmann commented Jun 2, 2020

zaneb left a comment

zaneb Jun 2, 2020

root device hints #495

root device hints #495

Conversation

dhellmann commented Apr 29, 2020 • edited Loading

metal3-io-bot commented Apr 29, 2020

mikkosest commented May 4, 2020

dhellmann commented May 4, 2020

maelk commented May 5, 2020

dhellmann commented May 5, 2020

maelk commented May 6, 2020

derekhiggins commented May 11, 2020

dhellmann commented May 11, 2020

dhellmann commented May 29, 2020

zaneb left a comment

Choose a reason for hiding this comment

maelk commented Jun 1, 2020

maelk commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dhellmann commented Jun 1, 2020

dhellmann commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann commented Jun 2, 2020

dhellmann commented Jun 2, 2020

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann commented Apr 29, 2020 •

edited

Loading