CPU manager phase 1 #49186

ConnorDoyle · 2017-07-19T07:35:30Z

What this PR does / why we need it:

Implements phases 1 and 2 of the CPU manager design proposal. Implements the none and static cpu pinning policies.

E2e tests are in progress and will be added in a subsequent PR.
User / operator docs for kubernetes/kubernetes.github.io are merged.

Release note:

TODO

Plan to break this into smaller PRs...

add UpdateContainerResources method to CRI (893 LOC) Add UpdateContainerResources method to CRI #46105
cpuset helpers and unit tests (632 LOC) Add cpuset helper library. #51132
cpu manager interfaces (238 LOC) CPU manager interfaces. #51140
flag, feature gate, wiring into Kubelet, none policy and unit tests CPU manager wiring and none policy #51357
Static policy, topology, takeByTopology and unit tests (1512 LOC) CPU manager static policy #51180
CPU manager node e2e tests. (375 LOC) Node e2e tests for the CPU Manager. #51041

Starting the following three pods in order on an 8-core single socket system:

cpuset-visualizer-exclusive-1

apiVersion: v1
kind: Pod
metadata:
  name: cpuset-visualizer-exclusive-1
spec:
  containers:
  - image: quay.io/connordoyle/cpuset-visualizer
    name: cpuset-visualizer-exclusive-1
    resources:
      requests:
        cpu: 1
        memory: "256M"
      limits:
        cpu: 1
        memory: "256M"

cpuset-visualizer-exclusive-3

apiVersion: v1
kind: Pod
metadata:
  name: cpuset-visualizer-exclusive-3
spec:
  containers:
  - image: quay.io/connordoyle/cpuset-visualizer
    name: cpuset-visualizer-exclusive-3
    resources:
      requests:
        cpu: 3
        memory: "256M"
      limits:
        cpu: 3
        memory: "256M"

cpuset-visualizer-exclusive-4

apiVersion: v1
kind: Pod
metadata:
  name: cpuset-visualizer-exclusive-4
spec:
  containers:
  - image: quay.io/connordoyle/cpuset-visualizer
    name: cpuset-visualizer-exclusive-4
    resources:
      requests:
        cpu: 3
        memory: "256M"
      limits:
        cpu: 3
        memory: "256M"

Gave the following allocations. Note that cpu 0 was not used for pods in this case, per kube-reserved and system-reserved as described in the proposal.

cc @sjenning @flyingcougar @nqn @teferi @vishh

k8s-ci-robot · 2017-07-19T07:35:39Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

k8s-github-robot · 2017-07-19T07:35:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ConnorDoyle
We suggest the following additional approvers: gmarek, vishh

Assign the PR to them by writing /assign @gmarek @vishh in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

ConnorDoyle · 2017-07-19T17:51:26Z

pkg/kubelet/apis/cri/v1alpha1/runtime/api.proto

+    // ID of the container to update.
+    string container_id = 1;
+    // Resource configuration specific to Linux containers.
+    LinuxContainerResources linux = 15;


Note: would make more sense for this to be 2.

ConnorDoyle · 2017-07-19T18:39:03Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -248,6 +251,9 @@ func (m *kubeGenericRuntimeManager) generateLinuxContainerConfig(container *v1.C
 		cpuShares = milliCPUToShares(cpuRequest.MilliValue())
 	}
 	lc.Resources.CpuShares = cpuShares
+	// SETH set initial cpuset here maybe?


TODO: kill comment?

ConnorDoyle · 2017-07-19T18:42:35Z

pkg/kubelet/cpumanager/policy_static.go

+		return cpuset.NewCPUSet(), fmt.Errorf("not enough cpus available to satisfy request")
+	}
+
+	// TODO(CD): Acquire CPUs topologically instead of sequentially...


TODO: delete these lines

ConnorDoyle · 2017-07-19T18:43:26Z

pkg/kubelet/kubelet.go

@@ -1241,6 +1257,9 @@ func (kl *Kubelet) initializeModules() error {
 	// Start resource analyzer
 	kl.resourceAnalyzer.Start()

+	// Step 9: Start the CPU manager


Nit: remove Step 9 prefix

ConnorDoyle · 2017-07-19T18:44:23Z

pkg/kubemark/hollow_kubelet.go

@@ -145,6 +145,7 @@ func GetHollowKubeletConfig(
 	c.VolumeStatsAggPeriod.Duration = time.Minute
 	c.CgroupRoot = ""
 	c.ContainerRuntime = kubetypes.DockerContainerRuntime
+	c.CPUManagerPolicy = "noop"


TODO: use const from cpumanager package instead of this raw string

ConnorDoyle · 2017-07-19T18:47:03Z

pkg/kubelet/cpumanager/cpu_manager.go

+		glog.Infof("[cpumanager] detected CPU topology: %v", topo)
+		newPolicy = NewStaticPolicy(topo)
+	default:
+		glog.Warningf("[cpumanager] Invalid policy, fallback to default policy - 'noop'")


Nit: interpolate PolicyNoop const in this log output

ConnorDoyle · 2017-07-19T18:50:13Z

/assign @derekwaynecarr @vishh
/unassign @pmorie @euank

ConnorDoyle · 2017-07-20T21:56:21Z

@teferi and @nqn could you please help turn the CLA gate green? There's a wiki entry on how it works here: https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ

sjenning · 2017-07-20T23:33:37Z

/assign

sjenning · 2017-07-20T23:42:51Z

@ConnorDoyle thanks for opening this PR 👍 We do need to start moving this forward and getting review if we are going to make 1.8

I did want to clean up the commit history on our dev branch before opening a PR. I would like to maintain credit though for each author. Maybe we can squash by author and rework the commit messages so that we don't have "wip" commits and whatnot.

sjenning · 2017-07-21T02:15:33Z

Reviewer note: the first commit in this PR is the commit for #46105. I think we might want to review the CRI change separately in that PR, which needs rebasing if we want to keep it open.

Thoughts?

ConnorDoyle · 2017-07-21T03:32:10Z

I can squash consecutive commits by author and coalesce the messages. Wanted to do the big rebase first to stay sane; now we're caught up with master.

…

On Jul 20, 2017, at 16:43, Seth Jennings ***@***.***> wrote: @ConnorDoyle thanks for opening this PR 👍 We do need to start moving this forward and getting review if we are going to make 1.8 I did want to clean up the commit history on our dev branch before opening a PR. I would like to maintain credit though for each author. Maybe we can squash by author and rework the commit messages so that we don't have "wip" commits and whatnot. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ConnorDoyle · 2017-08-22T21:27:48Z

Rebased

rohitsardesai83 · 2017-08-23T11:00:37Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -116,8 +115,12 @@ func (m *kubeGenericRuntimeManager) startContainer(podSandboxID string, podSandb
 		m.recordContainerEvent(pod, container, containerID, v1.EventTypeWarning, events.FailedToCreateContainer, "Error: %v", grpc.ErrorDesc(err))
 		return grpc.ErrorDesc(err), ErrCreateContainer
 	}
+	err = m.cpuManager.RegisterContainer(pod, container, containerID)
+	if err != nil {
+		m.recorder.Eventf(ref, v1.EventTypeWarning, events.FailedToStartContainer, "Failed to register container with CPU manager: %v", err)


Should we introduce a new events constant here say events.FailedToRegisterContainer or something ?
since this is not a failure to start the container but a registration failure ?

Not sure, haven't examined the event classifications closely enough to have an opinion about this. However, we are bailing out of startContainer on account of the registration failure so it doesn't seem too far off.

rohitsardesai83 · 2017-08-23T11:01:57Z

pkg/kubelet/kuberuntime/kuberuntime_container.go

@@ -810,6 +813,7 @@ func (m *kubeGenericRuntimeManager) removeContainer(containerID string) error {
 	if err := m.removeContainerLog(containerID); err != nil {
 		return err
 	}
+	m.cpuManager.UnregisterContainer(containerID)


what do we do in case we get an error while unregistering the container ?

Right now the error is discarded but that probably is not the right thing to do. In case unregistration fails we probably want to fail removeContainer so that it gets called again. Otherwise we could leak/strand CPUs.

CC @sjenning

This has been updated so that errors are surfaced to the caller of killContainer and removeContainer.

- Typos - Type naming. - File rename. - Added alpha warning to --cpu-manager-policy config description.

ConnorDoyle · 2017-08-24T05:16:52Z

Addressed some of the review comments. The following TODOs are not done yet but are hidden by the GitHub UI:

comment from @vishh What is result? This method is complex. It deserves more description on why it's complex. What are the use cases it is trying to tackle?
comment from @vishh How is cross-socket assignment prevented?
comment from @vishh Why not have a registration phase at the pod creation time? ...
comment from @vishh Add a feature gate...

- Require kubereserved.cpu + systemreserved.cpu > 0. - Updated shared pool and exclusive allocation accordingly.

vishh · 2017-08-25T18:03:10Z

Awesome. Thanks @ConnorDoyle!

…

On Fri, Aug 25, 2017 at 11:01 AM, Connor Doyle ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/api/types.go <#49186 (comment)> : > @@ -3109,6 +3109,8 @@ const ( NodeMemoryPressure NodeConditionType = "MemoryPressure" // NodeDiskPressure means the kubelet is under pressure due to insufficient available disk. NodeDiskPressure NodeConditionType = "DiskPressure" + // NodeCPUPressure means the kubelet is under pressure due to insufficient available cpu. @vishh <https://github.com/vishh> -- Updated to remove CPU pressure node condition and evictions, require cpu reservation and manage the shared pool accordingly in the static policy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49186 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGvIKMNXpgyzhP9AjFQ8PBPRWWQ-LG70ks5sbwvpgaJpZM4OcVm-> .

@sjenning

Automatic merge from submit-queue (batch tested with PRs 50033, 49988, 51132, 49674, 51207) Add cpuset helper library. Blocker for CPU manager #49186 (1 of 6) @sjenning @derekwaynecarr ```release-note NONE ```

k8s-github-robot · 2017-08-29T17:39:40Z

@ConnorDoyle PR needs rebase

@sjenning

Automatic merge from submit-queue (batch tested with PRs 51439, 51361, 51140, 51539, 51585) CPU manager interfaces. Please review / merge #51132 first. Blocker for CPU manager #49186 (3 of 6) @sjenning @derekwaynecarr

ConnorDoyle · 2017-08-30T15:54:53Z

Addressed the vast majority of review comments and broke this down into smaller PRs. Those are all linked from the top PR description comment here. Continuing reviews together with @derekwaynecarr there. Thanks everyone for reviews on this monster PR -- closing.

@balajismaniam

…epolicy Automatic merge from submit-queue (batch tested with PRs 49971, 51357, 51616, 51649, 51372) CPU manager wiring and `none` policy Blocker for CPU manager #49186 (4 of 6) * Previous PR in this series: #51140 * Next PR in this series: #51180 cc @balajismaniam @derekwaynecarr @sjenning **Release note**: ```release-note NONE ``` TODO: - [X] In-memory CPU manager state - [x] Kubelet config value - [x] Feature gate - [X] None policy - [X] Unit tests - [X] CPU manager instantiation - [x] Calls into CPU manager from Kubelet container runtime

@derekwaynecarr

Automatic merge from submit-queue (batch tested with PRs 51180, 51893) CPU manager static policy Blocker for CPU manager #49186 (5 of 6) * Previous PR in this series: #51357 * Next PR in this series: #51041 cc @derekwaynecarr @sjenning @flyingcougar @balajismaniam Attempting to be fairly accurate with main authorship at least at a file level -- please let me know if anyone has a better idea on how to improve this. For posterity, here are the Kubelet flags to run the static policy (assuming `/kube-reserved` is a cgroup that exists for all required controllers) `--feature-gates=CPUManager=true --cpu-manager-policy=static --cpu-manager-reconcile-period=5s --enforce-node-allocatable=pods,kube-reserved --kube-reserved-cgroup=/kube-reserved --kube-reserved=cpu=500m` **Release note**: ```release-note NONE ```

@ConnorDoyle

Automatic merge from submit-queue Node e2e tests for the CPU Manager. **What this PR does / why we need it**: - Adds node e2e tests for the CPU Manager implementation in #49186. **Special notes for your reviewer**: - Previous PR in this series: #51180 - Only `test/e2e_node/cpu_manager_test.go` must be reviewed as a part of this PR (i.e., the last commit). Rest of the comments belong in #51357 and #51180. - The tests have been on run on `n1-standard-n4` and `n1-standard-n2` instances on GCE. To run this node e2e test, use the following command: ```sh make test-e2e-node TEST_ARGS='--feature-gates=DynamicKubeletConfig=true' FOCUS="CPU Manager" SKIP="" PARALLELISM=1 ``` CC @ConnorDoyle @sjenning

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 19, 2017

k8s-github-robot assigned pmorie and euank Jul 19, 2017

ConnorDoyle commented Jul 19, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2017

ConnorDoyle commented Jul 19, 2017

View reviewed changes

k8s-ci-robot assigned derekwaynecarr and vishh and unassigned pmorie and euank Jul 19, 2017

ConnorDoyle force-pushed the cpu-manager branch from 683ed5b to 51c7de5 Compare July 20, 2017 18:19

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2017

ConnorDoyle force-pushed the cpu-manager branch from 51c7de5 to 5900345 Compare July 20, 2017 22:13

k8s-ci-robot assigned sjenning Jul 20, 2017

ConnorDoyle force-pushed the cpu-manager branch from a170a97 to e1d6aae Compare July 21, 2017 04:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2017

ConnorDoyle added 2 commits August 22, 2017 17:13

Fix govet and golint problems.

d484971

Fix golint errors.

78bcfde

ConnorDoyle mentioned this pull request Aug 23, 2017

CPU manager static policy #51180

Merged

rohitsardesai83 reviewed Aug 23, 2017

View reviewed changes

Addressed some review comments.

17a986f

- Typos - Type naming. - File rename. - Added alpha warning to --cpu-manager-policy config description.

ConnorDoyle added 4 commits August 23, 2017 22:26

Add cpuset helper library.

605a1a6

Renamed CPUSet.AsSlice() => CPUSet.ToSlice()

133a328

Add CPUSetBuilder, make CPUSet immutable.

dab8475

Moved cpuset to /pkg/kubelet/cm.

3ef6345

ConnorDoyle mentioned this pull request Aug 24, 2017

node label for policy and condition for CPUPressure kubernetes/community#970

Closed

ConnorDoyle added 2 commits August 23, 2017 23:26

Release lock before UpdateContainerResources.

ffda4b2

Removed eviction and node condition from CPU man.

14e1457

- Require kubereserved.cpu + systemreserved.cpu > 0. - Updated shared pool and exclusive allocation accordingly.

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 25, 2017

ConnorDoyle mentioned this pull request Aug 25, 2017

CPU manager wiring and none policy #51357

Merged

7 tasks

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

ConnorDoyle closed this Aug 30, 2017

sjenning mentioned this pull request Sep 5, 2017

Further differentiate performance characteristics associated with pod level QoS kubernetes/enhancements#276

Closed

ConnorDoyle deleted the cpu-manager branch September 6, 2017 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPU manager phase 1 #49186

CPU manager phase 1 #49186

ConnorDoyle commented Jul 19, 2017 •

edited

Loading

k8s-ci-robot commented Jul 19, 2017

k8s-github-robot commented Jul 19, 2017

ConnorDoyle Jul 19, 2017

ConnorDoyle Jul 19, 2017

ConnorDoyle Jul 19, 2017

ConnorDoyle Jul 19, 2017

ConnorDoyle Jul 19, 2017

ConnorDoyle Jul 19, 2017

ConnorDoyle commented Jul 19, 2017

ConnorDoyle commented Jul 20, 2017

sjenning commented Jul 20, 2017

sjenning commented Jul 20, 2017

sjenning commented Jul 21, 2017

ConnorDoyle commented Jul 21, 2017 via email

ConnorDoyle commented Aug 22, 2017

rohitsardesai83 Aug 23, 2017

ConnorDoyle Aug 24, 2017

rohitsardesai83 Aug 23, 2017

ConnorDoyle Aug 24, 2017

ConnorDoyle Aug 29, 2017

ConnorDoyle commented Aug 24, 2017 •

edited

Loading

vishh commented Aug 25, 2017 via email

k8s-github-robot commented Aug 29, 2017

ConnorDoyle commented Aug 30, 2017

CPU manager phase 1 #49186

CPU manager phase 1 #49186

Conversation

ConnorDoyle commented Jul 19, 2017 • edited Loading

k8s-ci-robot commented Jul 19, 2017

k8s-github-robot commented Jul 19, 2017

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

ConnorDoyle commented Jul 19, 2017

ConnorDoyle commented Jul 20, 2017

sjenning commented Jul 20, 2017

sjenning commented Jul 20, 2017

sjenning commented Jul 21, 2017

ConnorDoyle commented Jul 21, 2017 via email

ConnorDoyle commented Aug 22, 2017

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

ConnorDoyle commented Aug 24, 2017 • edited Loading

vishh commented Aug 25, 2017 via email

k8s-github-robot commented Aug 29, 2017

ConnorDoyle commented Aug 30, 2017

ConnorDoyle commented Jul 19, 2017 •

edited

Loading

ConnorDoyle commented Aug 24, 2017 •

edited

Loading