kubelet: cm: refactor QoS logic into seperate interface by sjenning · Pull Request #41833 · kubernetes/kubernetes

sjenning · 2017-02-21T20:24:25Z

This commit has no functional change. It refactors the QoS cgroup logic into a new QOSContainerManager interface to allow for better isolation for QoS cgroup features coming down the pike.

This is a breakout of the refactoring component of my QoS memory limits PR #41149 which will need to be rebased on top of this.

@vishh @derekwaynecarr

k8s-reviewable · 2017-02-21T20:24:32Z

This change is

derekwaynecarr · 2017-02-21T20:57:20Z

Per our chat @vishh , I will take over review of this PR.

derekwaynecarr

few minor nits, update then I will tag.

derekwaynecarr · 2017-02-21T21:25:41Z

 	// - Ensures that the Docker daemon is in a container.
 	// - Creates the system container where all non-containerized processes run.
-	Start(*v1.Node) error
+	Start(*v1.Node, func() []*v1.Pod) error


can you create a type in kubelet/cm that is

// ActivePodsFunc returns pods bound to the kubelet that are active (i.e. non-terminal state) type ActivePodsFunc func() []*v1.Pod

so the signature is clear about the call expects?

derekwaynecarr · 2017-02-21T21:26:37Z

 	// GetQOSContainersInfo returns the names of top level QoS containers
 	GetQOSContainersInfo() QOSContainersInfo
+
+	// UpdateQOSCgroups returns the names of top level QoS containers


godoc is wrong, it performs housekeeping updates to ensure that the top level QoS containers have their desired state. also note that this operation is thread-safe.

or needs to be.

derekwaynecarr · 2017-02-21T21:27:22Z

 }

-func (cm *containerManagerImpl) setupNode() error {
+func (cm *containerManagerImpl) setupNode(getPods func() []*v1.Pod) error {


activePodsFn ActivePodsFunc as argument.

derekwaynecarr · 2017-02-21T21:28:11Z

 }

-func (cm *containerManagerImpl) Start(node *v1.Node) error {
+func (cm *containerManagerImpl) Start(node *v1.Node, getPods func() []*v1.Pod) error {


same here, take an ActivePodsFunc, and make sure active name propagates.

derekwaynecarr · 2017-02-21T21:28:50Z

+	return m.qosContainersInfo
+}
+
+func (m *qosContainerManagerImpl) Start(nodeInfo *v1.Node, getPods func() []*v1.Pod) error {


same here..

derekwaynecarr · 2017-02-21T21:29:10Z

+	qosContainersInfo QOSContainersInfo
+	subsystems        *CgroupSubsystems
+	cgroupManager     CgroupManager
+	getPods           func() []*v1.Pod


derekwaynecarr · 2017-02-21T22:19:18Z

@k8s-bot -- lets try now!

derekwaynecarr · 2017-02-22T02:09:25Z

@k8s-bot test this

sjenning · 2017-02-22T15:01:20Z

@k8s-bot gce etcd3 e2e test this
@k8s-bot cvm gce e2e test this

sjenning · 2017-02-22T22:26:10Z

@k8s-bot test this

sjenning · 2017-02-23T00:49:13Z

@k8s-bot gce etcd3 e2e test this

derekwaynecarr · 2017-02-23T21:54:28Z

+	m.Lock()
+	defer m.Unlock()
+
+	qosConfigs := map[v1.PodQOSClass]*CgroupConfig{


do you mind if we add this in a follow-on and just have this return nil after the lock/defer lock call?

derekwaynecarr · 2017-02-23T21:55:45Z

one minor request so the change is entirely structural, we can do the writes in our follow-on for memory/cpu but at least we can get the scaffolding in asap.

sjenning · 2017-02-24T02:14:29Z

@k8s-bot gce etcd3 e2e test this
@k8s-bot non-cri e2e test this
@k8s-bot non-cri node e2e test this

derekwaynecarr · 2017-02-24T03:23:10Z

 			if err := pcm.EnsureExists(pod); err != nil {
 				return fmt.Errorf("failed to ensure that the pod: %v cgroups exist and are correctly applied: %v", pod.UID, err)
 			}
+			if err := kl.containerManager.UpdateQOSCgroups(); err != nil {


@sjenning -- this is the wrong order... this should be before EnsureExists, not after. sorry for just catching this.

i also wonder if we should change the pcm.EnsureExists step for now to the following:

if !pcm.Exists {
updateQOSCgroups()
pcm.EnsureExists()
}

in this way we don't attempt any update when its not necessary.

derekwaynecarr · 2017-02-24T03:29:30Z

 		// been decreased earlier
 		reduceCpuLimits = false
 	}
+	if err := kl.containerManager.UpdateQOSCgroups(); err != nil {


this needs to be after KillPod(), not before.

sjenning · 2017-02-27T20:40:07Z

rebased on top of #41234 that will hopefully merge soon

derekwaynecarr · 2017-02-27T21:11:45Z

i know @vishh asked that this be called "Containers".

similar to the discussion on node allocatable, i am not going to hold up this pr on naming conventions.

we can fix that across the board in a later pr.

right now "Cgroups" is consistent with "PodContainerManager" terminology.

i also want to go on the record that "container" is not defined in the kernel, but "cgroup" is. and container is an overloaded term in a project like this.

derekwaynecarr · 2017-02-27T21:14:31Z

 	// - Ensures that the Docker daemon is in a container.
 	// - Creates the system container where all non-containerized processes run.
-	Start(*v1.Node) error
+	Start(*v1.Node, ActivePodsFunc) error


i think we can move this to a struct in a follow-on. i do not want to hold this pr on that change.

derekwaynecarr · 2017-02-27T21:15:36Z

any nits can be handled in a follow-on. i am tagging this /lgtm and marking do-not-merge to ensure that node allocatable pr lands first.

sjenning · 2017-02-28T01:51:39Z

Rebased on top of node alloc PR w/ systemd fixes and it works. Add to add new commit to work with new node allocatable code ee2dcce

The changes mostly center around the cgroupRoot being modified from nodeConfig.CgroupRoot and the kubepods cgroup not being created at constructor time, but rather at setupNode() time, meaning the sanity checks in NewQOSContainerManager() needed to move to Start().

derekwaynecarr · 2017-02-28T19:34:29Z

/approve
/lgtm

k8s-github-robot · 2017-02-28T19:38:23Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: derekwaynecarr, k8s-merge-robot, sjenning

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

~~pkg/kubelet/OWNERS~~ [derekwaynecarr]

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

k8s-github-robot · 2017-03-01T09:44:09Z

Automatic merge from submit-queue (batch tested with PRs 38676, 41765, 42103, 41833, 41702)

Automatic merge from submit-queue (batch tested with PRs 41644, 42020, 41753, 42206, 42212) Burstable QoS cgroup has cpu shares assigned **What this PR does / why we need it**: This PR sets the Burstable QoS cgroup cpu shares value to the sum of the pods cpu requests in that tier. We need it for proper evaluation of CPU shares in the new QoS hierarchy. **Special notes for your reviewer**: It builds against the framework proposed for #41833

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 21, 2017

k8s-github-robot assigned feiskyer Feb 21, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 21, 2017

derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Feb 21, 2017

derekwaynecarr assigned derekwaynecarr and unassigned feiskyer Feb 21, 2017

derekwaynecarr added this to the v1.6 milestone Feb 21, 2017

derekwaynecarr suggested changes Feb 21, 2017

View reviewed changes

sjenning force-pushed the qos-refactor branch from 5a7ed57 to eec03d0 Compare February 21, 2017 21:44

derekwaynecarr mentioned this pull request Feb 21, 2017

Burstable QoS cgroup has cpu shares assigned #41753

Merged

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

sjenning force-pushed the qos-refactor branch from eec03d0 to 46c50b2 Compare February 22, 2017 17:08

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

sjenning force-pushed the qos-refactor branch from 46c50b2 to 3ea9825 Compare February 22, 2017 19:29

derekwaynecarr reviewed Feb 23, 2017

View reviewed changes

sjenning force-pushed the qos-refactor branch from 3ea9825 to 9d32b50 Compare February 23, 2017 22:21

derekwaynecarr reviewed Feb 24, 2017

View reviewed changes

derekwaynecarr mentioned this pull request Feb 24, 2017

Enforce Node Allocatable via cgroups #41234

Merged

5 tasks

sjenning force-pushed the qos-refactor branch from 357cf54 to df1e2d6 Compare February 27, 2017 14:50

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

sjenning force-pushed the qos-refactor branch from df1e2d6 to c23142a Compare February 27, 2017 20:38

derekwaynecarr approved these changes Feb 27, 2017

View reviewed changes

derekwaynecarr added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 27, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017

sjenning force-pushed the qos-refactor branch from ad951ff to ee2dcce Compare February 28, 2017 01:47

sjenning force-pushed the qos-refactor branch from ee2dcce to 0861dce Compare February 28, 2017 15:19

kubelet: cm: refactor QoS logic into seperate interface

b9adb66

sjenning force-pushed the qos-refactor branch from 0861dce to b9adb66 Compare February 28, 2017 15:19

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017

derekwaynecarr removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 28, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2017

k8s-github-robot merged commit 1351324 into kubernetes:master Mar 1, 2017

xialonglee mentioned this pull request Apr 10, 2017

Failed to start ContainerManager failed to initialise top level QOS containers #43856

Closed

sjenning deleted the qos-refactor branch August 16, 2017 02:18

Conversation

sjenning commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-reviewable commented Feb 21, 2017

Uh oh!

derekwaynecarr commented Feb 21, 2017

Uh oh!

derekwaynecarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Feb 21, 2017

Uh oh!

derekwaynecarr commented Feb 22, 2017

Uh oh!

sjenning commented Feb 22, 2017

Uh oh!

sjenning commented Feb 22, 2017

Uh oh!

sjenning commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Feb 23, 2017

Uh oh!

sjenning commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjenning commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Feb 27, 2017

Uh oh!

sjenning commented Feb 28, 2017

Uh oh!

derekwaynecarr commented Feb 28, 2017

Uh oh!

k8s-github-robot commented Feb 28, 2017

Uh oh!

k8s-github-robot commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sjenning commented Feb 21, 2017 •

edited

Loading