node: cpumanager: add options to reject non SMT-aligned workload #2626

ffromani · 2021-04-14T15:06:04Z

We propose a change in cpumanager to make the behaviour of latency-sensitive applications more predictable when running on SMT-enabled systems. The existing static policy is allowed to allocate single cpus (aka virtual threads on SMT-enabled systems), and this can cause physical core sharing which in turn is a major cause for noisy neighbours, which harms the behaviour of latency-sensitive applications.

We add extra constraints to the aformenentioned policy, while preserving all its properties, to avoid physical core sharing.

Implementation PR link: kubernetes/kubernetes#101432

Signed-off-by: Francesco Romani [email protected]

ffromani · 2021-04-14T15:15:48Z

On Apr 13 sig-node meeting we agreed this proposal need more discussion, hence I posted this PR to foster that discussion, but for the same reason I'm not adding approver yet.

xiaoxubeii · 2021-04-15T11:57:25Z

There are some discussion about external cpu manager policies. I think you may be interesting: https://mail.google.com/mail/u/0?ik=4e2f4a7bba&view=om&permmsgid=msg-f%3A1696952803188250466

Summary to be filled once WIP is removed Signed-off-by: Francesco Romani <[email protected]>

Signed-off-by: Swati Sehgal <[email protected]>

Capture implementation details of enabling smtaware CPU Manager policy

ffromani · 2021-04-19T16:56:12Z

There are some discussion about external cpu manager policies. I think you may be interesting: https://mail.google.com/mail/u/0?ik=4e2f4a7bba&view=om&permmsgid=msg-f%3A1696952803188250466

yes, we need to agree in the community about which way is the best to extend the cpumanager in the first place. This is a very important point I'll make sure to mention in the sig-node meeting.

This KEP describes the behaviour we want to achieve in the end and the PoC implementation we have: to reimplement it using external policies is a perfectly fine approach.

Signed-off-by: Francesco Romani <[email protected]>

…policy - Fix minor formatting and typos - Disambiguate the text in smtaware policy to explain when SMTAlignmentError occurs. Signed-off-by: Swati Sehgal <[email protected]>

Minor formatting/typo fixes and disambiguate explanation of smtaware policy

Signed-off-by: Francesco Romani <[email protected]>

ffromani · 2021-04-23T12:24:32Z

overhaul and update after sig-node discussion on April 20, 2021. Link to the PR TBD. Will cc relevant people ASAP.

…ementation Recent identification of a bug in the resourceAdmitHandlers Admit handler, led to changes in the implementation whereby ContainerManagerAdmissionError is no longer returned. This patch ensures that implementation strategy captured in the KEP matches the latest smtaware CPUManager implementation. Signed-off-by: Swati Sehgal <[email protected]>

Ensure Implementation stategy captured in the KEP matches latest implementation

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

Signed-off-by: Francesco Romani <[email protected]>

johnbelamaric · 2021-05-10T19:24:12Z

PRR looks good, will await SIG approval before approving

kad

Few nits in wordings...

In overall: I'd suggest to re-consider to have dedicated policy name that would re-use as much as possible code from static policy, instead of creating new flag/option field in kubelet config. Reasons for that is that there are other components that can consume settings of Kubelet for some reasons (e.g. NFD or third party operators). Adding new field that alter behaviour of static policy means that all of those needs to be changed.

But beside that, the whole cause of adding SMT aligned workload placement is good thing to add.

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

kad · 2021-05-11T15:22:19Z

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

+
+### Proposed Change
+
+We propose to add a new flag in Kubelet called `CPUManagerPolicyOptions` in the kubelet config or command line argument called `cpumanager-policy-options` which allows the user to specify the CPU Manager policy option. If the value of this option is specified to be `reject-non-smt-aligned`, it results in further refinements of the existing static policy.


Frankly speaking, I'd prefer as separate policy name "smt-aligned-static" instead of option field to existing policy.
There are might be other components in the wild that reads kubelet config to determine node state and configured TopologyManager/CPUManager/... states. Instead of trying to teach those new field to parse (policy=static, option=xxx), might be better to have different policy names for different behaviours (policy=a vs. policy=b).

We had a discussion on sig-node slack channel. The main issue of the options approach is that options can alter the behaviour of the static policy, but if legacy (= not updated) consumers/unaware operators looking only at the policy name can fail to detect this and run with wrong assumptions about the kubelet behaviour.
So, we will add a new policy name (static-with-options? static-enhanced?) which will be an alias of static, but will enable the consumption of the options. This means that to enable this new behaviour we are proposing, a operator would need to:

enable the feature gate (needed anyway)

change the policy name

actually provide options ( :) )

For more context: https://kubernetes.slack.com/archives/C0BP8PW9G/p1620737012440700

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

ffromani · 2021-05-11T17:50:18Z

/assign @derekwaynecarr
/assign @dchen1107
Sorry, I can't recall who we discussed will be the approver, so tagging both! :)

During the review it was pointed out that existing operators (software component or humans) make logic on the policy name. Options may change the behaviour of the policy in a non-backward-compatible way[1], thus we need a way to signal this to consumers; we agreed to add an alias for the static policy whose sole purpose is to make this change evident. +++ [1] the change we are proposing in this KEP is not, but the cpumanager infra change we are proposing which in turn makes the change possible would enable these future options. Signed-off-by: Francesco Romani <[email protected]>

ffromani · 2021-05-12T12:45:36Z

looking for a more descriptive name, changed from static-enhanced to configurable

klueska · 2021-05-12T13:25:17Z

/lgtm

Let’s get this merged and we can always come back and tweak naming details like this after we’ve iterated on the implementation a bit.

kad · 2021-05-12T15:10:28Z

/lgtm from my side as well. Thanks @fromanirh for updates.

derekwaynecarr

please remove the configurable alias and then I think this is good to proceed.

derekwaynecarr · 2021-05-13T17:24:05Z

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

+- add a new flag in Kubelet called `CPUManagerPolicyOptions` in the kubelet config or command line argument called `cpumanager-policy-options` which allows the user to specify the CPU Manager policy option.
+- finally, add a new cpu manager option called `reject-non-smt-aligned`; if present, this option will enable further refinements of the existing static policy.
+
+The addition of an alias for the `static` policy is motivated by the fact the new CPUManager options can alter the behaviour of the policy in a backward incompatible way.


The static policy as understood today is a baseline. I am not sure I understand the configurable alias as that alias has the same meaning as the existing static policy. At the moment, I think its fine to just have the policy options bit because 'configurable' has no meaning yet until such time we enumerate each behavior as named policy tokens.

basically, let's remove configurable alias at the moment until a future time lets you declare all attributes as policy options that inform a policy.

derekwaynecarr · 2021-05-13T17:25:34Z

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

+### Implementation strategy of reject-non-smt-aligned CPU Manager policy option
+
+- In order to introduce the SMT-alignment check in CPU Manager, we introduce a new flag in Kubelet to allow the user to specify `cpumanager-policy-options` which when specified with `reject-non-smt-aligned` as its value provides the capability to modify the behaviour of static policy to strictly guarantee allocation of whole cores to a workload.
+- We add a new policy called `configurable`. Only if this policy is selected the options, if given, will be considered. Otherwise, they will be ignored.


please remove configurable until a future time.

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

derekwaynecarr · 2021-05-13T17:27:26Z

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

+  - [X] Feature gate (also fill in values in `kep.yaml`).
+    - Feature gate name: `CPUManagerPolicyOptions`.
+    - Components depending on the feature gate: kubelet
+  - [X] Change the kubelet configuration to set the CPUManager policy to `configurable`


remove this and just set 'static' as existing policy.

derekwaynecarr · 2021-05-13T17:27:54Z

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md

+  - [X] Change the kubelet configuration to set the CPUManager policy to `configurable`
+  - [X] Change the kubelet configuration adding the CPUManager policy option to `reject-non-smt-aligned`
+* **Does enabling the feature change any default behavior?**
+  - Yes, it makes the behaviour of the CPUManager static policy more restrictive and can lead to pod admission rejection.


note for others that cpu manager static policy can already reject pods, this just adds another reason for said rejection.

Address review comments Signed-off-by: Francesco Romani <[email protected]>

derekwaynecarr · 2021-05-13T18:21:21Z

/lgtm
/approve

ehashman

/approve
for PRR

#2626 (comment)

k8s-ci-robot · 2021-05-13T18:22:36Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~keps/prod-readiness/OWNERS~~ [ehashman]
~~keps/sig-node/OWNERS~~ [derekwaynecarr]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 14, 2021

k8s-ci-robot requested review from dchen1107 and derekwaynecarr April 14, 2021 15:06

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 14, 2021

ffromani mentioned this pull request Apr 14, 2021

node: cpumanager: add options to reject non SMT-aligned workload #2625

Open

12 tasks

ffromani force-pushed the cpumanager-policies-thread-placement branch from 7dddd86 to 50bd260 Compare April 16, 2021 13:17

WIP: node: add cpumanager policies with grained thread allocation

bfa538b

Summary to be filled once WIP is removed Signed-off-by: Francesco Romani <[email protected]>

ffromani force-pushed the cpumanager-policies-thread-placement branch from 50bd260 to bfa538b Compare April 16, 2021 13:22

swatisehgal and others added 2 commits April 19, 2021 12:01

Capture implementation details of enabling smtaware CPU Manager policy

af676e1

Signed-off-by: Swati Sehgal <[email protected]>

Merge pull request #1 from swatisehgal/add-smtaware-impl-details

dd2f88d

Capture implementation details of enabling smtaware CPU Manager policy

KEP: clarify admission vs accounting, add impl notes

1c26db7

Signed-off-by: Francesco Romani <[email protected]>

ffromani force-pushed the cpumanager-policies-thread-placement branch from 6a2667c to 1c26db7 Compare April 19, 2021 17:02

swatisehgal and others added 4 commits April 20, 2021 13:29

Minor formatting/typo fixes and disambiguate explanation of smtaware …

dad1428

…policy - Fix minor formatting and typos - Disambiguate the text in smtaware policy to explain when SMTAlignmentError occurs. Signed-off-by: Swati Sehgal <[email protected]>

Merge pull request #2 from swatisehgal/minor-fixes

730c7dd

Minor formatting/typo fixes and disambiguate explanation of smtaware policy

fix TOC

57ece95

Signed-off-by: Francesco Romani <[email protected]>

updates after sig-node meeting 20210420

eb1fc6b

Signed-off-by: Francesco Romani <[email protected]>

ffromani changed the title ~~WIP: node: add cpumanager policies with grained thread allocation~~ node: add cpumanager policies with grained thread allocation Apr 23, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2021

swatisehgal and others added 2 commits April 23, 2021 17:14

Merge pull request #3 from swatisehgal/update-imple-details

b4a0a36

Ensure Implementation stategy captured in the KEP matches latest implementation

swatisehgal mentioned this pull request Apr 23, 2021

node: cpumanager: add options to reject non SMT-aligned workload kubernetes/kubernetes#101432

Merged

odinuge reviewed Apr 24, 2021

View reviewed changes

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md Show resolved Hide resolved

keps/sig-node/2625-cpumanager-policies-thread-placement/README.md Outdated Show resolved Hide resolved

fix example allocation table

ae20bb1

Signed-off-by: Francesco Romani <[email protected]>

kad reviewed May 11, 2021

View reviewed changes

ffromani force-pushed the cpumanager-policies-thread-placement branch from f90b869 to af201ed Compare May 11, 2021 17:48

k8s-ci-robot assigned dchen1107 and derekwaynecarr May 11, 2021

ffromani force-pushed the cpumanager-policies-thread-placement branch from af201ed to def5c71 Compare May 11, 2021 18:03

ffromani force-pushed the cpumanager-policies-thread-placement branch from def5c71 to 7dd48d6 Compare May 12, 2021 12:44

k8s-ci-robot assigned klueska May 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021

klueska mentioned this pull request May 12, 2021

Additional cases for reconcileState testing kubernetes/kubernetes#101909

Merged

derekwaynecarr requested changes May 13, 2021

View reviewed changes

README: postpone configurable alias addition

0ac5a81

Address review comments Signed-off-by: Francesco Romani <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2021

ehashman approved these changes May 13, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021

k8s-ci-robot merged commit 01abf7d into kubernetes:master May 13, 2021

k8s-ci-robot added this to the v1.22 milestone May 13, 2021

ffromani mentioned this pull request Jun 10, 2021

Add cpumanager policy options kubernetes/kubernetes#102055

Closed

ffromani deleted the cpumanager-policies-thread-placement branch June 30, 2021 14:00

ffromani mentioned this pull request Jul 1, 2021

WIP: DNM: add cpumanager policy alias kubernetes/kubernetes#103401

Closed

swatisehgal mentioned this pull request Sep 30, 2021

REQUEST: New kubernetes-sigs membership for @swatisehgal kubernetes/org#3023

Closed

7 tasks

klueska mentioned this pull request Feb 9, 2024

KEP-4176: A Static Policy Option to spread hyperthreads across physical CPUs #4177

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: cpumanager: add options to reject non SMT-aligned workload #2626

node: cpumanager: add options to reject non SMT-aligned workload #2626

ffromani commented Apr 14, 2021 •

edited

Loading

ffromani commented Apr 14, 2021 •

edited

Loading

xiaoxubeii commented Apr 15, 2021

ffromani commented Apr 19, 2021

ffromani commented Apr 23, 2021

johnbelamaric commented May 10, 2021

kad left a comment

kad May 11, 2021

ffromani May 11, 2021

ffromani May 11, 2021 •

edited

Loading

ffromani commented May 11, 2021

ffromani commented May 12, 2021

klueska commented May 12, 2021

kad commented May 12, 2021

derekwaynecarr left a comment

derekwaynecarr May 13, 2021

derekwaynecarr May 13, 2021

derekwaynecarr May 13, 2021

derekwaynecarr May 13, 2021

derekwaynecarr May 13, 2021

derekwaynecarr commented May 13, 2021

ehashman left a comment

k8s-ci-robot commented May 13, 2021


		### Proposed Change

		We propose to add a new flag in Kubelet called `CPUManagerPolicyOptions` in the kubelet config or command line argument called `cpumanager-policy-options` which allows the user to specify the CPU Manager policy option. If the value of this option is specified to be `reject-non-smt-aligned`, it results in further refinements of the existing static policy.

node: cpumanager: add options to reject non SMT-aligned workload #2626

node: cpumanager: add options to reject non SMT-aligned workload #2626

Conversation

ffromani commented Apr 14, 2021 • edited Loading

ffromani commented Apr 14, 2021 • edited Loading

xiaoxubeii commented Apr 15, 2021

ffromani commented Apr 19, 2021

ffromani commented Apr 23, 2021

johnbelamaric commented May 10, 2021

kad left a comment

Choose a reason for hiding this comment

kad May 11, 2021

Choose a reason for hiding this comment

ffromani May 11, 2021

Choose a reason for hiding this comment

ffromani May 11, 2021 • edited Loading

Choose a reason for hiding this comment

ffromani commented May 11, 2021

ffromani commented May 12, 2021

klueska commented May 12, 2021

kad commented May 12, 2021

derekwaynecarr left a comment

Choose a reason for hiding this comment

derekwaynecarr May 13, 2021

Choose a reason for hiding this comment

derekwaynecarr May 13, 2021

Choose a reason for hiding this comment

derekwaynecarr May 13, 2021

Choose a reason for hiding this comment

derekwaynecarr May 13, 2021

Choose a reason for hiding this comment

derekwaynecarr May 13, 2021

Choose a reason for hiding this comment

derekwaynecarr commented May 13, 2021

ehashman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 13, 2021

ffromani commented Apr 14, 2021 •

edited

Loading

ffromani commented Apr 14, 2021 •

edited

Loading

ffromani May 11, 2021 •

edited

Loading