Merge pull request #15782 from x7upLime/fix-minikube_mount

Fixes mount cleaning mechanism

cmd/minikube/cmd/delete.go

@@ -584,9 +584,10 @@ func deleteMachineDirectories(cc *config.ClusterConfig) {
 	}
 }
 
-// killMountProcess kills the mount process, if it is running
+// killMountProcess looks for the legacy path and for profile path for a pidfile,
+// it then tries to kill all the pids listed in the pidfile (one or more)
 func killMountProcess() error {
-	profile := viper.GetString("profile")
+	profile := ClusterFlagValue()
 	paths := []string{
 		localpath.MiniPath(), // legacy mount-process path for backwards compatibility
 		localpath.Profile(profile),
@@ -601,49 +602,120 @@ func killMountProcess() error {
 	return nil
 }
 
+// killProcess takes a path to look for a pidfile (space-separated),
+// it reads the file and converts it to a bunch of pid ints,
+// then it tries to kill each one of them.
+// If no errors were encountered, it cleans the pidfile
 func killProcess(path string) error {
 	pidPath := filepath.Join(path, constants.MountProcessFileName)
 	if _, err := os.Stat(pidPath); os.IsNotExist(err) {
 		return nil
 	}
-
 	klog.Infof("Found %s ...", pidPath)
-	out, err := os.ReadFile(pidPath)
+
+	ppp, err := getPids(pidPath)
 	if err != nil {
-		return errors.Wrap(err, "ReadFile")
+		return err
 	}
-	klog.Infof("pidfile contents: %s", out)
-	pid, err := strconv.Atoi(string(out))
-	if err != nil {
-		return errors.Wrap(err, "error parsing pid")
+
+	// we're trying to kill each process, without stopping at first error encountered
+	// error handling is done below
+	var errs []error
+	for _, pp := range ppp {
+		err := trySigKillProcess(pp)
+		if err != nil {
+			errs = append(errs, err)
+		}
+
 	}
-	// os.FindProcess does not check if pid is running :(
-	entry, err := ps.FindProcess(pid)
+
+	if len(errs) == 1 {
+		// if we've encountered only one error, we're returning it:
+		return errs[0]
+	} else if len(errs) != 0 {
+		// if multiple errors were encountered, combine them into a single error
+		out.Styled(style.Failure, "Multiple errors encountered:")
+		for _, e := range errs {
+			out.Err("%v\n", e)
+		}
+		return errors.New("multiple errors encountered while closing mount processes")
+	}
+
+	// if no errors were encoutered, it's safe to delete pidFile
+	if err := os.Remove(pidPath); err != nil {
+		return errors.Wrap(err, "while closing mount-pids file")
+	}
+
+	return nil
+}
+
+// trySigKillProcess takes a PID as argument and tries to SIGKILL it.
+// It performs an ownership check of the pid,
+// before trying to send a sigkill signal to it
+func trySigKillProcess(pid int) error {
+	itDoes, err := isMinikubeProcess(pid)
 	if err != nil {
-		return errors.Wrap(err, "ps.FindProcess")
+		return err
 	}
-	if entry == nil {
-		klog.Infof("Stale pid: %d", pid)
-		if err := os.Remove(pidPath); err != nil {
-			return errors.Wrap(err, "Removing stale pid")
-		}
-		return nil
+
+	if !itDoes {
+		return fmt.Errorf("stale pid: %d", pid)
 	}
 
-	// We found a process, but it still may not be ours.
-	klog.Infof("Found process %d: %s", pid, entry.Executable())
 	proc, err := os.FindProcess(pid)
 	if err != nil {
-		return errors.Wrap(err, "os.FindProcess")
+		return errors.Wrapf(err, "os.FindProcess: %d", pid)
 	}
 
 	klog.Infof("Killing pid %d ...", pid)
 	if err := proc.Kill(); err != nil {
 		klog.Infof("Kill failed with %v - removing probably stale pid...", err)
-		if err := os.Remove(pidPath); err != nil {
-			return errors.Wrap(err, "Removing likely stale unkillable pid")
-		}
-		return errors.Wrap(err, fmt.Sprintf("Kill(%d/%s)", pid, entry.Executable()))
+		return errors.Wrapf(err, "removing likely stale unkillable pid: %d", pid)
 	}
+
 	return nil
 }
+
+// doesPIDBelongToMinikube tries to find the process with that PID
+// and checks if the executable name contains the string "minikube"
+var isMinikubeProcess = func(pid int) (bool, error) {
+	entry, err := ps.FindProcess(pid)
+	if err != nil {
+		return false, errors.Wrapf(err, "ps.FindProcess for %d", pid)
+	}
+	if entry == nil {
+		klog.Infof("Process not found. pid %d", pid)
+		return false, nil
+	}
+
+	klog.Infof("Found process %d", pid)
+	if !strings.Contains(entry.Executable(), "minikube") {
+		klog.Infof("process %d was not started by minikube", pid)
+		return false, nil
+	}
+
+	return true, nil
+}
+
+// getPids opens the file at PATH and tries to read
+// one or more space separated pids
+func getPids(path string) ([]int, error) {
+	out, err := os.ReadFile(path)
+	if err != nil {
+		return nil, errors.Wrap(err, "ReadFile")
+	}
+	klog.Infof("pidfile contents: %s", out)
+
+	pids := []int{}
+	strPids := strings.Fields(string(out))
+	for _, p := range strPids {
+		intPid, err := strconv.Atoi(p)
+		if err != nil {
+			return nil, err
+		}
+
+		pids = append(pids, intPid)
+	}
+
+	return pids, nil
+}

cmd/minikube/cmd/delete_test.go

@@ -19,7 +19,9 @@ package cmd
 import (
 	"fmt"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/docker/machine/libmachine"
@@ -220,3 +222,64 @@ func TestDeleteAllProfiles(t *testing.T) {
 
 	viper.Set(config.ProfileName, "")
 }
+
+// TestTryKillOne spawns a go child process that waits to be SIGKILLed,
+// then tries to execute the tryKillOne function on it;
+// if after tryKillOne the process still exists, we consider it a failure
+func TestTryKillOne(t *testing.T) {
+
+	var waitForSig = []byte(`
+package main
+
+import (
+	"os"
+	"os/signal"
+	"syscall"
+)
+
+// This is used to unit test functions that send termination
+// signals to processes, in a cross-platform way.
+func main() {
+	ch := make(chan os.Signal, 1)
+	done := make(chan struct{})
+	defer close(ch)
+
+	signal.Notify(ch, syscall.SIGHUP)
+	defer signal.Stop(ch)
+
+	go func() {
+		<-ch
+		close(done)
+	}()
+
+	<-done
+}
+`)
+	td := t.TempDir()
+	tmpfile := filepath.Join(td, "waitForSig.go")
+
+	if err := os.WriteFile(tmpfile, waitForSig, 0o600); err != nil {
+		t.Fatalf("copying source to %s: %v\n", tmpfile, err)
+	}
+
+	processToKill := exec.Command("go", "run", tmpfile)
+	err := processToKill.Start()
+	if err != nil {
+		t.Fatalf("while execing child process: %v\n", err)
+	}
+	pid := processToKill.Process.Pid
+
+	isMinikubeProcess = func(int) (bool, error) {
+		return true, nil
+	}
+
+	err = trySigKillProcess(pid)
+	if err != nil {
+		t.Fatalf("while trying to kill child proc %d: %v\n", pid, err)
+	}
+
+	// waiting for process to exit
+	if err := processToKill.Wait(); !strings.Contains(err.Error(), "killed") {
+		t.Fatalf("unable to kill process: %v\n", err)
+	}
+}

cmd/minikube/cmd/mount.go

@@ -21,6 +21,7 @@ import (
 	"net"
 	"os"
 	"os/signal"
+	"path/filepath"
 	"runtime"
 	"strconv"
 	"strings"
@@ -35,11 +36,13 @@ import (
 	"k8s.io/minikube/pkg/minikube/detect"
 	"k8s.io/minikube/pkg/minikube/driver"
 	"k8s.io/minikube/pkg/minikube/exit"
+	"k8s.io/minikube/pkg/minikube/localpath"
 	"k8s.io/minikube/pkg/minikube/mustload"
 	"k8s.io/minikube/pkg/minikube/out"
 	"k8s.io/minikube/pkg/minikube/reason"
 	"k8s.io/minikube/pkg/minikube/style"
 	pkgnetwork "k8s.io/minikube/pkg/network"
+	"k8s.io/minikube/pkg/util/lock"
 	"k8s.io/minikube/third_party/go9p/ufs"
 )
 
@@ -202,15 +205,18 @@ var mountCmd = &cobra.Command{
 		out.Infof("Bind Address: {{.Address}}", out.V{"Address": net.JoinHostPort(bindIP, fmt.Sprint(port))})
 
 		var wg sync.WaitGroup
+		pidchan := make(chan int)
 		if cfg.Type == nineP {
 			wg.Add(1)
-			go func() {
+			go func(pid chan int) {
+				pid <- os.Getpid()
 				out.Styled(style.Fileserver, "Userspace file server: ")
 				ufs.StartServer(net.JoinHostPort(bindIP, strconv.Itoa(port)), debugVal, hostPath)
 				out.Step(style.Stopped, "Userspace file server is shutdown")
 				wg.Done()
-			}()
+			}(pidchan)
 		}
+		pid := <-pidchan
 
 		// Unmount if Ctrl-C or kill request is received.
 		c := make(chan os.Signal, 1)
@@ -222,11 +228,17 @@ var mountCmd = &cobra.Command{
 				if err != nil {
 					out.FailureT("Failed unmount: {{.error}}", out.V{"error": err})
 				}
+
+				err = removePidFromFile(pid)
+				if err != nil {
+					out.FailureT("Failed removing pid from pidfile: {{.error}}", out.V{"error": err})
+				}
+
 				exit.Message(reason.Interrupted, "Received {{.name}} signal", out.V{"name": sig})
 			}
 		}()
 
-		err = cluster.Mount(co.CP.Runner, ip.String(), vmPath, cfg)
+		err = cluster.Mount(co.CP.Runner, ip.String(), vmPath, cfg, pid)
 		if err != nil {
 			if rtErr, ok := err.(*cluster.MountError); ok && rtErr.ErrorType == cluster.MountErrorConnect {
 				exit.Error(reason.GuestMountCouldNotConnect, "mount could not connect", rtErr)
@@ -266,3 +278,56 @@ func getPort() (int, error) {
 	defer l.Close()
 	return l.Addr().(*net.TCPAddr).Port, nil
 }
+
+// removePidFromFile looks at the default locations for the mount-pids file,
+// for the profile in use. If a file is found and its content shows PID, PID gets removed.
+func removePidFromFile(pid int) error {
+	profile := ClusterFlagValue()
+	paths := []string{
+		localpath.MiniPath(), // legacy mount-process path for backwards compatibility
+		localpath.Profile(profile),
+	}
+
+	for _, path := range paths {
+		err := removePid(path, strconv.Itoa(pid))
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// removePid reads the file at PATH and tries to remove PID from it if found
+func removePid(path string, pid string) error {
+	// is it the file we're looking for?
+	pidPath := filepath.Join(path, constants.MountProcessFileName)
+	if _, err := os.Stat(pidPath); os.IsNotExist(err) {
+		return nil
+	}
+
+	// we found the correct file
+	// we're reading the pids...
+	out, err := os.ReadFile(pidPath)
+	if err != nil {
+		return errors.Wrap(err, "readFile")
+	}
+
+	pids := []string{}
+	// we're splitting the mount-pids file content into a slice of strings
+	// so that we can compare each to the PID we're looking for
+	strPids := strings.Fields(string(out))
+	for _, p := range strPids {
+		// If we find the PID, we don't add it to the slice
+		if p == pid {
+			continue
+		}
+
+		// if p doesn't correspond to PID, we add to a list
+		pids = append(pids, p)
+	}
+
+	// we write the slice that we obtained back to the mount-pids file
+	newPids := strings.Join(pids, " ")
+	return lock.WriteFile(pidPath, []byte(newPids), 0o644)
+}

pkg/minikube/cluster/mount.go

@@ -19,13 +19,20 @@ package cluster
 import (
 	"fmt"
 	"os/exec"
+	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
 
 	"github.com/pkg/errors"
+	"github.com/spf13/viper"
 	"k8s.io/klog/v2"
 	"k8s.io/minikube/pkg/minikube/command"
+	"k8s.io/minikube/pkg/minikube/constants"
+	"k8s.io/minikube/pkg/minikube/exit"
+	"k8s.io/minikube/pkg/minikube/localpath"
+	"k8s.io/minikube/pkg/minikube/reason"
+	"k8s.io/minikube/pkg/util/lock"
 )
 
 // MountConfig defines the options available to the Mount command
@@ -73,7 +80,7 @@ func (m *MountError) Error() string {
 }
 
 // Mount runs the mount command from the 9p client on the VM to the 9p server on the host
-func Mount(r mountRunner, source string, target string, c *MountConfig) error {
+func Mount(r mountRunner, source string, target string, c *MountConfig, pid int) error {
 	if err := Unmount(r, target); err != nil {
 		return &MountError{ErrorType: MountErrorUnknown, UnderlyingError: errors.Wrap(err, "umount")}
 	}
@@ -90,6 +97,11 @@ func Mount(r mountRunner, source string, target string, c *MountConfig) error {
 		return &MountError{ErrorType: MountErrorUnknown, UnderlyingError: errors.Wrapf(err, "mount with cmd %s ", rr.Command())}
 	}
 
+	profile := viper.GetString("profile")
+	if err := lock.AppendToFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(fmt.Sprintf(" %s", strconv.Itoa(pid))), 0o644); err != nil {
+		exit.Error(reason.HostMountPid, "Error writing mount pid", err)
+	}
+
 	klog.Infof("mount successful: %q", rr.Output())
 	return nil
 }