node: resourcemanager: API to check exclusive assignment availability #128728

ffromani · 2024-11-10T13:30:17Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

There are now at least two pieces of code which wants to know if resource managers (CPU, Memory) support exclusive assignment:

VPA, for its internal needs
node container manager, to fix cgroup params

Currently, code needing to know if exclusive assignment is available has mostly two options:

infer from available APIs. Works, keep somehow the responsability respected, but it's clumsy
peeks in the global config, which introduces unnecessary coupling and possibly bugs (e.g. cpumanager policy is called "static", while the memorymanager policy is called "Static")

So, it seems the time to have an official API the code outside the resource/container manager can query safely, concisely and in a supported way.

Which issue(s) this PR fixes:

Fixes #129531

Special notes for your reviewer:

Prompted by #128727 et. al.

Does this PR introduce a user-facing change?

NONE

ffromani · 2024-11-10T13:30:33Z

/sig node

ffromani · 2024-11-10T13:30:43Z

/hold

reviewable, but I'd like to add more tests

ffromani · 2024-11-10T13:31:49Z

pkg/kubelet/kubelet.go

 		}
-		if utilfeature.DefaultFeatureGate.Enabled(features.MemoryManager) {
-			if kl.containerManager.GetNodeConfig().ExperimentalMemoryManagerPolicy == "static" {


cc @tallclair due to historycal reasons, memory manager policy names are CamelCase (e.g. "Static") so you may have a hidden bug here. My PR wants to remove this class of possible bugs.

Thanks @ffromani , until the API approach is finished #130559 PR tries to fix the hidden bug you have described.

ffromani · 2024-11-10T13:32:04Z

pkg/kubelet/cm/node_container_manager_linux.go

-	if cm.cpuManager.GetAllocatableCPUs().IsEmpty() {
+	// Only do so when the cpumanager is not exclusively allocating CPUs, as it will do its own updating of the cpuset when
+	// CPUs are exclusively allocated.
+	if !cm.cpuManager.CanAllocateExclusively(v1.ResourceCPU) {


/cc @haircommander

this is cleaner, LGTM

ffromani · 2024-11-10T13:34:53Z

pkg/kubelet/kubelet.go

@@ -2840,17 +2840,13 @@ func isPodResizeInProgress(pod *v1.Pod, podStatus *kubecontainer.PodStatus) bool
 // Returns true if the resize can proceed.
 func (kl *Kubelet) canResizePod(pod *v1.Pod) (bool, v1.PodResizeStatus) {
 	if v1qos.GetPodQOS(pod) == v1.PodQOSGuaranteed && !utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScalingExclusiveCPUs) {
-		if utilfeature.DefaultFeatureGate.Enabled(features.CPUManager) {


removed FGs because since 1.32 both managers are GA

Do you think we need to consider https://kubernetes.io/docs/tasks/administer-cluster/cpu-management-policies/#cpu-policy-static--options which some of them are alpha, some beta?

ffromani · 2024-11-11T10:47:56Z

perhaps the VPA use case can be actually handled by the same API being added #127525 + a similar API for memory management; if so, the point of this PR will be moot. Depends on what VPA code axtually needs to know

haircommander · 2024-11-11T17:59:04Z

perhaps the VPA use case can be actually handled by the same API being added #127525 + a similar API for memory management; if so, the point of this PR will be moot. Depends on what VPA code axtually needs to know

I am open to either approach, but we should separate the new cgroupManager API from #127525 if we wanted to get it in as a bug fix (seems there's also a featuregate addition in there)

bart0sh · 2024-11-12T17:14:52Z

/triage accepted

k8s-ci-robot · 2025-01-08T17:36:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ffromani
Once this PR has been reviewed and has the lgtm label, please assign klueska 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:

pkg/kubelet/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

esotsal · 2025-03-12T07:25:12Z

Overall question , have we thought about the tax/impacts in API from resources point of view ? I mean it depends how often it will be called etc but would be nice to have a figure and taking it into account in API dimensioning.

ffromani · 2025-03-12T07:33:46Z

Overall question , have we thought about the tax/impacts in API from resources point of view ? I mean it depends how often it will be called etc but would be nice to have a figure and taking it into account in API dimensioning.

No, I didn't run benchmark or profiling (do we do in general, as habit? do we do on special cases?). However most of the complexity comes from the need to traverse the layers. The actual check is either extremely cheap (hardcoded constant!) or very cheap (mutex + hashtable lookup)

esotsal · 2025-03-12T07:37:38Z

while writing tests, I was checking again the API and I'm inclined to change it a bit. The CanAllocateExclusively API should also take a pod object as its argument. The reason is: some managers only act on Guaranteed QoS pods, and they have this check internally. Failing to take the pod QoS into account in the API moves the burden on the calling code, which is another form of implementation detail leaking and unnecessary coupling which we are trying to avoid.

it is not so easy though. We need to call this functionality also in node_container_manager (one of the two key usecase), where we don't have a pod object handy.

I think it is worth the effort to give it a try

esotsal · 2025-03-12T07:40:20Z

/test pull-kubernetes-node-kubelet-podresize
/test pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2
/test pull-kubernetes-node-kubelet-serial-podresize

ffromani · 2025-03-12T07:47:28Z

while writing tests, I was checking again the API and I'm inclined to change it a bit. The CanAllocateExclusively API should also take a pod object as its argument. The reason is: some managers only act on Guaranteed QoS pods, and they have this check internally. Failing to take the pod QoS into account in the API moves the burden on the calling code, which is another form of implementation detail leaking and unnecessary coupling which we are trying to avoid.

it is not so easy though. We need to call this functionality also in node_container_manager (one of the two key usecase), where we don't have a pod object handy.

I think it is worth the effort to give it a try

uhm, not sure where I can get a pod object from here though.

ffromani · 2025-03-12T07:58:35Z

while writing tests, I was checking again the API and I'm inclined to change it a bit. The CanAllocateExclusively API should also take a pod object as its argument. The reason is: some managers only act on Guaranteed QoS pods, and they have this check internally. Failing to take the pod QoS into account in the API moves the burden on the calling code, which is another form of implementation detail leaking and unnecessary coupling which we are trying to avoid.

it is not so easy though. We need to call this functionality also in node_container_manager (one of the two key usecase), where we don't have a pod object handy.

I think it is worth the effort to give it a try

uhm, not sure where I can get a pod object from here though.

nope, in the node container manager flows there's no meanigful pod objects to be retreived. So we need to keep it like that for the time being.

ffromani · 2025-03-12T07:58:55Z

/test pull-kubernetes-node-kubelet-podresize
/test pull-kubernetes-e2e-inplace-pod-resize-containerd-main-v2
/test pull-kubernetes-node-kubelet-serial-podresize

ffromani · 2025-03-12T08:49:44Z

/retest-required

ffromani · 2025-03-12T09:18:30Z

/test pull-kubernetes-unit

cache test failure is unlikely (read: is not) this PR's fault

There are now at least two pieces of code which wants to know if resource managers (CPU, Memory) support exclusive assignment: - VPA, for its internal needs - node container manager, to fix cgroup params Currently, code needing to know if exclusive assignment is available has mostly two options: - infer from available APIs. Works, keep somehow the responsability respected, but it's clumsy - peeks in the global config, which introduces unnecessary coupling and possibly bugs (e.g. cpumanager policy is called "static", while the memorymanager policy is called "Static") So, it seems the time to have an official API the code outside the resource/container manager can query safely, concisely and in a supported way. Signed-off-by: Francesco Romani <[email protected]>

ffromani · 2025-03-19T17:26:20Z

/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2

ffromani · 2025-03-20T08:19:16Z

/test pull-kubernetes-node-crio-cgrpv2-imagevolume-e2e

ffromani · 2025-03-20T08:20:05Z

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

ffromani · 2025-03-20T13:16:19Z

/test pull-kubernetes-node-kubelet-serial-containerd
/test pull-kubernetes-node-kubelet-serial-containerd-sidecar-containers
/test pull-kubernetes-node-kubelet-serial-cpu-manager
/test pull-kubernetes-node-kubelet-serial-hugepages
/test pull-kubernetes-node-kubelet-serial-memory-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager

ffromani · 2025-03-20T13:57:23Z

/test pull-kubernetes-node-kubelet-serial-cpu-manager-kubetest2

k8s-ci-robot · 2025-03-20T16:04:10Z

@ffromani: 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-containerd-kubetest2	`3b54618`	link	false	`/test pull-kubernetes-node-kubelet-serial-containerd-kubetest2`
pull-kubernetes-node-kubelet-serial-memory-manager	`3b54618`	link	false	`/test pull-kubernetes-node-kubelet-serial-memory-manager`
pull-kubernetes-node-kubelet-serial-containerd	`3b54618`	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-sigs/prow repository. I understand the commands that are listed here.

ffromani · 2025-03-20T21:01:53Z

/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2

k8s-ci-robot · 2025-03-22T00:12:53Z

PR needs rebase.

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-triage-robot · 2025-06-24T19:07:54Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-ci-robot requested review from kannon92 and klueska November 10, 2024 13:30

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. area/kubelet and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 10, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2024

ffromani commented Nov 10, 2024

View reviewed changes

k8s-ci-robot requested a review from haircommander November 10, 2024 13:32

ffromani commented Nov 10, 2024

View reviewed changes

ffromani mentioned this pull request Nov 10, 2024

memmanager:cleanup: drop Experimental prefix #128727

Merged

ffromani force-pushed the resmrgr-introspection branch 2 times, most recently from ab81fd7 to bf3a165 Compare November 10, 2024 14:51

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 12, 2024

ffromani force-pushed the resmrgr-introspection branch from bf3a165 to 38482fd Compare January 8, 2025 17:36

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2025

ffromani force-pushed the resmrgr-introspection branch from 8cf6d09 to d31331c Compare March 12, 2025 07:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2025

ffromani force-pushed the resmrgr-introspection branch from d31331c to 0b79dc8 Compare March 12, 2025 08:27

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2025

ffromani force-pushed the resmrgr-introspection branch from 0b79dc8 to 4b6aabc Compare March 18, 2025 09:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2025

ffromani force-pushed the resmrgr-introspection branch from 4b6aabc to 3b54618 Compare March 18, 2025 10:17

ffromani mentioned this pull request Mar 19, 2025

topology manager presubmit jobs seems to be skipping all the tests #125950

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2025

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2025

node: resourcemanager: API to check exclusive assignment availability #128728

Are you sure you want to change the base?

node: resourcemanager: API to check exclusive assignment availability #128728

Uh oh!

Conversation

ffromani commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

ffromani commented Nov 10, 2024

Uh oh!

ffromani commented Nov 10, 2024

Uh oh!

ffromani Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

esotsal Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

ffromani Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

haircommander Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

ffromani Nov 10, 2024

Choose a reason for hiding this comment

Uh oh!

esotsal Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

ffromani commented Nov 11, 2024

Uh oh!

haircommander commented Nov 11, 2024

Uh oh!

bart0sh commented Nov 12, 2024

Uh oh!

k8s-ci-robot commented Jan 8, 2025

Uh oh!

esotsal commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ffromani commented Mar 12, 2025

Uh oh!

esotsal commented Mar 12, 2025

Uh oh!

esotsal commented Mar 12, 2025

Uh oh!

ffromani commented Mar 12, 2025

Uh oh!

ffromani commented Mar 12, 2025

Uh oh!

ffromani commented Mar 12, 2025

Uh oh!

ffromani commented Mar 12, 2025

Uh oh!

ffromani commented Mar 12, 2025

Uh oh!

ffromani commented Mar 19, 2025

Uh oh!

ffromani commented Mar 20, 2025

Uh oh!

ffromani commented Mar 20, 2025

Uh oh!

ffromani commented Mar 20, 2025

Uh oh!

ffromani commented Mar 20, 2025

Uh oh!

k8s-ci-robot commented Mar 20, 2025

Uh oh!

ffromani commented Mar 20, 2025

Uh oh!

k8s-ci-robot commented Mar 22, 2025

Uh oh!

k8s-triage-robot commented Jun 24, 2025

Uh oh!

ffromani commented Nov 10, 2024 •

edited

Loading

esotsal commented Mar 12, 2025 •

edited

Loading