Additional small clean-ups from openshift/installer review#159
Additional small clean-ups from openshift/installer review#159markmc merged 2 commits intoopenshift-metal3:masterfrom
Conversation
|
Don't merge until latest rebase is in, to avoid creating conflicts. |
| osimage = "/resourceGroups/rhcos_images/providers/Microsoft.Compute/images/rhcostestimage" | ||
| case baremetal.Name: | ||
| // FIXME: baremetal | ||
| osimage, err = rhcos.QEMU(ctx) |
There was a problem hiding this comment.
This doesn't need fixing, from @russellb
using the same image as libvirt is correct for the bootstrap VM
we do have: https://github.com/openshift-metal3/kni-installer/issues/37
which covers "use the right image for masters and workers"
There was a problem hiding this comment.
Yeah also see #100 where we need to consume two different images which fixes that issue, so I bypassed this abstraction - I think removing this FIXME is reasonable.
There was a problem hiding this comment.
This is ... ugh ...
Some of the underlying assumptions of the rhcos.Image abstraction:
- The same image is used for bootstrap, masters, and workers
- That image information is included in the generated machines manifests and also to terraform
I think we should follow that basic idea and just have an "oh hey, our bootstrap node boot image is different!" special case
There was a problem hiding this comment.
Yeah agree we need a better abstraction here which caters for the situation where the bootstrap and master images are different, I hacked around it in #100 but would welcome feedback on possible approaches there.
| } | ||
| azure.ConfigMasters(machines, clusterID.InfraID) | ||
| case baremetaltypes.Name: | ||
| // FIXME: baremetal |
There was a problem hiding this comment.
This was introduced in a rebase, before we created machines for baremetal. We create them now, so this FIXME (and the one below,I think) aren't needed.
There was a problem hiding this comment.
The thing that bothers me now with this code is that we're not using rhcosImage - i.e. nowhere in our machines resource are we describing the bootimage we used, unlike other platforms (except libvirt)
There was a problem hiding this comment.
I've filed #160 now
Even though the baremetal code path doesn't use the rhcosImage variable right now, it bothers me that the variable contains the wrong data for masters ...
As requested in the openshift/installer review, the FIXME's in shared code should be removed and tracked elsewhere.
This sets the order correctly in any code where baremetal is in the wrong place, i.e. it should always occure after azure but before libvirt.
|
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/898/ |
|
Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/900/ |
| } | ||
| azure.ConfigMasters(machines, clusterID.InfraID) | ||
| case baremetaltypes.Name: | ||
| // FIXME: baremetal |
There was a problem hiding this comment.
The thing that bothers me now with this code is that we're not using rhcosImage - i.e. nowhere in our machines resource are we describing the bootimage we used, unlike other platforms (except libvirt)
| } | ||
| azure.ConfigMasters(machines, clusterID.InfraID) | ||
| case baremetaltypes.Name: | ||
| // FIXME: baremetal |
There was a problem hiding this comment.
I've filed #160 now
Even though the baremetal code path doesn't use the rhcosImage variable right now, it bothers me that the variable contains the wrong data for masters ...
| osimage = "/resourceGroups/rhcos_images/providers/Microsoft.Compute/images/rhcostestimage" | ||
| case baremetal.Name: | ||
| // FIXME: baremetal | ||
| osimage, err = rhcos.QEMU(ctx) |
There was a problem hiding this comment.
This is ... ugh ...
Some of the underlying assumptions of the rhcos.Image abstraction:
- The same image is used for bootstrap, masters, and workers
- That image information is included in the generated machines manifests and also to terraform
I think we should follow that basic idea and just have an "oh hey, our bootstrap node boot image is different!" special case
4 participants
See commits for details, but this removes FIXME's and fixes some ordering issues.