Merge pull request #3985 from tstromberg/mount-intr

More reliable unmount w/ SIGINT, particularly on kvm2

cmd/minikube/cmd/mount.go

@@ -98,6 +98,7 @@ var mountCmd = &cobra.Command{
 		}
 		defer api.Close()
 		host, err := api.Load(config.GetMachineName())
+
 		if err != nil {
 			exit.WithError("Error loading api", err)
 		}
@@ -163,22 +164,32 @@ var mountCmd = &cobra.Command{
 			go func() {
 				console.OutStyle("fileserver", "Userspace file server: ")
 				ufs.StartServer(net.JoinHostPort(ip.String(), strconv.Itoa(port)), debugVal, hostPath)
+				console.OutStyle("stopped", "Userspace file server is shutdown")
 				wg.Done()
 			}()
 		}
 
+		// Use CommandRunner, as the native docker ssh service dies when Ctrl-C is received.
+		runner, err := machine.CommandRunner(host)
+		if err != nil {
+			exit.WithError("Failed to get command runner", err)
+		}
+
 		// Unmount if Ctrl-C or kill request is received.
 		c := make(chan os.Signal, 1)
 		signal.Notify(c, os.Interrupt, syscall.SIGTERM)
 		go func() {
 			for sig := range c {
 				console.OutStyle("unmount", "Unmounting %s ...", vmPath)
-				cluster.Unmount(host, vmPath)
+				err := cluster.Unmount(runner, vmPath)
+				if err != nil {
+					console.ErrStyle("failure", "Failed unmount: %v", err)
+				}
 				exit.WithCode(exit.Interrupted, "Exiting due to %s signal", sig)
 			}
 		}()
 
-		err = cluster.Mount(host, ip.String(), vmPath, cfg)
+		err = cluster.Mount(runner, ip.String(), vmPath, cfg)
 		if err != nil {
 			exit.WithError("mount failed", err)
 		}

pkg/minikube/cluster/mount.go

@@ -23,6 +23,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/golang/glog"
 	"github.com/pkg/errors"
 )
 
@@ -46,19 +47,21 @@ type MountConfig struct {
 	Options map[string]string
 }
 
-// hostRunner is the subset of host.Host used for mounting
-type hostRunner interface {
-	RunSSHCommand(cmd string) (string, error)
+// mountRunner is the subset of CommandRunner used for mounting
+type mountRunner interface {
+	CombinedOutput(string) (string, error)
 }
 
 // Mount runs the mount command from the 9p client on the VM to the 9p server on the host
-func Mount(h hostRunner, source string, target string, c *MountConfig) error {
-	if err := Unmount(h, target); err != nil {
+func Mount(r mountRunner, source string, target string, c *MountConfig) error {
+	if err := Unmount(r, target); err != nil {
 		return errors.Wrap(err, "umount")
 	}
 
 	cmd := fmt.Sprintf("sudo mkdir -m %o -p %s && %s", c.Mode, target, mntCmd(source, target, c))
-	out, err := h.RunSSHCommand(cmd)
+	glog.Infof("Will run: %s", cmd)
+	out, err := r.CombinedOutput(cmd)
+	glog.Infof("mount err=%s, out=%s", err, out)
 	if err != nil {
 		return errors.Wrap(err, out)
 	}
@@ -127,9 +130,31 @@ func mntCmd(source string, target string, c *MountConfig) string {
 	return fmt.Sprintf("sudo mount -t %s -o %s %s %s", c.Type, strings.Join(opts, ","), source, target)
 }
 
+// umountCmd returns a command for unmounting
+func umountCmd(target string, force bool) string {
+	flag := ""
+	if force {
+		flag = "-f "
+	}
+	// grep because findmnt will also display the parent!
+	return fmt.Sprintf("findmnt -T %s | grep %s && sudo umount %s%s || true", target, target, flag, target)
+}
+
 // Unmount unmounts a path
-func Unmount(h hostRunner, target string) error {
-	out, err := h.RunSSHCommand(fmt.Sprintf("findmnt -T %s && sudo umount %s || true", target, target))
+func Unmount(r mountRunner, target string) error {
+	cmd := umountCmd(target, false)
+	glog.Infof("Will run: %s", cmd)
+	out, err := r.CombinedOutput(cmd)
+	if err == nil {
+		return nil
+	}
+	glog.Warningf("initial unmount error: %v, out: %s", err, out)
+
+	// Try again, using force if needed.
+	cmd = umountCmd(target, true)
+	glog.Infof("Will run: %s", cmd)
+	out, err = r.CombinedOutput(cmd)
+	glog.Infof("unmount force err=%v, out=%s", err, out)
 	if err != nil {
 		return errors.Wrap(err, out)
 	}

pkg/minikube/cluster/mount_test.go

@@ -23,19 +23,19 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-type mockMountHost struct {
+type mockMountRunner struct {
 	cmds []string
 	T    *testing.T
 }
 
-func newMockMountHost(t *testing.T) *mockMountHost {
-	return &mockMountHost{
+func newMockMountRunner(t *testing.T) *mockMountRunner {
+	return &mockMountRunner{
 		T:    t,
 		cmds: []string{},
 	}
 }
 
-func (m *mockMountHost) RunSSHCommand(cmd string) (string, error) {
+func (m *mockMountRunner) CombinedOutput(cmd string) (string, error) {
 	m.cmds = append(m.cmds, cmd)
 	return "", nil
 }
@@ -54,7 +54,7 @@ func TestMount(t *testing.T) {
 			target: "target",
 			cfg:    &MountConfig{Type: "9p", Mode: os.FileMode(0700)},
 			want: []string{
-				"findmnt -T target && sudo umount target || true",
+				"findmnt -T target | grep target && sudo umount target || true",
 				"sudo mkdir -m 700 -p target && sudo mount -t 9p -o dfltgid=0,dfltuid=0 src target",
 			},
 		},
@@ -77,7 +77,7 @@ func TestMount(t *testing.T) {
 				"cache":    "fscache",
 			}},
 			want: []string{
-				"findmnt -T /target && sudo umount /target || true",
+				"findmnt -T /target | grep /target && sudo umount /target || true",
 				"sudo mkdir -m 777 -p /target && sudo mount -t 9p -o cache=fscache,dfltgid=72,dfltuid=82,noextend,version=9p2000.u 10.0.0.1 /target",
 			},
 		},
@@ -89,34 +89,34 @@ func TestMount(t *testing.T) {
 				"version": "9p2000.L",
 			}},
 			want: []string{
-				"findmnt -T tgt && sudo umount tgt || true",
+				"findmnt -T tgt | grep tgt && sudo umount tgt || true",
 				"sudo mkdir -m 700 -p tgt && sudo mount -t 9p -o dfltgid=0,dfltuid=0,version=9p2000.L src tgt",
 			},
 		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			h := newMockMountHost(t)
-			err := Mount(h, tc.source, tc.target, tc.cfg)
+			r := newMockMountRunner(t)
+			err := Mount(r, tc.source, tc.target, tc.cfg)
 			if err != nil {
 				t.Fatalf("Mount(%s, %s, %+v): %v", tc.source, tc.target, tc.cfg, err)
 			}
-			if diff := cmp.Diff(h.cmds, tc.want); diff != "" {
+			if diff := cmp.Diff(r.cmds, tc.want); diff != "" {
 				t.Errorf("command diff (-want +got): %s", diff)
 			}
 		})
 	}
 }
 
 func TestUnmount(t *testing.T) {
-	h := newMockMountHost(t)
-	err := Unmount(h, "/mnt")
+	r := newMockMountRunner(t)
+	err := Unmount(r, "/mnt")
 	if err != nil {
 		t.Fatalf("Unmount(/mnt): %v", err)
 	}
 
-	want := []string{"findmnt -T /mnt && sudo umount /mnt || true"}
-	if diff := cmp.Diff(h.cmds, want); diff != "" {
+	want := []string{"findmnt -T /mnt | grep /mnt && sudo umount /mnt || true"}
+	if diff := cmp.Diff(r.cmds, want); diff != "" {
 		t.Errorf("command diff (-want +got): %s", diff)
 	}
 }