Add feature gate and kubelet flags for Topology Manager #74411

nolancon · 2019-02-22T11:46:56Z

What this PR does / why we need it:
Create feature gate for Topology Manager and add Kubelet flags.
Topology Manager based merged design proposal here: kubernetes/community#1680
Issue for tracking PRs: #72828

What type of PR is this?

/kind feature

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-02-22T11:47:04Z

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

nolancon · 2019-02-22T11:48:56Z

@lmdaly
/cc @ConnorDoyle @balajismaniam

fejta-bot · 2019-02-22T12:26:23Z

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.

dashpole · 2019-03-09T01:16:52Z

pkg/features/kube_features.go

@@ -167,6 +167,12 @@ const (
 	//
 	// Enable pods to consume pre-allocated huge pages of varying page sizes
 	HugePages utilfeature.Feature = "HugePages"
+
+        // owner: @lmdaly
+	// alpha: v1.13


If we are just adding this now, this should be 1.14/1.15, right?

Changed to 1.15

dashpole · 2019-03-09T01:17:25Z

pkg/kubelet/kubelet.go

-
-	if utilfeature.DefaultFeatureGate.Enabled(features.NodeLease) {
-		klet.nodeLeaseController = nodelease.NewController(klet.clock, klet.heartbeatClient, string(klet.nodeName), kubeCfg.NodeLeaseDurationSeconds, klet.onRepeatedHeartbeatFailure)
+


nit: remove whitespace changes.

lmdaly · 2019-05-28T09:14:05Z

@dashpole could you ok-to-test this pr?

klueska · 2019-05-28T12:26:14Z

pkg/kubelet/kubelet.go

@@ -852,7 +852,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 	}
 	klet.AddPodSyncLoopHandler(activeDeadlineHandler)
 	klet.AddPodSyncHandler(activeDeadlineHandler)
-
+	klet.admitHandlers.AddPodAdmitHandler(klet.containerManager.GetTopologyPodAdmitHandler())


I would probably add this line as part of #74357 so that this current PR can be merged independent of all other PRs in this chain.

klueska · 2019-05-28T12:27:33Z

pkg/kubelet/apis/config/v1beta1/defaults.go

@@ -149,6 +149,9 @@ func SetDefaults_KubeletConfiguration(obj *kubeletconfigv1beta1.KubeletConfigura
 		// Keep the same as default NodeStatusUpdateFrequency
 		obj.CPUManagerReconcilePeriod = metav1.Duration{Duration: 10 * time.Second}
 	}
+	if obj.TopologyManagerPolicy == "" {
+		obj.TopologyManagerPolicy = "Preferred"


Is it OK that "Preferred" is capitalized here, but is lower-case in the TopologyManager itself? Does it get normalized to lower-case somewhere?

Doesn't seem to make a difference, but changed anyway for consistency.

dashpole · 2019-05-28T16:23:54Z

/ok-to-test

lmdaly · 2019-05-30T23:06:34Z

@liggitt the policy is passed into the New Topology Manager func where it gets checked.
https://github.com/kubernetes/kubernetes/pull/74357/files#diff-06aa1b61a9bc4a50aa8d8b0a69d95bb2R289 - passed in
and checked here https://github.com/kubernetes/kubernetes/pull/73580/files#diff-a19f204f56e91b0871491b88c0e0b139R71

Is there additional check we should be doing before this?

liggitt · 2019-05-31T01:39:54Z

Is there additional check we should be doing before this?

func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) validates a config object and should verify the specified option is valid.

does the default preferred value map to the current implicit behavior, or is the entire idea of policies new?

liggitt · 2019-05-31T01:43:08Z

if the TopologyManager feature is disabled, should that prevent someone from setting a policy or from setting a particular policy like strict? we don't want to let people set things in config that are silently ignored, so I'd expect something along the lines of this:

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

Lines 108 to 113 in 990695c

    
           if kc.RotateCertificates && !localFeatureGate.Enabled(features.RotateKubeletClientCertificate) { 
        
           	allErrors = append(allErrors, fmt.Errorf("invalid configuration: RotateCertificates %v requires feature gate RotateKubeletClientCertificate", kc.RotateCertificates)) 
        
           } 
        
           if kc.ServerTLSBootstrap && !localFeatureGate.Enabled(features.RotateKubeletServerCertificate) { 
        
           	allErrors = append(allErrors, fmt.Errorf("invalid configuration: ServerTLSBootstrap %v requires feature gate RotateKubeletServerCertificate", kc.ServerTLSBootstrap)) 
        
           }

lmdaly · 2019-05-31T10:07:09Z

@liggitt Will add the check for the policy and feature gate setting. The policies are a new concept and give additional functionality to resource allocation for cpu and devices

kad · 2019-05-31T15:16:58Z

pkg/kubelet/apis/config/v1beta1/defaults.go

@@ -149,6 +149,9 @@ func SetDefaults_KubeletConfiguration(obj *kubeletconfigv1beta1.KubeletConfigura
 		// Keep the same as default NodeStatusUpdateFrequency
 		obj.CPUManagerReconcilePeriod = metav1.Duration{Duration: 10 * time.Second}
 	}
+	if obj.TopologyManagerPolicy == "" {
+		obj.TopologyManagerPolicy = "preferred"


shouldn't we have default policy 'none' - meaning, "do not do anything, work as older version of kubelet, even if the feature gate is enabled" ?

@lmdaly:
The policies are a new concept and give additional functionality to resource allocation for cpu and devices

I agree with @kad that the default value of the config field should preserve current behavior

nolancon · 2019-06-24T11:47:53Z

@liggitt @kad this PR has been updated - Topology Manager default policy is now "none". None policy will do nothing and behave as before, even if feature gate is enabled.

liggitt · 2019-06-28T17:53:14Z

pkg/kubelet/apis/config/v1beta1/defaults.go

@@ -149,6 +149,9 @@ func SetDefaults_KubeletConfiguration(obj *kubeletconfigv1beta1.KubeletConfigura
 		// Keep the same as default NodeStatusUpdateFrequency
 		obj.CPUManagerReconcilePeriod = metav1.Duration{Duration: 10 * time.Second}
 	}
+	if obj.TopologyManagerPolicy == "" {
+		obj.TopologyManagerPolicy = "none"


nit, define constants for the allowed values (in the same file as the config type) and use them here, in validation, and in fuzzing

liggitt · 2019-06-28T17:53:52Z

staging/src/k8s.io/kubelet/config/v1beta1/types.go

@@ -409,6 +409,13 @@ type KubeletConfiguration struct {
 	// Default: "10s"
 	// +optional
 	CPUManagerReconcilePeriod metav1.Duration `json:"cpuManagerReconcilePeriod,omitempty"`
+	// TopologyManagerPolicy is the name of the policy to use.
+	// Requires the TopologyManager feature gate to be enabled.


"Policies other than none require the TopologyManager feature gate to be enabled."

liggitt · 2019-06-28T18:03:47Z

thanks, one nit on constants, one on doc, lgtm otherwise. please squash before merge

Nit: remove capitalization of preferred Remove line from kubelet and add to separate PR for easier merge nit: dependency added to separate PR Add check to ensure strict policy cannot be set without feature gate enabled Topology Manager runs "none" policy by default. Added constants for policies and updated documentation.

nolancon · 2019-07-02T16:05:21Z

/retest

liggitt · 2019-07-03T18:27:50Z

/lgtm
/approve

k8s-ci-robot · 2019-07-03T18:28:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt, nolancon

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/kubeadm/OWNERS~~ [liggitt]
~~cmd/kubelet/OWNERS~~ [derekwaynecarr,liggitt]
~~pkg/features/OWNERS~~ [derekwaynecarr,liggitt]
~~pkg/kubelet/apis/config/OWNERS~~ [derekwaynecarr,liggitt]
~~staging/src/k8s.io/kubelet/config/OWNERS~~ [liggitt]

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

fejta-bot · 2019-07-03T22:15:09Z

/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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2019

k8s-ci-robot requested review from brendandburns and dashpole February 22, 2019 11:47

k8s-ci-robot requested review from balajismaniam and ConnorDoyle February 22, 2019 11:48

lmdaly mentioned this pull request Mar 4, 2019

Topology Manager Tracking Issue #72828

Closed

dashpole reviewed Mar 9, 2019

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 11, 2019

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 13, 2019

klueska reviewed May 28, 2019

View reviewed changes

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 28, 2019

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

lmdaly mentioned this pull request May 31, 2019

Updates to container manager and internal container lifecycle to accommodate TopologyManager #74357

Merged

kad reviewed May 31, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2019

nolancon mentioned this pull request Jun 24, 2019

Add Policy None for Topology Manager #79343

Merged

liggitt reviewed Jun 28, 2019

View reviewed changes

k8s-ci-robot assigned liggitt Jul 3, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2019

k8s-ci-robot merged commit bbef01a into kubernetes:master Jul 3, 2019

klueska mentioned this pull request Jul 18, 2019

REQUEST: New membership for klueska kubernetes/org#1018

Closed

6 tasks

liggitt added this to the v1.16 milestone Aug 6, 2019

nolancon mentioned this pull request Aug 26, 2019

REQUEST: New membership for nolancon kubernetes/org#1138

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature gate and kubelet flags for Topology Manager #74411

Add feature gate and kubelet flags for Topology Manager #74411

nolancon commented Feb 22, 2019

k8s-ci-robot commented Feb 22, 2019

nolancon commented Feb 22, 2019

fejta-bot commented Feb 22, 2019

dashpole Mar 9, 2019

nolancon Mar 11, 2019

dashpole Mar 9, 2019

nolancon Mar 11, 2019

lmdaly commented May 28, 2019

klueska May 28, 2019

klueska May 28, 2019

nolancon May 29, 2019

dashpole commented May 28, 2019

lmdaly commented May 30, 2019 •

edited

liggitt commented May 31, 2019

liggitt commented May 31, 2019

lmdaly commented May 31, 2019

kad May 31, 2019

liggitt May 31, 2019

nolancon commented Jun 24, 2019 •

edited

liggitt Jun 28, 2019

liggitt Jun 28, 2019

liggitt commented Jun 28, 2019

nolancon commented Jul 2, 2019

liggitt commented Jul 3, 2019

k8s-ci-robot commented Jul 3, 2019

fejta-bot commented Jul 3, 2019


		if utilfeature.DefaultFeatureGate.Enabled(features.NodeLease) {
		klet.nodeLeaseController = nodelease.NewController(klet.clock, klet.heartbeatClient, string(klet.nodeName), kubeCfg.NodeLeaseDurationSeconds, klet.onRepeatedHeartbeatFailure)

Add feature gate and kubelet flags for Topology Manager #74411

Add feature gate and kubelet flags for Topology Manager #74411

Conversation

nolancon commented Feb 22, 2019

k8s-ci-robot commented Feb 22, 2019

nolancon commented Feb 22, 2019

fejta-bot commented Feb 22, 2019

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

lmdaly commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented May 28, 2019

lmdaly commented May 30, 2019 • edited

liggitt commented May 31, 2019

liggitt commented May 31, 2019

lmdaly commented May 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolancon commented Jun 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jun 28, 2019

nolancon commented Jul 2, 2019

liggitt commented Jul 3, 2019

k8s-ci-robot commented Jul 3, 2019

fejta-bot commented Jul 3, 2019

lmdaly commented May 30, 2019 •

edited

nolancon commented Jun 24, 2019 •

edited