Fix a bug whereby reusable CPUs and devices were not being honored #93189

klueska · 2020-07-17T14:58:15Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

/kind bug

/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Previously, it was possible for reusable CPUs and reusable devices (i.e.
those previously consumed by init containers) to not be reused by
subsequent init containers or app containers if the TopologyManager was
enabled. This would happen because hint generation for the
TopologyManager was not considering the reusable devices when it made
its hint calculation.

As such, it would sometimes:

Generate a hint for a differnent NUMA node, causing the CPUs and
devices to be allocated from that node instead of the one where the
reusable devices live; or
End up thinking there were not enough CPUs or devices to allocate and
throw a TopologyAffinity admission error

This patch fixes this by ensuring that reusable CPUs and devices are
considered as part of TopologyHint generation. This frunctionality is
difficult to unit test since it spans multiple components, but an e2e
test will be added in a subsequent patch to test this functionality.

Does this PR introduce a user-facing change?:

Fixed a bug whereby the allocation of reusable CPUs and devices was not being honored when the TopologyManager was enabled

derekwaynecarr · 2020-07-17T15:01:54Z

/milestone 1.19

k8s-ci-robot · 2020-07-17T15:01:56Z

@derekwaynecarr: The provided milestone is not valid for this repository. Milestones in this repository: [next-candidate, v1.13, v1.14, v1.15, v1.16, v1.17, v1.18, v1.19, v1.20, v1.21, v1.22, v2.0]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.19

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.

derekwaynecarr · 2020-07-17T15:02:16Z

/milestone v1.19

derekwaynecarr · 2020-07-17T15:02:42Z

we should prepare a cherrypick for this as well.

/approve
/lgtm

fejta-bot · 2020-07-18T00:33:13Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-07-18T11:45:12Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot · 2020-07-18T19:39:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, klueska

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:

~~pkg/kubelet/cm/cpumanager/OWNERS~~ [derekwaynecarr,klueska]
~~pkg/kubelet/cm/devicemanager/OWNERS~~ [derekwaynecarr,klueska]
~~pkg/kubelet/cm/topologymanager/OWNERS~~ [derekwaynecarr,klueska]

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

nolancon

Two small nits. LGTM otherwise

nolancon · 2020-07-20T10:34:09Z

pkg/kubelet/cm/topologymanager/bitmask/bitmask.go

@@ -33,6 +33,7 @@ type BitMask interface {
 	IsEqual(mask BitMask) bool
 	IsEmpty() bool
 	IsSet(bit int) bool
+	AnySet(bitss []int) bool


Nit: /bitss/bits

nolancon · 2020-07-20T10:34:40Z

pkg/kubelet/cm/topologymanager/bitmask/bitmask.go

@@ -120,6 +121,16 @@ func (s *bitMask) IsSet(bit int) bool {
 	return (*s & (1 << uint64(bit))) > 0
 }

+// AnysSet checks bit in mask to see if any provided bit is set to one


Nit: /AnysSet/AnySet

Previously, it was possible for reusable CPUs and reusable devices (i.e. those previously consumed by init containers) to not be reused by subsequent init containers or app containers if the TopologyManager was enabled. This would happen because hint generation for the TopologyManager was not considering the reusable devices when it made its hint calculation. As such, it would sometimes: 1) Generate a hint for a differnent NUMA node, causing the CPUs and devices to be allocated from that node instead of the one where the reusable devices live; or 2) End up thinking there were not enough CPUs or devices to allocate and throw a TopologyAffinity admission error This patch fixes this by ensuring that reusable CPUs and devices are considered as part of TopologyHint generation. This frunctionality is difficult to unit test since it spans multiple components, but an e2e test will be added in a subsequent patch to test this functionality.

nolancon · 2020-07-20T11:43:35Z

/lgtm

klueska · 2020-07-20T21:56:42Z

/retest

fejta-bot · 2020-07-21T02:23:38Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

liggitt · 2020-07-21T03:26:59Z

/retest

klueska · 2020-07-21T05:16:22Z

/retest

…9-upstream-release-1.18 Automated cherry pick of #93189: Add AnySet() to topologymanager bitmask API

… being honored xref https://bugzilla.redhat.com/show_bug.cgi?id=1868634 xref kubernetes#93189 Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

k8s-ci-robot requested review from rohitagarwal003 and vikaschoudhary16 July 17, 2020 14:59

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 17, 2020

k8s-ci-robot added this to the v1.19 milestone Jul 17, 2020

k8s-ci-robot assigned derekwaynecarr Jul 17, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2020

This was referenced Jul 17, 2020

Automated cherry pick of #93189: Fix a bug whereby reusable CPUs and devices were not being #93190

Closed

Automated cherry pick of #93189: Fix a bug whereby reusable CPUs and devices were not being #93191

Closed

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 17, 2020

klueska force-pushed the upstream-fix-bug-topology-manager branch from 5bd96a2 to b101e89 Compare July 18, 2020 12:59

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 18, 2020

klueska force-pushed the upstream-fix-bug-topology-manager branch from b101e89 to 6a3b5c0 Compare July 18, 2020 19:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 18, 2020

klueska force-pushed the upstream-fix-bug-topology-manager branch from 6a3b5c0 to 5ba6f81 Compare July 18, 2020 20:32

nolancon reviewed Jul 20, 2020

View reviewed changes

klueska added 3 commits July 20, 2020 11:41

Add AnySet() to topologymanager bitmask API

9f5f401

Simplify logic in devicemanager TopologyHint generation

74fe936

klueska force-pushed the upstream-fix-bug-topology-manager branch from 5ba6f81 to 00df26a Compare July 20, 2020 11:41

k8s-ci-robot assigned nolancon Jul 20, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2020

k8s-ci-robot merged commit b6174e6 into kubernetes:master Jul 21, 2020

klueska mentioned this pull request Jul 21, 2020

Automated cherry pick of #93189: Add AnySet() to topologymanager bitmask API #93289

Merged

github-actions bot mentioned this pull request Jul 27, 2020

Week Ending July 26, 2020 dev-obs/actus#198

Open

k8s-ci-robot added a commit that referenced this pull request Aug 4, 2020

Merge pull request #93289 from klueska/automated-cherry-pick-of-#9318…

62f15e4

…9-upstream-release-1.18 Automated cherry pick of #93189: Add AnySet() to topologymanager bitmask API

cynepco3hahue mentioned this pull request Aug 13, 2020

BUG 1868634: UPSTREAM: 93189: Fix a bug whereby reusable CPUs and devices were not being honored openshift/kubernetes#315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug whereby reusable CPUs and devices were not being honored #93189

Fix a bug whereby reusable CPUs and devices were not being honored #93189

klueska commented Jul 17, 2020 •

edited

derekwaynecarr commented Jul 17, 2020

k8s-ci-robot commented Jul 17, 2020

derekwaynecarr commented Jul 17, 2020

derekwaynecarr commented Jul 17, 2020

fejta-bot commented Jul 18, 2020

fejta-bot commented Jul 18, 2020

k8s-ci-robot commented Jul 18, 2020

nolancon left a comment

nolancon Jul 20, 2020

klueska Jul 20, 2020

nolancon Jul 20, 2020

klueska Jul 20, 2020

nolancon commented Jul 20, 2020

klueska commented Jul 20, 2020

fejta-bot commented Jul 21, 2020

liggitt commented Jul 21, 2020

klueska commented Jul 21, 2020

Fix a bug whereby reusable CPUs and devices were not being honored #93189

Fix a bug whereby reusable CPUs and devices were not being honored #93189

Conversation

klueska commented Jul 17, 2020 • edited

derekwaynecarr commented Jul 17, 2020

k8s-ci-robot commented Jul 17, 2020

derekwaynecarr commented Jul 17, 2020

derekwaynecarr commented Jul 17, 2020

fejta-bot commented Jul 18, 2020

fejta-bot commented Jul 18, 2020

k8s-ci-robot commented Jul 18, 2020

nolancon left a comment

Choose a reason for hiding this comment

nolancon Jul 20, 2020

Choose a reason for hiding this comment

klueska Jul 20, 2020

Choose a reason for hiding this comment

nolancon Jul 20, 2020

Choose a reason for hiding this comment

klueska Jul 20, 2020

Choose a reason for hiding this comment

nolancon commented Jul 20, 2020

klueska commented Jul 20, 2020

fejta-bot commented Jul 21, 2020

liggitt commented Jul 21, 2020

klueska commented Jul 21, 2020

klueska commented Jul 17, 2020 •

edited