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
Remove option to disable OSM #13381
Remove option to disable OSM #13381
Conversation
Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>
/kind api-change |
@ahmedwaleedmalik: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>
920ecb7
to
dc8d6d8
Compare
/label needs-release-testing |
@ahmedwaleedmalik: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/label needs-release-testing |
/retest |
1 similar comment
/retest |
Signed-off-by: Waleed Malik <ahmedwaleedmalik@gmail.com>
/assign @xrstf |
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 had a similar deprecation/removal in #13316, so here's a question I had to ask myself: Is KKP going to be capable of differentiating between "this *bool
field is unset because the cluster is old and still using MC user-data" and "this *bool
is unset because the cluster is new and uses OSM by default"? Is there any migration code required that KKP will fail to run when it's confused about which type of cluster it is processing?
Maybe it makes sense to talk about this in SIG Cluster Management so we have a consistent way forward to deprecating fields in this release. As it stands, my PR will default the deprecated field to true, while this PR will go the opposite direction, and I think it would be nice if we adjusted behaviour.
Yeah, it should. As explained in the "reviewer notes," we'll have pre-flight checks in the installer to block users who have not enabled OSM. Migration, not sure, if we'll build it in the installer or just leave it up to the user. |
Gotcha, I misunderstood the reviewer notes for some reason saying we will only reject if the field is set to |
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.
/approve
LGTM label has been added. Git tree hash: eb6eef185c1293efeb32bb56a0c01b8f698e83a4
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
The user-data plugins from the machine-controller have been removed. OSM is mandatory now to create a functional cluster since we can't fall back on user data generated by the machine-controller anymore.
Which issue(s) this PR fixes:
Fixes #
What type of PR is this?
/kind cleanup
Special notes for your reviewer:
A follow-up PR for this will add guardrails in the installer to prevent upgrades if OSM has been explicitly disabled for a cluster. Users will have to update their clusters before the KKP upgrade. We can maybe add a flag to automate this as well but it's TBD.
Does this PR introduce a user-facing change? Then add your Release Note here:
Documentation: