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 GAed feature gates TopologyManager #121252

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Remove GAed feature gates TopologyManager #121252

merged 1 commit into from
Oct 18, 2023

Conversation

tukwila
Copy link
Contributor

@tukwila tukwila commented Oct 16, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

All of these feature gates have been GAed, and can be removed since v1.29:

  • TopologyManager

Fixes # None

Special notes for your reviewer:

release note

Remove GAed feature gates TopologyManager

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/693-topology-manager

@tukwila
Remove GAed feature gates TopologyManager
f69e1fe 
Signed-off-by: guangli.bao <guangli.bao@daocloud.io>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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. labels Oct 16, 2023
@tukwila
Copy link
Contributor Author

tukwila commented Oct 16, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 16, 2023
swatisehgal
swatisehgal reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a release note and link the Topology Manager KEP in the additional documentation section.

Also, I see you have a couple of other PRs trying to remove feature gates of GA'd features:
#121246
#121255

Can we perhaps combine them as separate commits into a single PR?

@pacoxu
Copy link
Member

pacoxu commented Oct 17, 2023

/sig node
/release-note-edit

Remove GAed feature gates TopologyManager

@k8s-ci-robot
Copy link
Contributor

@pacoxu: /release-note-edit must be used with a single release note block.

In response to this:

/sig node
/release-note-edit

Remove GAed feature gates TopologyManager

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.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 17, 2023
@pacoxu
Copy link
Member

pacoxu commented Oct 17, 2023

/release-note-edit

Remove GAed feature gates TopologyManager

@k8s-ci-robot
Copy link
Contributor

@pacoxu: /release-note-edit must be used with a single release note block.

In response to this:

/release-note-edit

Remove GAed feature gates TopologyManager

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.

@tukwila
Copy link
Contributor Author

tukwila commented Oct 17, 2023

Please add a release note and link the Topology Manager KEP in the additional documentation section.

Also, I see you have a couple of other PRs trying to remove feature gates of GA'd features: #121246 #121255

Can we perhaps combine them as separate commits into a single PR?

Yes, I have tried that before, but it always hanged for a feature owner. Please look at this PR: #120261
Seprate PR can be reviewed by different feature owner. Do you think so?

swatisehgal
swatisehgal reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have tried that before, but it always hanged for a feature owner. Please look at this PR: #120261 Seprate PR can be reviewed by different feature owner. Do you think so?

I see, my thought process was since we are updating pkg/features/kube_features.go for all these features doing so in separate PRs would mean that we will need approval from the same set of owners (https://github.com/kubernetes/kubernetes/blob/master/OWNERS_ALIASES#L491-L523) on all PRs as opposed to getting approval in a single PR.

Regardless of whether we do that in a single PR or in multiple separate ones, you will need approval from feature specific owners (e.g. apiextensions-apiserver OWNERS for API server updates in case of OpenAPIV3 feature gate removal).

Having said that, I acknowledge @liggitt's comment where he is indicating his preference of separate PR per feature gate as features are mostly SIG specific which is a valid point so let's go that way.

/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 17, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 03b4883e4133ed517867a26ec7d330f9ceaf6684

@swatisehgal
Copy link
Contributor

/assign @dims
for features approval

@bart0sh
Copy link
Contributor

bart0sh commented Oct 17, 2023

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Oct 17, 2023
@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. and removed 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. labels Oct 17, 2023
@dims
Copy link
Member

dims commented Oct 17, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, tukwila

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 17, 2023
@haircommander
Copy link
Contributor

/release-note-edit

Remove GAed feature gates TopologyManager

@k8s-ci-robot
Copy link
Contributor

@haircommander: /release-note-edit must be used with a single release note block.

In response to this:

/release-note-edit

Remove GAed feature gates TopologyManager

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.

@haircommander
Copy link
Contributor

/release-note-edit release-note Remove GAed feature gates TopologyManager

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 18, 2023
@k8s-ci-robot k8s-ci-robot merged commit 2fcddbd into kubernetes:master Oct 18, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 18, 2023
@tengqm
Copy link
Contributor

tengqm commented Oct 18, 2023

Have we updated the docs on k8s website?

@tukwila tukwila mentioned this pull request Oct 18, 2023
Merged
@tukwila
Copy link
Contributor Author

tukwila commented Oct 18, 2023

Have we updated the docs on k8s website?

yes, i commit one docs PR:
kubernetes/website#43554

@sftim
Copy link
Contributor

sftim commented Oct 20, 2023

Changelog suggestion

Removed the `TopologyManager` feature gate (feature had already graduated to stable)

@sftim
Copy link
Contributor

sftim commented Oct 20, 2023

/release-note-edit

Removed the `TopologyManager` feature gate (feature had already graduated to stable)

@k8s-ci-robot
Copy link
Contributor

@sftim: /release-note-edit must be used with a release note block.

In response to this:

/release-note-edit

Removed the `TopologyManager` feature gate (feature had already graduated to stable)

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.

@sftim
Copy link
Contributor

sftim commented Oct 20, 2023

Hmm, not sure how to achieve that when the note I want includes backticks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swatisehgal swatisehgal swatisehgal left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@dims dims

@swatisehgal swatisehgal

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.
9 participants
@tukwila @pacoxu @k8s-ci-robot @swatisehgal @bart0sh @dims @haircommander @tengqm @sftim