8000 [PodLevelResources] Event for unsupported pod-level resource manager alignment by KevinTMtz · Pull Request #132634 · kubernetes/kubernetes · GitHub
[go: up one dir, main page]
Skip to content

[PodLevelResources] Event for unsupported pod-level resource manager alignment #132634

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
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[PodLevelResources] Event for unsupported pod-level resource manager alignment #132634

wants to merge 1 commit into from

Conversation

KevinTMtz
Copy link
Contributor
@KevinTMtz KevinTMtz < 8000 /strong> commented Jun 30, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR is related to:

Fixes #132445

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2025
@k8s-ci-robot
Copy link
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 30, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @KevinTMtz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

< 8000 /div>
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 30, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jun 30, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KevinTMtz
Once this PR has been reviewed and has the lgtm label, please assign yujuhong for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@KevinTMtz
Copy link
Contributor Author

/assign @ndixita

@KevinTMtz KevinTMtz changed the title Event for unsupported pod-level resource manager alignment [PodLevelResources] Event for unsupported pod-level resource manager alignment Jun 30, 2025
@KevinTMtz KevinTMtz force-pushed the pod-level-resources-cpu-memory-event branch from b111ff0 to 5c27349 Compare June 30, 2025 21:38
@KevinTMtz KevinTMtz force-pushed the pod-level-resources-cpu-memory-event branch from 5c27349 to 5c3437a Compare June 30, 2025 21:40
ffromani
ffromani reviewed Jul 1, 2025
View reviewed changes
pkg/kubelet/kubelet.go
resourcehelper.IsPodLevelResourcesSet(pod) &&
v1qos.GetPodQOS(pod) == v1.PodQOSGuaranteed {

if kl.containerManager.GetNodeConfig().CPUManagerPolicy == string(cpumanager.PolicyStatic) {
Copy link
Contributor
@ffromani ffromani Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

please no, this is fragile and create unnecessary coupling among modules. This is another instance (alongside in-place VPA) which calls for #128728
I acknowledge the current solution is pretty much the only solution, but the proper way forward IMO is to add better internal API (and later de-entangle the container manager...)

@ffromani ffromani mentioned this pull request Jul 1, 2025
Open
@ffromani
Copy link
Contributor
ffromani commented Jul 1, 2025

/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 Jul 1, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 1, 2025
ndixita
ndixita reviewed Jul 1, 2025
View reviewed changes
pkg/kubelet/kubelet.go
@@ -1898,6 +1900,23 @@ func (kl *Kubelet) SyncPod(ctx context.Context, updateType kubetypes.SyncPodType
}
}

// If pod-level resources are set, CPU and Memory managers alignment is skipped,
// an event is surfaced to inform the user when skipped in the corresponding pod.
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not want to skip alignment for all cases.

  • If the container-level resources are set (regardless of pod-resources being set), align the resources at container-level
  • If only pod-level resources are set, then skip the alignment

ndixita
ndixita reviewed Jul 1, 2025
View reviewed changes
pkg/kubelet/cm/cpumanager/policy_static.go
@@ -463,6 +464,14 @@ func (p *staticPolicy) allocateCPUs(s state.State, numCPUs int, numaAffinity bit

func (p *staticPolicy) guaranteedCPUs(pod *v1.Pod, container *v1.Container) int {
qos := v1qos.GetPodQOS(pod)

// The CPU manager static policy does not support pod-level resources.
if utilfeature.DefaultFeatureGate.Enabled(features.PodLevelResources) && resourcehelper.IsPodLevelResourcesSet(pod) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be checked after QoS check, otherwise the message would be logged for pods with burstable and best-effort QoS classes as well

ndixita
ndixita reviewed Jul 1, 2025
View reviewed changes
Copy link
8000
Contributor
@ndixita ndixita left a comment

Choose a reason for hiding this comment

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

We also need changes for topology manager. For guaranteed pods,

  • If container-level resources are set with R=L (regardless or pod-level resources being set/unset) and scope=container, topology manager will still align the resources
  • If only pod-level resources are set, or container-level are set but R!=L, skip alignment with the event being recorded

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

@ndixita ndixita ndixita left review comments

@ffromani ffromani ffromani left review comments

@klueska klueska Awaiting requested review from klueska

@krmayankk krmayankk Awaiting requested review from krmayankk

Assignees

@ndixita ndixita

Labels
area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Status: Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Event for unsupported pod-level resource policies for alignment managers
4 participants
@KevinTMtz @k8s-ci-robot @ffromani @ndixita
0