Create and manage hostFirmwareSettings and firmwareSchema resources by bfournie · Pull Request #938 · metal3-io/baremetal-operator

bfournie · 2021-07-13T19:38:53Z

After inspection the baremetalhost_controller retrieve BIOS settings from Ironic and will create the hostFirmwareSettings and firmwareSchema (if BIOS registry data is received) resources. In Preparing, any Spec settings that have been changed will be used to build clean_steps containing the BIOS settings. The changed values will be verified against the expected limits in the firmwareSchema, any failures will case an error in Preparing. The building of clean steps also uses the 3 BIOS settings that were added to the BMH CRD (#302).

The new hostfirmwarecontroller also does validation on the input values but only logs failures. As a follow-on PR, a validation webhook will be used to validate the input values.

See also metal3-io/metal3-docs#173

bfournie · 2021-07-13T19:55:48Z

/test-integration

bfournie · 2021-07-13T23:24:28Z

/test-integration

bfournie · 2021-07-14T21:13:36Z

/test-integration

bfournie · 2021-07-14T23:05:17Z

/test-integration

bfournie · 2021-07-16T18:50:53Z

/test-integration

bfournie · 2021-07-16T19:09:34Z

/test-integration

dtantsur

I haven't finished the review, but here are some comments.

bfournie · 2021-07-25T22:17:54Z

/test-integration

bfournie · 2021-07-26T20:56:45Z

/test-integration

bfournie · 2021-09-22T13:37:10Z

/test-integration

dtantsur

I'm leaving a bunch of comments here. Most are nits except for two:

A question/suggestion on API fields
A concern about updating Spec automatically.

bfournie · 2021-09-29T00:12:54Z

/test-integration

bfournie · 2021-09-29T16:52:35Z

/test-integration

dtantsur · 2021-10-04T13:19:09Z

/approve

Thanks!

metal3-io-bot · 2021-10-04T13:19:19Z

[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

andfasano · 2021-10-06T17:25:53Z

Thanks for the effort @bfournie, it seems that all the points have been addressed.

/lgtm

zaneb

I don't see any code to move the state back to Preparing (from Ready) if the settings are out of date. We'll also need to watch the HostFirmwareSettings in the BMH controller so we'll see if the Spec changes while we're in the Ready state.

Re copying the Status fields to the Spec, I don't think ironic changing the fields is a problem because we were only going to do a copy if that field wasn't mentioned in the Spec, so it wouldn't overwrite an existing value. However, given that we're using a comparison of Spec and Status to determine when config has drifted, perhaps it's for the best if we only include values the user has specifically said they care about.

zaneb · 2021-10-07T21:24:31Z

+	if err = r.Get(ctx, req.NamespacedName, hfs); err != nil {
+		if k8serrors.IsNotFound(err) {
+			// A resource doesn't exist, create one
+			if err = r.newHostFirmwareSettings(info, req.NamespacedName); err != nil {


So this controller watches the BaremetalHost and automatically creates a HostFirmwareSettings object for it if it doesn't exist?

This is slightly unorthodox and will result in a lot of extra reconciles (every time the BMH changes we'll have to re-run this) but I guess it should be ok. We'll need to make doubly sure there are no unnecessary writes, otherwise we could get an infinite recursion between the two controllers.

zaneb · 2021-10-07T21:47:00Z

+	firmwareSchema = &metal3v1alpha1.FirmwareSchema{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      schemaName,
+			Namespace: info.hfs.ObjectMeta.Namespace,


We talked about adding a CLI option to put these into a fixed namespace (regardless of which namespace the BMH is in) when the operator is running cluster-wide.

Yes that is still in plan as a follow on PR.

zaneb · 2021-10-08T01:01:34Z

+	h.Write([]byte(fmt.Sprintf("%v", keys)))
+	hash := fmt.Sprintf("%x", h.Sum(nil))[:8]
+
+	return "schema-" + hash


IIRC we decided to put the vendor and model in the name as well as the shortened hash. Did that turn out not to be feasible?

That ended up causing problems since the vendor and model come from introspection data and that may not have been retrieved yet when the resource is created. if its retrieved later the hashing will be incorrect.

I thought we wouldn't have the schema data available until after inspection either?
There is something to be said for not mixing data from two different sources though, since they could change independently.

There's not a dependency on inspection to get the schema data as we get the schema directly from the BMC while we get the inspection data from IPA. So yeah I don't we should use the vendor/model in the name.

zaneb · 2021-10-08T01:03:13Z

+func GetSchemaName(schema map[string]metal3v1alpha1.SettingSchema) string {
+
+	// Schemas from the same vendor and model should be identical for both keys and values.
+	// Hash the keys of the map to get a identifier unique to this schema.


Why not include the values as well? Just having the keys sounds like it would risk collisions between different firmware versions.

I'm not sure if we would practically see differences in values but I'll add the values into the hash to prevent that happening.

Change to use the Preparing state end time as a trigger that cleaning has just completed instead of relying on state difference. In addition, the following changes were made to address follow-on comments in metal3-io#938. - added Preparing state to OperationalMetrics so its end time can be used for level-trigger - split Status Settings into LocalData (ReadOnly or Unique fields) and writeable Settings - changed name of condition `UpdateRequested` to `FirmwareSettingsChangeDetected` - add each hostFirmwareSettings as owner of its FirmwareSchema so that garbage collection can be done - Change the SetOwnerReference/SetControllerReference calls to before Create so the Update could be removed - When building BIOS settings for cleaning make sure the hfs settings don't overwrite the vendor-agnostic settings - When generating the hash for the firmwareSchema include the schema values in addition to the names in the hash

metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 13, 2021

bfournie changed the title ~~Create and manage hostFirmwareSettings and firmwareSchema resources~~ [WIP]Create and manage hostFirmwareSettings and firmwareSchema resources Jul 13, 2021

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 13, 2021

bfournie force-pushed the hostfirmwaresettingsandschema branch from 99046c3 to 4cc75d1 Compare July 13, 2021 19:49

bfournie changed the title ~~[WIP]Create and manage hostFirmwareSettings and firmwareSchema resources~~ Create and manage hostFirmwareSettings and firmwareSchema resources Jul 13, 2021

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

bfournie force-pushed the hostfirmwaresettingsandschema branch from 4cc75d1 to f744f9f Compare July 13, 2021 22:11

bfournie force-pushed the hostfirmwaresettingsandschema branch from f744f9f to b2e0b54 Compare July 14, 2021 20:27

bfournie force-pushed the hostfirmwaresettingsandschema branch from b2e0b54 to 8a6033d Compare July 14, 2021 22:59

bfournie force-pushed the hostfirmwaresettingsandschema branch 2 times, most recently from a31b9c2 to aa9509c Compare July 16, 2021 18:47

bfournie force-pushed the hostfirmwaresettingsandschema branch from aa9509c to 0766e06 Compare July 16, 2021 18:58

bfournie mentioned this pull request Jul 16, 2021

Add new CRDs for BIOS configuration #901

Merged

dtantsur reviewed Jul 19, 2021

View reviewed changes

bfournie force-pushed the hostfirmwaresettingsandschema branch 5 times, most recently from cb2c990 to dfdf13f Compare July 25, 2021 21:10

bfournie force-pushed the hostfirmwaresettingsandschema branch from dfdf13f to a206781 Compare July 26, 2021 20:40

dtantsur reviewed Aug 2, 2021

View reviewed changes

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

elfosardo approved these changes Sep 23, 2021

View reviewed changes

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

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

Comment thread pkg/provisioner/ironic/clients/client.go

dtantsur reviewed Sep 23, 2021

View reviewed changes

bfournie force-pushed the hostfirmwaresettingsandschema branch 2 times, most recently from 35f9dfc to 5265479 Compare September 23, 2021 15:32

andfasano reviewed Sep 24, 2021

View reviewed changes

Comment thread controllers/metal3.io/hostfirmwaresettings_controller.go

Comment thread controllers/metal3.io/hostfirmwaresettings_controller.go

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

bfournie force-pushed the hostfirmwaresettingsandschema branch 2 times, most recently from 794bfa7 to b7a96e7 Compare September 28, 2021 11:11

andfasano reviewed Sep 28, 2021

View reviewed changes

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

Comment thread controllers/metal3.io/hostfirmwaresettings_controller.go

bfournie force-pushed the hostfirmwaresettingsandschema branch from b7a96e7 to 3fa388b Compare September 28, 2021 21:57

Merge branch 'master' into hostfirmwaresettingsandschema

23b89fb

bfournie force-pushed the hostfirmwaresettingsandschema branch from 3fa388b to 23b89fb Compare September 29, 2021 16:48

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

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

metal3-io-bot merged commit 3820040 into metal3-io:master Oct 6, 2021

bfournie deleted the hostfirmwaresettingsandschema branch October 7, 2021 12:54

zaneb reviewed Oct 8, 2021

View reviewed changes

bfournie mentioned this pull request Oct 14, 2021

Change hfs controller to be level-triggered when handling bmh updates #995

Merged

Conversation

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

Uh oh!

Uh oh!

bfournie commented Jul 13, 2021

Uh oh!

bfournie commented Jul 13, 2021

Uh oh!

bfournie commented Jul 14, 2021

Uh oh!

bfournie commented Jul 14, 2021

Uh oh!

bfournie commented Jul 16, 2021

Uh oh!

bfournie commented Jul 16, 2021

Uh oh!

dtantsur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfournie commented Jul 25, 2021

Uh oh!

bfournie commented Jul 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfournie commented Sep 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dtantsur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfournie commented Sep 29, 2021

Uh oh!

bfournie commented Sep 29, 2021

Uh oh!

dtantsur commented Oct 4, 2021

Uh oh!

metal3-io-bot commented Oct 4, 2021

Uh oh!

andfasano commented Oct 6, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfournie commented Jul 13, 2021 •

edited

Loading