-
Notifications
You must be signed in to change notification settings - Fork 401
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
[OCPCLOUD-1209] Add feature gate to bootstrap mode manifests #2647
[OCPCLOUD-1209] Add feature gate to bootstrap mode manifests #2647
Conversation
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 good, thanks Danil, couple of suggestions, have you spun up a cluster with this yet?
i gave this pr a test drive in conjunction with using installer patch openshift/installer#4947 , i created the manifests, added the external feature gate and created the cluster. the cluster is mostly healthy, but the mco is still having trouble with the rendered configurations. i see this output from the installer:
when i examine the master machineconfigpool, i see this in the
|
I have tested this on its own as well and can see that it resolved the issue of the kubelet flags differing, but we are still seeing differences in the kubelet config (jsons available if you would like to see). I think if we try this in conjunction with #2547 we should hopefully see this issue is resolved completely. IMO, this PR is doing the job it was intended to do as per my conversations with @yuqi-zhang on slack /lgtm |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Danil-Grigorev, JoelSpeed The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7247053
to
706d1d2
Compare
New changes are detected. LGTM label has been removed. |
@Danil-Grigorev Can you update this PR to add an exception for the lint rule that's blocking this from passing the verify stage? Qi is doing it for the same place here, just need to copy this line over https://github.com/openshift/machine-config-operator/pull/2547/files#diff-ee4889f36bd8b3bb13f51dd15721cbc3f0eae1cf4d6bb8635dec6de843e78c4dR50 |
706d1d2
to
afce61e
Compare
@Danil-Grigorev: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@Danil-Grigorev: PR needs rebase. 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. |
/close This was included in #2668 |
@JoelSpeed: Closed this PR. 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. |
- What I did
Added feature gate into list of supported manifests to render during install time. Additionally makes sure openshift only feature gates are excluded from Kubelet config list too, so it won't be confusing when the feature gate is present in Kubelet config, but Kubelet is not running it.
- How to verify it
- Description for the changelog