Add new CRDs for BIOS configuration by bfournie · Pull Request #901 · metal3-io/baremetal-operator

bfournie · 2021-05-31T12:46:22Z

Add a new CRD for the HostFirmwareSettings 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.
In addition a CRD is added for the FirmwareSchema used to provide a schema for setting the firmware values.

metal3-io-bot · 2021-05-31T12:46:32Z

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.

stbenjam · 2021-06-01T13:29:52Z

/ok-to-test

zaneb

Note that you might need the patch from #892 to properly generate all of the files.

elfosardo · 2021-06-18T09:44:41Z

/test-integration

elfosardo · 2021-06-18T09:45:38Z

/lgtm

metal3-io-bot · 2021-06-18T09:45:39Z

@elfosardo: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

/lgtm

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.

zaneb · 2021-07-01T20:28:23Z

+
+	// The count of the number of times this schema is used by
+	// a HostFirmwareSettings resource
+	ReferenceCount int `json:"referenceCount" required:"true"`


If there's a reason to have a Status it would probably be just for this. You can't write the Status atomically with creating (or updating) the resource though, so I suspect this would also do better in the Spec.
TBH I'm not entirely convinced a reference count is the best way to manage cleanup, but I'm withholding judgement on that.

Sure can move this to Spec.

I've been reading up on Garbage Collection in k8s, and I think we should be able to use the owner references to manage this automatically (saves a lot of work implementing this in the controller). You can have multiple owner references, which I didn't know before. (Only one of these can be marked as a 'controller', which is what we do for Secrets so that's what confused me. However, we don't need to do that here.)

So it's safe to ditch this field.

zaneb · 2021-07-01T20:58:08Z

+	FirmwareSchema *SchemaReference `json:"schema,omitempty"`
+
+	// Settings are the actual firmware settings stored as name/value pairs
+	// +patchStrategy=merge


This is more important on the Spec than the Status.

As per the doc, the Settings in the Status would contain all the settings read from Ironic and, upon reading, the BMO would copy these settings to the Spec but only include those that do not have the "ReadOnly" or "Unique" set in the FirmwareSchema for this setting. The Values in the Spec could then be easily updated by the user without having to change/write the Names. The BMO can compare the spec values to the expected values in the Status and only send the differences to clean steps. So with that design we'd need Settings in both places.

I meant the +patchStrategy=merge :)

bfournie · 2021-07-03T12:11:26Z

/test-integration

bfournie · 2021-07-07T17:53:14Z

/test-integration

elfosardo · 2021-07-08T11:02:45Z

/test-integration

bfournie · 2021-07-08T15:24:56Z

/test-integration

zaneb

This looks basically good. Couple of very minor things inline.

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.

bfournie · 2021-07-09T15:50:54Z

/test-integration

zaneb

/lgtm

dtantsur

/approve

Looks good to me, but please consider fixing my comments in a follow-up.

dtantsur · 2021-07-13T12:49:22Z

+type SettingSchema struct {
+
+	// The type of setting.
+	// +kubebuilder:validation:Enum=Enumeration;String;Integer;Boolean;Password


Do we expect to support non-Redfish implementation? If yes, we cannot add this validation.

Yes, for non-redfish types there will be no BIOS registry data returned and therefore no schema, so no validations can be done on the settings. I will note this in the CRD.

Well, other drivers may decide to implement schema as well.

dtantsur · 2021-07-13T12:50:35Z

+		return false
+	}
+
+	if schema.ReadOnly != nil && *schema.ReadOnly == true {


I wonder if we need any logging in this function. Otherwise debugging can be challenging.

Or can we maybe return an error instead and log it somewhere at the caller place?

Good point about the debugging, I will add more granular debugging to help determine error in settings.

Changed CheckSettingIsValid() in #938 to return an error which is now logged.

metal3-io-bot · 2021-07-13T12:53:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur, elfosardo

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

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

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 31, 2021

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 31, 2021

bfournie force-pushed the firmwaresettings branch from e1c67be to 3f1e24d Compare May 31, 2021 17:57

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 Jun 1, 2021

bfournie force-pushed the firmwaresettings branch from 3f1e24d to 4d738ef Compare June 1, 2021 13:49

zaneb reviewed Jun 3, 2021

View reviewed changes

andfasano mentioned this pull request Jun 4, 2021

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

Merged

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2021

bfournie mentioned this pull request Jun 7, 2021

Add proposal to do bulk set of BIOS configuration metal3-io/metal3-docs#173

Merged

bfournie force-pushed the firmwaresettings branch from 4d738ef to 8ea3b5a Compare June 14, 2021 19:51

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2021

bfournie force-pushed the firmwaresettings branch from 8ea3b5a to 5af4aeb Compare June 16, 2021 23:50

metal3-io-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 16, 2021

bfournie changed the title ~~Add new CRD for BIOS configuration~~ Add new CRDs for BIOS configuration Jun 16, 2021

bfournie force-pushed the firmwaresettings branch from 5af4aeb to 38d9da7 Compare June 17, 2021 00:57

elfosardo approved these changes Jun 18, 2021

View reviewed changes

bfournie force-pushed the firmwaresettings branch 4 times, most recently from 9afde93 to a6b9b66 Compare June 28, 2021 12:42

zaneb reviewed Jul 1, 2021

View reviewed changes

bfournie force-pushed the firmwaresettings branch 2 times, most recently from db13966 to 59df014 Compare July 2, 2021 17:23

bfournie force-pushed the firmwaresettings branch from 59df014 to 0b67ebb Compare July 7, 2021 17:41

zaneb reviewed Jul 9, 2021

View reviewed changes

Comment thread apis/metal3.io/v1alpha1/firmwareschema_types.go Outdated

Comment thread apis/metal3.io/v1alpha1/hostfirmwaresettings_types.go Outdated

bfournie force-pushed the firmwaresettings branch from 0b67ebb to 9712791 Compare July 9, 2021 15:33

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2021

bfournie force-pushed the firmwaresettings branch from 9712791 to 9e08275 Compare July 9, 2021 15:42

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2021

Add new CRD for BIOS configuration

a425cf1

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.

bfournie force-pushed the firmwaresettings branch from 9e08275 to a425cf1 Compare July 9, 2021 15:47

zaneb reviewed Jul 10, 2021

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2021

dtantsur reviewed Jul 13, 2021

View reviewed changes

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

metal3-io-bot merged commit f56cd18 into metal3-io:master Jul 13, 2021

bfournie deleted the firmwaresettings branch January 6, 2022 18:48

Conversation

bfournie commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metal3-io-bot commented May 31, 2021

Uh oh!

stbenjam commented Jun 1, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elfosardo commented Jun 18, 2021

Uh oh!

elfosardo commented Jun 18, 2021

Uh oh!

metal3-io-bot commented Jun 18, 2021

Uh oh!

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!

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!

bfournie commented Jul 3, 2021

Uh oh!

bfournie commented Jul 7, 2021

Uh oh!

elfosardo commented Jul 8, 2021

Uh oh!

bfournie commented Jul 8, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bfournie commented Jul 9, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

metal3-io-bot commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bfournie commented May 31, 2021 •

edited

Loading