Topology Manager Scope (container | pod) Feature #92967

cezaryzukowski · 2020-07-10T16:16:54Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
To enable pod-level resource affinity (details in the update to the KEP).

Special notes for your reviewer:
Add support for pod-level affinity next to the existing container-level affinity. Add the "--topology-manager-scope" flag to the kubelet binary and the "topologyManagerScope" field in the kubelet v1beta1 configuration. The value for both can be either "pod" or "container" (defaults to "container").

Does this PR introduce a user-facing change?:

New flag is introduced, i.e. --topology-manager-scope=container|pod. 
The default value is the "container" scope.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
[KEP]: kubernetes/enhancements#1752

k8s-ci-robot · 2020-07-10T16:17:03Z

Welcome @cezaryzukowski!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-07-10T16:17:03Z

Hi @cezaryzukowski. 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.

fejta-bot · 2020-07-10T17:04:29Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

k-wiatrzyk · 2020-07-13T09:21:12Z

/sig node

k-wiatrzyk · 2020-07-14T10:09:20Z

/cc @klueska

SergeyKanzhelev · 2020-07-15T22:00:27Z

cmd/kubelet/app/options/options.go

@@ -530,6 +530,8 @@ func AddKubeletConfigFlags(mainfs *pflag.FlagSet, c *kubeletconfig.KubeletConfig
 	fs.Int32Var(&c.PodsPerCore, "pods-per-core", c.PodsPerCore, "Number of Pods per core that can run on this Kubelet. The total number of Pods on this Kubelet cannot exceed max-pods, so max-pods will be used if this calculation results in a larger number of Pods allowed on the Kubelet. A value of 0 disables this limit.")
 	fs.BoolVar(&c.ProtectKernelDefaults, "protect-kernel-defaults", c.ProtectKernelDefaults, "Default kubelet behaviour for kernel tuning. If set, kubelet errors if any of kernel tunables is different than kubelet defaults.")
 	fs.StringVar(&c.ReservedSystemCPUs, "reserved-cpus", c.ReservedSystemCPUs, "A comma-separated list of CPUs or CPU ranges that are reserved for system and kubernetes usage. This specific list will supersede cpu counts in --system-reserved and --kube-reserved.")
+	fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container', 'pod'.")


mention default value:

Suggested change

fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container', 'pod'.")

fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container' (default), 'pod'.")

SergeyKanzhelev · 2020-07-15T22:10:17Z

cmd/kubelet/app/options/options.go

@@ -530,6 +530,8 @@ func AddKubeletConfigFlags(mainfs *pflag.FlagSet, c *kubeletconfig.KubeletConfig
 	fs.Int32Var(&c.PodsPerCore, "pods-per-core", c.PodsPerCore, "Number of Pods per core that can run on this Kubelet. The total number of Pods on this Kubelet cannot exceed max-pods, so max-pods will be used if this calculation results in a larger number of Pods allowed on the Kubelet. A value of 0 disables this limit.")
 	fs.BoolVar(&c.ProtectKernelDefaults, "protect-kernel-defaults", c.ProtectKernelDefaults, "Default kubelet behaviour for kernel tuning. If set, kubelet errors if any of kernel tunables is different than kubelet defaults.")
 	fs.StringVar(&c.ReservedSystemCPUs, "reserved-cpus", c.ReservedSystemCPUs, "A comma-separated list of CPUs or CPU ranges that are reserved for system and kubernetes usage. This specific list will supersede cpu counts in --system-reserved and --kube-reserved.")
+	fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container', 'pod'.")


this sentence is really hard to read and parse. Try to rephrase. Maybe like this:

Suggested change

fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container', 'pod'.")

fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Scope to which topology hints applied. Topology Manager collects hints from Hint Providers and applies them to defined scope to ensure the pod admission. Possible values: 'container' (default), 'pod'.")

SergeyKanzhelev · 2020-07-15T22:13:02Z

pkg/kubelet/apis/config/types.go

@@ -67,6 +67,12 @@ const (
 	// SingleNumaNodeTopologyManager Policy iis a mode in which kubelet only allows


unrelated to this PR, but mentioning anyway:

Suggested change

// SingleNumaNodeTopologyManager Policy iis a mode in which kubelet only allows

// SingleNumaNodeTopologyManager Policy is a mode in which kubelet only allows

SergeyKanzhelev · 2020-07-15T22:16:15Z

pkg/kubelet/apis/config/validation/validation.go

@@ -117,6 +117,9 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error
 	if kc.TopologyManagerPolicy != kubeletconfig.NoneTopologyManagerPolicy && !localFeatureGate.Enabled(features.TopologyManager) {
 		allErrors = append(allErrors, fmt.Errorf("invalid configuration: TopologyManager %v requires feature gate TopologyManager", kc.TopologyManagerPolicy))
 	}
+	if kc.TopologyManagerScope != kubeletconfig.ContainerScopeTopology && !localFeatureGate.Enabled(features.TopologyManager) {
+		allErrors = append(allErrors, fmt.Errorf("invalid configuration: TopologyScope %v requires feature gate TopologyManager", kc.TopologyManagerScope))


Suggested change

allErrors = append(allErrors, fmt.Errorf("invalid configuration: TopologyScope %v requires feature gate TopologyManager", kc.TopologyManagerScope))

allErrors = append(allErrors, fmt.Errorf("invalid configuration: TopologyManagerScope %v requires feature gate TopologyManager", kc.TopologyManagerScope))

SergeyKanzhelev · 2020-07-15T22:18:46Z

pkg/kubelet/apis/config/types.go

@@ -221,6 +227,12 @@ type KubeletConfiguration struct {
 	// TopologyManagerPolicy is the name of the policy to use.
 	// Policies other than "none" require the TopologyManager feature gate to be enabled.
 	TopologyManagerPolicy string
+	// Topology Manager Scope represents the scope of topology hint generation
+	// that topology manager requests and hint providers generates.


Suggested change

// that topology manager requests and hint providers generates.

// that topology manager requests and hint providers generate.

SergeyKanzhelev · 2020-07-15T22:19:17Z

pkg/kubelet/cm/cpumanager/cpu_manager.go

@@ -77,6 +77,11 @@ type Manager interface {
 	// and is consulted to achieve NUMA aware resource alignment among this
 	// and other resource controllers.
 	GetTopologyHints(*v1.Pod, *v1.Container) map[string][]topologymanager.TopologyHint
+
+	// GetTopologyHints implements the topologymanager. HintProvider Interface


name in comment should match the name of the method

Suggested change

// GetTopologyHints implements the topologymanager. HintProvider Interface

// GetPodLevelTopologyHints implements the topologymanager.HintProvider Interface

Changed for GetPodTopologyHints

SergeyKanzhelev · 2020-07-15T22:21:48Z

pkg/kubelet/cm/cpumanager/policy.go

@@ -34,4 +34,8 @@ type Policy interface {
 	// and is consulted to achieve NUMA aware resource alignment among this
 	// and other resource controllers.
 	GetTopologyHints(s state.State, pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint
+	// GetTopologyHints implements the topologymanager. HintProvider Interface


name should match

Suggested change

// GetTopologyHints implements the topologymanager. HintProvider Interface

// GetPodLevelTopologyHints implements the topologymanager.HintProvider Interface

Changed for GetPodTopologyHints

SergeyKanzhelev · 2020-07-15T22:25:14Z

pkg/kubelet/cm/devicemanager/topology_hints.go

@@ -78,6 +78,12 @@ func (m *ManagerImpl) GetTopologyHints(pod *v1.Pod, container *v1.Container) map
 	return deviceHints
 }

+// GetPodLevelTopologyHints implements the TopologyManager HintProvider Interface which


Suggested change

// GetPodLevelTopologyHints implements the TopologyManager HintProvider Interface which

// GetPodLevelTopologyHints implements the topologymanager.HintProvider Interface which

SergeyKanzhelev · 2020-07-15T22:28:08Z

pkg/kubelet/cm/topologymanager/topology_manager.go

@@ -37,6 +37,17 @@ const (
 	// present on a machine and the TopologyManager is enabled, an error will
 	// be returned and the TopologyManager will not be loaded.
 	maxAllowableNUMANodes = 8
+	// containerScopeTopology specifies the TopologyManagerScope per container.
+	containerScopeTopology = "container"


why is it scopetopology, not topologyscope?

Suggested change

containerScopeTopology = "container"

containerTopologyScope = "container"

I'd also question dropping the Manager from the name. Why change terminology? Is the shorter name better here?

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

* Add podDevices() func * Add getPodDeviceRequest() func Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

* Extract common tests cases that will be used for both GetTopologyHints() and GetPodTopologyHints() * Extract machineInfo as it will be used for both functions as well Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

* Add tests for getPodRequestedCPU() Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Pod object is more flexible to use and construct * Update TestGetTopologyHints() to work according to new test cases * Update topologyHintTestCase{} to include proper field Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

* Add additional test cases returned by getPodScopeTestCases() Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

A suite of e2e tests was created for Topology Manager so as to test pod scope alignment feature. Co-authored-by: Pawel Rapacz <p.rapacz@partner.samsung.com> Co-authored-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com> Signed-off-by: Cezary Zukowski <c.zukowski@samsung.com>

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

k8s-ci-robot · 2020-11-12T11:27:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~cmd/kubelet/OWNERS~~ [derekwaynecarr,smarterclayton]
~~pkg/kubelet/apis/config/OWNERS~~ [smarterclayton]
~~pkg/kubelet/cm/OWNERS~~ [derekwaynecarr,klueska,smarterclayton]
~~staging/src/k8s.io/kubelet/config/OWNERS~~ [smarterclayton]
~~test/e2e_node/OWNERS~~ [derekwaynecarr,klueska,smarterclayton]

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

k-wiatrzyk · 2020-11-12T12:00:18Z

/test pull-kubernetes-e2e-kind-ipv6

klueska · 2020-11-12T12:07:33Z

/lgtm

k8s-ci-robot · 2020-11-12T12:16:02Z

@cezaryzukowski: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-storage-snapshot	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-gce-storage-snapshot`
pull-kubernetes-e2e-gce-storage-slow	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-gce-storage-slow`
pull-kubernetes-e2e-azure-disk-windows	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-azure-disk-windows`
pull-kubernetes-e2e-aks-engine-azure	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-aks-engine-azure`
pull-kubernetes-e2e-azure-file	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-azure-file`
pull-kubernetes-e2e-azure-file-windows	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-azure-file-windows`
pull-kubernetes-e2e-gce-csi-serial	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-gce-csi-serial`
pull-kubernetes-e2e-azure-disk	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-azure-disk`
pull-kubernetes-e2e-azure-disk-vmss	80b891f9a734e46dc525393123110fb787f37ac3	link	`/test pull-kubernetes-e2e-azure-disk-vmss`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k-wiatrzyk · 2020-11-12T12:17:32Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2020

k8s-ci-robot requested review from pwittrock and sjenning July 10, 2020 16:17

k-wiatrzyk force-pushed the tm-scope branch 2 times, most recently from 97d9b81 to c237f8f Compare July 14, 2020 09:49

k8s-ci-robot requested a review from klueska July 14, 2020 10:09

SergeyKanzhelev reviewed Jul 15, 2020

View reviewed changes

swhan91 and others added 12 commits November 12, 2020 12:25

Implement the cpumanager.GetPodTopologyHints() function

27b7bcb

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Implement devicemanager.GetPodLevelTopologyHints() function

9da0912

* Add podDevices() func * Add getPodDeviceRequest() func Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Update topologymanager tests after adding scopes

f5c0fe4

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Move common tests from topologymanager under scope

c786c9a

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Move scope specific tests from topologymanager under particular scopes

656a08a

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Refactor topology hints tests for cpumanager

35b1f28

* Extract common tests cases that will be used for both GetTopologyHints() and GetPodTopologyHints() * Extract machineInfo as it will be used for both functions as well Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Add tests for GetPodTopologyHints() for cpumanager

7ad65bf

* Add tests for getPodRequestedCPU() Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Add tests for GetPodTopologyHints() for devicemanager

ba1e8ab

* Add additional test cases returned by getPodScopeTestCases() Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Add tests for getPodDeviceRequest() for devicemanager

7b0ccaa

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

Run ./update-all.sh

b771491

Signed-off-by: Krzysztof Wiatrzyk <k.wiatrzyk@samsung.com>

k-wiatrzyk force-pushed the tm-scope branch from f8bf54e to b771491 Compare November 12, 2020 11:26

This comment has been minimized.

Sign in to view

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

k8s-ci-robot merged commit 75463bd into kubernetes:master Nov 12, 2020

This was referenced Dec 2, 2020

Node memory and cpu capacity inside limited LXD container detected wrong #92638

Closed

Node cpu capacity inside limited LXD container detected wrong in 1.19 #97066

Closed

liggitt moved this from Assigned to API review completed, 1.20 in API Reviews Jan 20, 2021

klueska mentioned this pull request Feb 9, 2021

Add klueska as an approver in pkg/kubelet/OWNERS #98932

Merged

swatisehgal mentioned this pull request Nov 25, 2022

Exposing TopologyManagerScope as a separate field k8stopologyawareschedwg/noderesourcetopology-api#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topology Manager Scope (container | pod) Feature #92967

Topology Manager Scope (container | pod) Feature #92967

cezaryzukowski commented Jul 10, 2020 •

edited

Loading

k8s-ci-robot commented Jul 10, 2020

k8s-ci-robot commented Jul 10, 2020

fejta-bot commented Jul 10, 2020

k-wiatrzyk commented Jul 13, 2020

k-wiatrzyk commented Jul 14, 2020

SergeyKanzhelev Jul 15, 2020 •

edited

Loading

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020 •

edited

Loading

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020

SergeyKanzhelev Jul 15, 2020

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020 •

edited

Loading

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020

k-wiatrzyk Jul 24, 2020

SergeyKanzhelev Jul 15, 2020

k-wiatrzyk Jul 24, 2020

k8s-ci-robot commented Nov 12, 2020

This comment has been minimized.

k-wiatrzyk commented Nov 12, 2020

klueska commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020 •

edited

Loading

k-wiatrzyk commented Nov 12, 2020

	fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container', 'pod'.")
	fs.StringVar(&c.TopologyManagerScope, "topology-manager-scope", c.TopologyManagerScope, "Topology Manager Scope represents the scope of topology hint generation that topology manager requests and hint providers generates. Possible values: 'container' (default), 'pod'.")

		@@ -67,6 +67,12 @@ const (
		// SingleNumaNodeTopologyManager Policy iis a mode in which kubelet only allows

	allErrors = append(allErrors, fmt.Errorf("invalid configuration: TopologyScope %v requires feature gate TopologyManager", kc.TopologyManagerScope))
	allErrors = append(allErrors, fmt.Errorf("invalid configuration: TopologyManagerScope %v requires feature gate TopologyManager", kc.TopologyManagerScope))

	// that topology manager requests and hint providers generates.
	// that topology manager requests and hint providers generate.

	// GetTopologyHints implements the topologymanager. HintProvider Interface
	// GetPodLevelTopologyHints implements the topologymanager.HintProvider Interface

	containerScopeTopology = "container"
	containerTopologyScope = "container"

Topology Manager Scope (container | pod) Feature #92967

Topology Manager Scope (container | pod) Feature #92967

Conversation

cezaryzukowski commented Jul 10, 2020 • edited Loading

k8s-ci-robot commented Jul 10, 2020

k8s-ci-robot commented Jul 10, 2020

fejta-bot commented Jul 10, 2020

k-wiatrzyk commented Jul 13, 2020

k-wiatrzyk commented Jul 14, 2020

SergeyKanzhelev Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyKanzhelev Jul 15, 2020 • edited Loading

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

SergeyKanzhelev Jul 15, 2020 • edited Loading

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

k8s-ci-robot commented Nov 12, 2020

This comment has been minimized.

k-wiatrzyk commented Nov 12, 2020

klueska commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020 • edited Loading

k-wiatrzyk commented Nov 12, 2020

cezaryzukowski commented Jul 10, 2020 •

edited

Loading

SergeyKanzhelev Jul 15, 2020 •

edited

Loading

SergeyKanzhelev Jul 15, 2020 •

edited

Loading

SergeyKanzhelev Jul 15, 2020 •

edited

Loading

k8s-ci-robot commented Nov 12, 2020 •

edited

Loading