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: topologymgr: Graduate Kubelet Topology Manager to GA #116093

Merged
merged 7 commits into from
Mar 9, 2023
Merged

node: topologymgr: Graduate Kubelet Topology Manager to GA #116093

merged 7 commits into from
Mar 9, 2023

Conversation

swatisehgal
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

Captures changes to graduate Kubelet Topology Manager to GA.

Which issue(s) this PR fixes:

Issue: #693

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Graduate Kubelet Topology Manager to GA.

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

KEP: https://github.com/kubernetes/enhancements/pull/3745

@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/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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. area/kubelet area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 27, 2023
@swatisehgal swatisehgal mentioned this pull request Feb 27, 2023
Closed
11 tasks
@swatisehgal swatisehgal force-pushed the topologymanager-ga-graduation branch from ed98724 to 2be932c Compare February 27, 2023 17:45
@bart0sh
Copy link
Contributor

bart0sh commented Feb 28, 2023

/triage accepted
/priority important-soon
/assign @SergeyKanzhelev @klueska @mrunalp

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 28, 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 Feb 28, 2023
@swatisehgal
Copy link
Contributor Author

/cc @ffromani

ffromani
ffromani reviewed Feb 28, 2023
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

minor comments inside.
LGTM once the dependencies are merged.

@ffromani
Copy link
Contributor

ok, according to kubernetes/enhancements#693 this seems the last PR needed (and this seems correct to me). So let's discuss the inline comments and should be fine.

@swatisehgal swatisehgal force-pushed the topologymanager-ga-graduation branch from 2be932c to 8946faa Compare March 1, 2023 12:12
ffromani
ffromani reviewed Mar 1, 2023
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

I apologize for the late notice, but it seems we missed a trivial rename. Besides that, everything else LGTM

If you agree with the rename, you can either push a separate commit with the rename or amend the existing last one

@swatisehgal
node: topologymgr: Remove ResourceAllocator as TM is always enabled
937d330 
With Topology Manager enabled by default, we no longer need
`resourceAllocator` as Topology Manager serves as the main
PodAdmitHandler completely responsible for admission check
based on hints received from the hintProviders and the
subsequent allocation of the corresponding resources to a
pod as can be seen here:
https://github.com/kubernetes/kubernetes/blob/v1.26.0/pkg/kubelet/cm/topologymanager/scope.go#L150

With regard to DRA, the passing of `cm.draManager` into
resourceAllocator seems redundant as no admission checks
(and allocation of resources handled by DRA) is taking place
in `Admit` method of resourceAllocator. DRA has a completely
different model to the rest of the resource managers where
pod is only scheduled on a node once resources are reserved
for it. Because of this, admission checks or waiting for
resources to be provisioned after the pod has been scheduled
on the node is not required.

Before making the above change, it was verified that DRA Manager
is instantiated in `NewContainerManager`:
https://github.com/kubernetes/kubernetes/blob/v1.26.0/pkg/kubelet/cm/container_manager_linux.go#L318

Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal swatisehgal force-pushed the topologymanager-ga-graduation branch from 1e8c8fd to 937d330 Compare March 6, 2023 12:51
ffromani
ffromani reviewed Mar 6, 2023
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 323de0a288bbeaa0912d42e766b58fe229521037

@swatisehgal
node: topologymgr: remove comments with feature gate references
ae964a4 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: topologymgr: update node e2e test tags
e5ad3cb 
Signed-off-by: Swati Sehgal <[email protected]>
@swatisehgal
node: topologymgr: update autogenerated code
bea99ae 
Changes committed after running:
`./hack/update-codegen.sh`

Signed-off-by: Swati Sehgal <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2023
@k8s-ci-robot k8s-ci-robot added area/code-generation 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. labels Mar 7, 2023
@liggitt
Copy link
Member

liggitt commented Mar 7, 2023

/approve
API (kubelet config file mechanics / validation) lgtm

One question about dropping some errors cases out of the NewManager function when no topology policy is configured, will let kubelet reviewers weigh in on that and add final lgtm

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2023
@liggitt liggitt added this to the v1.27 milestone Mar 7, 2023
SergeyKanzhelev
SergeyKanzhelev approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

Created an issue to follow up and minimize the number of calls needed on initialization when policy is none. Should not block GA

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b731551beb60787d54646cc9938918153b443cb3

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: klueska, liggitt, SergeyKanzhelev, swatisehgal

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

@SergeyKanzhelev
Copy link
Member

/retest

@k8s-ci-robot
Copy link
Contributor

@swatisehgal: The following test 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-e2e-gce bea99ae link unknown /test pull-kubernetes-e2e-gce

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.

@pacoxu
Copy link
Member

pacoxu commented Mar 9, 2023

/retest-required
/skip

@k8s-ci-robot k8s-ci-robot merged commit 8d5c96f into kubernetes:master Mar 9, 2023
@swatisehgal swatisehgal deleted the topologymanager-ga-graduation branch March 9, 2023 09:21
@Shubham82 Shubham82 mentioned this pull request Dec 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@feiskyer feiskyer Awaiting requested review from feiskyer

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@dims dims Awaiting requested review from dims

@mrunalp mrunalp Awaiting requested review from mrunalp

@bart0sh bart0sh Awaiting requested review from bart0sh

@pohly pohly Awaiting requested review from pohly

@klueska klueska Awaiting requested review from klueska

@ffromani ffromani Awaiting requested review from ffromani

Assignees

@dims dims

@mrunalp mrunalp

@klueska klueska

@liggitt liggitt

@derekwaynecarr derekwaynecarr

@SergeyKanzhelev SergeyKanzhelev

@ffromani ffromani

Labels
api-review Categorizes an issue or PR as actively needing an API review. 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/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/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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
API Reviews
Status: API review completed, 1.27
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project
[sig-release] Bug Triage
Archived in project
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.