From 205376fc6d8cf69d812f7d4d45a7a8e491e1c691 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Mon, 28 Oct 2019 22:31:57 -0700 Subject: [PATCH 1/5] WIP: Consistently set 'ulimit -n' across runtimes --- pkg/minikube/cruntime/cruntime.go | 5 ++++ pkg/provision/buildroot.go | 6 +++- test/integration/functional_test.go | 21 +++++++++++++- test/integration/start_stop_delete_test.go | 32 ++++++++++++++++++---- 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/pkg/minikube/cruntime/cruntime.go b/pkg/minikube/cruntime/cruntime.go index a7a17e9ffd..35351bd1da 100644 --- a/pkg/minikube/cruntime/cruntime.go +++ b/pkg/minikube/cruntime/cruntime.go @@ -25,6 +25,11 @@ import ( "k8s.io/minikube/pkg/minikube/out" ) +const ( + // OpenFilesMax is the maximum number of open files allowed by container runtimes. Arbitrary, but commonly used. + OpenFilesMax = 1048576 +) + // CommandRunner is the subset of command.Runner this package consumes type CommandRunner interface { Run(string) error diff --git a/pkg/provision/buildroot.go b/pkg/provision/buildroot.go index 2d6ff83e43..6001f86138 100644 --- a/pkg/provision/buildroot.go +++ b/pkg/provision/buildroot.go @@ -108,6 +108,8 @@ Requires= minikube-automount.service docker.socket [Service] Type=notify +# Automatically set options +Environment=default-ulimit=nofile=99997:99997 ` if noPivot { log.Warn("Using fundamentally insecure --no-pivot option") @@ -117,6 +119,7 @@ Environment=DOCKER_RAMDISK=yes ` } engineConfigTmpl += ` +# Passed in-options {{range .EngineOptions.Env}}Environment={{.}} {{end}} @@ -133,7 +136,7 @@ ExecReload=/bin/kill -s HUP $MAINPID # Having non-zero Limit*s causes performance problems due to accounting overhead # in the kernel. We recommend using cgroups to do container-local accounting. -LimitNOFILE=infinity +LimitNOFILE=1048576 LimitNPROC=infinity LimitCORE=infinity @@ -160,6 +163,7 @@ WantedBy=multi-user.target DockerPort: dockerPort, AuthOptions: p.AuthOptions, EngineOptions: p.EngineOptions, + // OpenFilesMax: cruntime.OpenFilesLimit, } escapeSystemdDirectives(&engineConfigContext) diff --git a/test/integration/functional_test.go b/test/integration/functional_test.go index b3af56ec5c..51053cd9d6 100644 --- a/test/integration/functional_test.go +++ b/test/integration/functional_test.go @@ -93,6 +93,7 @@ func TestFunctional(t *testing.T) { {"PersistentVolumeClaim", validatePersistentVolumeClaim}, {"TunnelCmd", validateTunnelCmd}, {"SSHCmd", validateSSHCmd}, + {"MySQL", validateMySQL}, } for _, tc := range tests { tc := tc @@ -284,7 +285,7 @@ func validateCacheCmd(ctx context.Context, t *testing.T, profile string) { if NoneDriver() { t.Skipf("skipping: cache unsupported by none") } - for _, img := range []string{"busybox", "busybox:1.28.4-glibc"} { + for _, img := range []string{"busybox", "busybox:1.28.4-glibc", "mysql:5.6"} { rr, err := Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "cache", "add", img)) if err != nil { t.Errorf("%s failed: %v", rr.Args, err) @@ -465,6 +466,24 @@ func validateSSHCmd(ctx context.Context, t *testing.T, profile string) { } } +// validateMySQL validates a minimalist MySQL deployment +func validateMySQL(ctx context.Context, t *testing.T, profile string) { + rr, err := Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "replace", "--force", "-f", filepath.Join(*testdataDir, "mysql.yaml"))) + if err != nil { + t.Fatalf("%s failed: %v", rr.Args, err) + } + + names, err := PodWait(ctx, t, profile, "default", "app=mysql", 2*time.Minute) + if err != nil { + t.Errorf("nginx: %v", err) + } + + rr, err = Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "exec", names[0], "--", "mysql", "-ppassword", "-e", "show databases;")) + if err != nil { + t.Fatalf("%s failed: %v", rr.Args, err) + } +} + // startHTTPProxy runs a local http proxy and sets the env vars for it. func startHTTPProxy(t *testing.T) (*http.Server, error) { port, err := freeport.GetFreePort() diff --git a/test/integration/start_stop_delete_test.go b/test/integration/start_stop_delete_test.go index bf575baeee..504bbacbb8 100644 --- a/test/integration/start_stop_delete_test.go +++ b/test/integration/start_stop_delete_test.go @@ -23,12 +23,14 @@ import ( "fmt" "os/exec" "path/filepath" + "strconv" "strings" "testing" "time" "github.com/docker/machine/libmachine/state" "k8s.io/minikube/pkg/minikube/constants" + "k8s.io/minikube/pkg/minikube/cruntime" ) func TestStartStop(t *testing.T) { @@ -102,9 +104,27 @@ func TestStartStop(t *testing.T) { t.Fatalf("%s failed: %v", rr.Args, err) } - if _, err := PodWait(ctx, t, profile, "default", "integration-test=busybox", 2*time.Minute); err != nil { + names, err := PodWait(ctx, t, profile, "default", "integration-test=busybox", 2*time.Minute) + if err != nil { t.Fatalf("wait: %v", err) } + + // Use this pod to confirm that the runtime resource limits are sane + rr, err = Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "exec", names[0], "--", "/bin/sh", "-c", "ulimit -n")) + if err != nil { + t.Fatalf("ulimit: %v", err) + } + + got, err := strconv.ParseInt(strings.TrimSpace(rr.Stdout.String()), 10, 64) + if err != nil { + t.Errorf("ParseInt(%q): %v", rr.Stdout.String(), err) + } + + // Arbitrary value set by some container runtimes. If higher, apps like MySQL may make bad decisions. + expected := int64(cruntime.OpenFilesMax) + if got != expected { + t.Errorf("got max-files=%d, expected %d", got, expected) + } } rr, err = Run(t, exec.CommandContext(ctx, Target(), "stop", "-p", profile, "--alsologtostderr", "-v=3")) @@ -134,10 +154,12 @@ func TestStartStop(t *testing.T) { t.Errorf("status = %q; want = %q", got, state.Running) } - // Normally handled by cleanuprofile, but not fatal there - rr, err = Run(t, exec.CommandContext(ctx, Target(), "delete", "-p", profile)) - if err != nil { - t.Errorf("%s failed: %v", rr.Args, err) + if !*cleanup { + // Normally handled by cleanuprofile, but not fatal there + rr, err = Run(t, exec.CommandContext(ctx, Target(), "delete", "-p", profile)) + if err != nil { + t.Errorf("%s failed: %v", rr.Args, err) + } } }) } From 9bacb6a8e4bfa2b44ec43befc09f36b6a537136e Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Mon, 28 Oct 2019 22:33:18 -0700 Subject: [PATCH 2/5] Remove test settings from buildroot --- pkg/provision/buildroot.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/provision/buildroot.go b/pkg/provision/buildroot.go index 6001f86138..00c8fd6dd9 100644 --- a/pkg/provision/buildroot.go +++ b/pkg/provision/buildroot.go @@ -109,7 +109,7 @@ Requires= minikube-automount.service docker.socket Type=notify # Automatically set options -Environment=default-ulimit=nofile=99997:99997 +Environment=default-ulimit=nofile=1048576:1048576 ` if noPivot { log.Warn("Using fundamentally insecure --no-pivot option") @@ -136,7 +136,7 @@ ExecReload=/bin/kill -s HUP $MAINPID # Having non-zero Limit*s causes performance problems due to accounting overhead # in the kernel. We recommend using cgroups to do container-local accounting. -LimitNOFILE=1048576 +LimitNOFILE=infinity LimitNPROC=infinity LimitCORE=infinity From f34b51db6580b6de9cb213603616702353d8eef2 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 29 Oct 2019 07:56:31 -0700 Subject: [PATCH 3/5] default-ulimit should be an arg, not an environment var --- pkg/provision/buildroot.go | 6 +--- test/integration/start_stop_delete_test.go | 2 +- test/integration/testdata/mysql.yaml | 35 ++++++++++++++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 test/integration/testdata/mysql.yaml diff --git a/pkg/provision/buildroot.go b/pkg/provision/buildroot.go index 00c8fd6dd9..a362ed7d81 100644 --- a/pkg/provision/buildroot.go +++ b/pkg/provision/buildroot.go @@ -107,9 +107,6 @@ Requires= minikube-automount.service docker.socket [Service] Type=notify - -# Automatically set options -Environment=default-ulimit=nofile=1048576:1048576 ` if noPivot { log.Warn("Using fundamentally insecure --no-pivot option") @@ -119,7 +116,6 @@ Environment=DOCKER_RAMDISK=yes ` } engineConfigTmpl += ` -# Passed in-options {{range .EngineOptions.Env}}Environment={{.}} {{end}} @@ -131,7 +127,7 @@ Environment=DOCKER_RAMDISK=yes # will catch this invalid input and refuse to start the service with an error like: # Service has more than one ExecStart= setting, which is only allowed for Type=oneshot services. ExecStart= -ExecStart=/usr/bin/dockerd -H tcp://0.0.0.0:{{.DockerPort}} -H unix:///var/run/docker.sock --tlsverify --tlscacert {{.AuthOptions.CaCertRemotePath}} --tlscert {{.AuthOptions.ServerCertRemotePath}} --tlskey {{.AuthOptions.ServerKeyRemotePath}} {{ range .EngineOptions.Labels }}--label {{.}} {{ end }}{{ range .EngineOptions.InsecureRegistry }}--insecure-registry {{.}} {{ end }}{{ range .EngineOptions.RegistryMirror }}--registry-mirror {{.}} {{ end }}{{ range .EngineOptions.ArbitraryFlags }}--{{.}} {{ end }} +ExecStart=/usr/bin/dockerd -H tcp://0.0.0.0:{{.DockerPort}} -H unix:///var/run/docker.sock --default-ulimit=nofile=1048576:1048576 --tlsverify --tlscacert {{.AuthOptions.CaCertRemotePath}} --tlscert {{.AuthOptions.ServerCertRemotePath}} --tlskey {{.AuthOptions.ServerKeyRemotePath}} {{ range .EngineOptions.Labels }}--label {{.}} {{ end }}{{ range .EngineOptions.InsecureRegistry }}--insecure-registry {{.}} {{ end }}{{ range .EngineOptions.RegistryMirror }}--registry-mirror {{.}} {{ end }}{{ range .EngineOptions.ArbitraryFlags }}--{{.}} {{ end }} ExecReload=/bin/kill -s HUP $MAINPID # Having non-zero Limit*s causes performance problems due to accounting overhead diff --git a/test/integration/start_stop_delete_test.go b/test/integration/start_stop_delete_test.go index 504bbacbb8..e520ccb843 100644 --- a/test/integration/start_stop_delete_test.go +++ b/test/integration/start_stop_delete_test.go @@ -123,7 +123,7 @@ func TestStartStop(t *testing.T) { // Arbitrary value set by some container runtimes. If higher, apps like MySQL may make bad decisions. expected := int64(cruntime.OpenFilesMax) if got != expected { - t.Errorf("got max-files=%d, expected %d", got, expected) + t.Errorf("'ulimit -n' returned %d, expected %d", got, expected) } } diff --git a/test/integration/testdata/mysql.yaml b/test/integration/testdata/mysql.yaml new file mode 100644 index 0000000000..fd90187228 --- /dev/null +++ b/test/integration/testdata/mysql.yaml @@ -0,0 +1,35 @@ +apiVersion: v1 +kind: Service +metadata: + name: mysql +spec: + ports: + - port: 3306 + selector: + app: mysql +--- +apiVersion: apps/v1 # for versions before 1.9.0 use apps/v1beta2 +kind: Deployment +metadata: + name: mysql +spec: + selector: + matchLabels: + app: mysql + strategy: + type: Recreate + template: + metadata: + labels: + app: mysql + spec: + containers: + - image: mysql:5.6 + name: mysql + env: + # Use secret in real usage + - name: MYSQL_ROOT_PASSWORD + value: password + ports: + - containerPort: 3306 + name: mysql From a2e684cf8dd0e554987fcc3dc17edcfe0d962fe8 Mon Sep 17 00:00:00 2001 From: tstromberg Date: Tue, 29 Oct 2019 13:13:33 -0700 Subject: [PATCH 4/5] Retry mysql check, as mysqld doesn't come up fully configured immediately --- test/integration/functional_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/integration/functional_test.go b/test/integration/functional_test.go index 66f99f608c..3837a80abe 100644 --- a/test/integration/functional_test.go +++ b/test/integration/functional_test.go @@ -486,14 +486,17 @@ func validateMySQL(ctx context.Context, t *testing.T, profile string) { t.Fatalf("%s failed: %v", rr.Args, err) } - names, err := PodWait(ctx, t, profile, "default", "app=mysql", 2*time.Minute) - if err != nil { - t.Errorf("nginx: %v", err) + // Retry, as mysqld first comes up without users configured. Scan for names in case of a reschedule. + mysql := func() error { + names, err := PodWait(ctx, t, profile, "default", "app=mysql", 5*time.Second) + if err != nil { + return err + } + rr, err = Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "exec", names[0], "--", "mysql", "-ppassword", "-e", "show databases;")) + return err } - - rr, err = Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "exec", names[0], "--", "mysql", "-ppassword", "-e", "show databases;")) - if err != nil { - t.Fatalf("%s failed: %v", rr.Args, err) + if err = retry.Expo(mysql, 1*time.Second, 2*time.Minute); err != nil { + t.Errorf("mysql failing: %v", err) } } From f6bd4df15cb96904ff58876ad9ad1f952dabe12a Mon Sep 17 00:00:00 2001 From: tstromberg Date: Tue, 29 Oct 2019 14:21:53 -0700 Subject: [PATCH 5/5] Address feedback --- pkg/minikube/cruntime/cruntime.go | 5 ----- pkg/provision/buildroot.go | 4 +++- test/integration/start_stop_delete_test.go | 3 +-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/pkg/minikube/cruntime/cruntime.go b/pkg/minikube/cruntime/cruntime.go index 35351bd1da..a7a17e9ffd 100644 --- a/pkg/minikube/cruntime/cruntime.go +++ b/pkg/minikube/cruntime/cruntime.go @@ -25,11 +25,6 @@ import ( "k8s.io/minikube/pkg/minikube/out" ) -const ( - // OpenFilesMax is the maximum number of open files allowed by container runtimes. Arbitrary, but commonly used. - OpenFilesMax = 1048576 -) - // CommandRunner is the subset of command.Runner this package consumes type CommandRunner interface { Run(string) error diff --git a/pkg/provision/buildroot.go b/pkg/provision/buildroot.go index a362ed7d81..c000472975 100644 --- a/pkg/provision/buildroot.go +++ b/pkg/provision/buildroot.go @@ -126,6 +126,9 @@ Environment=DOCKER_RAMDISK=yes # a sequence of commands, which is not the desired behavior, nor is it valid -- systemd # will catch this invalid input and refuse to start the service with an error like: # Service has more than one ExecStart= setting, which is only allowed for Type=oneshot services. + +# NOTE: default-ulimit=nofile is set to an arbitrary number for consistency with other +# container runtimes. If left unlimited, it may result in OOM issues with MySQL. ExecStart= ExecStart=/usr/bin/dockerd -H tcp://0.0.0.0:{{.DockerPort}} -H unix:///var/run/docker.sock --default-ulimit=nofile=1048576:1048576 --tlsverify --tlscacert {{.AuthOptions.CaCertRemotePath}} --tlscert {{.AuthOptions.ServerCertRemotePath}} --tlskey {{.AuthOptions.ServerKeyRemotePath}} {{ range .EngineOptions.Labels }}--label {{.}} {{ end }}{{ range .EngineOptions.InsecureRegistry }}--insecure-registry {{.}} {{ end }}{{ range .EngineOptions.RegistryMirror }}--registry-mirror {{.}} {{ end }}{{ range .EngineOptions.ArbitraryFlags }}--{{.}} {{ end }} ExecReload=/bin/kill -s HUP $MAINPID @@ -159,7 +162,6 @@ WantedBy=multi-user.target DockerPort: dockerPort, AuthOptions: p.AuthOptions, EngineOptions: p.EngineOptions, - // OpenFilesMax: cruntime.OpenFilesLimit, } escapeSystemdDirectives(&engineConfigContext) diff --git a/test/integration/start_stop_delete_test.go b/test/integration/start_stop_delete_test.go index e520ccb843..f9bcdd9034 100644 --- a/test/integration/start_stop_delete_test.go +++ b/test/integration/start_stop_delete_test.go @@ -30,7 +30,6 @@ import ( "github.com/docker/machine/libmachine/state" "k8s.io/minikube/pkg/minikube/constants" - "k8s.io/minikube/pkg/minikube/cruntime" ) func TestStartStop(t *testing.T) { @@ -121,7 +120,7 @@ func TestStartStop(t *testing.T) { } // Arbitrary value set by some container runtimes. If higher, apps like MySQL may make bad decisions. - expected := int64(cruntime.OpenFilesMax) + expected := int64(1048576) if got != expected { t.Errorf("'ulimit -n' returned %d, expected %d", got, expected) }