Skip to content
This repository was archived by the owner on Jul 23, 2019. It is now read-only.

Additional small clean-ups from openshift/installer review #159

Merged
markmc merged 2 commits into from
Jul 16, 2019
Merged

Additional small clean-ups from openshift/installer review #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85510d9
pkg/asset: remove FIXME from baremetal code
stbenjam Jul 15, 2019
e3b3168
pkg/asset/machines: order baremetal calls alphabetically
stbenjam Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions pkg/asset/machines/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ func (m *Master) Generate(dependencies asset.Parents) error {
}
azure.ConfigMasters(machines, clusterID.InfraID)
case baremetaltypes.Name:
// FIXME: baremetal
Copy link
Copy Markdown
Member Author

@stbenjam stbenjam Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@markmc markmc Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

@markmc markmc Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

mpool := defaultBareMetalMachinePoolPlatform()
mpool.Set(ic.Platform.BareMetal.DefaultMachinePlatform)
mpool.Set(pool.Platform.BareMetal)
Expand Down Expand Up @@ -275,17 +274,17 @@ func (m *Master) Machines() ([]machineapi.Machine, error) {
scheme := runtime.NewScheme()
awsapi.AddToScheme(scheme)
azureapi.AddToScheme(scheme)
baremetalapi.AddToScheme(scheme)
gcpapi.AddToScheme(scheme)
libvirtapi.AddToScheme(scheme)
openstackapi.AddToScheme(scheme)
baremetalapi.AddToScheme(scheme)
decoder := serializer.NewCodecFactory(scheme).UniversalDecoder(
awsprovider.SchemeGroupVersion,
azureprovider.SchemeGroupVersion,
baremetalprovider.SchemeGroupVersion,
gcpprovider.SchemeGroupVersion,
libvirtprovider.SchemeGroupVersion,
openstackprovider.SchemeGroupVersion,
baremetalprovider.SchemeGroupVersion,
)

machines := []machineapi.Machine{}
Expand Down
1 change: 0 additions & 1 deletion pkg/asset/machines/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
machineSets = append(machineSets, set)
}
case baremetaltypes.Name:
// FIXME: baremetal
mpool := defaultBareMetalMachinePoolPlatform()
mpool.Set(ic.Platform.BareMetal.DefaultMachinePlatform)
mpool.Set(pool.Platform.BareMetal)
Expand Down
1 change: 0 additions & 1 deletion pkg/asset/rhcos/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ func (i *Image) Generate(p asset.Parents) error {
//TODO(serbrech): change to right image once available.
osimage = "/resourceGroups/rhcos_images/providers/Microsoft.Compute/images/rhcostestimage"
case baremetal.Name:
// FIXME: baremetal
osimage, err = rhcos.QEMU(ctx)
Copy link
Copy Markdown
Member Author

@stbenjam stbenjam Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown

@hardys hardys Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@markmc markmc Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ... ugh ...

Some of the underlying assumptions of the rhcos.Image abstraction:

  1. The same image is used for bootstrap, masters, and workers
  2. 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

Copy link
Copy Markdown

@hardys hardys Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case none.Name, vsphere.Name:
default:
Expand Down