-
Notifications
You must be signed in to change notification settings - Fork 15
Enable platform dependent bootstrap ignition files #102
Conversation
Only lightly tested so far, but lets ensure it passes CI, feedback welcome on the approach - I discussed it with @staebler and it sounds like it may be a reasonable approach to enable platform specific additions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach taken elsewhere is to place files that should be used on all platform in a _base
folder. For example, bootkube.service would move to data/data/bootstrap/_base/systemd/units/bootkube.service
.
Also, there is no baremetal
platform type in the installer. Bare metal installations use the none
platform. But note that not all none
-platform installations are bare-metal installations.
@staebler There is a baremetal platform in our fork (it's why it exists), it will be contributed back to openshift/installer soon. This fork implements baremetal IPI. |
@hardys I think the approach is good. The only concern I have is that looking at the implementation of addStorageFiles, I don't think it would be possible for a platform to override a base configuration file. If for example, we wanted a custom |
Ah, I didn't even notice that this wasn't https://github.com/openshift/installer. |
Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/731/ |
Thanks for the feedback - for this fork I wanted to avoid causing us more rebase pain, so decided not to move the files, but it sounds like I should create another PR to openshift/installer which aligns with that convention, then we can rebase and drop this patch when it hopefully lands :) As mentioned this fork implements a new baremetal platform, and this is part of our cleanup to enable proposing the additions to enable baremetal IPI without impact to other platforms. |
Some of the additions made should only be enabled for the baremetal platform, so move these to data/data/bootstrap/baremetal and adjust the code to conditionally read this depending on the platform name. Closes: openshift-metal3#62 Related: openshift-metal3#64
eec38a3
to
6aba139
Compare
This is a good point, I need to do some testing but AFAICS recent ignition versions do validation so probably duplicate files will break, so we'll need to handle it in the installer. I'll look into that, but I think the other issues raised by @staebler are now fixed. |
Build ABORTED, see build http://10.8.144.11:8080/job/dev-tools/737/ |
Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/738/ |
Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/739/ |
@stbenjam I took a look at how we can handle colliding files/units, and I think we'll need to add some logic to track the append index for each filename, then insert instead of append when we find a colliding name. Since that's logically separate from this change, and we don't yet have any colliding files, perhaps that can be a followup if/when needed? |
Sure, sounds good to me. I also agree with deferring the move to |
@celebdor, @yboaron @cybertron can you check you're happy with this as it touches the keepalived and coredns integration, also please can you confirm if this will close both #62 and #64 ? If you're happy then I think we're probably good to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this does wrap up the installer parts of those changes. The behavior here mirrors what happens in MCO for platform-specific files, so lgtm.
"systemd-journal-gatewayd.socket": {}, | ||
"approve-csr.service": {}, | ||
// baremetal platform services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term our plan is to move away from using systemd services on the bootstrap node too, but in the meantime this looks good.
…etal These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
These were missed in #102 and were added as part of the baremetal integration.
These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
These were missed in #102 and were added as part of the baremetal integration.
These were missed in openshift-metal3#102 and were added as part of the baremetal integration.
Some of the additions made should only be enabled for the
baremetal platform, so move these to data/data/bootstrap/baremetal
and adjust the code to conditionally read this depending on the
platform name.
Closes: #62
Related: #64