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

node: graduate CPUManagerPolicyOptions to beta #105012

Merged
merged 1 commit into from Oct 8, 2021
Merged

node: graduate CPUManagerPolicyOptions to beta #105012

merged 1 commit into from Oct 8, 2021

Conversation

ffromani
Copy link
Contributor

@ffromani ffromani commented Sep 14, 2021 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Changes to graduate the CPUManagerPolicyOptions to beta, and to introduce the CPUManagerPolicy{Alpha,Beta}Options new feature gate as described in kubernetes/enhancements#2933

Which issue(s) this PR fixes:

Fixes N/A

Special notes for your reviewer:

N/A

The CPUManager policy options are now enabled, and we introduce a graduation path for the new CPU Manager policy options.

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

KEP update: kubernetes/enhancements#2933

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. 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 Sep 14, 2021
@ffromani
Copy link
Contributor Author

/hold
I'd like to check if it's better to add/fix e2e tests in this PR or on a followup one; additionally I want to doublecheck I covered all the feature gate changes.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2021
@ffromani ffromani mentioned this pull request Sep 14, 2021
Open
4 tasks
@ffromani
Copy link
Contributor Author

/sig node

@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 Sep 14, 2021
@ffromani
Copy link
Contributor Author

FYI @swatisehgal @klueska

@ehashman ehashman added this to Triage in SIG Node PR Triage Sep 14, 2021
klueska
klueska reviewed Sep 14, 2021
View reviewed changes
pkg/features/kube_features.go Outdated

// owner: @fromanirh
// alpha: v1.23
// beta: perma-alpha, see KEP for details
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the terminoligy that was agreed upon for this? /cc @johnbelamaric

Copy link
Contributor

Choose a reason for hiding this comment

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

The PR here reflects what was written in the KEP. We will finalize the terminology and how we want to handle the newly introduced experimental options after a follow-up discussion in the Production Readiness Subproject meeting on 22nd September. I have already added an agenda item for this here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Let's hold on this until we know what the final terminology will be then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had the PRR subproject meeting, and no major objections were raised from PRR perspective; however, it was pointed out that this is effectively more a arch/api-review concern than a PRR concern, and to move the discussion on sig-arch proper. This is what me and @swatisehgal are currently doing. I will post updates as we have them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Email thread here: https://groups.google.com/u/1/g/kubernetes-sig-architecture/c/Nxsc7pfe5rw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR to align the KEP with the outcome of the discussion: kubernetes/enhancements#2993

@MadhavJivrajani
Copy link
Contributor

/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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 20, 2021
@MadhavJivrajani MadhavJivrajani moved this from Triage to Waiting on Author in SIG Node PR Triage Sep 20, 2021
@ffromani ffromani force-pushed the cpumanager-policy-options-beta branch from 34f5469 to d6b384e Compare September 27, 2021 08:24
@k8s-ci-robot k8s-ci-robot added area/kubelet area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 27, 2021
@ffromani
Copy link
Contributor Author

/hold cancel
we had a consensus on sig-arch, documented in the PR itself

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2021
@ffromani ffromani force-pushed the cpumanager-policy-options-beta branch from b16e305 to 58048f7 Compare September 29, 2021 09:28
@ffromani
node: graduate CPUManagerPolicyOptions to beta
077c0aa 
We graduate the `CPUManagerPolicyOptions` feature to beta
in the 1.23 cycle, and we add new experimental feature gates
to guard new options which are planned in the 1.23 and in the
following cycles.

We introduce additional feature gate called `CPUManagerPolicyAlphaOptions` and
`CPUManagerPolicyBetaOptions`. The basic idea is to avoid the
cumbersome process of adding a feature gate for each option, and to have
feature gates which track the maturity level of _groups_ of options.
Besides this change, the graduation process, and the process in general,
for adding new policy options is still unchanged.

The `full-pcpus-only` option added in the 1.22 cycle is intentionally
moved into the beta policy options

For more details:
- KEP: kubernetes/enhancements#2933
- sig-arch discussion:
  https://groups.google.com/u/1/g/kubernetes-sig-architecture/c/Nxsc7pfe5rw

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani ffromani force-pushed the cpumanager-policy-options-beta branch from 58048f7 to 077c0aa Compare September 29, 2021 09:40
@ffromani
Copy link
Contributor Author

Left one more comment. Also, it looks like the commit message needs to be updated to reflect the new feature gate names.

right, both good points. I addressed them in my last upload. Thanks for your quick reviews!

@ffromani
Copy link
Contributor Author

/test pull-kubernetes-node-kubelet-serial-containerd

@ffromani
Copy link
Contributor Author

none of the serial lane failures (whose signal we should improve with a separate effort) are related to this PR

@SergeyKanzhelev SergeyKanzhelev moved this from Triage to PRs - Needs Reviewer in SIG Node CI/Test Board Sep 29, 2021
@SergeyKanzhelev
Copy link
Member

/assign @klueska

@klueska
Copy link
Contributor

klueska commented Sep 29, 2021 •
edited

/lgtm
/approve

For kubelet and node e2e tests changes

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2021
@ffromani
Copy link
Contributor Author

Thanks Kevin. @ehashman @derekwaynecarr could you please review the pkg/features changes?
The serial lanes failures are known issues/flakes, I'll track the issues and their resolution separately.

@ffromani
Copy link
Contributor Author

/assign @derekwaynecarr
Per #105012 (comment)

@pacoxu pacoxu moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Sep 30, 2021
@ffromani
Copy link
Contributor Author

/test pull-kubernetes-node-kubelet-serial-containerd

1 similar comment
@ffromani
Copy link
Contributor Author

ffromani commented Oct 1, 2021

/test pull-kubernetes-node-kubelet-serial-containerd

@ffromani ffromani mentioned this pull request Oct 1, 2021
Merged
@k8s-ci-robot
Copy link
Contributor

@fromanirh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-node-kubelet-serial 077c0aa link false /test pull-kubernetes-node-kubelet-serial
pull-kubernetes-node-kubelet-serial-containerd 077c0aa link false /test pull-kubernetes-node-kubelet-serial-containerd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@ffromani
Copy link
Contributor Author

ffromani commented Oct 1, 2021

so, the failures on the serial lane are:

@derekwaynecarr
Copy link
Member

/approve
/lgtm

node changes look good, will need separate approval for feature gate.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, fromanirh, klueska

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 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 63f66e6 into kubernetes:master Oct 8, 2021
SIG Node CI/Test Board automation moved this from PRs - Needs Reviewer to Done Oct 8, 2021
SIG Node PR Triage automation moved this from Needs Approver to Done Oct 8, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 8, 2021
@klueska klueska mentioned this pull request Oct 31, 2022
Merged
@ffromani ffromani deleted the cpumanager-policy-options-beta branch July 17, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klueska klueska klueska left review comments

@swatisehgal swatisehgal swatisehgal left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@klueska klueska

@derekwaynecarr derekwaynecarr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. 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/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 Node CI/Test Board
Done
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ffromani @MadhavJivrajani @SergeyKanzhelev @klueska @k8s-ci-robot @derekwaynecarr @swatisehgal