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

Add support for consuming whole NUMA nodes in CPUManager CPU assignments #102015

Merged
merged 6 commits into from Oct 15, 2021
Merged

Add support for consuming whole NUMA nodes in CPUManager CPU assignments #102015

merged 6 commits into from Oct 15, 2021

Conversation

klueska
Copy link
Contributor

@klueska klueska commented May 14, 2021 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The algorithm used to make CPU allocations in the CPUManager currently assumes that sockets have a 1:1 mapping with NUMA nodes. This is not true in the general case, however, and this PR updates the algorithm to account for this.

NUMA is a little different than the existing socket --> core --> cpu relationship, in that NUMA can fit into this hierarchy in one of 2 places, either as numa --> socket --> core --> cpu or socket --> numa --> core --> cpu. This PR takes both of these situations into account and makes sure to perform its CPU allocations appropriately.

Release notes:

Support allocating whole NUMA nodes in the CPUManager when there is not a 1:1 mapping between socket and NUMA node

@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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 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. labels May 14, 2021
@klueska
Copy link
Contributor Author

klueska commented May 14, 2021

/cc @nolancon
/cc @fromanirh
/cc @ranchothu

@k8s-ci-robot
Copy link
Contributor

@klueska: GitHub didn't allow me to request PR reviews from the following users: ranchothu.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @nolancon
/cc @fromanirh
/cc @ranchothu

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.

@klueska
Copy link
Contributor Author

klueska commented May 14, 2021

/hold to prevent inadvertent merge due to premature LGTM

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 14, 2021
@ffromani
Copy link
Contributor

/triage accepted
/priority backlog
I'll be reviewing ASAP

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. 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 May 14, 2021
@klueska klueska force-pushed the upstream-add-numa-to-cpu-assignment-algo branch 3 times, most recently from 983f915 to 13724eb Compare May 14, 2021 19:33
@adisky adisky added this to Needs Approver in SIG Node PR Triage May 17, 2021
@adisky adisky moved this from Needs Approver to Needs Reviewer in SIG Node PR Triage May 17, 2021
@ehashman
Copy link
Member

/assign @fromanirh

@ffromani ffromani force-pushed the upstream-add-numa-to-cpu-assignment-algo branch from bee4620 to 4f572d7 Compare October 15, 2021 06:41
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 15, 2021
@klueska
Add NUMA support to the CPU assignment algorithm in the CPUManager
17c7e86 
Signed-off-by: Kevin Klues <kklues@nvidia.com>
@ffromani
Copy link
Contributor

@klueska I just added a couple of very-minor, nonblocking comment to 43142d2 . From my POV the changes are a natural extension to the current code and I'm fine with them.

@ffromani
Copy link
Contributor

/test pull-kubernetes-integration

@ffromani
Copy link
Contributor

image

yet reported as failed? :\

@klueska klueska force-pushed the upstream-add-numa-to-cpu-assignment-algo branch from 4f572d7 to c03038f Compare October 15, 2021 09:10
@klueska
Copy link
Contributor Author

klueska commented Oct 15, 2021

@fromanirh just rebased on master to see if that helps.

@ffromani
Copy link
Contributor

/test pull-kubernetes-integration
pod timing out when starting, hardly broke by this PR

klueska and others added 5 commits October 15, 2021 10:29
@klueska
Abstract out whether NUMA or Sockets come first in the memory hierarchy
aff54a0 
This allows us to get rid of the check for determining which one is higher all
throughout the code. Now we just check once and instantiate an interface of the
appropriate type that makes sure the ordering in the hierarchy is preserved
through the appropriate calls.

Signed-off-by: Kevin Klues <kklues@nvidia.com>
@ffromani @klueska
cpumanager: topology: use rich cmp package
15caa13 
User the `cmp.Diff` package in the unit tests, moving away from
`reflect.DeepEqual`. This gives us a clearer picture of the differences
when the tests fail.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani @klueska
cpumanager: test: use proper subtests
f6ccc44 
The exisiting unit tests where performing subtests without
actually using the full features of the testing package
(https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks)

Update them with fairly minimal changes. The patch is deceptively
large because we need to move the code inside a new block.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani @klueska
cpumanager: test NUMA node support for CPU assign (1)
547996f 
This batch of tests adds a real topology on which each physical socket
has multiple NUMA zones. Taken by a real dual xeon 6320 gold.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani @klueska
cpumanager: test NUMA node support for CPU assign (2)
4bae656 
This batch of tests adds a fake topology on which each numa node
has multiple sockets. We didn't find yet a real HW topology in the wild
like this, but we need one to fully exercise the code.

So, until we find a HW topology, we add a fake one flipping
the NUMA/socket config of the existing xeon dual gold 6320.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@klueska klueska force-pushed the upstream-add-numa-to-cpu-assignment-algo branch from c03038f to 4bae656 Compare October 15, 2021 10:33
@klueska
Copy link
Contributor Author

klueska commented Oct 15, 2021

This now /lgtm from my perspective.
Thanks to @fromanirh for help generating such comprehensive tests.

ffromani
ffromani reviewed Oct 15, 2021
View reviewed changes
pkg/kubelet/cm/cpumanager/cpu_assignment.go
@@ -26,20 +26,133 @@ import (
"k8s.io/kubernetes/pkg/kubelet/cm/cpuset"
)

type numaOrSocketsFirstFuncs interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a very nice solution, I like the direction.
nit: should be rename this interface like coresHierarchy or cpuHierararchy or so? Again I can't find a half-decent suggestion, so very minor atm (we can improve later)

Copy link
Contributor

Choose a reason for hiding this comment

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

or memoryHierarchy even?

Copy link
Contributor Author

@klueska klueska Oct 15, 2021 •
edited

Choose a reason for hiding this comment

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

I don't feel too strongly about the name of the interface, but I like the name numaOrSocketsFirst for the name of the field in the cpuAccumulator, because it lets me make calls like:

acc.numaOrSocketsFirst.takeFullFirstLevel()
acc.numaOrSocketsFirst.takeFullSecondLevel()

Which I think makes it very clear what it is doing. I just named the interface as an extension of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, this makes it clear and the name nicer. Let's keep this name.

Copy link
Contributor

Choose a reason for hiding this comment

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

This solution is very nice indeed! I think we can leave the interface name as numaOrSocketsFirstFuncs for now.

@ffromani
Copy link
Contributor

code changes /lgtm (again minor/nit comment inside), natural and clean extension of the existing codebase

@ffromani
Copy link
Contributor

/test pull-kubernetes-e2e-gce-ubuntu-containerd
another likely flake (monitoring and reporting if needed)

swatisehgal
swatisehgal approved these changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

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

Having the ability to support systems where it is incorrect to assume the 1:1 mapping between sockets and NUMA nodes and allocating CPUs taking both NUMA nodes and CPUs sockets into consideration is a great addition to the code base.

Thanks @klueska @fromanirh for this improvement!
/lgtm

pkg/kubelet/cm/cpumanager/cpu_assignment.go
@@ -26,20 +26,133 @@ import (
"k8s.io/kubernetes/pkg/kubelet/cm/cpuset"
)

type numaOrSocketsFirstFuncs interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

This solution is very nice indeed! I think we can leave the interface name as numaOrSocketsFirstFuncs for now.

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: klueska, 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

@klueska
Copy link
Contributor Author

klueska commented Oct 15, 2021

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 55e1d2f into kubernetes:master Oct 15, 2021
SIG Node PR Triage automation moved this from Waiting on Author to Done Oct 15, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 15, 2021
@klueska klueska mentioned this pull request Nov 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffromani ffromani ffromani left review comments

@swatisehgal swatisehgal swatisehgal approved these changes

@nolancon nolancon Awaiting requested review from nolancon

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@krmayankk krmayankk Awaiting requested review from krmayankk

Assignees

@swatisehgal swatisehgal

@ffromani ffromani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@klueska @k8s-ci-robot @ffromani @ehashman @k8s-triage-robot @swatisehgal