Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MCO Flattened Ignition proposal #467
Add MCO Flattened Ignition proposal #467
Changes from all commits
54fce76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Another alternative could be to just merge the installer generated ignition config into the rendered config (I guess we'd need to create a MachineConfig object derived from the installer generated user-data secret, then rely on the existing MergeMachineConfigs logic in MCO.
@runcom that would also solve the issue with the management of the pointer ignition, since we'd convert any data from the installer-created ignition to a MachineConfig, thus the pointer config can just be statically tempated as in your recent implementation?
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.
To clarify this idea, something like:
We could potentially flag some warning when this happens, and use it as a migration path to eventually deprecate the direct ignition-configs customization in favor of MachineConfig manifests?
This would also solve the issue for IPI baremetal without any MCO API changes, since we could consume the existing rendered config directly (we did this previously ref openshift/installer#3276 but that was also reverted for the same reason as above)
@cgwalters @crawford any thoughts?
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.
I guess there are some RBAC considerations:
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.
I looked at the code, and it's probably simplest to have the installer generate a MachineConfig manifest that contains any config provided via
create ignition-configs
That avoids any changes to the MCO (other than restoring the reverted pointer-ignition change from @runcom) and potentially allows us to warn the user when config is provided via that interface.
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.
I think I'm generally in favour of the MCO managing the stub configs coming from installer in one way or another, and this approach will probably work. I think we should keep the following points in mind:
openshift-installer create ignition-configs
would have been supplied to the nodes manually. Is this a case we care about?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.
good points, I care about solving point number 2 as it seems we really never wanted to advertise the use of that (last we checked with @crawford )
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.
I think we can avoid this if instead the installer creates a MachineConfig manifest with any user customizations it finds on the
create cluster
phase, and we could either skip generating this or leave it empty in the case where the stub config output via thecreate ignition-configs
phase is unmodified?We can also warn the user if the decision is made to deprecate the modification of the stub config with this approach (which may be harder if we have the MCO process the stub config, in addition to the ordering concern you raise above).
Good point - IIUC this has only ever worked for the UPI case, where you download the stub config from the installer
create ignition-configs
phase, then host some per-host modified configs somewhere outside of the cluster?In the IPI case, there's only a single secret per role, so per-node config is not currently supported via that workflow.
So, I think to ensure both workflows continue to work the same as today, we just need to ensure that the new MachineConfig object either isn't generated or is empty at the
create ignition-configs
stage, and that we can detect and re-generate that asset in the case where some user-customization happened betweencreate ignition-configs
andcreate cluster
(I guess we can compare the assets loaded from file with that generated inside the installer).