Extend BaremetalHost CRD to support BIOS configuration in baremetal server

[longkb]

This commit aims to add BIOSConfig as new property into
BareMetalHostSpec. The BaremetalHost CRD is also extend with bios
field. This option allow Metal3 user to set BIOS settings (#207) via bios field
in YAML file.

Co-Authored-By: Dao Cong Tien tiendc@vn.fujitsu.com
Co-Authored-By: Nguyen Phuong An annp@vn.fujitsu.com
Signed-off-byKim Bao Long longkb@vn.fujitsu.com

[nordixinfra]

Can one of the admins verify this patch?

[dhellmann]

This looks like a nicely minimal set of new options. I have a few questions about things like names that I would like for us to work out because we're setting precedents, but the overall approach looks OK.

[dhellmann]

I think we want the enum values to be all lower case.

"LegacyBios" feels redundant. Is there another type of legacy boot interface? Should we just call it "bios"?

[longkb]

I think we want the enum values to be all lower case.

Done. I will start the enum with lower case.

Should we just call it "bios"?

Hmm, I think bios may make the user confuse. legacy is better, I think.

[dhellmann]

Do all processor types support this?

I'm thinking about how many different axes we have for deciding if a BIOS setting is relevant to a given host, and whether we need some way to separate the ones that aren't applicable in some cases. Do we want to put them in their own struct (with a name like DellIntelBIOS or DellR640BIOS or something) or if we want to just keep the option list very short, abstract things as you've done here, and have the implementation ignore anything that isn't relevant for its platform. I could see either approach working.

[longkb]

If we want to put some optional BIOS setting that depend on a given host (given hardware driver), I think we can declare extraArgs in CRD with interface type as I have done in previous PR:

extraArgs interface{} `json:"optionalSettings,omitempty"`

The YAML file may look like:

  bios:
    bootMode: uefi
    extraArgs:
      setting1: "value1"
      setting2: "value2"

But in this case, the validator for these settings must be defined in indicated hardware driver (iRMC, iLO, ...), not in metal3's API.

How to you think about it?

[dhellmann]

If we want to put some optional BIOS setting that depend on a given host (given hardware driver), I think we can declare extraArgs in CRD with interface type as I have done in previous PR:

extraArgs interface{} `json:"optionalSettings,omitempty"`

The YAML file may look like:

  bios:
    bootMode: uefi
    extraArgs:
      setting1: "value1"
      setting2: "value2"

But in this case, the validator for these settings must be defined in indicated hardware driver (iRMC, iLO, ...), not in metal3's API.

How to you think about it?

No, we can't do that. It's bad API design to have informal fields like that, but beyond the design issue I know that in the near future kubernetes is going to require more formal validation for CRDs to ensure that the structure of the data passed in matches what is expected and does not include any "extra" fields. This requirement was presented as addressing a security issue that comes up when someone stores extra fields in a resource today and then in the future that resource definition changes to include the fields, because the existing data would never have been validated.

So, we must find a way to do this by declaring everything explicitly.

[dhellmann]

We should think about the naming for boolean options like this one and the previous one so we can establish a good naming convention.

FooEnabled can be read as either "foo is enabled" or "foo should be enabled".

EnableFoo or UseFoo are more imperative forms. I'm not sure about using those in a declarative interface.

For the Online flag, I didn't include a verb at all, because I thought the verb was implicit in a declarative interface ("make the online setting match what I give you in this field"). Does that approach make sense, too? Should we call this flag "Virtualization" and the previous one "Hyperthreading"?

I'm curious about the opinions of other reviewers.

[longkb]

FooEnabled can be read as either "foo is enabled" or "foo should be enabled".

EnableFoo or UseFoo are more imperative forms. I'm not sure about using those in a declarative interface.

+1 I think EnableFoo is better in our case. I will change to EnableFoo :)

[dhellmann]

Can we use an enum type internally, too? I'm not sure how well special string types mix with kubebuilder.

[longkb]

Ok. I will define the enum type in go lang :)

[dhellmann]

This field is now handled separately and should be removed here.

[russellb]

Just FYI since Doug had reviewed this - he's off on vacation for a couple of weeks. Let me know if this can wait until he's back, or if you'd really like this looked at sooner and I'll pick it up.

[longkb]

Just FYI since Doug had reviewed this - he's off on vacation for a couple of weeks. Let me know if this can wait until he's back, or if you'd really like this looked at sooner and I'll pick it up.

Thanks @russellb
This PR is OK for waiting. But I have the PR #289 that needs to be looked at soon. Could you help me to review it? :)
Thank you very much!

[longkb]

I would like to update BIOS property from BIOSConfig to *BIOSConfig in order to make it be nil in the case of bios is not set in BaremetalHost YAML file

[stbenjam]

I think this makes sense

[longkb]

Thank you :) 👍

[stbenjam]

One question but I think this looks good to me otherwise

[stbenjam]

Could this be named UEFI instead (all caps), as it's an acronym?

[longkb]

Thanks @stbenjam . I will update this name to UEFI :)

[dhellmann]

I found an interesting pattern in Hive today, where they also have platform-specific settings. They use a struct with pointers to platform-specific structs as a sort of "union" type. https://github.com/openshift/hive/blob/master/pkg/apis/hive/v1alpha1/clusterdeployment_types.go#L309

[longkb]

I found an interesting pattern in Hive today, where they also have platform-specific settings. They use a struct with pointers to platform-specific structs as a sort of "union" type. https://github.com/openshift/hive/blob/master/pkg/apis/hive/v1alpha1/clusterdeployment_types.go#L309

That's great! Thanks @dhellmann
If we apply this pattern. Our bios configuration should look like:

  bios:
    irmc:
      bootMode: "uefi"
      enableHyperThreading: true

Am I right? :)

[longkb]

Thanks @stbenjam . I will update this name to UEFI :)

[longkb]

Thank you :) 👍

[hardys]

Having platform specific schema for this interface seems reasonable, but how can we handle differences in vendor support for specific settings on different hardware?

E.g as above something like:

  bios:
    irmc:
      bootMode: "uefi"
      enableHyperThreading: true

Is fine to expose features that may only be applicable when using irmc vs redfish or whatever, but how can we handle features which are accessible via e.g redfish but only when hardware exists - is there a standard subset we can reasonably expect?

I guess there will be some common features like bootmode, hyperthreading, virtualization support etch which probably are common, but my concern is when there are more "advanced" settings which depend on the installed hardware we may find it difficult to define a static schema that can support it?

[dhellmann]

I guess there will be some common features like bootmode, hyperthreading, virtualization support etch which probably are common, but my concern is when there are more "advanced" settings which depend on the installed hardware we may find it difficult to define a static schema that can support it?

Shouldn't the user receive an error if they try to change a setting that cannot be applied because the host doesn't have the relevant hardware? I would expect that to trigger a provisioning failure.

The platform-specific options can still be optional, even if they're well-defined.

[dtantsur]

Why mixing boot mode with BIOS configuration? Ironic has a different API for dealing with it (through capabilities), let's make it top level.

[dhellmann]

Why mixing boot mode with BIOS configuration? Ironic has a different API for dealing with it (through capabilities), let's make it top level.

How does the user of the hardware change the setting directly? It's a BIOS setting, isn't it?

[dhellmann]

I would expect to see something like:

 bios:
    bootMode: "uefi"
    irmc:
      enableHyperThreading: true

[longkb]

Why mixing boot mode with BIOS configuration? Ironic has a different API for dealing with it (through capabilities), let's make it top level.

Yes, you're right. But some vendors also support configure boot mode via BIOS setting.

[longkb]

I would expect to see something like:

 bios:
    bootMode: "uefi"
    irmc:
      enableHyperThreading: true

Thanks. bios will include some common settings and vendor's settings as your suggestion :)

[zaneb]

I don't know what other folks think, but I think it would make more sense to just have a boolean legacyMode.
On a non-x86 architecture it wouldn't make a lot of sense to pass "legacy" and it would definitely make no sense to pass "uefi". It seems less awkward to just have a boolean that is ignored on architectures that don't need it.

[stbenjam]

I agree this should be a top-level item and separate. I don't think it should be a boolean, what if we need to support something else later? It can be omitted in either case.

[dhellmann]

Yeah, moving bootMode out of this struct makes sense to me.

[longkb]

Having platform specific schema for this interface seems reasonable, but how can we handle differences in vendor support for specific settings on different hardware?

E.g as above something like:

  bios:
    irmc:
      bootMode: "uefi"
      enableHyperThreading: true

Is fine to expose features that may only be applicable when using irmc vs redfish or whatever, but how can we handle features which are accessible via e.g redfish but only when hardware exists - is there a standard subset we can reasonably expect?

I guess there will be some common features like bootmode, hyperthreading, virtualization support etch which probably are common, but my concern is when there are more "advanced" settings which depend on the installed hardware we may find it difficult to define a static schema that can support it?

If we get Driver from BMC access [1], we can know which hardware type are using now, and which advanced BIOS setting is supported by the installed hardware.

[1] https://github.com/metal3-io/baremetal-operator/blob/master/pkg/bmc/access.go#L38

[longkb]

Why mixing boot mode with BIOS configuration? Ironic has a different API for dealing with it (through capabilities), let's make it top level.

Yes, you're right. But some vendors also support configure boot mode via BIOS setting.

[longkb]

I would expect to see something like:

 bios:
    bootMode: "uefi"
    irmc:
      enableHyperThreading: true

Thanks. bios will include some common settings and vendor's settings as your suggestion :)

[longkb]

I have update bios with some advanced settings from iRMC drivers :)

[stbenjam]

The variable name should be UEFI as well

[stbenjam]

What do people think about calling this something else like BootConfig? UEFI is not BIOS.

[dhellmann]

I don't know where the line is between BIOS and UEFI. Are the other hardware-related settings that we can configure BIOS even if the boot mode is UEFI? Are these just "hardware settings" or are they all related to how the hardware boots?

[zaneb]

BIOS became something of a generic term at some point during the 25 year period when it was more or less the only game in town for booting in the PC architecture. Technically BIOS and UEFI are different firmware standards for PCs, and we not only want to support both but also firmware for completely different architectures (e.g. OpenFirmware). Practically, I don't think anybody would be overly confused by BIOSConfig.
I would describe these as hardware settings that have to be configured prior to boot, i.e. in the firmware. BootConfig is not an unreasonable name, and seems preferable.

[stbenjam]

Based on the discussion in the metal3-docs proposal, this should be renamed "Firmware" -- metal3-io/metal3-docs#63

[dhellmann]

The code looks good. I honestly have reservations about trying to provide generic names for firmware settings. The proposed here interface will be practically impossible to implement for a generic driver like Redfish.

@dtantsur Sorry for the late reply. Yes, I think that the current code can't support setting BIOS through redfish. If we need to add support for redfish, we may need to add a field bmcType(or something similar) to let users indicate the actual BMC, but this seems to go against the original intention of using redfish.

Can't we use the Redfish API to ask the BMC what it is?

[zouy414]

The code looks good. I honestly have reservations about trying to provide generic names for firmware settings. The proposed here interface will be practically impossible to implement for a generic driver like Redfish.

@dtantsur Sorry for the late reply. Yes, I think that the current code can't support setting BIOS through redfish. If we need to add support for redfish, we may need to add a field bmcType(or something similar) to let users indicate the actual BMC, but this seems to go against the original intention of using redfish.

Can't we use the Redfish API to ask the BMC what it is?

@dhellmann For iRMC I can get its BMC type through the following command:

# curl -s -k -u <username>:<password> -H "Content-type: application/json" -X GET https://<ip-address>/redfish/v1/Managers
{
  "@odata.id":"\/redfish\/v1\/Managers",
  "@odata.type":"#ManagerCollection.ManagerCollection",
  "Name":"Manager Collection",
  "Members@odata.count":1,
  "Members":[
    {
      "@odata.id":"\/redfish\/v1\/Managers\/iRMC"
    }
  ],
  "Oem":{
  },
  "@Redfish.Copyright":"Copyright 2014-2020 DMTF. For the full DMTF copyright policy, see http:\/\/www.dmtf.org\/about\/policies\/copyright",
  "@odata.etag":"1618479521"
}

But I'm not sure if this is a standard method(@dtantsur Do you know a better way? ). In addition, I think we also need to be able to get this information through ironic.

By the way, could you please review this patch? As what demonCoder95 has said above, I also believe that this patch is ready to be merged.

[zouy414]

I have rebase this PR, but it seems that I need to re-execute test-integration. Could you give a /test-integration comment. @demonCoder95 @fmuyassarov @zaneb

[zaneb]

/test-integration

[zouy414]

@andfasano @dhellmann @dtantsur @zaneb This PR has been launched for more than a year and has been recognized by some people,and we have another job that depends on this PR. So I want to know what else needs to be done to help this PR merge?
As for how to support redfish BIOS settings based on this PR, I can open another PR for discussion.

[zaneb]

The code looks good. I honestly have reservations about trying to provide generic names for firmware settings. The proposed here interface will be practically impossible to implement for a generic driver like Redfish.

Thanks for pointing this out, I hadn't really thought about that earlier.

I do think we have some implementation options for this in future. For example, we can now get a list of all the available BIOS options from Ironic, which we're going to use to implement Bob's proposal metal3-io/metal3-docs#173. So we could make a list of all the names that e.g. VirtualizationEnabled is known by on hardware platforms that we know about and then compare it to the list of available BIOS settings for the Host to see which one shows up.

The existence of at least one plausible way of implementing this API makes me inclined to go ahead with this PR, so I am not removing my approval.

[andfasano]

The code looks good. I honestly have reservations about trying to provide generic names for firmware settings. The proposed here interface will be practically impossible to implement for a generic driver like Redfish.

Thanks for pointing this out, I hadn't really thought about that earlier.

I do think we have some implementation options for this in future. For example, we can now get a list of all the available BIOS options from Ironic, which we're going to use to implement Bob's proposal metal3-io/metal3-docs#173. So we could make a list of all the names that e.g. VirtualizationEnabled is known by on hardware platforms that we know about and then compare it to the list of available BIOS settings for the Host to see which one shows up.

The existence of at least one plausible way of implementing this API makes me inclined to go ahead with this PR, so I am not removing my approval.

But Bob's proposal is mainly about defining a new CR for managing firmware settings (and they could be many), whereas in the current one the public API of BMH is directly modified to address just three of them. I also have some concerns similar to the ones raised by @dtantsur but mostly focusing on the fact that new fields for BMH are going to be defined.
Even though in general the overall code of the PR looks ok, this particular design decision does not seem to scale well in future (when maybe other settings will be required), as the fields are directly hardcoded (and somehow not fully inline with the general idea of decomposing BMH).

I think that a more general approach could allow folks to focus on the settings they are currently interested in but will not affect too much the scalability.

[zaneb]

They have different purposes. Bulk set is good for onboarding a bunch of servers from a particular manufacturer. But if you want to e.g. create a Kubernetes node on baremetal with hyperthreading disabled froma pool of hardware, then there should be a vendor-independent way of requesting this in the API.

[andfasano]

Ok, so it looks like the current approach and the one in #901 could coexists, addressing some of my concerns. AFAICS the current BMH interface will remain vendor-neutral and will be expanded when required/needed.

FWIW, this PR had a pretty long history, and currently it supports the configuration of the VirtualizationEnabled, SimultaneousMultithreadingEnabled and SriovEnabled settings just for ilo4/ilo5/irmc, but I didn't see any mention of that in the documentation. So I'd suggest to fix the documentation as required in a separate PR.

/lgtm

[zouy414]

Thanks. I will open a new PR to fix the documentation.
/retest

[metal3-io-bot]

@Hellcatlk: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

Thanks. I will open a new PR to fix the documentation.
/retest

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.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

[zouy414]

/test

[metal3-io-bot]

@Hellcatlk: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test

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.

[zouy414]

@andfasano I'm sorry, I updated the PR because CI failed. Could you /lgtm again?

[zouy414]

/test-integration

[andfasano]

/lgtm

[demonCoder95]

/test-integration

[zaneb]

@Hellcatlk if you set your membership of the org to public then you won't have to have someone add ok-to-test to your PRs any more. The bot unfortunately can't see you if your membership is set to private.

[zouy414]

@Hellcatlk if you set your membership of the org to public then you won't have to have someone add ok-to-test to your PRs any more. The bot unfortunately can't see you if your membership is set to private.

Thank you for your reminder, I have set the membership to public now.

[zouy414]

FWIW, this PR had a pretty long history, and currently it supports the configuration of the VirtualizationEnabled, SimultaneousMultithreadingEnabled and SriovEnabled settings just for ilo4/ilo5/irmc, but I didn't see any mention of that in the documentation. So I'd suggest to fix the documentation as required in a separate PR.

@andfasano I found this part in the design:

The proposal only deals with the generic BIOS configurations (listed above), and nothing vendor specific will be developed/tested, although, formatting for those will be part of this specification.

I think this part is already expressed our support only VirtualizationEnabled SimultaneousMultithreadingEnabled and SriovEnabled.

[andfasano]

@Hellcatlk I was mainly referring to the BMO docs https://github.com/metal3-io/baremetal-operator/blob/master/docs/api.md (or if there's any other relevant part where a user could easily find the supported platforms)