ListContainers should return empty list when no containers match

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",

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
 }

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, " ")))
 }
 

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)
+			}
+		})
+	}
+}

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, " ")))
 }