memory manager: specify the container cpuset.memory during the creation #98924

cynepco3hahue · 2021-02-09T17:15:53Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Set the container cpuset.memory during the creation and avoid an additional
call the resources update of the container.
Following the PR - #98019

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

Signed-off-by: Artyom Lukianov alukiano@redhat.com

k8s-ci-robot · 2021-02-09T17:16:01Z

@cynepco3hahue: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2021-02-09T17:16:02Z

Hi @cynepco3hahue. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

cynepco3hahue · 2021-02-09T17:16:09Z

/assign @klueska

kikisdeliveryservice · 2021-02-11T03:07:02Z

/ok-to-test
triage/accepted

cynepco3hahue · 2021-02-11T09:49:11Z

/retest

klueska · 2021-03-02T13:00:56Z

pkg/kubelet/cm/memorymanager/memory_manager.go


+// GetMemory provides NUMA nodes that used to allocate the container memory
+func (m *manager) GetMemoryNUMANodes(pod *v1.Pod, container *v1.Container) string {


It feels odd to have this return a string. I think (from an abstraction perspective) it would be better to return an []int and do the conversion to a string at its point of use.

Or better yet, a sets.Int to avoid the need for tmpNodes below.

good point!

klueska · 2021-03-02T13:08:13Z

pkg/kubelet/cm/memorymanager/memory_manager.go

@@ -82,6 +81,9 @@ type Manager interface {
 	// and is consulted to achieve NUMA aware resource alignment among this
 	// and other resource controllers.
 	GetPodTopologyHints(*v1.Pod) map[string][]topologymanager.TopologyHint
+
+	// GetMemory provides NUMA nodes that used to allocate the container memory


nit: s/nodes that used/nodes that are used/

klueska · 2021-03-02T13:08:26Z

pkg/kubelet/cm/memorymanager/memory_manager.go


+// GetMemory provides NUMA nodes that used to allocate the container memory


nit: s/nodes that used/nodes that are used/

klueska · 2021-03-02T14:43:48Z

pkg/kubelet/cm/internal_container_lifecycle_linux.go

@@ -31,5 +34,16 @@ func (i *internalContainerLifecycleImpl) PreCreateContainer(pod *v1.Pod, contain
 		}
 	}

+	if i.memoryManager != nil {
+		numaNodes := i.memoryManager.GetMemoryNUMANodes(pod, container)
+		if len(numaNodes) > 0 {


It's probably better to use numaNodes.Len() here incase the underlying abstraction ever changes.

klueska · 2021-03-02T14:45:10Z

pkg/kubelet/cm/memorymanager/memory_manager.go

-	if len(nodes) < 1 {
-		klog.V(5).Infof("[memorymanager] update container resources is skipped due to memory blocks are empty")
-		return nil
+	if len(numaNodes) < 1 {


I would just do == 0 instead of < 1 here.
I would also explicitly return nil, to make it obvious that this is the exceptional case with no elements.

Set the container cpuset.memory during the creation and avoid an additional call to the resources update of the container. Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

klueska · 2021-03-02T15:33:07Z

/approve

k8s-ci-robot · 2021-03-02T15:33:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cynepco3hahue, 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/OWNERS~~ [klueska]

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

klueska · 2021-03-02T15:35:39Z

Let me know when you get tests passing and I'll back through and do one more pass and add an LGTM

cynepco3hahue · 2021-03-02T16:01:36Z

@klueska A lot of thanks, I am waiting for the PR #98572 to be merged.

cynepco3hahue · 2021-03-02T17:13:15Z

/retest

cynepco3hahue · 2021-03-02T17:13:55Z

/test pull-kubernetes-node-kubelet-serial-memory-manager

cynepco3hahue · 2021-03-02T18:02:20Z

/test pull-kubernetes-node-kubelet-serial-memory-manager

cynepco3hahue · 2021-03-02T18:16:21Z

/test pull-kubernetes-node-kubelet-serial-memory-manager

klueska · 2021-03-03T09:32:18Z

/lgtm

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 9, 2021

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 9, 2021

k8s-ci-robot assigned klueska Feb 9, 2021

k8s-ci-robot requested review from odinuge and Random-Liu February 9, 2021 17:16

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 Feb 9, 2021

ehashman added this to Triage in SIG Node PR Triage Feb 9, 2021

cynepco3hahue mentioned this pull request Feb 10, 2021

Define the pinned CPU and memory during the container creation mgjeong/kubernetes#37

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 11, 2021

kikisdeliveryservice 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 Feb 11, 2021

kikisdeliveryservice moved this from Triage to Needs Reviewer in SIG Node PR Triage Feb 11, 2021

kikisdeliveryservice added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Feb 11, 2021

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 11, 2021

cynepco3hahue force-pushed the set_container_memory_during_creation branch from 99ef69f to 92ddcf7 Compare February 15, 2021 14:10

cynepco3hahue force-pushed the set_container_memory_during_creation branch from 92ddcf7 to 92859c1 Compare March 2, 2021 10:51

klueska reviewed Mar 2, 2021

View reviewed changes

cynepco3hahue force-pushed the set_container_memory_during_creation branch from 92859c1 to f1d352a Compare March 2, 2021 14:31

klueska reviewed Mar 2, 2021

View reviewed changes

memory manager: specify the container cpuset.memory during the creation

95b2777

Set the container cpuset.memory during the creation and avoid an additional call to the resources update of the container. Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

cynepco3hahue force-pushed the set_container_memory_during_creation branch from f1d352a to 95b2777 Compare March 2, 2021 15:02

klueska approved these changes Mar 2, 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 Mar 2, 2021

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

k8s-ci-robot merged commit ad80c76 into kubernetes:master Mar 3, 2021

SIG Node PR Triage automation moved this from Needs Reviewer to Done Mar 3, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memory manager: specify the container cpuset.memory during the creation #98924

memory manager: specify the container cpuset.memory during the creation #98924

cynepco3hahue commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

cynepco3hahue commented Feb 9, 2021

kikisdeliveryservice commented Feb 11, 2021

cynepco3hahue commented Feb 11, 2021

klueska Mar 2, 2021

cynepco3hahue Mar 2, 2021

cynepco3hahue Mar 2, 2021

klueska Mar 2, 2021

cynepco3hahue Mar 2, 2021

klueska Mar 2, 2021

cynepco3hahue Mar 2, 2021

klueska Mar 2, 2021

cynepco3hahue Mar 2, 2021

klueska Mar 2, 2021

cynepco3hahue Mar 2, 2021

klueska commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

klueska commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

klueska commented Mar 3, 2021


		// GetMemory provides NUMA nodes that used to allocate the container memory
		func (m manager) GetMemoryNUMANodes(pod v1.Pod, container *v1.Container) string {

memory manager: specify the container cpuset.memory during the creation #98924

memory manager: specify the container cpuset.memory during the creation #98924

Conversation

cynepco3hahue commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

cynepco3hahue commented Feb 9, 2021

kikisdeliveryservice commented Feb 11, 2021

cynepco3hahue commented Feb 11, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klueska commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

klueska commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

cynepco3hahue commented Mar 2, 2021

klueska commented Mar 3, 2021