feat: modify container runtime data dir #3072

alexeldeib · 2020-04-15T05:41:29Z

Reason for Change:
Allow customization of the container runtime data directory.

The location of this directory will be subject to heavy disk IO and offloading it from the main OS disk can significantly stabilize the system. Solutions already exist to hack this onto nodes, but we'd like to enable it at provision time. This PR does that.

Issue Fixed:
n/a

Requirements:

Notes:

I haven't made API changes to AKS engine in a while, I wasn't sure what all tests I should add. Do we want an e2e covering this? What's the best way to test the full template generation, cmd/generate_test?

alexeldeib · 2020-04-15T05:42:35Z

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

@@ -282,6 +282,9 @@ 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"}}
+  {{- if HasContainerDataDir}}
+  echo -e "root = \"{{GetContainerDataDir}}\"\n$(cat /etc/containerd/config.toml)" > /etc/containerd/config.toml


So, this is ugly. An alternative is doing it in cloud-init for non-vhd installs and doing it in the VHD otherwise. Thoughts?

So, we don't want to just get the whole generic key/val implementation over with first of all? See the way we use the GetKubeletConfigKeyVals go template layer func in cloud-init (parts/k8s/cloud-init/masternodecustomdata.yml and parts/k8s/cloud-init/nodecustomdata.yml) to accommodate generic key/val injection at runtime.

The part I wasn't sure about was how much to generalize this for docker vs containerd because the config files are totally different.

Are you thinking we could do a func like for kubelet, 1 each for docker and containerd, and have those functions accept the raw map[string]string and spit out the raw CRI-specific JSON/TOML? Then we can avoid any conditionals in the bash

Exactly 💯

Not trying to introduce scope creep, but I think we'll thank ourselves later for doing it the generic way. Thoughts?

I'd really like to avoid "containerdConfig" and "dockerConfig", but we're kind of hiding it no matter what

i'm on board with that, will update

I think we can stick with a single containerRuntimeConfig, and then have per-runtime go template helper funcs. As long as we can assume that key/val strings are common across all runtimes (I think we can), then we can use a single interface.

is there a good way to handle errors in the template funcs? I want to do something like this, but struggling to find a good place to drop the Marshal to catch the error. I could do raw string manipulation, but that feels error prone (pun intended) and I wouldn't be guaranteed to construct valid json anyway.

// GetDockerConfig returns the full docker daemon configuration including user overrides. func (k *KubernetesConfig) GetDockerConfig() (string, error) { config := map[string]interface{}{ "live-restore": true, "log-driver": "json-file", "log-opts": { "max-size": "50m", "max-file": "5", }, } dataDir, ok := k.ContainerRuntimeConfig[ContainerDataDirKey] if ok { config["data-root"] = dataDir } b, err := json.MarshalIndent(config, "", " ") return string(b), err }

and containerd uses toml, even more fun

codecov · 2020-04-15T05:56:53Z

Codecov Report

Merging #3072 into master will increase coverage by 0.02%.
The diff coverage is 75.83%.

@@            Coverage Diff             @@
##           master    #3072      +/-   ##
==========================================
+ Coverage   71.03%   71.05%   +0.02%     
==========================================
  Files         147      147              
  Lines       25553    25671     +118     
==========================================
+ Hits        18152    18241      +89     
- Misses       6271     6294      +23     
- Partials     1130     1136       +6

Impacted Files	Coverage Δ
pkg/api/types.go	`94.45% <ø> (ø)`
pkg/api/vlabs/types.go	`73.30% <ø> (ø)`
pkg/engine/templates_generated.go	`37.76% <ø> (ø)`
pkg/api/convertertoapi.go	`93.42% <60.00%> (-0.28%)`	⬇️
pkg/engine/template_generator.go	`81.70% <65.62%> (-0.81%)`	⬇️
pkg/api/vlabs/validate.go	`79.27% <73.33%> (-0.09%)`	⬇️
pkg/api/common/helper.go	`75.61% <78.94%> (+0.83%)`	⬆️
pkg/api/common/const.go	`40.00% <100.00%> (+40.00%)`	⬆️
pkg/api/converterfromapi.go	`93.94% <100.00%> (+0.05%)`	⬆️
pkg/api/defaults.go	`92.44% <100.00%> (+0.02%)`	⬆️

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 22c5fb6...d2ba320. Read the comment docs.

jackfrancis · 2020-04-15T17:28:38Z

Thank you @alexeldeib!

alexeldeib · 2020-04-17T16:14:47Z

@jackfrancis how do you feel about this? Can get some more test coverage if it's agreeable. Curious what you think about adding types like this for containerd/docker config and stashing it internally in common.

alexeldeib · 2020-04-17T17:47:26Z

pkg/api/vlabs/validate.go

+	if k.ContainerRuntime == Containerd {
+		_, err := common.GetContainerdConfig(k.ContainerRuntimeConfig, nil)
+		if err != nil {
+			return err


I realized the way I'm doing this now, these errors are basically guaranteed not to get hit...they're reducing coverage in the diff but I think keeping them is correct for future sanity

yay for functional options

alexeldeib · 2020-04-17T19:23:04Z

pkg/api/common/helper.go

+}
+
+// GetContainerdConfig transforms the default containerd config with overrides. Overrides may be nil.
+func GetContainerdConfig(opts map[string]string, overrides []func(*ContainerdConfig) error) (string, error) {


I came up with the functional setters to work around the fact that there are templated values in the docker/containerd config normally, but we don't want that logic in the helper functions (for testing purposes and circular imports).

I realize now that having both map[string]string and []func(*ContainerdConfig) error) is a bit redundant, for example we could have a separate helper with signature func help(map[string]string) []func(*ContainerdConfig) error) for containerd/docker each and use only functional setters to construct the final object

alexeldeib · 2020-04-17T19:23:38Z

pkg/api/common/const.go

+	DefaultContainerdConfig = ContainerdConfig{
+		Version: 2,
+		// should this be true? seems maybe not https://github.com/containerd/containerd/blob/master/docs/ops.md#base-configuration
+		Subreaper: false,


I thought recommended docs say to set this to true? Is false a kube thing?

@cpuguy83 should be able to answer

Let's remove the setting. It is not part of the config anymore and never really made sense I don't think.
The config was changed to no_subreaper in containerd/containerd#1822 and later removed altogether as far as I can tell (can't find where it was removed, but the config doesn't exist).

jackfrancis · 2020-04-17T19:55:34Z

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

@@ -282,6 +282,9 @@ 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"}}
+  {{- if HasValidContainerdConfig}}


I think we should do this in the cloud-init file spec directly, rather than as a post-cloud-init shell command on top of the cloud-init-paved file.

Do you happen to know if apt will avoid overwriting the old daemon config if it already exists? That was my only concern, other than some confusion about VHD vs non-VHD.

Also do you think it's reasonable to remove the validity check and write whatever we have once we've passed validation, or should we attempt to provide a sane default if we know it's invalid but still got here somehow?

of course doing that dropped my PR coverage again :P

mboersma · 2020-04-17T20:21:04Z

pkg/api/common/helper.go

+}
+
+// ContainerdSandboxImageOverrider produces a function to transform containerd config by setting the SandboxImage.
+func ContainerdSandboxImageOverrider(image string) func(*ContainerdConfig) error {


This is an Overrider rather than just an Override func so we can use "curried" funcs with an image arg baked in elsewhere?

That was my thinking, the semantics are already fairly clear so I'm happy to keep the naming consistent (Override) if you think it makes sense

jackfrancis · 2020-04-17T22:42:47Z

parts/k8s/cloud-init/nodecustomdata.yml

-            [plugins."io.containerd.grpc.v1.cri".containerd.runtimes.untrusted]
-              {{/* note: runc really should not be used for untrusted workloads... should we remove this? This is here because it was here before */}}
-              runtime_type = "io.containerd.runc.v2"
-    #EOF


We need to make sure we keep these #EOF sentinel chars at the end of the data stream, which the CSE scripts use to determine that cloud-init has paved the entire file.

alexeldeib · 2020-04-18T00:26:07Z

felt like it was time for some

alexeldeib · 2020-04-20T16:10:36Z

@jackfrancis @mboersma friendly ping for another review, think this is good to go and squashed

jackfrancis · 2020-04-20T16:22:36Z

@alexeldeib, will do, could you add example usage of this new configuration in one of the agent pools, and the masterProfile, to: test/e2e/test_cluster_configs/everything.json. 👈 We use that via jenkins to regularly E2E test cluster config functionality.

Thanks again for this!

jackfrancis

lgtm pending E2E test config intregation of this foo + successful run

cpuguy83 · 2020-04-20T16:29:22Z

pkg/api/common/types.go

+type ContainerdConfig struct {
+	OomScore  int     `toml:"oom_score,omitempty"`
+	Root      string  `toml:"root,omitempty"`
+	Subreaper bool    `toml:"subreaper"`


Please remove this, it's not a real config. I removed it from the containerd docs: containerd/containerd#4194

Same for the actual generated toml.

gah, thought I pulled this out. will remove

alexeldeib · 2020-04-20T16:35:40Z

@jackfrancis this would be cluster-wide since it's in KubernetesConfig like ContainerRuntime (although it definitely could be per agent pool). Would you like me to add a new file/test case with this, or update the everything cluster to use this by default?

jackfrancis · 2020-04-20T16:42:33Z

@alexeldeib Right, sorry, that's obviously evident in the changeset, thanks for the clarification. :)

So yeah, let's put it into KubernetesConfig of the everything cluster (unless that doesn't work w/ a windows pool, which that config includes?). We can consider making this config a per-pool config if we ever get to the larger (and more meaningful) refactor work of actually making the container runtime per-pool configurable.

alexeldeib · 2020-04-20T22:19:12Z

pkg/engine/template_generator.go

+		"GetDockerConfig": func() string {
+			var overrides []func(*common.DockerConfig) error
+
+			if cs.Properties.HasNSeriesSKU() {


I already squashed but L704-709 were not present which failed N series VMs with GPUs before this

jackfrancis · 2020-04-21T22:57:42Z

parts/k8s/cloud-init/nodecustomdata.yml

-        }
-      }{{end}}
-    }
+{{IndentString (GetDockerConfig (IsNSeriesSKU .VMSize)) 4}}


You're teaching me new things about go template syntax

jackfrancis

/lgtm

This reverts commit 12313eb.

…"" This reverts commit f519f0a.

… (#3179)""" This reverts commit 0f0572e.

…ir (Azure#3072) (Azure#3179)"""" This reverts commit 5af21fe.

…ir (#3072) (#3179)"""" (#3393) This reverts commit 5af21fe.

…e data dir (#3072) (#3179)"""" (#3393)" This reverts commit c4e226e.

…r runtime data dir (#3072) (#3179)"""" (#3393)"" This reverts commit b88027d.

alexeldeib commented Apr 15, 2020

View reviewed changes

alexeldeib force-pushed the ace/root branch from e00c4f0 to 0ac218b Compare April 15, 2020 05:43

alexeldeib force-pushed the ace/root branch 3 times, most recently from 5424520 to e2ae09a Compare April 17, 2020 11:59

alexeldeib force-pushed the ace/root branch 2 times, most recently from 76453bd to 22fe938 Compare April 17, 2020 17:09

alexeldeib commented Apr 17, 2020

View reviewed changes

jackfrancis reviewed Apr 17, 2020

View reviewed changes

mboersma reviewed Apr 17, 2020

View reviewed changes

alexeldeib force-pushed the ace/root branch from 29da29b to 4a68be7 Compare April 17, 2020 20:51

jackfrancis reviewed Apr 17, 2020

View reviewed changes

alexeldeib force-pushed the ace/root branch from c1d24b1 to 2c4d125 Compare April 18, 2020 00:24

alexeldeib force-pushed the ace/root branch 3 times, most recently from 0b037ec to bc21273 Compare April 18, 2020 18:34

jackfrancis previously approved these changes Apr 20, 2020

View reviewed changes

cpuguy83 suggested changes Apr 20, 2020

View reviewed changes

alexeldeib dismissed jackfrancis’s stale review via 5496b06 April 20, 2020 16:53

alexeldeib force-pushed the ace/root branch from bc21273 to 5496b06 Compare April 20, 2020 16:53

alexeldeib mentioned this pull request Apr 20, 2020

Consider disabling the instance ephemeral storage by default kubernetes-sigs/cluster-api-provider-azure#551

Closed

alexeldeib force-pushed the ace/root branch from 5496b06 to b71d292 Compare April 20, 2020 22:16

alexeldeib commented Apr 20, 2020

View reviewed changes

alexeldeib closed this Apr 21, 2020

alexeldeib force-pushed the ace/root branch from 1954b64 to 22c5fb6 Compare April 21, 2020 20:40

alexeldeib reopened this Apr 21, 2020

alexeldeib force-pushed the ace/root branch 6 times, most recently from 494026b to 82277aa Compare April 21, 2020 21:15

feat: modify container runtime data dir

d2ba320

alexeldeib force-pushed the ace/root branch from 82277aa to d2ba320 Compare April 21, 2020 22:11

jackfrancis reviewed Apr 21, 2020

View reviewed changes

jackfrancis approved these changes Apr 21, 2020

View reviewed changes

jackfrancis merged commit b6ea0dc into Azure:master Apr 21, 2020

fmotrifork mentioned this pull request Apr 24, 2020

aks-engine 0.50.0 fishworks/fish-food#697

Merged

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

feat: modify container runtime data dir (Azure#3072)

b48fb70

alexeldeib mentioned this pull request May 1, 2020

cherry-pick: modify container runtime data dir #3179

Merged

4 tasks

xuto2 pushed a commit that referenced this pull request May 13, 2020

feat: modify container runtime data dir (#3072) (#3179)

12313eb

xuto2 pushed a commit that referenced this pull request May 15, 2020

Revert "feat: modify container runtime data dir (#3072) (#3179)"

f519f0a

This reverts commit 12313eb.

xuto2 pushed a commit that referenced this pull request May 15, 2020

Revert "Revert "feat: modify container runtime data dir (#3072) (#3179)…

0f0572e

…"" This reverts commit f519f0a.

xuto2 pushed a commit that referenced this pull request May 16, 2020

Revert "Revert "Revert "feat: modify container runtime data dir (#3072)…

5af21fe

… (#3179)""" This reverts commit 0f0572e.

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

Revert "Revert "Revert "Revert "feat: modify container runtime data d…

2227301

…ir (Azure#3072) (Azure#3179)"""" This reverts commit 5af21fe.

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

Revert "Revert "Revert "Revert "feat: modify container runtime data d…

c4e226e

…ir (#3072) (#3179)"""" (#3393) This reverts commit 5af21fe.

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

Revert "Revert "Revert "Revert "Revert "feat: modify container runtim…

b88027d

…e data dir (#3072) (#3179)"""" (#3393)" This reverts commit c4e226e.

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

Revert "Revert "Revert "Revert "Revert "Revert "feat: modify containe…

1eba161

…r runtime data dir (#3072) (#3179)"""" (#3393)"" This reverts commit b88027d.

arbielsk mentioned this pull request Oct 15, 2020

Allow for definition of registry mirrors #3941

Closed

feat: modify container runtime data dir #3072

feat: modify container runtime data dir #3072

Conversation

alexeldeib commented Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib Apr 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

codecov bot commented Apr 15, 2020 • edited Loading

Codecov Report

jackfrancis commented Apr 15, 2020

alexeldeib commented Apr 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Apr 18, 2020

alexeldeib commented Apr 20, 2020 • edited Loading

jackfrancis commented Apr 20, 2020

jackfrancis left a 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 commented Apr 20, 2020

jackfrancis commented Apr 20, 2020

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

alexeldeib commented Apr 15, 2020 •

edited

Loading

alexeldeib Apr 15, 2020 •

edited

Loading

codecov bot commented Apr 15, 2020 •

edited

Loading

alexeldeib commented Apr 17, 2020 •

edited

Loading

alexeldeib commented Apr 20, 2020 •

edited

Loading