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
Only perform machineconfig reconciliation during OpenShift upgrades #3473
Only perform machineconfig reconciliation during OpenShift upgrades #3473
Changes from all commits
4453bae
a9b4658
75b3aef
17eca61
63c5b22
64ae5eb
3cf3559
e257811
ab2d8fd
24d4bd2
74c0e0f
946cca1
bf0e699
c4abfed
50b7663
a303211
65a717c
c233a05
c9f1e5c
7dd2a23
a33896b
761c4fb
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.
Is this logic still valid, given that the install process (as changed in this PR) will forcibly allow reconciliation before completing the install?
I'm worried that this will prevent us from being able to roll out completely new MachineConfigs (e.g. as done in #3781) without causing immediate reboots.
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.
Yeah, we need to update it for entirely-new MCs, since that was outside of the scope of the original PR in March.
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.
Ugh, okay, so, one of the edge cases here is if a customer creates a new machineconfigpool. The reason why I allowed creates was so that if a new MCP was created, then the proper MCs would get created for it. Since you can't really know what happened to the object (just that something happened), we can't really reliably special-case created MCPs creating the objects, because that should be fine? I'll try and put in some heuristics, I guess.
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've had this question for a while - why do we care about customers creating new MachineConfigPools at all for either the current dnsmasq or new /etc/hosts MachineConfigs? It should be sufficient to create MachineConfigs that apply to role: master and role: worker, as every node should be a member of one of those two roles, even if they belong to a custom MCP.
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 circle back on updating this for the new etchosts controller, I think we should merge this PR and #3818 seperately and use ARO-9990 as the follow up to get them to work together.
While testing the etchosts controller I found it was sufficient to target the MachineConfigs at the worker role, instead of making a new MachineConfig for each MCP.