feat: enable alternate kube-reserved cgroups #3201

alexeldeib · 2020-05-05T23:02:09Z

Reason for Change:
Kubernetes enforces resource control through cgroups. Kubelet understands several cgroup configuration options like --kube-reserved-cgroup, --system-reserved-cgroup which specify how to partition a system for resource control. Kubelet subtracts the resource quantities specified through kube-reserved and system-reserved from total node resources to arrive at node allocatable.

Currently AKS-Engine exposes the kubelet flags for configuring enforcement of these options, but there's no way to actually set up the cgroups beforehand. This PR makes that possible by creating a systemd slice and appropriate drop-ins for Kubelet and the container runtime to exist in. That is, when using the features enabled in this PR a cgroup will be created which corresponds to kube-reserved, leaving other system daemons in the system slice. The system slice roughly acts as system-reserved when a user specifies kubeReservedCgroup.

This PR does not align enforcement of the provided value against kubelet -- we could probably do this by checking for a flag that matches --kube-reserved-cgroup , but I figured I'd open as is for feedback.

Issue Fixed:
consider this PR the feature request 😄

Requirements:
Can place kubelet and docker into arbitrary systemd slices.

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

codecov · 2020-05-05T23:27:34Z

Codecov Report

Merging #3201 into master will increase coverage by 0.00%.
The diff coverage is 81.81%.

@@           Coverage Diff           @@
##           master    #3201   +/-   ##
=======================================
  Coverage   71.43%   71.43%           
=======================================
  Files         147      147           
  Lines       25653    25664   +11     
=======================================
+ Hits        18324    18333    +9     
- Misses       6187     6188    +1     
- Partials     1142     1143    +1

Impacted Files	Coverage Δ
pkg/api/types.go	`94.35% <ø> (ø)`
pkg/api/vlabs/types.go	`73.30% <ø> (ø)`
pkg/engine/cse.go	`100.00% <ø> (ø)`
pkg/engine/templates_generated.go	`39.64% <ø> (ø)`
pkg/engine/template_generator.go	`81.47% <77.77%> (-0.05%)`	⬇️
pkg/api/converterfromapi.go	`93.65% <100.00%> (+0.01%)`	⬆️
pkg/api/convertertoapi.go	`93.16% <100.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c0d7ac...74f195f. Read the comment docs.

alexeldeib · 2020-05-05T23:45:49Z

@jackfrancis curious how you feel about this approach vs trying to detect and/or match more tightly what the user might provide as a raw kubelet flag. I found this a bit more palatable, even if it requires more manual jiggering.

alexeldeib · 2020-05-06T06:41:58Z

Cc @xuto2

jackfrancis · 2020-05-06T17:07:14Z

parts/k8s/cloud-init/masternodecustomdata.yml

+    Before=slices.target
+    Requires=-.slice
+    After=-.slice
+    #EOF


This #EOF sentinel chars pattern is meant to work in concert w/ the wait_for_file func in the CSE bootstrap scripts. If the following is true:

in the event that CSE runs prior to cloud-init being finished paving the filesystem, these missing files will prevent CSE from completing successfully/deterministically

...then, we should keep the #EOF and add the appropriate wait_for_file invocations against these new files (probably inside the ensureKubelet func). Unfortunately, becase the CSE files themselves do not operate against a per-pool or per-master context, we have to pass that context via env vars.

Either that, or we can generalize this approach and pick a "default " cgroup so that we always deliver this new cgroup implementation flavor, and allow the value of kubeReservedCgroup to be user-configured (we could apply that per-node/per-master user configuration in cloud-init, and skip the CSE env var boilerplate thing).

Hope that makes sense!

I'm going to always deliver the cgroup and default it to kubereserved, but not run anything in it. That's effectively a no-op unless users manually move stuff into the slice.

Is it just me, or should

aks-engine/parts/k8s/cloud-init/artifacts/cse_config.sh

Line 317 in a43a9c4

DOCKER_MOUNT_FLAGS_SYSTEMD_FILE=/etc/systemd/system/docker.service.d/clear_mount_propagation_flags.conf

wait for the file to exist?

Yeah, I'm not sure what's going on there...

meh, this got a little tricky since the name of the file determines the slice. That doesn't work for the current setup with the system slice, since it already exists (and not via this filepath). I don't really think there's any harm forcing users to pick a slice, but I'm also totally fine with doing this clusterwide and evolving it if we need. That's what I stuck with for now. The end goal is to pin down one really well-isolated config anyway.

alexeldeib · 2020-05-11T16:16:46Z

Creating some monday morning notifications, let me know how this looks to you @jackfrancis?

jackfrancis · 2020-05-11T17:09:45Z

docs/topics/clusterdefinitions.md

@@ -65,6 +65,7 @@ $ aks-engine get-versions
 | gcHighThreshold                  | no                        | Sets the --image-gc-high-threshold value on the kublet configuration. Default is 85. [See kubelet Garbage Collection](https://kubernetes.io/docs/concepts/cluster-administration/kubelet-garbage-collection/)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             |
 | gcLowThreshold                   | no                        | Sets the --image-gc-low-threshold value on the kublet configuration. Default is 80. [See kubelet Garbage Collection](https://kubernetes.io/docs/concepts/cluster-administration/kubelet-garbage-collection/)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
 | kubeletConfig                    | no                        | Configure various runtime configuration for kubelet. See `kubeletConfig` [below](#feat-kubelet-config)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
+| kubeReservedCgroup               | no                        | The name of a systemd slice to create for containtment of both kubelet and the container runtime. This should not point to an existing systemd slice. Defaults to the empty string, which means kubelet and the container runtime will be placed in the system slice.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   |


I think it's more accurate to say:

Defaults to "kubereserved", which means kubelet and the container runtime will be placed in the system slice.

Is that right?

No, I think the current phrasing is correct. The default behavior is always to put them in the system slice unless specified. If you specify anything else, that slice will be created and the units will be placed inside it.

Will update for clarity

I think the "Defaults to the empty string" is what's confusing to me, as the default of that property will actually be "kubereserved".

jackfrancis · 2020-05-11T17:14:33Z

pkg/api/defaults.go

@@ -434,6 +434,10 @@ func (cs *ContainerService) setOrchestratorDefaults(isUpgrade, isScale bool) {
 			o.KubernetesConfig.ContainerRuntimeConfig = make(map[string]string)
 		}

+		if o.KubernetesConfig.KubeReservedCgroup == "" {


Based on this, it is actually not possible for a fully populated (post-defaults enforcement) api model to have a "" value of KubeReservedCgroup. In which case, the HasKubeReservedCgroup func is redundant, right?

whoops, I'll remove this

This is leftover from playing around with per-nodepool and defaulting, but it didn't work out the way i'd like

That explains everything! :)

pkg/api/common/const.go

jackfrancis · 2020-05-11T21:05:19Z

docs/topics/clusterdefinitions.md

@@ -65,6 +65,7 @@ $ aks-engine get-versions
 | gcHighThreshold                  | no                        | Sets the --image-gc-high-threshold value on the kublet configuration. Default is 85. [See kubelet Garbage Collection](https://kubernetes.io/docs/concepts/cluster-administration/kubelet-garbage-collection/)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             |
 | gcLowThreshold                   | no                        | Sets the --image-gc-low-threshold value on the kublet configuration. Default is 80. [See kubelet Garbage Collection](https://kubernetes.io/docs/concepts/cluster-administration/kubelet-garbage-collection/)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
 | kubeletConfig                    | no                        | Configure various runtime configuration for kubelet. See `kubeletConfig` [below](#feat-kubelet-config)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
+| kubeReservedCgroup               | no                        | The name of a systemd slice to create for containtment of both kubelet and the container runtime. When this value is a non-empty string, a file will be dropped at `/etc/systemd/system/$KUBE_RESERVED_CGROUP.slice` creating a systemd slice. Both kubelet and docker will run in this slice. This should not point to an existing systemd slice. If this value is unspecified or specified as the empty string, kubelet and the container runtime will run in the system slice by default.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   |


s/containtment/containment

Also, to be clear the default "kubeReservedCgroup": "" results in no functional change compared to current aks-engine:master, correct?

no functional change compared to current aks-engine:master, correct

Correct, that is the intent.

Will fix spelling

jackfrancis · 2020-05-11T21:08:34Z

parts/k8s/cloud-init/artifacts/cse_config.sh

@@ -306,6 +306,7 @@ installContainerd() {
 ensureContainerd() {
  wait_for_file 1200 1 /etc/systemd/system/containerd.service.d/exec_start.conf || exit {{GetCSEErrorCode "ERR_FILE_WATCH_TIMEOUT"}}
  wait_for_file 1200 1 /etc/containerd/config.toml || exit {{GetCSEErrorCode "ERR_FILE_WATCH_TIMEOUT"}}
+  wait_for_file 1200 1 /etc/systemd/system/containerd.service.d/kubereserved-slice.conf|| exit {{GetCSEErrorCode "ERR_FILE_WATCH_TIMEOUT"}}


This should be inside a {{- if HasKubeReservedCgroup}} block, no?

Nice catch

My last set of commits...not so clean

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

jackfrancis · 2020-05-11T23:52:49Z

test/e2e/test_cluster_configs/everything.json

 					"containerRuntimeConfig": {
 						"dataDir": "/mnt/docker"
 					},
+					"kubeReservedCgroup": "kubesystem",


warning: dups!

cries in merge commits

jackfrancis

lgtm

acs-bot · 2020-05-12T00:29:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib, jackfrancis

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:

~~OWNERS~~ [jackfrancis]

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

* feat: enable alternate kube-reserved cgroups (#3201) * fix bad commit

acs-bot added the size/L label May 5, 2020

alexeldeib force-pushed the ace/systemd branch from 8b6d98e to 7a49a46 Compare May 5, 2020 23:13

alexeldeib force-pushed the ace/systemd branch 2 times, most recently from 5a3d08c to c1493ec Compare May 5, 2020 23:51

jackfrancis reviewed May 6, 2020

View reviewed changes

mboersma added this to In progress in backlog via automation May 6, 2020

alexeldeib force-pushed the ace/systemd branch from a6c1e32 to 2e35cea Compare May 6, 2020 20:34

jackfrancis reviewed May 11, 2020

View reviewed changes

alexeldeib commented May 11, 2020

View reviewed changes

pkg/api/common/const.go Outdated Show resolved Hide resolved

jackfrancis reviewed May 11, 2020

View reviewed changes

alexeldeib added 5 commits May 11, 2020 16:28

feat: enable alternate kube-reserved cgroups

3364dab

chore: add template test for coverage

386a1a8

chore: simplify to cluster-wide setting

281c374

docs: clarify slice creation and usage

87acfd4

chore: don't default always on

c627ca9

Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

alexeldeib force-pushed the ace/systemd branch from 9863566 to 3811159 Compare May 11, 2020 23:30

jackfrancis reviewed May 11, 2020

View reviewed changes

fix: spelling typos and containerd pivot

74f195f

alexeldeib force-pushed the ace/systemd branch from 3811159 to 74f195f Compare May 11, 2020 23:55

backlog automation moved this from In progress to Review in progress May 12, 2020

jackfrancis approved these changes May 12, 2020

View reviewed changes

acs-bot added the approved label May 12, 2020

jackfrancis merged commit 9f3ec93 into Azure:master May 12, 2020

backlog automation moved this from Review in progress to Done May 12, 2020

fmotrifork mentioned this pull request May 26, 2020

aks-engine 0.51.0 fishworks/fish-food#748

Merged

alexeldeib added a commit to alexeldeib/aks-engine that referenced this pull request Jun 24, 2020

feat: enable alternate kube-reserved cgroups (Azure#3201)

5a6a1d1

alexeldeib added a commit to alexeldeib/aks-engine that referenced this pull request Jun 24, 2020

feat: enable alternate kube-reserved cgroups (Azure#3201)

02505d2

alexeldeib mentioned this pull request Jun 24, 2020

feat: enable alternate kube-reserved cgroups #3530

Merged

alexeldeib added a commit to alexeldeib/aks-engine that referenced this pull request Jun 24, 2020

feat: enable alternate kube-reserved cgroups (Azure#3201)

34f2d03

alexeldeib mentioned this pull request Jun 24, 2020

[wip] enable settings kube-reserved cgroup Azure/AgentBaker#133

Closed

xuto2 pushed a commit that referenced this pull request Jun 24, 2020

feat: enable alternate kube-reserved cgroups (#3530)

156f6ca

* feat: enable alternate kube-reserved cgroups (#3201) * fix bad commit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable alternate kube-reserved cgroups #3201

feat: enable alternate kube-reserved cgroups #3201

alexeldeib commented May 5, 2020 •

edited

codecov bot commented May 5, 2020 •

edited

alexeldeib commented May 5, 2020

alexeldeib commented May 6, 2020

jackfrancis May 6, 2020

alexeldeib May 6, 2020

alexeldeib May 6, 2020

jackfrancis May 6, 2020

alexeldeib May 6, 2020 •

edited

alexeldeib commented May 11, 2020

jackfrancis May 11, 2020

alexeldeib May 11, 2020

jackfrancis May 11, 2020

jackfrancis May 11, 2020

alexeldeib May 11, 2020

alexeldeib May 11, 2020

jackfrancis May 11, 2020

jackfrancis May 11, 2020

jackfrancis May 11, 2020

alexeldeib May 11, 2020 •

edited

jackfrancis May 11, 2020

alexeldeib May 11, 2020

jackfrancis May 11, 2020

alexeldeib May 11, 2020

jackfrancis left a comment

acs-bot commented May 12, 2020

feat: enable alternate kube-reserved cgroups #3201

feat: enable alternate kube-reserved cgroups #3201

Conversation

alexeldeib commented May 5, 2020 • edited

codecov bot commented May 5, 2020 • edited

Codecov Report

alexeldeib commented May 5, 2020

alexeldeib commented May 6, 2020

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

alexeldeib May 6, 2020 • edited

Choose a reason for hiding this comment

alexeldeib commented May 11, 2020

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

alexeldeib May 11, 2020 • edited

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

jackfrancis left a comment

Choose a reason for hiding this comment

acs-bot commented May 12, 2020

alexeldeib commented May 5, 2020 •

edited

codecov bot commented May 5, 2020 •

edited

alexeldeib May 6, 2020 •

edited

alexeldeib May 11, 2020 •

edited