Review of install-config structure #57
Comments
Do we need those other interface values at all? The baremetal operator is only setting the driver. |
I'm not sure why they were added - I think they were added by @derekhiggins when we enabled idrac support in openshift-metal3/dev-scripts@bad8da7#diff-589e5ae70d225e720b2445d3447fe986. |
This sounds good to me. How about we make the baremetal platform config for the installer just look like the CRD?
I believe we can use much more structured data now that @yrobla fixed the issues with using nested data structures, so we can probably use whatever structure we want. |
Yes! Big +1 to using the CRD here. |
|
Here's a draft alternative Here are the key changes:
apiVersion: v1beta4
baseDomain: test.metalkube.org
metadata:
name: ostest
compute:
- name: worker
replicas: 1
controlPlane:
name: master
replicas: 3
pullSecret: ...
sshKey: ...
platform:
baremetal:
secrets:
- apiVersion: v1
metadata:
name: openshift-master-0-bmc-secret
data:
username: admin
password: password
- apiVersion: v1
metadata:
name: openshift-master-1-bmc-secret
data:
username: admin
password: password
- apiVersion: v1
metadata:
name: openshift-master-2-bmc-secret
data:
username: admin
password: password
- apiVersion: v1
metadata:
name: openshift-worker-0-bmc-secret
data:
username: admin
password: password
bareMetalHosts:
- apiVersion: metal3.io/v1alpha1
kind: BareMetalHost
metadata:
name: openshift-master-0
labels:
role: master
spec:
bmc:
address: ipmi://192.168.111.1:6230
credentialsName: openshift-master-0-bmc-secret
bootMACAddress: 00:bd:3e:96:4f:3d
hardwareProfile: default
- apiVersion: metal3.io/v1alpha1
kind: BareMetalHost
metadata:
name: openshift-master-1
labels:
role: master
spec:
bmc:
address: ipmi://192.168.111.1:6231
credentialsName: openshift-master-1-bmc-secret
bootMACAddress: 00:bd:3e:96:4f:41
hardwareProfile: default
- apiVersion: metal3.io/v1alpha1
kind: BareMetalHost
metadata:
name: openshift-master-2
labels:
role: master
spec:
bmc:
address: ipmi://192.168.111.1:6232
credentialsName: openshift-master-2-bmc-secret
bootMACAddress: 00:bd:3e:96:4f:45
hardwareProfile: default
- apiVersion: metal3.io/v1alpha1
kind: BareMetalHost
metadata:
name: openshift-worker-0
spec:
bmc:
address: ipmi://192.168.111.1:6233
credentialsName: openshift-worker-0-bmc-secret
bootMACAddress: 00:bd:3e:96:4f:48
hardwareProfile: default |
|
My first impression of this is that it's very verbose and repetitive. Credentials separate from the hosts is kind of awkward to handle. It will require a bunch of code in the installer to parse this structure, although maybe there's some we could re-use. From my perspective, the benefit to using something like the CRD is we would have consistent terminology everywhere. What about something flatter? platform:
baremetal:
hosts:
- name: openshift-master-0
role: master
spec:
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6230
bootMACAddress: 00:bd:3e:96:4f:3d
hardwareProfile: default
- name: openshift-master-1
role: master
spec:
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6231
bootMACAddress: 00:bd:3e:96:4f:41
hardwareProfile: default
- name: openshift-master-2
role: master
spec:
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6230
bootMACAddress: 00:bd:3e:96:4f:45
hardwareProfile: default
- name: openshift-worker-0
spec:
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6233
bootMACAddress: 00:bd:3e:96:4f:48
hardwareProfile: default |
|
If we prefer it to be exact to the CRD specification, could we check with the installer team to get their thoughts? They obviously have the most experience in how the platforms gather their data. Of course by far we have the most information to gather, cloud providers essentially just need a couple of things like credentials/regions. |
|
@stbenjam Thanks for the feedback. Your flattened version is certainly nicer. I think this will come down to deciding on the value of matching the kube objects exactly, or just being similar. I'm thinking we should just start trying to implement it and see if we find using the exact resources makes life easier enough or not. I'm certainly leaning toward your flattened version, though. |
|
For the flattened version, I guess we don't really need platform:
baremetal:
hosts:
- name: openshift-master-0
role: master
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6230
bootMACAddress: 00:bd:3e:96:4f:3d
hardwareProfile: default
- name: openshift-master-1
role: master
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6231
bootMACAddress: 00:bd:3e:96:4f:41
hardwareProfile: default
- name: openshift-master-2
role: master
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6230
bootMACAddress: 00:bd:3e:96:4f:45
hardwareProfile: default
- name: openshift-worker-0
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6233
bootMACAddress: 00:bd:3e:96:4f:48
hardwareProfile: default |
No, we don't need them if the defaults are good (they are). |
|
A couple minor notes on the last draft flattened version:
|
|
I think that makes sense. I played around with today, and I'm able to reuse a lot of things from baremetal-operator, like the BMC parsing and it's hardware profiles. Works well. BUT terraform 0.12 still has weird quirks with nested data structures: you can finally have maps of maps 🎉 , but not maps with maps and strings 😞 like we have here: - name: openshift-master-0
role: master
bmc:
username: admin
password: password
address: ipmi://192.168.111.1:6230It might be possible using HCL2 but haven't looked at it yet. I wonder for the first pass if maybe we just use the current structure -- possibly renaming the word |
|
I don't think I understand the limitation. Would something like this be sufficient? - name: openshift-master-0
role: master
bmc_username: admin
bmc_password: password
bmc_address: ipmi://192.168.111.1:6230We could open a WIP PR without this perhaps, but I really feel like this needs to get done before this is included in a release. |
|
I also don't understand how the install-config.yaml format is limited in any way by what terraform can do. This is input into the installer, not terraform. Is the issue figuring out how we'll pass the data into terraform templates? |
Er, yes, sorry, you're right. I just had it stuck in my head that with 0.12 we could make it look nicer, and have minimal transformation from the platform config to terraform variables. But, we can do more work in the go side of things and turn it into a structure terraform is happy with (essentially the same flatter layout we have today). It's not really pretty but no one ever sees it. I'll get a PR together with those changes. |
|
Have some WIP PR's up, they need the 4.2 rebase first. Dev-scripts openshift-metal3/dev-scripts#578 This is the install config: apiVersion: v1beta4
baseDomain: test.metalkube.org
metadata:
name: ostest
compute:
- name: worker
replicas: 1
controlPlane:
name: master
replicas: 3
platform:
baremetal:
api_vip: 192.168.111.5
hosts:
- name: openshift-master-0
role: master
bmc:
address: ipmi://192.168.111.1:6230
username: admin
password: password
bootMACAddress: 00:ba:98:e9:69:24
hardwareProfile: unknown
deployKernel: http://172.22.0.1/images/ironic-python-agent.kernel
deployRamdisk: http://172.22.0.1/images/ironic-python-agent.initramfs
- name: openshift-master-1
role: master
bmc:
address: ipmi://192.168.111.1:6231
username: admin
password: password
bootMACAddress: 00:ba:98:e9:69:28
hardwareProfile: unknown
deployKernel: http://172.22.0.1/images/ironic-python-agent.kernel
deployRamdisk: http://172.22.0.1/images/ironic-python-agent.initramfs
- name: openshift-master-2
role: master
bmc:
address: ipmi://192.168.111.1:6232
username: admin
password: password
bootMACAddress: 00:ba:98:e9:69:2c
hardwareProfile: unknown
deployKernel: http://172.22.0.1/images/ironic-python-agent.kernel
deployRamdisk: http://172.22.0.1/images/ironic-python-agent.initramfs
image:
source: "http://172.22.0.1/images/rhcos-ootpa-latest.qcow2"
checksum: 2b3b1e19e18627d89da400b63430d5bb
pullSecret: |
<redacted>We still need to deal with getting rid of the image URL's |
|
Also, hardware platform is "unknown" as that's the default in BMO: https://github.com/metal3-io/baremetal-operator/blob/master/pkg/hardware/profile.go#L49 Our VM's seem to be using /dev/sda, not vda, so I couldn't use the libvirt profile. |
|
This looks like a great first step and makes sense as a first PR. I do think we should work out how to drop the URL options too, though. |
|
I guess, is there a reason to just not call it |
I know there was work about some container that would download the images automatically and serve them up, is that something we could run on the bootstrap node? It would be easy for the installer to just use a known path hosted on an http server at $API_VIP. |
I wanted to be able to tell the difference between the cases where it tried to figure out the profile and couldn't and it used the default profile because that matched. When we get the profile matching doing more useful stuff, I expect we'll revisit that UX. |
The installer shouldn't set the image URLs on the host objects at all. For the control plane we want to register them as "externally provisioned" to avoid driving them through the provisioning process again. For workers we want to let the actuator deal with provisioning them based on the number of worker nodes requested. Something does need to set the URL on the MachineSet and MachineDeployment. I know @sadasu is working on that in another PR for the machine-api-operator integration, and we weren't sure where that URL would come from. |
Got it, we won't pass that info through. The URL's I'm looking at here are the ones currently defined in the install-config that we give to terraform to provision the nodes. We'd like the installer to derive them from information it knows already, without the user having to tell us where they are. |
It seems we have a couple of options. Either the images need to be served from the bootstrap VM, or from the provisioning host running the installer. In either case, the installer should be able to download the correct images (both RHCOS and IPA) on its own. |
I'd like to discuss any changes we might want to make to the
baremetalplatform config in theinstall-config.yamlfile before we submit our changes back toopenshift/installer. When looking over the current structure, I had a few thoughts:We use the name
nodes, which matches Ironic terminology, but is confusing for OpenShift, where aNodeis something else. I suggest we usehostsor evenbareMetalHosts, aligning with the Kubernetes objects that this info relates to.We have
master_nodes. Note thatopenshift-installdropped the use ofmastersandworkersterminology. Instead, it now usescomputeandcontrolPlane. We should align with that.There is information about a given host listed in different sections. For example,
openshift-master-0is listed 4 times. Could we restructure this so that all information aboutopenshift-master-0is together under a single heading?Finally, I think for the purposes of this config, Ironic should be considered an implementation detail. I wonder if we should do anything to hide Ironic-isms. For example, I'm looking at
driver,management_interface,power_interface, andvendor_interface. Maybe that could be simplified with a single option likemanagementType: ipmi. We could define additional management types over time that map to a set of options on the backend.For reference, here is a copy of an existing
install-config.yaml:The text was updated successfully, but these errors were encountered: