kubelet: Add a flag "MaxAllowableNUMANodes" #124148

cyclinder · 2024-04-02T10:12:23Z

add a flag MaxAllowableNUMANodes for kubelet, which can make maxAllowableNUMANodes in the kubelet's TopologyManager configurable.

What type of PR is this?

/kind feature

What this PR does / why we need it:

add a flag MaxAllowableNUMANodes for kubelet, which configures maxAllowableNUMANodes in the kubelet's TopologyManager.

Which issue(s) this PR fixes:

Fixes #124144

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a flag "MaxAllowableNUMANodes" to configures maxAllowableNUMANodes in the kubelet's TopologyManager

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

k8s-ci-robot · 2024-04-02T10:12:27Z

Please note that we're already in Test Freeze for the release-1.30 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.30.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Apr 2 08:19:47 UTC 2024.

cyclinder · 2024-04-02T10:12:58Z

/sig-node

kannon92 · 2024-04-02T12:07:16Z

cmd/kubelet/app/options/options.go

@@ -90,6 +90,16 @@ type KubeletFlags struct {
 	// additional configs to overwrite what is provided by default and in the KubeletConfigFile flag
 	KubeletDropinConfigDirectory string

+	// maxAllowableNUMANodes specifies the maximum number of NUMA Nodes that


This should be moved to the kubelet config I think. We are aiming to avoid adding parameters to the kubelet CLI. I'd add it both in the staging directory and pkg/kubelet/apis/config.

thanks for the review.

We are aiming to avoid adding parameters to the kubelet CLI.

yes, makes sense, I'll add it later.

I also don't think this should be a top-level flag, but rather a topology-manager policy option. See the following for the PR that added policy options, including the very first option: #112914

A policy option would indeed let us plug in the existing framework without new API reviews. Perhaps this is the way to go for all the new flags and features for topology manager?

I also don't think this should be a top-level flag, but rather a topology-manager policy option. See the following for the PR that added policy options, including the very first option:
#112914.

Interesting, I would have thought that this seems more apt as a top level kubelet flag behind a feature gate given its nature and relevance across all policies and policy options. If the value is not specified and feature gate not enabled, we can default to the old value of 8 and preserve backward compatibility with minimal code changes.

We would have to go through graduation process irrespective of its implementation as a top level flag or a policy option. What is the rationale behind proposing this as a policy option?

So what's the conclusion here? We only need a policy option or a feature-gate. I prefer to have a policy option here.

I still believe it should be a policy option. It scopes it beneath the TopologyManager better than having a top level flag.

Based on your comments, I've made some updates, Could you retake a review? thanks!

kannon92

https://github.com/kubernetes/kubernetes/pull/124148/files#r1547741506

pkg/kubelet/cm/topologymanager/topology_manager_test.go

kannon92 · 2024-04-02T18:34:09Z

/cc @klueska @ffromani

kannon92 · 2024-04-02T18:34:37Z

/triage accepted
/priority important-longterm

cmd/kubelet/app/options/options.go

ffromani · 2024-04-03T09:48:11Z

pkg/kubelet/cm/topologymanager/topology_manager.go

-	// generate hints for them. As such, if more NUMA Nodes than this are
-	// present on a machine and the TopologyManager is enabled, an error will
-	// be returned and the TopologyManager will not be loaded.
-	maxAllowableNUMANodes = 8


I'd keep this constant but renamed like DefaultMaxAllowableNUMANodes - if user doesn't specify a new value, this is the recommended default. The user should probably not be enabled to set less (what's the point?) but if they set more, they should IMO get a warning both in the logs and from the docs

That make sense, thanks for the comment.

but if they set more, they should IMO get a warning both in the logs and from the docs

Can you tell me where this warning or log or docs would be better if they set more? thanks!

swatisehgal · 2024-04-03T14:07:23Z

/cc

swatisehgal

We would need a KEP for this work so please ensure that this is captured in the Node 1.31 planning document: https://docs.google.com/document/d/1U10J0WwgWXkdYrqWGGvO8iH2HKeerQAlygnqgDgWv4E/edit.

swatisehgal · 2024-04-03T17:08:01Z

cmd/kubelet/app/options/options.go

@@ -90,6 +90,16 @@ type KubeletFlags struct {
 	// additional configs to overwrite what is provided by default and in the KubeletConfigFile flag
 	KubeletDropinConfigDirectory string

+	// maxAllowableNUMANodes specifies the maximum number of NUMA Nodes that


I also don't think this should be a top-level flag, but rather a topology-manager policy option. See the following for the PR that added policy options, including the very first option:
#112914.

Interesting, I would have thought that this seems more apt as a top level kubelet flag behind a feature gate given its nature and relevance across all policies and policy options. If the value is not specified and feature gate not enabled, we can default to the old value of 8 and preserve backward compatibility with minimal code changes.

We would have to go through graduation process irrespective of its implementation as a top level flag or a policy option. What is the rationale behind proposing this as a policy option?

k8s-ci-robot · 2024-04-10T06:50:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cyclinder
Once this PR has been reviewed and has the lgtm label, please assign klueska for approval. For more information see the Kubernetes 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:

cmd/kubelet/OWNERS
pkg/kubelet/cm/topologymanager/OWNERS

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

pkg/kubelet/cm/topologymanager/policy_options.go

…Options add a flag MaxAllowableNUMANodes for kubelet, which can make maxAllowableNUMANodes in the kubelet's TopologyManager configurable. Signed-off-by: cyclidner <kuocyclinder@gmail.com>

cyclinder · 2024-04-30T06:37:25Z

Hi @PiotrProkop @ffromani Could you please take a look?

PiotrProkop · 2024-04-30T06:57:44Z

The changes looks good to me. I just think we need KEP for this effort(should be pretty straightforward, I can help with the KEP if needed) and when it will be approved we can track this change for k8s 1.31 😄

ffromani · 2024-04-30T07:03:21Z

The changes looks good to me. I just think we need KEP for this effort(should be pretty straightforward, I can help with the KEP if needed) and when it will be approved we can track this change for k8s 1.31 😄

I agree (this is also something @swatisehgal highlighted e.g. in #124148 (comment)). Either approaches - a new flag or a policy option are fine to me, but either will require some extra process as it stands today

cyclinder · 2024-04-30T07:15:38Z

require some extra process as it stands today

Is the extra process a KEP or something else? I am willing to submit a KEP for this 😄

ffromani · 2024-04-30T07:19:16Z

require some extra process as it stands today

Is the extra process a KEP or something else? I am willing to submit a KEP for this 😄

Regarding the extra process I was thinking to the API review for the flag addition. AFAIK the flag addition won't need a Feature Gate nor a KEP, at least not in the general case. If so, it will emerge during the API review I think.
For the policy option, it will require a KEP as the process stands today AFAIK.

EDIT: OTOH the new option can likely start from beta stage.

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. labels Apr 2, 2024

k8s-ci-robot added area/kubelet 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 Apr 2, 2024

k8s-ci-robot requested review from bart0sh and haircommander April 2, 2024 10:13

kannon92 reviewed Apr 2, 2024

View reviewed changes

kannon92 suggested changes Apr 2, 2024

View reviewed changes

kannon92 reviewed Apr 2, 2024

View reviewed changes

pkg/kubelet/cm/topologymanager/topology_manager_test.go Outdated Show resolved Hide resolved

k8s-ci-robot requested review from ffromani and klueska April 2, 2024 18:34

ffromani reviewed Apr 3, 2024

View reviewed changes

k8s-ci-robot requested a review from swatisehgal April 3, 2024 14:07

bart0sh added this to Triage in SIG Node PR Triage Apr 3, 2024

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Apr 3, 2024

swatisehgal reviewed Apr 3, 2024

View reviewed changes

cyclinder force-pushed the add_flag_kubelet branch from dc7b59f to 43e4bae Compare April 10, 2024 06:49

PiotrProkop reviewed Apr 10, 2024

View reviewed changes

pkg/kubelet/cm/topologymanager/policy_options.go Show resolved Hide resolved

cyclinder force-pushed the add_flag_kubelet branch from 43e4bae to 8ae4250 Compare April 10, 2024 07:18

kubelet: Add a flag "MaxAllowableNUMANodes" for TopologyManagerPolicy…

931a061

…Options add a flag MaxAllowableNUMANodes for kubelet, which can make maxAllowableNUMANodes in the kubelet's TopologyManager configurable. Signed-off-by: cyclidner <kuocyclinder@gmail.com>

cyclinder force-pushed the add_flag_kubelet branch from 8ae4250 to 931a061 Compare April 10, 2024 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Add a flag "MaxAllowableNUMANodes" #124148

kubelet: Add a flag "MaxAllowableNUMANodes" #124148

cyclinder commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

cyclinder commented Apr 2, 2024

kannon92 Apr 2, 2024

cyclinder Apr 3, 2024

klueska Apr 3, 2024

ffromani Apr 3, 2024

swatisehgal Apr 3, 2024

cyclinder Apr 8, 2024

klueska Apr 9, 2024

cyclinder Apr 10, 2024

kannon92 left a comment

kannon92 commented Apr 2, 2024

kannon92 commented Apr 2, 2024

ffromani Apr 3, 2024

cyclinder Apr 8, 2024

cyclinder Apr 10, 2024

swatisehgal commented Apr 3, 2024

swatisehgal left a comment

swatisehgal Apr 3, 2024

k8s-ci-robot commented Apr 10, 2024

cyclinder commented Apr 30, 2024

PiotrProkop commented Apr 30, 2024

ffromani commented Apr 30, 2024

cyclinder commented Apr 30, 2024

ffromani commented Apr 30, 2024 •

edited

kubelet: Add a flag "MaxAllowableNUMANodes" #124148

Are you sure you want to change the base?

kubelet: Add a flag "MaxAllowableNUMANodes" #124148

Conversation

cyclinder commented Apr 2, 2024

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?

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

k8s-ci-robot commented Apr 2, 2024

cyclinder commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannon92 left a comment

Choose a reason for hiding this comment

kannon92 commented Apr 2, 2024

kannon92 commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swatisehgal commented Apr 3, 2024

swatisehgal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 10, 2024

cyclinder commented Apr 30, 2024

PiotrProkop commented Apr 30, 2024

ffromani commented Apr 30, 2024

cyclinder commented Apr 30, 2024

ffromani commented Apr 30, 2024 • edited

ffromani commented Apr 30, 2024 •

edited