-
Notifications
You must be signed in to change notification settings - Fork 39k
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 DynamicKubeletConfig feature gate from the code #112643
remove DynamicKubeletConfig feature gate from the code #112643
Conversation
node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), | ||
compareNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false), | ||
}, | ||
{ |
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.
Do we want to retain some tests that ensure fields are dropped, etc? I think this could be useful since the API logic after removing the feature gate seems to have some backwards compatibility (for 1.25 nodes and under).
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 last node that can do dynamic kubelet config is 1.24 which will be out of support based on our skew policy. This was actually my question - how much we want to clean up/validate these fields or we want to simply remove any logic related to these fields?
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 don't think adding the logic to validate that these fields (e.g. pkg/apis/core/validation/validation.go) are not set is needed as it may break some automation or something. So it doesn't hurt to have some values in these fields. But should we keep the validation for them - this is a question
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.
Same for clearing these fields. For now I just removed the feature gate, but do we need to keep that logic of clearing the fields at all or any value is fine.
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'd lean towards clearing the fields in all cases to avoid ambiguity, especially if there's no expectation that a 1.26 kube-apiserver interacts with 1.24 nodes/clients. If there's a possibility that clients expect the fields to remain set for existing nodes, or something like that, then we may need to be more careful... but overall I prefer clearing the fields because if the fields remain set it looks like dynamic config is enabled when it's actually not.
@liggitt 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.
looks like we dropped this feature in 1.24 kubelets, so 1.23 kubelets are the last nodes that support it
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.
agree we want to keep a test that ensures new nodes don't get this field set and updates of existing nodes don't newly set it
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.
Sorry about versions confusions, yes, I meant removed in 1.24.
Is there any guidance on validating these fields:
kubernetes/pkg/apis/core/validation/validation.go
Line 5262 in b208f5c
func validateNodeConfigSourceSpec(source *core.NodeConfigSource, fldPath *field.Path) field.ErrorList { |
Should we:
- require them being not set (this may break some customers who still attempts to set these fields)
- remove any validation
- keep everything as is
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 would leave the current clear-on-create / clear-on-update-if-unset code in place, document the field is deprecated and may no longer be set in new nodes, leave the unit test in place to make sure the clearing code works, and leave it at that
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.
leave the validation in place if the field is non-empty
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
8f9743d
to
a06710d
Compare
/remove-sig api-machinery |
/retest |
/priority important-soon |
83b05c4
to
39e49a9
Compare
/retest |
A suggestion for the release note: The `DynamicKubeletConfig` feature gate has been removed from the API server. Dynamic kubelet reconfiguration now cannot be used even when older nodes are still attempting to rely on it. This is aligned with the Kubernetes version skew policy. |
Hi, Thanks for raising the exception request. The exception request has been approved by 1.26 release team and your updated deadline to make any changes to your KEP is 18:00 PST Tuesday 11 October 2022. |
/approve |
/assign @liggitt |
API doc change lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, liggitt, SergeyKanzhelev 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 |
/lgtm |
/retest |
1 similar comment
/retest |
What type of PR is this?
/kind cleanup
/sig node
KEP: kubernetes/enhancements#281
/cc @mtaufen
What this PR does / why we need it:
Mechanical removal of the feature gate from code. This will block from using this feature even when older nodes are still using it. This is aligned with the version skew policy.
Special notes for your reviewer:
There are still some logic in code related to dynamic kubelet configuration. For example, validation of the related fields and setting the node status.
Does this PR introduce a user-facing change?