Bug 1796996: baremetal: map hardware profile to baremetal-operator default #2969

stbenjam · 2020-01-23T23:57:10Z

We rely on vendored baremetal-operator code to handle various
information about baremetal servers including BMC credentials and
hardware profiles. The BMO uses the string 'unknown' as
hardware.DefaultProfileName.

As these string values are exposed to the user in the baremetal platform
as part of the install-config, it felt awkward if a user wanted to
explicitly use the default hardware profile to have to write the string
'unknown'. In golang, people are generally just writing the constant,
'hardware.DefaultProfileName`. So, in the terraform variables, we mapped
the string value 'default' to mean that, but it wasn't done for creating
the machines for workers.

Now that you can deploy workers during installation as a day-1
operation, if you specify the string 'default' for workers, they get
stuck in the 'match profile' state of the BMO. Keep in mind, these
baremetalhosts are all defined in the 'hosts' section of the install
config, but the profiles are now working differently depending on
whether it's part of the control plane (i.e. created by terraform) or
not.

This PR ensures the behavior is the same in both instances, so you can
use the string 'default' for workers and masters.

fixes #2705

We rely on vendored baremetal-operator code to handle various information about baremetal servers including BMC credentials and hardware profiles. The BMO uses the string 'unknown' as `hardware.DefaultProfileName`. As these string values are exposed to the user in the baremetal platform as part of the install-config, it felt awkward if a user wanted to explicitly use the default hardware profile to have to write the string 'unknown'. In golang, people are generally just writing the constant, 'hardware.DefaultProfileName`. So, in the terraform variables, we mapped the string value 'default' to mean that, but it wasn't done for creating the machines for workers. Now that you can deploy workers during installation as a day-1 operation, if you specify the string 'default' for workers, they stuck stuck in the 'match profile' state of the BMO. Keep in mind, these baremetalhosts are all defined in the 'hosts' section of the install config, but the profiles are now working differently depending on whether it's part of the control plane (i.e. created by terraform) or not. This PR ensures the behavior is the same in both instances, so you can use the string 'default' for workers and masters. fixes openshift#2705

stbenjam · 2020-01-23T23:58:33Z

/assign @hardys
/cc @dhellmann

Hopefully the explanation is clear what's going on here. Maybe in retrospect this wasn't a good idea, but I think it has to be consistent. The masters mapping is done in

installer/pkg/tfvars/baremetal/baremetal.go

Lines 45 to 47 in 96d416e

    
           if host.HardwareProfile == "default" { 
        
           	host.HardwareProfile = hardware.DefaultProfileName 
        
           }

stbenjam · 2020-01-24T01:43:31Z

/label platform/baremetal

metal3ci · 2020-01-24T03:12:52Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1448/

hardys · 2020-01-24T09:15:05Z

/cc karmab

hardys · 2020-01-24T09:15:20Z

/retest

hardys · 2020-01-24T09:15:51Z

/approve

hardys · 2020-01-24T09:17:01Z

Looks good, I re-kicked the metal3ci as the previous run timed out before getting to deploy any workers

stbenjam · 2020-01-24T10:01:37Z

Failure might also be because of the wrong interface name for metal3 openshift-metal3/dev-scripts#897 (comment)

metal3ci · 2020-01-24T10:47:26Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1449/

dhellmann · 2020-01-24T14:04:50Z

Another solution to this would be to fix the metal3 code.

The current "default" value is based on the fact that the matching code to pick a profile based on inspection data hasn't been written. There's a completely different approach for that planned now, so we could drop the placeholder code that sets the value to "unknown" and add some better names for the profiles we do have so it's clear when any of them should be used explicitly.

stbenjam · 2020-01-24T14:07:08Z

Another solution to this would be to fix the metal3 code.

The current "default" value is based on the fact that the matching code to pick a profile based on inspection data hasn't been written. There's a completely different approach for that planned now, so we could drop the placeholder code that sets the value to "unknown" and add some better names for the profiles we do have so it's clear when any of them should be used explicitly.

That's fine longer term, but I think we need something now. This is simple and can land today before dev freeze, and cna be backported to 4.3.

openshift-ci-robot · 2020-01-31T17:28:42Z

@stbenjam: This pull request references Bugzilla bug 1796996, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1796996: baremetal: map hardware profile to baremetal-operator default

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 · 2020-01-31T17:28:58Z

/cc @dhellmann

Would you mind /lgtm if you're ok with this change?

Thanks

dhellmann

/lgtm

openshift-ci-robot · 2020-01-31T18:29:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, hardys

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:

~~pkg/asset/machines/baremetal/OWNERS~~ [hardys]

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

openshift-bot · 2020-01-31T18:47:24Z

/retest