From 2419cb72c7b29afefb0836f913086fc29e099f30 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Sat, 4 Apr 2020 12:41:54 -0700 Subject: [PATCH 01/10] Add test for multi-node without errors --- test/integration/no_error_shown_test.go | 62 +++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 test/integration/no_error_shown_test.go diff --git a/test/integration/no_error_shown_test.go b/test/integration/no_error_shown_test.go new file mode 100644 index 0000000000..59a9529f90 --- /dev/null +++ b/test/integration/no_error_shown_test.go @@ -0,0 +1,62 @@ +// +build integration + +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package integration + +import ( + "context" + "os/exec" + "strings" + "testing" +) + +// TestMultiNodeNoError asserts that there are no errors displayed +func TestMultiNodeNoError(t *testing.T) { + if NoneDriver() { + t.Skip("none driver always shows a warning") + } + MaybeParallel(t) + + if NeedsPortForward() { + t.Skip("Docker for Desktop can be noisy") + } + + profile := UniqueProfileName("no-error") + ctx, cancel := context.WithTimeout(context.Background(), Minutes(20)) + defer CleanupWithLogs(t, profile, cancel) + + // Use the most verbose logging for the simplest test. If it fails, something is very wrong. + args := append([]string{"start", "-p", profile, "--memory=1900", "-n=2", "--wait=false"}, StartArgs()...) + + rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) + if err != nil { + t.Errorf("failed to start minikube with args: %q : %v", rr.Command(), err) + } + + if rr.Stderr.Len() > 0 { + t.Errorf("unexpected stderr: %v", rr.Stderr.String()) + } + + stdout := rr.Stdout.String() + keywords := []string{"error", "fail", "warning", "conflict"} + for _, keyword := range keywords { + if strings.Contains(stdout, keyword) { + t.Errorf("unexpected %q in stdout: %s", keyword, stdout) + } + } +} From c922c16b97fedd9f388b60433152cdb364bed547 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Sat, 4 Apr 2020 12:50:44 -0700 Subject: [PATCH 02/10] Make this single-node for now --- test/integration/no_error_shown_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/no_error_shown_test.go b/test/integration/no_error_shown_test.go index 59a9529f90..299ff9d2ac 100644 --- a/test/integration/no_error_shown_test.go +++ b/test/integration/no_error_shown_test.go @@ -25,8 +25,8 @@ import ( "testing" ) -// TestMultiNodeNoError asserts that there are no errors displayed -func TestMultiNodeNoError(t *testing.T) { +// TestNoErrorShown asserts that there are no errors displayed +func TestNoErrorShown(t *testing.T) { if NoneDriver() { t.Skip("none driver always shows a warning") } @@ -40,8 +40,8 @@ func TestMultiNodeNoError(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), Minutes(20)) defer CleanupWithLogs(t, profile, cancel) - // Use the most verbose logging for the simplest test. If it fails, something is very wrong. - args := append([]string{"start", "-p", profile, "--memory=1900", "-n=2", "--wait=false"}, StartArgs()...) + // TODO: make this multi-node once it's stable + args := append([]string{"start", "-p", profile, "--memory=1900", "--wait=false"}, StartArgs()...) rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) if err != nil { From 3a32ce67106a58dc393285280bed310e18d58014 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Sat, 4 Apr 2020 13:24:31 -0700 Subject: [PATCH 03/10] Improve test --- test/integration/no_error_shown_test.go | 37 +++++++++++++++---------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/test/integration/no_error_shown_test.go b/test/integration/no_error_shown_test.go index 299ff9d2ac..9f28855377 100644 --- a/test/integration/no_error_shown_test.go +++ b/test/integration/no_error_shown_test.go @@ -25,38 +25,45 @@ import ( "testing" ) -// TestNoErrorShown asserts that there are no errors displayed +// TestMultiNodeNoErrorShown asserts that there are no errors displayed func TestNoErrorShown(t *testing.T) { if NoneDriver() { t.Skip("none driver always shows a warning") } MaybeParallel(t) - if NeedsPortForward() { - t.Skip("Docker for Desktop can be noisy") - } - - profile := UniqueProfileName("no-error") - ctx, cancel := context.WithTimeout(context.Background(), Minutes(20)) + profile := UniqueProfileName("nospam") + ctx, cancel := context.WithTimeout(context.Background(), Minutes(25)) defer CleanupWithLogs(t, profile, cancel) // TODO: make this multi-node once it's stable - args := append([]string{"start", "-p", profile, "--memory=1900", "--wait=false"}, StartArgs()...) + args := append([]string{"start", "-p", profile, "-n=1", "--memory=2250", "--wait=false"}, StartArgs()...) rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) if err != nil { t.Errorf("failed to start minikube with args: %q : %v", rr.Command(), err) } - if rr.Stderr.Len() > 0 { - t.Errorf("unexpected stderr: %v", rr.Stderr.String()) + for _, line := range strings.Split(rr.Stderr.String(), "\n") { + if strings.HasPrefix(line, "E") { + t.Errorf("unexpected error log in stderr: %q", line) + continue + } + + if strings.Contains(line, "kubectl") || strings.Contains(line, "slow") || strings.Contains(line, "long time") { + continue + } + if len(strings.TrimSpace(line)) > 0 { + t.Errorf("unexpected stderr line: %q", line) + } } - stdout := rr.Stdout.String() - keywords := []string{"error", "fail", "warning", "conflict"} - for _, keyword := range keywords { - if strings.Contains(stdout, keyword) { - t.Errorf("unexpected %q in stdout: %s", keyword, stdout) + for _, line := range strings.Split(rr.Stdout.String(), "\n") { + keywords := []string{"error", "fail", "warning", "conflict"} + for _, keyword := range keywords { + if strings.Contains(line, keyword) { + t.Errorf("unexpected %q in stdout line: %q", keyword, line) + } } } } From d89142f1e75d44320562caa50d0da10ffe254e3f Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 9 Apr 2020 05:00:42 -0700 Subject: [PATCH 04/10] Rename test --- .../{no_error_shown_test.go => error_spam_test.go} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename test/integration/{no_error_shown_test.go => error_spam_test.go} (92%) diff --git a/test/integration/no_error_shown_test.go b/test/integration/error_spam_test.go similarity index 92% rename from test/integration/no_error_shown_test.go rename to test/integration/error_spam_test.go index 9f28855377..98a4fa1508 100644 --- a/test/integration/no_error_shown_test.go +++ b/test/integration/error_spam_test.go @@ -25,8 +25,8 @@ import ( "testing" ) -// TestMultiNodeNoErrorShown asserts that there are no errors displayed -func TestNoErrorShown(t *testing.T) { +// TestErrorSpam asserts that there are no errors displayed +func TestErrorSpam(t *testing.T) { if NoneDriver() { t.Skip("none driver always shows a warning") } @@ -36,7 +36,7 @@ func TestNoErrorShown(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), Minutes(25)) defer CleanupWithLogs(t, profile, cancel) - // TODO: make this multi-node once it's stable + // This should likely use multi-node once it's ready args := append([]string{"start", "-p", profile, "-n=1", "--memory=2250", "--wait=false"}, StartArgs()...) rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) From 80d4ae529a6ef278b0374bd8ebd34dc5bb3cfde1 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 9 Apr 2020 22:41:00 -0700 Subject: [PATCH 05/10] Restart crio after applying the kic overlay (in background) --- pkg/minikube/bootstrapper/kubeadm/kubeadm.go | 34 ++++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go index b97707a5cf..ddfd8f0292 100644 --- a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go +++ b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go @@ -212,15 +212,18 @@ func (k *Bootstrapper) init(cfg config.ClusterConfig) error { return errors.Wrap(err, "run") } - // this is required for containerd and cri-o runtime. till we close https://github.com/kubernetes/minikube/issues/7428 - if driver.IsKIC(cfg.Driver) && cfg.KubernetesConfig.ContainerRuntime != "docker" { - if err := k.applyKicOverlay(cfg); err != nil { - return errors.Wrap(err, "apply kic overlay") - } - } - var wg sync.WaitGroup - wg.Add(3) + wg.Add(4) + + go func() { + // this is required for containerd and cri-o runtime. till we close https://github.com/kubernetes/minikube/issues/7428 + if driver.IsKIC(cfg.Driver) && cfg.KubernetesConfig.ContainerRuntime != "docker" { + if err := k.applyKicOverlay(cfg); err != nil { + glog.Errorf("failed to apply kic overlay: %v", err) + } + } + wg.Done() + }() go func() { if err := k.applyNodeLabels(cfg); err != nil { @@ -242,6 +245,7 @@ func (k *Bootstrapper) init(cfg config.ClusterConfig) error { } wg.Done() }() + wg.Wait() return nil } @@ -762,20 +766,30 @@ func startKubeletIfRequired(runner command.Runner, sm sysinit.Manager) error { // applyKicOverlay applies the CNI plugin needed to make kic work func (k *Bootstrapper) applyKicOverlay(cfg config.ClusterConfig) error { - // Allow no more than 5 seconds for apply kic overlay - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() + cmd := exec.CommandContext(ctx, "sudo", path.Join(vmpath.GuestPersistentDir, "binaries", cfg.KubernetesConfig.KubernetesVersion, "kubectl"), "create", fmt.Sprintf("--kubeconfig=%s", path.Join(vmpath.GuestPersistentDir, "kubeconfig")), "-f", "-") + b := bytes.Buffer{} if err := kicCNIConfig.Execute(&b, struct{ ImageName string }{ImageName: kic.OverlayImage}); err != nil { return err } + cmd.Stdin = bytes.NewReader(b.Bytes()) if rr, err := k.c.RunCmd(cmd); err != nil { return errors.Wrapf(err, "cmd: %s output: %s", rr.Command(), rr.Output()) } + + // CRIO needs to eventually know about CNI changes (#7380) + if cfg.KubernetesConfig.ContainerRuntime == "crio" { + sm := sysinit.New(k.c) + err := sm.Restart("crio") + return errors.Wrap(err, "restart crio") + } + return nil } From 94a1e87f13e0bce1b42d8cafd802fa6a94c2aea2 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 9 Apr 2020 22:43:03 -0700 Subject: [PATCH 06/10] Shorten comment --- pkg/minikube/bootstrapper/kubeadm/kubeadm.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go index ddfd8f0292..618437ef17 100644 --- a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go +++ b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go @@ -216,7 +216,7 @@ func (k *Bootstrapper) init(cfg config.ClusterConfig) error { wg.Add(4) go func() { - // this is required for containerd and cri-o runtime. till we close https://github.com/kubernetes/minikube/issues/7428 + // the overlay is required for containerd and cri-o runtime: see #7428 if driver.IsKIC(cfg.Driver) && cfg.KubernetesConfig.ContainerRuntime != "docker" { if err := k.applyKicOverlay(cfg); err != nil { glog.Errorf("failed to apply kic overlay: %v", err) From fbde33d10d110f9ff3a4628135dc8c51eee1a46b Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 9 Apr 2020 22:44:23 -0700 Subject: [PATCH 07/10] simplify --- pkg/minikube/bootstrapper/kubeadm/kubeadm.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go index 618437ef17..735032af12 100644 --- a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go +++ b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go @@ -783,11 +783,11 @@ func (k *Bootstrapper) applyKicOverlay(cfg config.ClusterConfig) error { return errors.Wrapf(err, "cmd: %s output: %s", rr.Command(), rr.Output()) } - // CRIO needs to eventually know about CNI changes (#7380) + // Inform cri-o that the CNI has changed if cfg.KubernetesConfig.ContainerRuntime == "crio" { - sm := sysinit.New(k.c) - err := sm.Restart("crio") - return errors.Wrap(err, "restart crio") + if err := sysinit.New(k.c).Restart("crio"); err != nil { + return errors.Wrap(err, "restart crio") + } } return nil From 7b8b6b027c5ea97e18684935d323476db1ca3624 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Fri, 10 Apr 2020 08:32:41 -0700 Subject: [PATCH 08/10] cert test flakiness: make write atomic, add log msgs --- pkg/minikube/assets/vm_assets.go | 16 ++++++++++++---- test/integration/functional_test.go | 9 +++++++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/pkg/minikube/assets/vm_assets.go b/pkg/minikube/assets/vm_assets.go index 3751a85e08..3d4541d754 100644 --- a/pkg/minikube/assets/vm_assets.go +++ b/pkg/minikube/assets/vm_assets.go @@ -93,15 +93,21 @@ func NewMemoryAssetTarget(d []byte, targetPath, permissions string) *MemoryAsset // NewFileAsset creates a new FileAsset func NewFileAsset(src, targetDir, targetName, permissions string) (*FileAsset, error) { glog.V(4).Infof("NewFileAsset: %s -> %s", src, path.Join(targetDir, targetName)) + f, err := os.Open(src) if err != nil { - return nil, errors.Wrapf(err, "Error opening file asset: %s", src) + return nil, errors.Wrap(err, "open") } + info, err := os.Stat(src) if err != nil { - return nil, errors.Wrapf(err, "Error getting info for %s", src) + return nil, errors.Wrapf(err, "stat") } - r := io.NewSectionReader(f, 0, info.Size()) + + if info.Size() == 0 { + glog.Warningf("NewFileAsset: %s is an empty file!", src) + } + return &FileAsset{ BaseAsset: BaseAsset{ SourcePath: src, @@ -109,7 +115,7 @@ func NewFileAsset(src, targetDir, targetName, permissions string) (*FileAsset, e TargetName: targetName, Permissions: permissions, }, - reader: r, + reader: io.NewSectionReader(f, 0, info.Size()), }, nil } @@ -117,6 +123,7 @@ func NewFileAsset(src, targetDir, targetName, permissions string) (*FileAsset, e func (f *FileAsset) GetLength() (flen int) { fi, err := os.Stat(f.SourcePath) if err != nil { + glog.Errorf("stat(%q) failed: %v", f.SourcePath, err) return 0 } return int(fi.Size()) @@ -126,6 +133,7 @@ func (f *FileAsset) GetLength() (flen int) { func (f *FileAsset) GetModTime() (time.Time, error) { fi, err := os.Stat(f.SourcePath) if err != nil { + glog.Errorf("stat(%q) failed: %v", f.SourcePath, err) return time.Time{}, err } return fi.ModTime(), nil diff --git a/test/integration/functional_test.go b/test/integration/functional_test.go index 8545e811a7..5f1ef0124b 100644 --- a/test/integration/functional_test.go +++ b/test/integration/functional_test.go @@ -814,11 +814,16 @@ func setupFileSync(ctx context.Context, t *testing.T, profile string) { testPem := "./testdata/minikube_test.pem" - err = copy.Copy(testPem, localTestCertPath()) - if err != nil { + // Write to a temp file for an atomic write + tmpPem := localTestCertPath() + ".pem" + if err := copy.Copy(testPem, tmpPem); err != nil { t.Fatalf("failed to copy %s: %v", testPem, err) } + if err := os.Rename(tmpPem, localTestCertPath()); err != nil { + t.Fatalf("failed to rename %s: %v", tmpPem, err) + } + want, err := os.Stat(testPem) if err != nil { t.Fatalf("stat failed: %v", err) From de45f94401231df539ca3f1fad38dc2bbc2d1bf6 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Fri, 10 Apr 2020 12:32:43 -0700 Subject: [PATCH 09/10] Skip containerd shutdown if Docker is bound to it --- pkg/minikube/cruntime/cruntime.go | 10 ++++++++++ pkg/minikube/cruntime/docker.go | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pkg/minikube/cruntime/cruntime.go b/pkg/minikube/cruntime/cruntime.go index 9bde921993..bbb410be13 100644 --- a/pkg/minikube/cruntime/cruntime.go +++ b/pkg/minikube/cruntime/cruntime.go @@ -166,6 +166,7 @@ func ContainerStatusCommand() string { // disableOthers disables all other runtimes except for me. func disableOthers(me Manager, cr CommandRunner) error { + // valid values returned by manager.Name() runtimes := []string{"containerd", "crio", "docker"} for _, name := range runtimes { @@ -178,13 +179,22 @@ func disableOthers(me Manager, cr CommandRunner) error { if r.Name() == me.Name() { continue } + + // Don't disable containerd if we are bound to it + if me.Name() == "Docker" && r.Name() == "containerd" && dockerBoundToContainerd(cr) { + glog.Infof("skipping containerd shutdown because we are bound to it") + continue + } + // runtime is already disabled, nothing to do. if !r.Active() { continue } + if err = r.Disable(); err != nil { glog.Warningf("disable failed: %v", err) } + // Validate that the runtime really is offline - and that Active & Disable are properly written. if r.Active() { return fmt.Errorf("%s is still active", r.Name()) diff --git a/pkg/minikube/cruntime/docker.go b/pkg/minikube/cruntime/docker.go index 236ae3510a..95f8554152 100644 --- a/pkg/minikube/cruntime/docker.go +++ b/pkg/minikube/cruntime/docker.go @@ -364,3 +364,18 @@ func DockerImagesPreloaded(runner command.Runner, images []string) bool { } return true } + +func dockerBoundToContainerd(runner command.Runner) bool { + // NOTE: assumes systemd + rr, err := runner.RunCmd(exec.Command("sudo", "systemctl", "cat", "docker.service")) + if err != nil { + glog.Warningf("unable to check if docker is bound to containerd") + return false + } + + if strings.Contains(rr.Stdout.String(), "\nBindsTo=containerd") { + return true + } + + return false +} From 35eb50fafa5f0dac8a907af5a86ff44ed71aee4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Str=C3=B6mberg?= Date: Fri, 10 Apr 2020 15:24:52 -0700 Subject: [PATCH 10/10] Revert "Restart crio after applying the kic overlay (in background)" --- pkg/minikube/bootstrapper/kubeadm/kubeadm.go | 34 ++++++-------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go index 735032af12..b97707a5cf 100644 --- a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go +++ b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go @@ -212,18 +212,15 @@ func (k *Bootstrapper) init(cfg config.ClusterConfig) error { return errors.Wrap(err, "run") } - var wg sync.WaitGroup - wg.Add(4) - - go func() { - // the overlay is required for containerd and cri-o runtime: see #7428 - if driver.IsKIC(cfg.Driver) && cfg.KubernetesConfig.ContainerRuntime != "docker" { - if err := k.applyKicOverlay(cfg); err != nil { - glog.Errorf("failed to apply kic overlay: %v", err) - } + // this is required for containerd and cri-o runtime. till we close https://github.com/kubernetes/minikube/issues/7428 + if driver.IsKIC(cfg.Driver) && cfg.KubernetesConfig.ContainerRuntime != "docker" { + if err := k.applyKicOverlay(cfg); err != nil { + return errors.Wrap(err, "apply kic overlay") } - wg.Done() - }() + } + + var wg sync.WaitGroup + wg.Add(3) go func() { if err := k.applyNodeLabels(cfg); err != nil { @@ -245,7 +242,6 @@ func (k *Bootstrapper) init(cfg config.ClusterConfig) error { } wg.Done() }() - wg.Wait() return nil } @@ -766,30 +762,20 @@ func startKubeletIfRequired(runner command.Runner, sm sysinit.Manager) error { // applyKicOverlay applies the CNI plugin needed to make kic work func (k *Bootstrapper) applyKicOverlay(cfg config.ClusterConfig) error { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + // Allow no more than 5 seconds for apply kic overlay + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - cmd := exec.CommandContext(ctx, "sudo", path.Join(vmpath.GuestPersistentDir, "binaries", cfg.KubernetesConfig.KubernetesVersion, "kubectl"), "create", fmt.Sprintf("--kubeconfig=%s", path.Join(vmpath.GuestPersistentDir, "kubeconfig")), "-f", "-") - b := bytes.Buffer{} if err := kicCNIConfig.Execute(&b, struct{ ImageName string }{ImageName: kic.OverlayImage}); err != nil { return err } - cmd.Stdin = bytes.NewReader(b.Bytes()) if rr, err := k.c.RunCmd(cmd); err != nil { return errors.Wrapf(err, "cmd: %s output: %s", rr.Command(), rr.Output()) } - - // Inform cri-o that the CNI has changed - if cfg.KubernetesConfig.ContainerRuntime == "crio" { - if err := sysinit.New(k.c).Restart("crio"); err != nil { - return errors.Wrap(err, "restart crio") - } - } - return nil }