Add proposal to do bulk set of BIOS configuration by bfournie · Pull Request #173 · metal3-io/metal3-docs

bfournie · 2021-03-30T13:36:34Z

Draft of a proposal to do bulk set of BIOS Configuration across
similar vendors.

metal3-io-bot · 2021-03-30T13:36:48Z

Hi @bfournie. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

dhellmann · 2021-03-31T14:11:22Z

/ok-to-test

bfournie · 2021-04-23T12:48:35Z

Updated to reflect changes made to Ironic BIOS Registry API (see https://review.opendev.org/c/openstack/ironic-specs/+/774681). Instead of a separate endpoint a subset of the particular registry information is now included with each BIOS Setting in the v1/nodes/{node_ident}/bios?detail=True API. This reduces complexity and size of the BIOS data. Also added text regarding validations of the BIOS settings using the registry before adding to clean steps.

In addition made changes regarding sequencing of clean-steps from the recent discussion regarding the granular approach to BIOS config.

Add a new CRD for the FirmwareSettings for BIOS configuration as described in the spec - metal3-io/metal3-docs#173. A new CRD is necessary since its not practical to include the number of settings in BareMetalHost.

furkatgofurov7 · 2021-06-02T13:43:21Z

Hi!
As agreed during the community meeting, let's have a lazy consensus of a week from today and if there are no further objections/comments on the proposal, we can go ahead with merging it next week.

zaneb

I'm very happy with the simplicity of this approach. I did start a little bikeshedding ;)
/approve

metal3-io-bot · 2021-07-02T15:42:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, 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~~ [andfasano,zaneb]

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

Add a new CRD for the FirmwareSettings for BIOS configuration as described in the spec - metal3-io/metal3-docs#173. A new CRD is necessary since its not practical to include the number of settings in BareMetalHost.

dhellmann · 2021-07-02T21:06:20Z

/uncc dhellmann

Add a new CRD for the FirmwareSettings for BIOS configuration as described in the spec - metal3-io/metal3-docs#173. A new CRD is necessary since its not practical to include the number of settings in BareMetalHost.

andfasano · 2021-07-06T08:57:24Z

+whenever the node moves to `manageable` or `cleaning`, or when the settings
+are updated. The baremetal-operator will manage the data as follows:
+
+- The node first transitions to manageable during the `Registration` state, so


typo: Registering

andfasano · 2021-07-06T09:45:18Z

+example, integer types will checked against minimum and maximum parameters and
+enumeration types will be compared against allowable values. The validation
+webhook can be leveraged here to log a message regarding the failure; the
+invalid BIOS Setting will not be added to clean-steps.


Wouldn't be better to follow the current approach? If a validation check fails then buildManualCleaningSteps() could return a proper error, thus triggering the usual retry mechanism and in this case the host status ErrorType will end up in PreparationError. This will also give the opportunity to an (external) user to fix the invalid setting before moving on.
Otherwise we'd risk to update only a portion of the vendorSettings (the desired end state), and this could lead potentially to undesired effects (so essentially the strategy suggested here is all or none)

I think this paragraph might be based on a misunderstanding of what a validating webhook does.

The webhook sits in the API pipeline between the input and the data store, so it allow us to reject an invalid update before it is stored in the resource. Ideally we should do this so that the user knows immediately if they submit an invalid change.

Without a webhook we end up with invalid config stored in the object, and then the controller has to decide what to do with it given that it's too late to reject it. Since we'd have to put the Host in an error state anyway, and retry after the user has fixed it, I agree there'd probably not be much point in doing a partial application.

Updated to state that validity checks will be done when building the manual clean steps and will return a failure for any failures, preventing partial configurations.

Also indicated that a validation webhook will be done as a follow-on, to catch validation errors before storing the data in a resource.

Add a new CRD for the FirmwareSettings for BIOS configuration as described in the spec - metal3-io/metal3-docs#173. A new CRD is necessary since its not practical to include the number of settings in BareMetalHost.

Proposal to get and set BIOS configuration and do a bulk set across similar vendors.

andfasano · 2021-07-15T16:16:46Z

Thanks @bfournie for the proposal, the current state looks fine to me.
/lgtm
/hold In case @zaneb would want to do a final pass

Add a new CRD for the FirmwareSettings for BIOS configuration as described in the spec - metal3-io/metal3-docs#173. A new CRD is necessary since its not practical to include the number of settings in BareMetalHost.

zaneb · 2021-08-19T15:13:26Z

/lgtm

andfasano · 2021-08-19T16:23:42Z

/hold cancel

Add a new CRD for the FirmwareSettings for BIOS configuration as described in the spec - metal3-io/metal3-docs#173. A new CRD is necessary since its not practical to include the number of settings in BareMetalHost.

metal3-io-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 30, 2021

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 31, 2021

dhellmann mentioned this pull request Mar 31, 2021

Extend BaremetalHost CRD to support BIOS configuration in baremetal server metal3-io/baremetal-operator#302

Merged

dhellmann reviewed Mar 31, 2021

View reviewed changes

bfournie force-pushed the bulk-set-bios branch 2 times, most recently from 56dac2c to 27358d0 Compare March 31, 2021 15:39

dhellmann reviewed Mar 31, 2021

View reviewed changes

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

zaneb reviewed Mar 31, 2021

View reviewed changes

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

bfournie force-pushed the bulk-set-bios branch from 27358d0 to 4fbb55e Compare April 5, 2021 17:15

bfournie force-pushed the bulk-set-bios branch from 4fbb55e to 3b5b995 Compare April 23, 2021 12:41

zaneb reviewed Apr 27, 2021

View reviewed changes

bfournie force-pushed the bulk-set-bios branch 5 times, most recently from 99db3fb to d77c8bb Compare May 6, 2021 13:09

bfournie force-pushed the bulk-set-bios branch from d77c8bb to 8fca72d Compare May 30, 2021 14:30

bfournie mentioned this pull request May 31, 2021

Add new CRDs for BIOS configuration metal3-io/baremetal-operator#901

Merged

zaneb reviewed Jun 3, 2021

View reviewed changes

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2021

zaneb reviewed Jun 3, 2021

View reviewed changes

Comment thread design/baremetal-operator/bulk-set-bios-config.md

bfournie force-pushed the bulk-set-bios branch 2 times, most recently from b29d7c6 to d3b9914 Compare July 4, 2021 20:56

andfasano reviewed Jul 9, 2021

View reviewed changes

bfournie force-pushed the bulk-set-bios branch from d3b9914 to f181e14 Compare July 9, 2021 18:25

lshilin-r reviewed Jul 12, 2021

View reviewed changes

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

andfasano reviewed Jul 12, 2021

View reviewed changes

Comment thread design/baremetal-operator/bulk-set-bios-config.md Outdated

bfournie mentioned this pull request Jul 13, 2021

Create and manage hostFirmwareSettings and firmwareSchema resources metal3-io/baremetal-operator#938

Merged

Proposal to do bulk set of BIOS configuration

2401652

Proposal to get and set BIOS configuration and do a bulk set across similar vendors.

bfournie force-pushed the bulk-set-bios branch from f181e14 to 2401652 Compare July 15, 2021 14:14

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 Jul 15, 2021

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

metal3-io-bot merged commit c0a6f38 into metal3-io:master Aug 19, 2021

bfournie deleted the bulk-set-bios branch August 19, 2021 19:05

Conversation

bfournie commented Mar 30, 2021

Uh oh!

metal3-io-bot commented Mar 30, 2021

Uh oh!

dhellmann commented Mar 31, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfournie commented Apr 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

furkatgofurov7 commented Jun 2, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metal3-io-bot commented Jul 2, 2021

Uh oh!

dhellmann commented Jul 2, 2021

Uh oh!

andfasano Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

zaneb Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

bfournie Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andfasano commented Jul 15, 2021

Uh oh!

zaneb commented Aug 19, 2021

Uh oh!

andfasano commented Aug 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants