From c3db098d4a686d94dee6b360e9db29e01b478580 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 14 Feb 2019 16:10:33 -0800 Subject: [PATCH] ListContainers should return empty list when no containers match --- Gopkg.lock | 4 +- pkg/drivers/none/none.go | 21 ++- pkg/minikube/cruntime/cri.go | 15 +++ pkg/minikube/cruntime/cruntime_test.go | 170 +++++++++++++++++++++++-- pkg/minikube/cruntime/docker.go | 19 ++- 5 files changed, 212 insertions(+), 17 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 2752a67341..571b119373 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -169,10 +169,11 @@ revision = "4030bb1f1f0c35b30ca7009e9ebd06849dd45306" [[projects]] - digest = "1:2e3c336fc7fde5c984d2841455a658a6d626450b1754a854b3b32e7a8f49a07a" + digest = "1:d2754cafcab0d22c13541618a8029a70a8959eb3525ff201fe971637e2274cd0" name = "github.com/google/go-cmp" packages = [ "cmp", + "cmp/cmpopts", "cmp/internal/diff", "cmp/internal/function", "cmp/internal/value", @@ -988,6 +989,7 @@ "github.com/docker/machine/libmachine/version", "github.com/golang/glog", "github.com/google/go-cmp/cmp", + "github.com/google/go-cmp/cmp/cmpopts", "github.com/google/go-containerregistry/pkg/authn", "github.com/google/go-containerregistry/pkg/name", "github.com/google/go-containerregistry/pkg/v1/remote", diff --git a/pkg/drivers/none/none.go b/pkg/drivers/none/none.go index 1a75954dd5..c53c6f630d 100644 --- a/pkg/drivers/none/none.go +++ b/pkg/drivers/none/none.go @@ -132,14 +132,27 @@ func (d *Driver) Kill() error { if err := stopKubelet(d.exec); err != nil { return errors.Wrap(err, "kubelet") } + + // First try to gracefully stop containers containers, err := d.runtime.ListContainers(cruntime.MinikubeContainerPrefix) if err != nil { return errors.Wrap(err, "containers") } + if len(containers) == 0 { + return nil + } // Try to be graceful before sending SIGKILL everywhere. if err := d.runtime.StopContainers(containers); err != nil { return errors.Wrap(err, "stop") } + + containers, err = d.runtime.ListContainers(cruntime.MinikubeContainerPrefix) + if err != nil { + return errors.Wrap(err, "containers") + } + if len(containers) == 0 { + return nil + } if err := d.runtime.KillContainers(containers); err != nil { return errors.Wrap(err, "kill") } @@ -151,7 +164,7 @@ func (d *Driver) Remove() error { if err := d.Kill(); err != nil { return errors.Wrap(err, "kill") } - // TODO(#3637): Make sure this calls into the bootstrapper to perform `kubeadm reset` + glog.Infof("Removing: %s", cleanupPaths) cmd := fmt.Sprintf("sudo rm -rf %s", strings.Join(cleanupPaths, " ")) if err := d.exec.Run(cmd); err != nil { glog.Errorf("cleanup incomplete: %v", err) @@ -187,8 +200,10 @@ func (d *Driver) Stop() error { if err != nil { return errors.Wrap(err, "containers") } - if err := d.runtime.StopContainers(containers); err != nil { - return errors.Wrap(err, "stop") + if len(containers) > 0 { + if err := d.runtime.StopContainers(containers); err != nil { + return errors.Wrap(err, "stop") + } } return nil } diff --git a/pkg/minikube/cruntime/cri.go b/pkg/minikube/cruntime/cri.go index 67f4595ef0..cbf4a1cd00 100644 --- a/pkg/minikube/cruntime/cri.go +++ b/pkg/minikube/cruntime/cri.go @@ -22,6 +22,8 @@ import ( "html/template" "path" "strings" + + "github.com/golang/glog" ) // listCRIContainers returns a list of containers using crictl @@ -30,16 +32,29 @@ func listCRIContainers(cr CommandRunner, filter string) ([]string, error) { if err != nil { return nil, err } + content = strings.TrimSpace(content) + // Otherwise, strings.Split will return []string{""} + if content == "" { + return []string{}, nil + } return strings.Split(content, "\n"), nil } // criCRIContainers kills a list of containers using crictl func killCRIContainers(cr CommandRunner, ids []string) error { + if len(ids) == 0 { + glog.Warningf("KillContainers was called with an empty list of ids, nothing to do.") + return nil + } return cr.Run(fmt.Sprintf("sudo crictl rm %s", strings.Join(ids, " "))) } // stopCRIContainers stops containers using crictl func stopCRIContainers(cr CommandRunner, ids []string) error { + if len(ids) == 0 { + glog.Warningf("StopContainers was called with an empty list of ids, nothing to do.") + return nil + } return cr.Run(fmt.Sprintf("sudo crictl stop %s", strings.Join(ids, " "))) } diff --git a/pkg/minikube/cruntime/cruntime_test.go b/pkg/minikube/cruntime/cruntime_test.go index 4e28455235..6f8097ade6 100644 --- a/pkg/minikube/cruntime/cruntime_test.go +++ b/pkg/minikube/cruntime/cruntime_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/minikube/pkg/minikube/console" ) @@ -97,24 +98,25 @@ const ( // FakeRunner is a command runner that isn't very smart. type FakeRunner struct { - cmds []string - services map[string]serviceState - t *testing.T + cmds []string + services map[string]serviceState + containers map[string]string + t *testing.T } // NewFakeRunner returns a CommandRunner which emulates a systemd host func NewFakeRunner(t *testing.T) *FakeRunner { return &FakeRunner{ - services: map[string]serviceState{}, - cmds: []string{}, - t: t, + services: map[string]serviceState{}, + cmds: []string{}, + t: t, + containers: map[string]string{}, } } // Run a fake command! func (f *FakeRunner) CombinedOutput(cmd string) (string, error) { f.cmds = append(f.cmds, cmd) - out := "" root := false args := strings.Split(cmd, " ") @@ -124,13 +126,16 @@ func (f *FakeRunner) CombinedOutput(cmd string) (string, error) { root = true bin, args = args[0], args[1:] } - if bin == "systemctl" { + switch bin { + case "systemctl": return f.systemctl(args, root) - } - if bin == "docker" { + case "docker": return f.docker(args, root) + case "crictl": + return f.crictl(args, root) + default: + return "", nil } - return out, nil } // Run a fake command! @@ -141,6 +146,81 @@ func (f *FakeRunner) Run(cmd string) error { // docker is a fake implementation of docker func (f *FakeRunner) docker(args []string, root bool) (string, error) { + switch cmd := args[0]; cmd { + case "ps": + // ps -a --filter="name=apiserver" --format="{{.ID}}" + if args[1] == "-a" && strings.HasPrefix(args[2], "--filter") { + filter := strings.Split(args[2], `"`)[1] + fname := strings.Split(filter, "=")[1] + ids := []string{} + f.t.Logf("Looking for containers matching %q", fname) + for id, cname := range f.containers { + if strings.Contains(cname, fname) { + ids = append(ids, id) + } + } + f.t.Logf("Found containers: %v", ids) + return strings.Join(ids, "\n"), nil + } + case "stop": + for _, id := range args[1:] { + f.t.Logf("Stopping id %q", id) + if f.containers[id] == "" { + return "", fmt.Errorf("no such container") + } + delete(f.containers, id) + } + case "rm": + // Skip "-f" argument + for _, id := range args[2:] { + f.t.Logf("Removing id %q", id) + if f.containers[id] == "" { + return "", fmt.Errorf("no such container") + } + delete(f.containers, id) + + } + + } + return "", nil +} + +// crictl is a fake implementation of crictl +func (f *FakeRunner) crictl(args []string, root bool) (string, error) { + switch cmd := args[0]; cmd { + case "ps": + // crictl ps -a --name=apiserver --quiet + if args[1] == "-a" && strings.HasPrefix(args[2], "--name") { + fname := strings.Split(args[2], "=")[1] + ids := []string{} + f.t.Logf("Looking for containers matching %q", fname) + for id, cname := range f.containers { + if strings.Contains(cname, fname) { + ids = append(ids, id) + } + } + f.t.Logf("Found containers: %v", ids) + return strings.Join(ids, "\n"), nil + } + case "stop": + for _, id := range args[1:] { + f.t.Logf("Stopping id %q", id) + if f.containers[id] == "" { + return "", fmt.Errorf("no such container") + } + delete(f.containers, id) + } + case "rm": + for _, id := range args[1:] { + f.t.Logf("Removing id %q", id) + if f.containers[id] == "" { + return "", fmt.Errorf("no such container") + } + delete(f.containers, id) + + } + + } return "", nil } @@ -281,3 +361,71 @@ func TestEnable(t *testing.T) { }) } } + +func TestContainerFunctions(t *testing.T) { + var tests = []struct { + runtime string + }{ + {"docker"}, + {"crio"}, + {"containerd"}, + } + + sortSlices := cmpopts.SortSlices(func(a, b string) bool { return a < b }) + for _, tc := range tests { + t.Run(tc.runtime, func(t *testing.T) { + runner := NewFakeRunner(t) + runner.containers = map[string]string{ + "abc0": "k8s_apiserver", + "fgh1": "k8s_coredns", + "xyz2": "k8s_storage", + "zzz": "unrelated", + } + cr, err := New(Config{Type: tc.runtime, Runner: runner}) + if err != nil { + t.Fatalf("New(%s): %v", tc.runtime, err) + } + + // Get the list of apiservers + got, err := cr.ListContainers("apiserver") + if err != nil { + t.Fatalf("ListContainers: %v", err) + } + want := []string{"abc0"} + if !cmp.Equal(got, want) { + t.Errorf("ListContainers(apiserver) = %v, want %v", got, want) + } + + // Stop the containers and assert that they have disappeared + cr.StopContainers(got) + got, err = cr.ListContainers("apiserver") + if err != nil { + t.Fatalf("ListContainers: %v", err) + } + want = []string{} + if diff := cmp.Diff(got, want, sortSlices); diff != "" { + t.Errorf("ListContainers(apiserver) unexpected results, diff (-got + want): %s", diff) + } + + // Get the list of everything else. + got, err = cr.ListContainers(MinikubeContainerPrefix) + if err != nil { + t.Fatalf("ListContainers: %v", err) + } + want = []string{"fgh1", "xyz2"} + if diff := cmp.Diff(got, want, sortSlices); diff != "" { + t.Errorf("ListContainers(apiserver) unexpected results, diff (-got + want): %s", diff) + } + + // Kill the containers and assert that they have disappeared + cr.KillContainers(got) + got, err = cr.ListContainers(MinikubeContainerPrefix) + if err != nil { + t.Fatalf("ListContainers: %v", err) + } + if len(got) > 0 { + t.Errorf("ListContainers(apiserver) = %v, want 0 items", got) + } + }) + } +} diff --git a/pkg/minikube/cruntime/docker.go b/pkg/minikube/cruntime/docker.go index 92f4c0bb54..51f0df5243 100644 --- a/pkg/minikube/cruntime/docker.go +++ b/pkg/minikube/cruntime/docker.go @@ -89,15 +89,30 @@ func (r *Docker) ListContainers(filter string) ([]string, error) { if err != nil { return nil, err } + content = strings.TrimSpace(content) + // Otherwise, strings.Split will return []string{""} + if content == "" { + return []string{}, nil + } return strings.Split(content, "\n"), nil } -// KillContainers forcibly removes a running pod based on ID +// KillContainers forcibly removes a running container based on ID func (r *Docker) KillContainers(ids []string) error { + if len(ids) == 0 { + glog.Warningf("KillContainers was called with an empty list of ids, nothing to do.") + return nil + } + glog.Infof("Killing containers: %s", ids) return r.Runner.Run(fmt.Sprintf("docker rm -f %s", strings.Join(ids, " "))) } -// StopContainers stops a running pod based on ID +// StopContainers stops a running container based on ID func (r *Docker) StopContainers(ids []string) error { + if len(ids) == 0 { + glog.Warningf("StopContainers was called with an empty list of ids, nothing to do.") + return nil + } + glog.Infof("Killing containers: %s", ids) return r.Runner.Run(fmt.Sprintf("docker stop %s", strings.Join(ids, " "))) }