Skip to content
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.

Sign up for GitHub

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

Jump to bottom

remove DynamicKubeletConfig feature gate from the code #112643

Merged
merged 1 commit into from Oct 12, 2022
Merged

remove DynamicKubeletConfig feature gate from the code #112643

merged 1 commit into from Oct 12, 2022

Conversation

SergeyKanzhelev
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev commented Sep 21, 2022 •
edited

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?

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.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/code-generation area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 21, 2022
mtaufen
mtaufen reviewed Sep 21, 2022
View reviewed changes
pkg/registry/core/node/strategy_test.go
node: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false),
compareNode: makeNode([]string{"2000::/10", "10.0.0.0/8"}, false, false),
},
{
Copy link
Contributor

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).

Copy link
Member Author

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?

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member

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

@k8s-triage-robot
Copy link

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.

@SergeyKanzhelev SergeyKanzhelev force-pushed the removeDynamicKubeletConfig branch 3 times, most recently from 8f9743d to a06710d Compare September 22, 2022 16:28
@leilajal
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 22, 2022
@SergeyKanzhelev
Copy link
Member Author

/retest

@pacoxu
Copy link
Member

pacoxu commented Sep 23, 2022

/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 23, 2022
@SergeyKanzhelev
Copy link
Member Author

/retest

@haircommander haircommander moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Sep 30, 2022
@enj enj added this to Needs Triage in SIG Auth Old Oct 3, 2022
@pacoxu pacoxu moved this from Waiting on Author to Needs Reviewer in SIG Node PR Triage Oct 9, 2022
@pacoxu pacoxu moved this from Needs Reviewer to Waiting on Author in SIG Node PR Triage Oct 9, 2022
@pacoxu pacoxu mentioned this pull request Oct 9, 2022
Closed
27 tasks
@sftim
Copy link
Contributor

sftim commented Oct 9, 2022

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.

@SergeyKanzhelev SergeyKanzhelev mentioned this pull request Oct 10, 2022
Merged
@leonardpahlke
Copy link
Member

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.
/milestone v1.26

@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 10, 2022
@dims
Copy link
Member

dims commented Oct 11, 2022

/approve

@dims
Copy link
Member

dims commented Oct 11, 2022

/assign @liggitt

@liggitt
Copy link
Member

liggitt commented Oct 11, 2022

API doc change lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2022
@pacoxu
Copy link
Member

pacoxu commented Oct 12, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2022
@pacoxu
Copy link
Member

pacoxu commented Oct 12, 2022

/retest

1 similar comment
@SergeyKanzhelev
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit 525280d into kubernetes:master Oct 12, 2022
SIG Node CI/Test Board automation moved this from PRs - Needs Reviewer to Done Oct 12, 2022
SIG Node PR Triage automation moved this from Waiting on Author to Done Oct 12, 2022
SIG Auth Old automation moved this from Needs Triage to Closed / Done Oct 12, 2022
@ialidzhikov ialidzhikov mentioned this pull request Dec 16, 2022
Closed
20 tasks
@SergeyKanzhelev SergeyKanzhelev deleted the removeDynamicKubeletConfig branch January 23, 2023 17:45
@SataQiu SataQiu mentioned this pull request Mar 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtaufen mtaufen mtaufen left review comments

@liggitt liggitt liggitt left review comments

@haircommander haircommander haircommander left review comments

Assignees

@liggitt liggitt

@pacoxu pacoxu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG Node CI/Test Board
Done
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
SIG Auth Old
Closed / Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet