Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes mount cleaning mechanism #15782

Merged
merged 39 commits into from
Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
9d4f296
Adjusts mount processes cleaning logic
x7upLime Feb 3, 2023
47981b7
Add integration test for mount-clean mechanism
x7upLime Feb 5, 2023
07f3f44
Fixes lint error
x7upLime Feb 5, 2023
849de90
Adds documentation for validateMountCmd test
x7upLime Feb 5, 2023
07d2070
Corrects error handling in killProcess
x7upLime Feb 5, 2023
e34548f
Adds description to lock.AppendToFile
x7upLime Feb 8, 2023
70a0ed2
Modifies integration test label
x7upLime Feb 8, 2023
3f47e72
Modularizes killProcess()
x7upLime Feb 8, 2023
60c3863
Changes klog version and adds it to go.sum
x7upLime Feb 8, 2023
b1bd26a
Adds unit test for new helper function tryKillOne
x7upLime Feb 9, 2023
e91f041
Stops govet from complaining
x7upLime Feb 9, 2023
badc971
Adds source for cross platform killable process to test
x7upLime Feb 10, 2023
ead0b40
Changes name to unittest tool
x7upLime Feb 12, 2023
b5d5bbe
Removes external lib dependency
x7upLime Feb 12, 2023
51e46b6
Refactors GetPids
x7upLime Feb 14, 2023
e28a1b4
Removes pidfile util
x7upLime Feb 15, 2023
ee22b47
Apply suggestions from code review
x7upLime Feb 18, 2023
43ff38b
Changes variable name in test
x7upLime Feb 18, 2023
3b3d290
Changes variable name in integrataion test
x7upLime Feb 18, 2023
db14047
Changes helper killProcess function name
x7upLime Feb 18, 2023
b368c64
Update cmd/minikube/cmd/delete.go
x7upLime Feb 18, 2023
1d5b316
Makes comment more clear
x7upLime Feb 18, 2023
b5e59b5
Renames sigkill function
x7upLime Mar 6, 2023
ddec310
Ensures sigkilling only minikube started processes
x7upLime Mar 7, 2023
d72f2fb
Changes pid owner conditional
x7upLime Mar 14, 2023
e5b57f8
Changes the way in which pid is written to mounts file
x7upLime Mar 22, 2023
9665bad
Generates docs
x7upLime Mar 22, 2023
c55a4ca
Adds the ownership check as a dependency
x7upLime Mar 22, 2023
82abc0d
Adapts unit test to new dependency
x7upLime Mar 22, 2023
595c8ee
Adapts TestTryKillOne log for failing
x7upLime Mar 22, 2023
8f50ee9
Corrects mendacious comment
x7upLime Mar 22, 2023
1398988
Regenerates docs
x7upLime Mar 22, 2023
b09cd43
Corrects style issue
x7upLime Mar 22, 2023
0170981
Saves us from passing func as parameter
x7upLime Mar 22, 2023
b196ee4
Apply suggestions from code review
x7upLime Apr 20, 2023
3d3e6a1
added stuff to reflect suggestions
x7upLime Apr 20, 2023
aab0cb6
Adds mechanism to remove pid from pidfile if mount is sigtermd
x7upLime Apr 21, 2023
455bcb1
refactors removePid logic
x7upLime Apr 22, 2023
8931619
Applied suggestions
x7upLime Apr 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 96 additions & 24 deletions cmd/minikube/cmd/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,8 @@ 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")
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
paths := []string{
Expand All @@ -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")
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
}

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.Wrap(err, fmt.Sprintf("os.FindProcess: %d", pid))
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
}

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.Wrap(err, fmt.Sprintf("Removing likely stale unkillable pid: %d", pid))
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
}

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.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid))
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
}
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
}
63 changes: 63 additions & 0 deletions cmd/minikube/cmd/delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ package cmd
import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"

"github.com/docker/machine/libmachine"
Expand Down Expand Up @@ -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)
}
}
74 changes: 71 additions & 3 deletions cmd/minikube/cmd/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"net"
"os"
"os/signal"
"path/filepath"
"runtime"
"strconv"
"strings"
Expand All @@ -29,17 +30,20 @@ import (

"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"k8s.io/klog/v2"
"k8s.io/minikube/pkg/minikube/cluster"
"k8s.io/minikube/pkg/minikube/constants"
"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"
)

Expand Down Expand Up @@ -202,15 +206,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)
Expand All @@ -222,11 +229,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)
Expand Down Expand Up @@ -266,3 +279,58 @@ 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 := viper.GetString("profile")
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
paths := []string{
localpath.MiniPath(), // legacy mount-process path for backwards compatibility
localpath.Profile(profile),
}

for _, path := range paths {
err := removePid(path, 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 int) 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")
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
}

pids := []int{}
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
strPids := strings.Fields(string(out))
for _, p := range strPids {
intPid, err := strconv.Atoi(p)
if err != nil {
return errors.Wrap(err, "while converting pids")
}
x7upLime marked this conversation as resolved.
Show resolved Hide resolved

// we skip the pid we're looking for
if intPid == pid {
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
continue
}

pids = append(pids, intPid)
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
}

// we convert the pids list back to string and write it back to file
newPids := fmt.Sprintf("%s ", strings.Trim(strings.Join(strings.Fields(fmt.Sprint(pids)), " "), "[]"))
x7upLime marked this conversation as resolved.
Show resolved Hide resolved
return lock.WriteFile(pidPath, []byte(newPids), 0o644)
}
14 changes: 13 additions & 1 deletion pkg/minikube/cluster/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")}
}
Expand All @@ -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
}
Expand Down
Loading