From 9d4f296c1ff0cdf3843b218eb3bfa8bb2afffae7 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Fri, 3 Feb 2023 21:06:49 +0200 Subject: [PATCH 01/39] Adjusts mount processes cleaning logic The delete mechanism called by the --kill flag logic inside the minikube mount command, relies on a .mount-process file inside the .minikube dotfolder in home directory. For this to work, the mount should've been instantiated by some mechanism that also creates the file; like k8s.io/minikube/pkg/minikube/node.configureMounts() To just add the local.WriteFile() logic from configureMounts() is not enough for the usecase in #15778: user's consequent "minikube mount" calls would break the cleaning logic, since the file's content gets overwritten at each new call. So on subsequent minikube mounts, a call to "minikube mount --kill=true" would remove just the last pid, leaving the other mounts unaltered, and no trace of them in any place...(leak?) For the same mechanism to work here, we should rely on some local.AppendFile() function, to add separated pids consequently. This way only "minikube mount" would use this new append logic, leaving unaltered the other functions. We now have a new behaviour for the .mount-process file, in order to address this, we should modify the delete logic from cmd/minikube/cmd/delete.go -- killProcess(), so that we can both kill a single pid, as well as multiple space-separated pids in the same way. ... Ah yes.. we're slightly modifying the "minikube mount" RunE anon function, in order to write the pid into the .mount-process file; as well as modifying cluster.Mount()'s signature to accept the pid. That should be more than safe, since cluster.Mount() is used only here --- cmd/minikube/cmd/delete.go | 81 ++++++++++++++++++++++------------- cmd/minikube/cmd/mount.go | 8 ++-- pkg/minikube/cluster/mount.go | 14 +++++- pkg/util/lock/lock.go | 20 +++++++++ 4 files changed, 90 insertions(+), 33 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index acdf12bc65a7..9461c985ff36 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -608,42 +608,65 @@ func killProcess(path string) error { } klog.Infof("Found %s ...", pidPath) - out, err := os.ReadFile(pidPath) + cnt, err := os.ReadFile(pidPath) if err != nil { return errors.Wrap(err, "ReadFile") } - klog.Infof("pidfile contents: %s", out) - pid, err := strconv.Atoi(string(out)) - if err != nil { - return errors.Wrap(err, "error parsing pid") - } - // os.FindProcess does not check if pid is running :( - entry, err := ps.FindProcess(pid) - if err != nil { - return errors.Wrap(err, "ps.FindProcess") - } - if entry == nil { - klog.Infof("Stale pid: %d", pid) - if err := os.Remove(pidPath); err != nil { - return errors.Wrap(err, "Removing stale pid") + + klog.Infof("pidfile contents: %s", cnt) + + var errs []error + ppp := strings.Fields(string(cnt)) + for _, pp := range ppp { + pid, err := strconv.Atoi(pp) + if err != nil { + errs = append(errs, errors.Wrap(err, fmt.Sprintf("error parsing pid for %s", pp))) + continue + } + + // os.FindProcess does not check if pid is running :( + entry, err := ps.FindProcess(pid) + if err != nil { + errs = append(errs, errors.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid))) + continue + } + if entry == nil { + klog.Infof("Stale pid: %d", pid) + errs = append(errs, errors.Wrap(err, fmt.Sprintf("removing stale pid: %d", pid))) + continue + } + + // 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 { + errs = append(errs, errors.Wrap(err, fmt.Sprintf("os.FindProcess: %d", pid))) + continue + } + + klog.Infof("Killing pid %d ...", pid) + if err := proc.Kill(); err != nil { + klog.Infof("Kill failed with %v - removing probably stale pid...", err) + errs = append(errs, errors.Wrap(err, fmt.Sprintf("Removing likely stale unkillable pid: %d", pid))) + continue } - return nil - } - // 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") } - 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") + err = nil + if len(errs) == 1 { + return err + } else if len(errs) != 0 { + out.Step(style.Failure, "Ecountered multiple errors:") + for _, e := range errs { + out.Err("%v\n", e) } - return errors.Wrap(err, fmt.Sprintf("Kill(%d/%s)", pid, entry.Executable())) + err = errors.New("multiple errors encountered while closing mount processes") } - return nil + + if err := os.Remove(pidPath); err != nil { + return errors.Wrap(err, "While closing mount-pids file") + } + + return err } diff --git a/cmd/minikube/cmd/mount.go b/cmd/minikube/cmd/mount.go index fc9fdc120855..60c8f8aeab0e 100644 --- a/cmd/minikube/cmd/mount.go +++ b/cmd/minikube/cmd/mount.go @@ -202,14 +202,16 @@ var mountCmd = &cobra.Command{ out.Infof("Bind Address: {{.Address}}", out.V{"Address": net.JoinHostPort(bindIP, fmt.Sprint(port))}) var wg sync.WaitGroup + pid := 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() - }() + }(pid) } // Unmount if Ctrl-C or kill request is received. @@ -226,7 +228,7 @@ var mountCmd = &cobra.Command{ } }() - 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) diff --git a/pkg/minikube/cluster/mount.go b/pkg/minikube/cluster/mount.go index 9e8163c589ff..5ac564ab6e23 100644 --- a/pkg/minikube/cluster/mount.go +++ b/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 } diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index efb8dc01b46a..11d0df311cbd 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -43,6 +43,26 @@ func WriteFile(filename string, data []byte, perm os.FileMode) error { return os.WriteFile(filename, data, perm) } +// AppendToFile +func AppendToFile(filename string, data []byte, perm os.FileMode) error { + spec := PathMutexSpec(filename) + klog.Infof("WriteFile acquiring %s: %+v", filename, spec) + releaser, err := mutex.Acquire(spec) + if err != nil { + return errors.Wrapf(err, "failed to acquire lock for %s: %+v", filename, spec) + } + + defer releaser.Release() + + fd, err := os.OpenFile(filename, os.O_APPEND|os.O_CREATE|os.O_WRONLY, perm) + if err != nil { + return errors.Wrapf(err, "failed to open %s: %+v", filename, spec) + } + + _, err = fd.Write(data) + return err +} + // PathMutexSpec returns a mutex spec for a path func PathMutexSpec(path string) mutex.Spec { s := mutex.Spec{ From 47981b75d976a39ce47415c1d58b55d043982b3c Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sun, 5 Feb 2023 18:37:31 +0200 Subject: [PATCH 02/39] Add integration test for mount-clean mechanism I copied as much code as I could from the previous tests that seemed more alike to the one I was trying to build. I put it inside the functional mount tests, with the other mount-command related tests, as I thought (as per comments on "TestFunctional()") it could safely share a profile in parallel.. --- .../integration/functional_test_mount_test.go | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index b09491d42061..83982623d069 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -33,6 +33,7 @@ import ( "testing" "time" + psutil "github.com/shirou/gopsutil/v3/process" "k8s.io/minikube/pkg/util/retry" ) @@ -280,4 +281,82 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no t.Fatalf("failed to find bind address with port 46464. Mount command out: \n%v", mountText) } }) + + t.Run("clean-mechanism", func(t *testing.T) { + tempDir := t.TempDir() + + ctx, cancel := context.WithTimeout(ctx, Minutes(10)) + + inNodeMounts := []string{"/mount1", "/mount2", "/mount3"} + + var mntProcs []*StartSession + for _, guestMount := range inNodeMounts { + args := []string{"mount", "-p", profile, fmt.Sprintf("%s:%s", tempDir, guestMount), "--alsologtostderr", "-v=1"} + mntProc, err := Start(t, exec.CommandContext(ctx, Target(), args...)) + if err != nil { + t.Fatalf("%v failed: %v", args, err) + } + + mntProcs = append(mntProcs, mntProc) + + } + + defer func() { + cancel() + if *cleanup { + os.RemoveAll(tempDir) + } + }() + + // are the mounts alive yet..? + checkMount := func() error { + for _, mnt := range inNodeMounts { + rr, err := Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "ssh", "findmnt -T", mnt)) + if err != nil { + // if something weird has happened from previous tests.. + // this could at least spare us some waiting + if strings.Contains(rr.Stdout.String(), fmt.Sprintf("Profile \"%s\" not found.", profile)) { + t.Fatalf("somehow the profile got cancelled; failing....") + } + return err + } + } + return nil + } + if err := retry.Expo(checkMount, time.Millisecond*500, Seconds(15)); err != nil { + // For local testing, allow macOS users to click prompt. If they don't, skip the test. + if runtime.GOOS == "darwin" { + t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-codesigned binaries to listen on non-localhost port") + } + t.Fatalf("mount was not ready in time: %v", err) + } + + checkProcs := func() { + procs, err := psutil.Processes() + if err != nil { + t.Fatalf("failed gathering processes: %v", err) + } + + for _, p := range procs { + cmdline, err := p.Cmdline() + if err != nil { + t.Fatalf("failed reading process cmdline: %v", err) + } + + for _, mnt := range inNodeMounts { + if strings.Contains(cmdline, fmt.Sprintf("-p\x20%s\x20%s:%s", profile, tempDir, mnt)) { + t.Fatalf("Found active mount processes") + } + } + } + } + + // exec the mount killer + _, err := Run(t, exec.Command(Target(), "mount", "-p", profile, "--kill=true")) + if err != nil { + t.Fatalf("failed while trying to kill mounts") + } + + checkProcs() + }) } From 07f3f440fffa9cadcb7838b532b2af6101b07cf4 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sun, 5 Feb 2023 19:36:07 +0200 Subject: [PATCH 03/39] Fixes lint error The message from the linter makes a good point.. we're storing a reference for the mount processes, but we're not using them.. Thus the committed lines. --- test/integration/functional_test_mount_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index 83982623d069..a886fe78507a 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -302,6 +302,14 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no } defer func() { + // Still trying to stop mount processes that could otherwise + // (if something weird happens...) leave the test run hanging + // The worst thing that could happen is that we try to kill + // something that was aleardy killed... + for _, mp := range mntProcs { + mp.Stop(t) + } + cancel() if *cleanup { os.RemoveAll(tempDir) From 849de9086b47b21a2700bb8068178f9a912a6234 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sun, 5 Feb 2023 19:46:56 +0200 Subject: [PATCH 04/39] Adds documentation for validateMountCmd test working towards #11571 :) --- test/integration/functional_test_mount_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index a886fe78507a..f3bc58b85939 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -46,6 +46,10 @@ const ( ) // validateMountCmd verifies the minikube mount command works properly +// for the platforms that support it, we're testing: +// - a generic 9p mount +// - a 9p mount on a specific port +// - cleaning-mechanism for profile-specific mounts func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // nolint if NoneDriver() { t.Skip("skipping: none driver does not support mount") From 07d20708a5cef1e6758416965d8642e11ec386fc Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sun, 5 Feb 2023 20:25:03 +0200 Subject: [PATCH 05/39] Corrects error handling in killProcess If we encountered just one err, we're returning it If we're returning multiple errors, we're outing 'em and return a multiple-errors error --- cmd/minikube/cmd/delete.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 9461c985ff36..a930bac40fb8 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -653,15 +653,14 @@ func killProcess(path string) error { } - err = nil if len(errs) == 1 { - return err + return errs[0] } else if len(errs) != 0 { out.Step(style.Failure, "Ecountered multiple errors:") for _, e := range errs { out.Err("%v\n", e) } - err = errors.New("multiple errors encountered while closing mount processes") + return errors.New("multiple errors encountered while closing mount processes") } if err := os.Remove(pidPath); err != nil { From e34548f9ae58fbe64a5d784fd0f2e1275cba2413 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 8 Feb 2023 17:19:44 +0200 Subject: [PATCH 06/39] Adds description to lock.AppendToFile --- pkg/util/lock/lock.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/util/lock/lock.go b/pkg/util/lock/lock.go index 11d0df311cbd..54154ff83fbd 100644 --- a/pkg/util/lock/lock.go +++ b/pkg/util/lock/lock.go @@ -43,7 +43,8 @@ func WriteFile(filename string, data []byte, perm os.FileMode) error { return os.WriteFile(filename, data, perm) } -// AppendToFile +// AppendToFile appends DATA bytes to the specified FILENAME in a mutually exclusive way. +// The file is created if it does not exist, using the specified PERM (before umask) func AppendToFile(filename string, data []byte, perm os.FileMode) error { spec := PathMutexSpec(filename) klog.Infof("WriteFile acquiring %s: %+v", filename, spec) From 70a0ed2aac704e13aeccb2a510672dd8c09500a3 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 8 Feb 2023 17:21:09 +0200 Subject: [PATCH 07/39] Modifies integration test label yeah.. perhaps that's a better name --- test/integration/functional_test_mount_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index f3bc58b85939..acd3750b0431 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -286,7 +286,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no } }) - t.Run("clean-mechanism", func(t *testing.T) { + t.Run("verify-cleanup", func(t *testing.T) { tempDir := t.TempDir() ctx, cancel := context.WithTimeout(ctx, Minutes(10)) From 3f47e72752db4e312e8c0949506fa65525921279 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Thu, 9 Feb 2023 00:54:29 +0200 Subject: [PATCH 08/39] Modularizes killProcess() adds helper functions for killing one process, given one pid adds helper function to read pids out of a given pidfile --- cmd/minikube/cmd/delete.go | 82 ++++++++++++++++++++----------------- pkg/util/pidfile/pidfile.go | 47 +++++++++++++++++++++ 2 files changed, 91 insertions(+), 38 deletions(-) create mode 100644 pkg/util/pidfile/pidfile.go diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index a930bac40fb8..b0984804bc8e 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -22,7 +22,6 @@ import ( "os" "os/exec" "path/filepath" - "strconv" "strings" "time" @@ -52,6 +51,7 @@ import ( "k8s.io/minikube/pkg/minikube/out/register" "k8s.io/minikube/pkg/minikube/reason" "k8s.io/minikube/pkg/minikube/style" + "k8s.io/minikube/pkg/util/pidfile" ) var ( @@ -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") paths := []string{ @@ -601,61 +602,38 @@ 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 ecnountered, 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) - cnt, err := os.ReadFile(pidPath) + + ppp, err := pidfile.GetPids(pidPath) if err != nil { - return errors.Wrap(err, "ReadFile") + return err } - klog.Infof("pidfile contents: %s", cnt) - + // we're giving a change to each pid, to be killed var errs []error - ppp := strings.Fields(string(cnt)) for _, pp := range ppp { - pid, err := strconv.Atoi(pp) - if err != nil { - errs = append(errs, errors.Wrap(err, fmt.Sprintf("error parsing pid for %s", pp))) - continue - } - - // os.FindProcess does not check if pid is running :( - entry, err := ps.FindProcess(pid) + err := tryKillOne(pp) if err != nil { - errs = append(errs, errors.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid))) - continue - } - if entry == nil { - klog.Infof("Stale pid: %d", pid) - errs = append(errs, errors.Wrap(err, fmt.Sprintf("removing stale pid: %d", pid))) - continue - } - - // 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 { - errs = append(errs, errors.Wrap(err, fmt.Sprintf("os.FindProcess: %d", pid))) - continue - } - - klog.Infof("Killing pid %d ...", pid) - if err := proc.Kill(); err != nil { - klog.Infof("Kill failed with %v - removing probably stale pid...", err) - errs = append(errs, errors.Wrap(err, fmt.Sprintf("Removing likely stale unkillable pid: %d", pid))) - continue + errs = append(errs, err) } } 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, we're outputting them + // and returning a new multiple-errors error: out.Step(style.Failure, "Ecountered multiple errors:") for _, e := range errs { out.Err("%v\n", e) @@ -663,9 +641,37 @@ func killProcess(path string) error { return errors.New("multiple errors encountered while closing mount processes") } + // if no errors were encoutered, it is safe to clean out the pidFile if err := os.Remove(pidPath); err != nil { return errors.Wrap(err, "While closing mount-pids file") } return err } + +// tryKillOne takes a PID as argument and tries to SIGKILL it +func tryKillOne(pid int) error { + entry, err := ps.FindProcess(pid) + if err != nil { + return errors.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid)) + } + if entry == nil { + klog.Infof("Stale pid: %d", pid) + return errors.Wrap(err, fmt.Sprintf("removing 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, fmt.Sprintf("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) + return errors.Wrap(err, fmt.Sprintf("Removing likely stale unkillable pid: %d", pid)) + } + + return nil +} diff --git a/pkg/util/pidfile/pidfile.go b/pkg/util/pidfile/pidfile.go new file mode 100644 index 000000000000..8751352a64ec --- /dev/null +++ b/pkg/util/pidfile/pidfile.go @@ -0,0 +1,47 @@ +/* +Copyright 2023 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pidfile + +import ( + "os" + "strconv" + "strings" + + "github.com/pkg/errors" + "k8s.io/klog" +) + +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 +} From 60c3863f87eafa9d901a354a44ed9be314c69645 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Thu, 9 Feb 2023 01:55:32 +0200 Subject: [PATCH 09/39] Changes klog version and adds it to go.sum uses klog/v2.. hopefully getting rid of the ci error. --- pkg/util/pidfile/pidfile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/pidfile/pidfile.go b/pkg/util/pidfile/pidfile.go index 8751352a64ec..1d4e029423f5 100644 --- a/pkg/util/pidfile/pidfile.go +++ b/pkg/util/pidfile/pidfile.go @@ -22,7 +22,7 @@ import ( "strings" "github.com/pkg/errors" - "k8s.io/klog" + "k8s.io/klog/v2" ) func GetPids(path string) ([]int, error) { From b1bd26ab060fcadf661d2ede651ff8a32e8daafd Mon Sep 17 00:00:00 2001 From: x7upLime Date: Thu, 9 Feb 2023 19:11:47 +0200 Subject: [PATCH 10/39] Adds unit test for new helper function tryKillOne also adds that pleasedontkillme pkg, for the trykillone to be tested against, so that we can run unit tests cross-platform: no sure exec.Command("sleep", ....) is avail on windows... --- cmd/minikube/cmd/delete_test.go | 24 ++++++++++++++++++++++++ tools/pleasedontkillme/main.go | 25 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 tools/pleasedontkillme/main.go diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index d91f6d5b3d58..761a5abe1e05 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/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,25 @@ 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) { + dontkillmeProc := exec.Command("go", "run", "tools/pleasedontkillme/main.go") + err := dontkillmeProc.Start() + if err != nil { + t.Fatalf("while execing child process") + } + pid := dontkillmeProc.Process.Pid + + err = tryKillOne(pid) + if err != nil { + t.Fatalf("while trying to kill child proc: %d", pid) + } + + // waiting for process to exit + if err := dontkillmeProc.Wait(); !strings.Contains(err.Error(), "killed") { + t.Fatalf("unable to kill process") + } +} diff --git a/tools/pleasedontkillme/main.go b/tools/pleasedontkillme/main.go new file mode 100644 index 000000000000..cad93b0ae134 --- /dev/null +++ b/tools/pleasedontkillme/main.go @@ -0,0 +1,25 @@ +package main + +import ( + "os" + "os/signal" + "syscall" +) + +// This is used to unittest functions that kill processes, +// in a cross-platform way. +func main() { + ch := make(chan os.Signal) + done := make(chan struct{}) + defer close(ch) + + signal.Notify(ch, syscall.SIGKILL) + defer signal.Stop(ch) + + go func() { + <-ch + close(done) + }() + + <-done +} From e91f041802d17f428b717ed16977655ec9ee5661 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Thu, 9 Feb 2023 19:18:31 +0200 Subject: [PATCH 11/39] Stops govet from complaining also this err message got me thinking.... "syscall.SIGKILL cannot be trapped (did you mean syscall.SIGTERM?)" --- tools/pleasedontkillme/main.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tools/pleasedontkillme/main.go b/tools/pleasedontkillme/main.go index cad93b0ae134..b99f85572c20 100644 --- a/tools/pleasedontkillme/main.go +++ b/tools/pleasedontkillme/main.go @@ -1,3 +1,19 @@ +/* +Copyright 2023 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package main import ( @@ -9,11 +25,11 @@ import ( // This is used to unittest functions that kill processes, // in a cross-platform way. func main() { - ch := make(chan os.Signal) + ch := make(chan os.Signal, 1) done := make(chan struct{}) defer close(ch) - signal.Notify(ch, syscall.SIGKILL) + signal.Notify(ch, syscall.SIGHUP) defer signal.Stop(ch) go func() { From badc9714035671b3cba1210be97d4bb2c2396955 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Fri, 10 Feb 2023 21:08:00 +0200 Subject: [PATCH 12/39] Adds source for cross platform killable process to test As a way to clean the minikube repo dir. --- cmd/minikube/cmd/delete_test.go | 37 ++++++++++++++++++++++++++++- tools/pleasedontkillme/main.go | 41 --------------------------------- 2 files changed, 36 insertions(+), 42 deletions(-) delete mode 100644 tools/pleasedontkillme/main.go diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index 761a5abe1e05..e44c4fe1a6b6 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -227,7 +227,42 @@ func TestDeleteAllProfiles(t *testing.T) { // 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) { - dontkillmeProc := exec.Command("go", "run", "tools/pleasedontkillme/main.go") + + var pleasedontkillmeSource = []byte(` +package main + +import ( + "os" + "os/signal" + "syscall" +) + +// This is used to unittest functions that kill 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, "pleasedontkillme.go") + + if err := os.WriteFile(tmpfile, pleasedontkillmeSource, 0o700); err != nil { + t.Fatalf("copying source to %s", tmpfile) + } + + dontkillmeProc := exec.Command("go", "run", tmpfile) err := dontkillmeProc.Start() if err != nil { t.Fatalf("while execing child process") diff --git a/tools/pleasedontkillme/main.go b/tools/pleasedontkillme/main.go deleted file mode 100644 index b99f85572c20..000000000000 --- a/tools/pleasedontkillme/main.go +++ /dev/null @@ -1,41 +0,0 @@ -/* -Copyright 2023 The Kubernetes Authors All rights reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package main - -import ( - "os" - "os/signal" - "syscall" -) - -// This is used to unittest functions that kill 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 -} From ead0b401ae2c10e25b9f9b058fad0bbc53a9afb6 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sun, 12 Feb 2023 21:18:59 +0200 Subject: [PATCH 13/39] Changes name to unittest tool --- cmd/minikube/cmd/delete_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index e44c4fe1a6b6..45a1c47b8c57 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -228,7 +228,7 @@ func TestDeleteAllProfiles(t *testing.T) { // if after tryKillOne the process still exists, we consider it a failure func TestTryKillOne(t *testing.T) { - var pleasedontkillmeSource = []byte(` + var waitForSig = []byte(` package main import ( @@ -237,8 +237,8 @@ import ( "syscall" ) -// This is used to unittest functions that kill processes, -// in a cross-platform way. +// This is used to unittest functions that send termination +// signals to processes, in a cross-platform way. func main() { ch := make(chan os.Signal, 1) done := make(chan struct{}) @@ -256,9 +256,9 @@ func main() { } `) td := t.TempDir() - tmpfile := filepath.Join(td, "pleasedontkillme.go") + tmpfile := filepath.Join(td, "waitForSig.go") - if err := os.WriteFile(tmpfile, pleasedontkillmeSource, 0o700); err != nil { + if err := os.WriteFile(tmpfile, waitForSig, 0o700); err != nil { t.Fatalf("copying source to %s", tmpfile) } From b5d5bbe38ccb990ed22a57ef31fd16e3f22f42aa Mon Sep 17 00:00:00 2001 From: x7upLime Date: Mon, 13 Feb 2023 00:20:09 +0200 Subject: [PATCH 14/39] Removes external lib dependency Since the test is failing on architectures/platform I can't reproduce on.. I'm readapting the test in such a way to minimize the amount of code that can be responsible for the error. We're not looking for all system processes in order to find our mount processes(ps -ef); we're looking directly at the mount processes that have been spawned by our test instead. To say that a process is no longer alive: We try to wait for conclusion to get the processState. If the wait results in an *ExitError, it means that either the proc has failed to run, or it exited failing(see doc); in both cases we're happy.. since we previously made a check to know that those same processes were alive. --- .../integration/functional_test_mount_test.go | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index acd3750b0431..454c9521fa3f 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -33,7 +33,6 @@ import ( "testing" "time" - psutil "github.com/shirou/gopsutil/v3/process" "k8s.io/minikube/pkg/util/retry" ) @@ -343,22 +342,26 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no t.Fatalf("mount was not ready in time: %v", err) } - checkProcs := func() { - procs, err := psutil.Processes() - if err != nil { - t.Fatalf("failed gathering processes: %v", err) - } - - for _, p := range procs { - cmdline, err := p.Cmdline() - if err != nil { - t.Fatalf("failed reading process cmdline: %v", err) - } - - for _, mnt := range inNodeMounts { - if strings.Contains(cmdline, fmt.Sprintf("-p\x20%s\x20%s:%s", profile, tempDir, mnt)) { - t.Fatalf("Found active mount processes") + checkProcsAlive := func(end chan bool) { + for _, mntp := range mntProcs { + // Trying to wait for process end + // if the wait fail with ExitError we know that the process + // doesnt' exist anymore.. + go func(end chan bool) { + err := mntp.cmd.Wait() + if _, ok := err.(*exec.ExitError); ok { + end <- true } + }(end) + + // Either we know that the mount process has ended + // or we fail after 1 second + // TODO: is there a better way? rather than waiting.. + select { + case <-time.After(1 * time.Second): + t.Fatalf("1s TIMEOUT: Process %d is still running\n", mntp.cmd.Process.Pid) + case <-end: + continue } } } @@ -369,6 +372,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no t.Fatalf("failed while trying to kill mounts") } - checkProcs() + end := make(chan bool, 1) + checkProcsAlive(end) }) } From 51e46b6d8d4959fd3145e5c6a28d8b39557a0736 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Tue, 14 Feb 2023 20:29:28 +0200 Subject: [PATCH 15/39] Refactors GetPids Moves GetPids inside cmd/delete.go as a helper function Adds description --- cmd/minikube/cmd/delete.go | 27 +++++++++++++++++++++++++-- pkg/util/pidfile/pidfile.go | 30 ------------------------------ 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index b0984804bc8e..a982c6f3ef02 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -22,6 +22,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "time" @@ -51,7 +52,6 @@ import ( "k8s.io/minikube/pkg/minikube/out/register" "k8s.io/minikube/pkg/minikube/reason" "k8s.io/minikube/pkg/minikube/style" - "k8s.io/minikube/pkg/util/pidfile" ) var ( @@ -613,7 +613,7 @@ func killProcess(path string) error { } klog.Infof("Found %s ...", pidPath) - ppp, err := pidfile.GetPids(pidPath) + ppp, err := GetPids(pidPath) if err != nil { return err } @@ -675,3 +675,26 @@ func tryKillOne(pid int) error { return 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 +} diff --git a/pkg/util/pidfile/pidfile.go b/pkg/util/pidfile/pidfile.go index 1d4e029423f5..11cfb379f417 100644 --- a/pkg/util/pidfile/pidfile.go +++ b/pkg/util/pidfile/pidfile.go @@ -15,33 +15,3 @@ limitations under the License. */ package pidfile - -import ( - "os" - "strconv" - "strings" - - "github.com/pkg/errors" - "k8s.io/klog/v2" -) - -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 -} From e28a1b411b5a0e4b5b83dd4172444fcdb5a350c2 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 15 Feb 2023 03:22:43 +0200 Subject: [PATCH 16/39] Removes pidfile util --- pkg/util/pidfile/pidfile.go | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 pkg/util/pidfile/pidfile.go diff --git a/pkg/util/pidfile/pidfile.go b/pkg/util/pidfile/pidfile.go deleted file mode 100644 index 11cfb379f417..000000000000 --- a/pkg/util/pidfile/pidfile.go +++ /dev/null @@ -1,17 +0,0 @@ -/* -Copyright 2023 The Kubernetes Authors All rights reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package pidfile From ee22b477957dcd8d219db5dda1acf2bc81a720e1 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 18 Feb 2023 17:19:31 +0200 Subject: [PATCH 17/39] Apply suggestions from code review --- cmd/minikube/cmd/delete.go | 8 ++++---- test/integration/functional_test_mount_test.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index a982c6f3ef02..76af0233036c 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -585,7 +585,7 @@ func deleteMachineDirectories(cc *config.ClusterConfig) { } // 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) +// it then tries to kill all the pids listed in the pidfile (one or more) func killMountProcess() error { profile := viper.GetString("profile") paths := []string{ @@ -602,7 +602,7 @@ func killMountProcess() error { return nil } -// killProcess takes a path to look for a pidfile(space-separated), +// 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 ecnountered, it cleans the pidfile @@ -634,14 +634,14 @@ func killProcess(path string) error { } else if len(errs) != 0 { // if multiple errors were encountered, we're outputting them // and returning a new multiple-errors error: - out.Step(style.Failure, "Ecountered multiple errors:") + 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 is safe to clean out the pidFile + // 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") } diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index 454c9521fa3f..3cf80516a1f6 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -327,7 +327,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no // if something weird has happened from previous tests.. // this could at least spare us some waiting if strings.Contains(rr.Stdout.String(), fmt.Sprintf("Profile \"%s\" not found.", profile)) { - t.Fatalf("somehow the profile got cancelled; failing....") + t.Fatalf("profile was deleted, cancelling the test") } return err } @@ -346,7 +346,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no for _, mntp := range mntProcs { // Trying to wait for process end // if the wait fail with ExitError we know that the process - // doesnt' exist anymore.. + // doesn't exist anymore.. go func(end chan bool) { err := mntp.cmd.Wait() if _, ok := err.(*exec.ExitError); ok { From 43ff38b27d606f40e6c8e66c6c655f534dc08a09 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 18 Feb 2023 17:27:25 +0200 Subject: [PATCH 18/39] Changes variable name in test --- cmd/minikube/cmd/delete_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index 45a1c47b8c57..c1d55ac057ec 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -262,12 +262,12 @@ func main() { t.Fatalf("copying source to %s", tmpfile) } - dontkillmeProc := exec.Command("go", "run", tmpfile) - err := dontkillmeProc.Start() + processToKill := exec.Command("go", "run", tmpfile) + err := processToKill.Start() if err != nil { t.Fatalf("while execing child process") } - pid := dontkillmeProc.Process.Pid + pid := processToKill.Process.Pid err = tryKillOne(pid) if err != nil { @@ -275,7 +275,7 @@ func main() { } // waiting for process to exit - if err := dontkillmeProc.Wait(); !strings.Contains(err.Error(), "killed") { + if err := processToKill.Wait(); !strings.Contains(err.Error(), "killed") { t.Fatalf("unable to kill process") } } From 3b3d2905a14f4778e51152a4c847d7f5a044a14e Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 18 Feb 2023 17:29:18 +0200 Subject: [PATCH 19/39] Changes variable name in integrataion test --- test/integration/functional_test_mount_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index 3cf80516a1f6..ea920a6781ab 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -290,10 +290,10 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no ctx, cancel := context.WithTimeout(ctx, Minutes(10)) - inNodeMounts := []string{"/mount1", "/mount2", "/mount3"} + guestMountPaths := []string{"/mount1", "/mount2", "/mount3"} var mntProcs []*StartSession - for _, guestMount := range inNodeMounts { + for _, guestMount := range guestMountPaths { args := []string{"mount", "-p", profile, fmt.Sprintf("%s:%s", tempDir, guestMount), "--alsologtostderr", "-v=1"} mntProc, err := Start(t, exec.CommandContext(ctx, Target(), args...)) if err != nil { @@ -321,7 +321,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no // are the mounts alive yet..? checkMount := func() error { - for _, mnt := range inNodeMounts { + for _, mnt := range guestMountPaths { rr, err := Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "ssh", "findmnt -T", mnt)) if err != nil { // if something weird has happened from previous tests.. From db14047b6523633bba23f303c1474f103bcd814f Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 18 Feb 2023 17:32:15 +0200 Subject: [PATCH 20/39] Changes helper killProcess function name --- cmd/minikube/cmd/delete.go | 6 +++--- cmd/minikube/cmd/delete_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 76af0233036c..f0e943393b79 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -621,7 +621,7 @@ func killProcess(path string) error { // we're giving a change to each pid, to be killed var errs []error for _, pp := range ppp { - err := tryKillOne(pp) + err := trySignalProcess(pp) if err != nil { errs = append(errs, err) } @@ -649,8 +649,8 @@ func killProcess(path string) error { return err } -// tryKillOne takes a PID as argument and tries to SIGKILL it -func tryKillOne(pid int) error { +// trySignalProcess takes a PID as argument and tries to SIGKILL it +func trySignalProcess(pid int) error { entry, err := ps.FindProcess(pid) if err != nil { return errors.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid)) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index c1d55ac057ec..4cd557fd7664 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -269,7 +269,7 @@ func main() { } pid := processToKill.Process.Pid - err = tryKillOne(pid) + err = trySignalProcess(pid) if err != nil { t.Fatalf("while trying to kill child proc: %d", pid) } From b368c64bb6fc46647e318c72dd94d9366d20adc2 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 18 Feb 2023 17:36:11 +0200 Subject: [PATCH 21/39] Update cmd/minikube/cmd/delete.go --- cmd/minikube/cmd/delete.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index f0e943393b79..a4a47d51931c 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -632,8 +632,7 @@ func killProcess(path string) error { // if we've encountered only one error, we're returning it: return errs[0] } else if len(errs) != 0 { - // if multiple errors were encountered, we're outputting them - // and returning a new multiple-errors error: + // 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) From 1d5b316fda54c1e1b55f6e6c9e0227b8d05fabcb Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 18 Feb 2023 17:40:05 +0200 Subject: [PATCH 22/39] Makes comment more clear --- cmd/minikube/cmd/delete.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index a4a47d51931c..6358b56c0796 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -618,7 +618,8 @@ func killProcess(path string) error { return err } - // we're giving a change to each pid, to be killed + // 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 := trySignalProcess(pp) From b5e59b5db4c8ce494f81d1598432127b595ebe43 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Mon, 6 Mar 2023 20:00:40 +0100 Subject: [PATCH 23/39] Renames sigkill function --- cmd/minikube/cmd/delete.go | 6 +++--- cmd/minikube/cmd/delete_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 6358b56c0796..02055e7e65a0 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -622,7 +622,7 @@ func killProcess(path string) error { // error handling is done below var errs []error for _, pp := range ppp { - err := trySignalProcess(pp) + err := trySigKillProcess(pp) if err != nil { errs = append(errs, err) } @@ -649,8 +649,8 @@ func killProcess(path string) error { return err } -// trySignalProcess takes a PID as argument and tries to SIGKILL it -func trySignalProcess(pid int) error { +// trySigKillProcess takes a PID as argument and tries to SIGKILL it +func trySigKillProcess(pid int) error { entry, err := ps.FindProcess(pid) if err != nil { return errors.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid)) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index 4cd557fd7664..82795bad79a4 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -269,7 +269,7 @@ func main() { } pid := processToKill.Process.Pid - err = trySignalProcess(pid) + err = trySigKillProcess(pid) if err != nil { t.Fatalf("while trying to kill child proc: %d", pid) } From ddec3100b936943cdb6ce3f51864d14dba0b46e3 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Tue, 7 Mar 2023 09:27:34 +0100 Subject: [PATCH 24/39] Ensures sigkilling only minikube started processes Adds a helper function to check that the executable name is minikube. --- cmd/minikube/cmd/delete.go | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 02055e7e65a0..02cab5956c99 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -651,17 +651,15 @@ func killProcess(path string) error { // trySigKillProcess takes a PID as argument and tries to SIGKILL it func trySigKillProcess(pid int) error { - entry, err := ps.FindProcess(pid) + itDoes, err := doesPIDBelongToMinikube(pid) if err != nil { - return errors.Wrap(err, fmt.Sprintf("ps.FindProcess for %d", pid)) + return err } - if entry == nil { - klog.Infof("Stale pid: %d", pid) - return errors.Wrap(err, fmt.Sprintf("removing stale pid: %d", pid)) + + 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, fmt.Sprintf("os.FindProcess: %d", pid)) @@ -676,6 +674,26 @@ func trySigKillProcess(pid int) error { return nil } +// doesPIDBelongToMinikube tries to find the process with that PID +// and checks if the executable name is "minikube" +func doesPIDBelongToMinikube(pid int) (bool, error) { + entry, err := ps.FindProcess(pid) + if err != nil { + return false, errors.Wrap(err, fmt.Sprintf("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 entry.Executable() != "minikube" { + 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) { From d72f2fb285bf0b350cc5de89205598ff1af3e170 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Tue, 14 Mar 2023 21:08:05 +0100 Subject: [PATCH 25/39] Changes pid owner conditional What happened in the failed integration tests was that the starting process was not "minikube", it was "minikube-linux-" and "minikube-darwin-" instead. --- cmd/minikube/cmd/delete.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 02cab5956c99..b2f8bd2f8abb 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -687,7 +687,8 @@ func doesPIDBelongToMinikube(pid int) (bool, error) { } klog.Infof("Found process %d", pid) - if entry.Executable() != "minikube" { + if !strings.Contains(entry.Executable(), "minikube") { + klog.Infof("process %d was not started by minikube", pid) return false, nil } From e5b57f8e894343c245b12b1cb67baad321f63ecd Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 20:53:00 +0100 Subject: [PATCH 26/39] Changes the way in which pid is written to mounts file The integration test error was due to the fact that multiple pids were written to the file without spaces between them, breaking the pid-kill logic --- pkg/minikube/node/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/node/config.go b/pkg/minikube/node/config.go index b2f51740f1ce..c0aad6afd3a8 100644 --- a/pkg/minikube/node/config.go +++ b/pkg/minikube/node/config.go @@ -89,7 +89,7 @@ func configureMounts(wg *sync.WaitGroup, cc config.ClusterConfig) { if err := mountCmd.Start(); err != nil { exit.Error(reason.GuestMount, "Error starting mount", err) } - if err := lock.WriteFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(strconv.Itoa(mountCmd.Process.Pid)), 0o644); err != nil { + if err := lock.AppendToFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(fmt.Sprintf("%s ", strconv.Itoa(mountCmd.Process.Pid))), 0o644); err != nil { exit.Error(reason.HostMountPid, "Error writing mount pid", err) } } From 9665bad429f94d5c621604fc7877e6e1ee363b15 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 20:54:36 +0100 Subject: [PATCH 27/39] Generates docs --- site/content/en/docs/contrib/tests.en.md | 671 +++++++++++++++++++++++ translations/de.json | 1 + translations/es.json | 1 + translations/fr.json | 1 + translations/ja.json | 1 + translations/ko.json | 1 + translations/pl.json | 1 + translations/ru.json | 1 + translations/strings.txt | 1 + translations/zh-CN.json | 1 + 10 files changed, 680 insertions(+) diff --git a/site/content/en/docs/contrib/tests.en.md b/site/content/en/docs/contrib/tests.en.md index 2c17563f3acc..ef9d2b3c94d5 100644 --- a/site/content/en/docs/contrib/tests.en.md +++ b/site/content/en/docs/contrib/tests.en.md @@ -399,6 +399,10 @@ Note: This test will fail on release PRs as the licenses file for the new versio #### validateMountCmd verifies the minikube mount command works properly +for the platforms that support it, we're testing: +- a generic 9p mount +- a 9p mount on a specific port +- cleaning-mechanism for profile-specific mounts #### validatePersistentVolumeClaim makes sure PVCs work properly @@ -490,6 +494,673 @@ verifies the docker/podman driver works with a custom subnet ## TestKicStaticIP starts minikube with the static IP flag +## TestPrintablePathCheckHandler + +## TestGet_progress + +## TestBzip2Decompressor + +## TestGzipDecompressor + +## TestTar + +## TestDecompressTarPermissions + +## TestTarBzip2Decompressor + +## TestDecompressor +is a helper function for testing generic decompressors. + +## TestTarGzipDecompressor + +## TestTarXzDecompressor + +## TestTarZstdDecompressor + +## TestXzDecompressor + +## TestZipDecompressor + +## TestDecompressZipPermissions + +## TestZstdDecompressor + +## TestBitBucketDetector + +## TestFileDetector + +## TestFileDetector_noPwd + +## TestFileDetector_relativeSymlink +If a relative symlink is passed in as the pwd to Detect, the resulting URL +can have an invalid path. + +## TestGCSDetector + +## TestGitDetector + +## TestGitHubDetector + +## TestGitLabDetector + +## TestS3Detector + +## TestDetect + +## TestFolderStorage_impl + +## TestFolderStorage + +## TestCopy + +## TestFileGetter_impl + +## TestFileGetter + +## TestFileGetter_sourceFile + +## TestFileGetter_sourceNoExist + +## TestFileGetter_dir + +## TestFileGetter_dirSymlink + +## TestFileGetter_GetFile + +## TestFileGetter_GetFile_Copy + +## TestFileGetter_percent2F +https://github.com/hashicorp/terraform/issues/8418 + +## TestFileGetter_ClientMode_notexist + +## TestFileGetter_ClientMode_file + +## TestFileGetter_ClientMode_dir + +## TestGCSGetter_impl + +## TestGCSGetter + +## TestGCSGetter_subdir + +## TestGCSGetter_GetFile + +## TestGCSGetter_GetGenerationFile + +## TestGCSGetter_GetFile_notfound + +## TestGCSGetter_ClientMode_dir + +## TestGCSGetter_ClientMode_file + +## TestGCSGetter_ClientMode_notfound + +## TestGCSGetter_Url + +## TestGCSGetter_GetFile_OAuthAccessToken + +## TestGitGetter_impl + +## TestGitGetter + +## TestGitGetter_branch + +## TestGitGetter_commitID + +## TestGitGetter_remoteWithoutMaster + +## TestGitGetter_shallowClone + +## TestGitGetter_shallowCloneWithTag + +## TestGitGetter_shallowCloneWithCommitID + +## TestGitGetter_branchUpdate + +## TestGitGetter_tag + +## TestGitGetter_GetFile + +## TestGitGetter_gitVersion + +## TestGitGetter_sshKey + +## TestGitGetter_sshSCPStyle + +## TestGitGetter_sshExplicitPort + +## TestGitGetter_sshSCPStyleInvalidScheme + +## TestGitGetter_submodule + +## TestGitGetter_setupGitEnv_sshKey + +## TestGitGetter_setupGitEnvWithExisting_sshKey + +## TestGitGetter_subdirectory_symlink + +## TestGitGetter_subdirectory + +## TestHgGetter_impl + +## TestHgGetter + +## TestHgGetter_branch + +## TestHgGetter_GetFile + +## TestHgGetter_HgArgumentsNotAllowed + +## TestHttpGetter_impl + +## TestHttpGetter_header + +## TestHttpGetter_requestHeader + +## TestHttpGetter_meta + +## TestHttpGetter_metaSubdir + +## TestHttpGetter_metaSubdirGlob + +## TestHttpGetter_none + +## TestHttpGetter_resume + +## TestHttpGetter_resumeNoRange +The server may support Byte-Range, but has no size for the requested object + +## TestHttpGetter_file + +## TestHttpGetter_http2server +tests that http.Request is not reused +between HEAD & GET, which would lead to race condition in HTTP/2. +This test is only meaningful for the race detector (go test -race). + +## TestHttpGetter_auth + +## TestHttpGetter_authNetrc + +## TestHttpGetter_cleanhttp +verify that the default httpClient no longer comes from http.DefaultClient + +## TestHttpGetter__RespectsContextCanceled + +## TestHttpGetter__XTerraformGetLimit + +## TestHttpGetter__XTerraformGetDisabled + +## TestHttpGetter__XTerraformGetDetected +test a source url with no protocol + +## TestHttpGetter__XTerraformGetProxyBypass + +## TestHttpGetter__XTerraformGetConfiguredGettersBypass + +## TestHttpGetter__endless_body + +## TestHttpGetter_subdirLink + +## TestS3Getter_impl + +## TestS3Getter + +## TestS3Getter_subdir + +## TestS3Getter_GetFile + +## TestS3Getter_GetFile_badParams + +## TestS3Getter_GetFile_notfound + +## TestS3Getter_ClientMode_dir + +## TestS3Getter_ClientMode_file + +## TestS3Getter_ClientMode_notfound + +## TestS3Getter_ClientMode_collision + +## TestS3Getter_Url + +## TestGet_badSchema + +## TestGet_file + +## TestGet_fileDecompressorExt +https://github.com/hashicorp/terraform/issues/11438 + +## TestGet_filePercent2F +https://github.com/hashicorp/terraform/issues/8418 + +## TestGet_fileDetect + +## TestGet_fileForced + +## TestGet_fileSubdir + +## TestGet_archive + +## TestGetAny_archive + +## TestGet_archiveRooted + +## TestGet_archiveSubdirWild + +## TestGet_archiveSubdirWildMultiMatch + +## TestGetAny_file + +## TestGetAny_dir + +## TestGetFile + +## TestGetFile_archive + +## TestGetFile_archiveChecksum + +## TestGetFile_archiveNoUnarchive + +## TestGetFile_checksum + +## TestGetFile_checksum_from_file + +## TestGetFile_checksumURL + +## TestGetFile_filename + +## TestGetFile_filename_path_traversal + +## TestGetFile_checksumSkip + +## TestParse + +## TestAddAuthFromNetrc + +## TestAddAuthFromNetrc_hasAuth + +## TestAddAuthFromNetrc_hasUsername + +## TestSourceDirSubdir + +## TestSourceSubdirGlob + +## TestRedactURL + +## TestDir + +## TestNewConstraint + +## TestConstraintCheck + +## TestConstraintsString + +## TestCollection + +## TestNewVersion + +## TestNewSemver + +## TestVersionCompare + +## TestVersionCompare_versionAndSemver + +## TestVersionEqual_nil + +## TestComparePreReleases + +## TestVersionMetadata + +## TestVersionPrerelease + +## TestVersionSegments + +## TestVersionSegments64 + +## TestVersionString + +## TestEqual + +## TestGreaterThan + +## TestLessThan + +## TestGreaterThanOrEqual + +## TestLessThanOrEqual + +## TestDecode_interface + +## TestDecode_interfaceInline + +## TestDecode_equal + +## TestDecode_flatMap + +## TestDecode_structure + +## TestDecode_structurePtr + +## TestDecode_structureArray + +## TestDecode_sliceExpand + +## TestDecode_structureMap + +## TestDecode_structureMapInvalid + +## TestDecode_interfaceNonPointer + +## TestDecode_intString + +## TestDecode_float32 + +## TestDecode_float64 + +## TestDecode_intStringAliased + +## TestDecode_Node + +## TestDecode_NestedNode + +## TestDecode_topLevelKeys +https://github.com/hashicorp/hcl/issues/60 + +## TestDecode_flattenedJSON + +## TestObjectListFilter + +## TestWalk + +## TestWalkEquality + +## TestWalkRewrite + +## TestIsValidFile + +## TestRunMultiplePaths + +## TestRunSubDirectories + +## TestRunStdin + +## TestRunStdinAndWrite + +## TestRunFileError + +## TestRunNoOptions + +## TestRunList + +## TestRunWrite + +## TestRunDiff + +## TestPosError_impl + +## TestType + +## TestListType + +## TestListOfMaps + +## TestListOfMaps_requiresComma + +## TestListType_leadComment + +## TestListType_lineComment + +## TestObjectType + +## TestObjectKey + +## TestCommentGroup + +## TestParse +Official HCL tests + +## TestParse_inline + +## TestFiles + +## TestFormatValidOutput +TestFormatParsable ensures that the output of Format() is can be parsed again. + +## TestPosition + +## TestNullChar + +## TestComment + +## TestOperator + +## TestBool + +## TestIdent + +## TestString + +## TestNumber + +## TestFloat + +## TestWindowsLineEndings + +## TestRealExample + +## TestScan_crlf + +## TestError + +## TestScanDigitsUnread + +## TestScanHeredocRegexpCompile + +## TestUnquote + +## TestTypeString + +## TestTokenValue + +## TestType + +## TestListType + +## TestObjectType + +## TestFlattenObjects + +## TestObjectKey + +## TestParse +Official HCL tests + +## TestParse_inline + +## TestPosition + +## TestComment + +## TestOperator + +## TestBool + +## TestIdent + +## TestString + +## TestNumber + +## TestFloat + +## TestRealExample + +## TestError + +## TestTypeString + +## TestLexMode + +## TestPackage + +## TestNow + +## TestNow + +## TestAdvanceLogs + +## TestWaitAdvance + +## TestAdvanceWithAfter + +## TestAdvanceWithAfterFunc + +## TestAfterFuncStop + +## TestNewTimerReset + +## TestNewTimerAsyncReset + +## TestNewTimerResetCausesWakeup + +## TestMultipleWaiters + +## TestSlowedAfter + +## TestFastAfter + +## TestSlowedAfterFunc + +## TestFastAfterFunc + +## TestSlowedNow + +## TestFastNow + +## TestAdvance + +## TestAdvanceMulti + +## TestStop + +## TestReset + +## TestStopReset + +## TestAdvanceAlreadyFired + +## TestAdvanceFast + +## TestAdvanceReset + +## TestAdvanceResetRacey + +## TestAll + +## TestErrorString + +## TestNewErr + +## TestNewErrWithCause + +## TestDeferredAnnotatef + +## TestAllErrors + +## TestNew + +## TestErrorf + +## TestTrace + +## TestAnnotate + +## TestAnnotatef + +## TestDeferredAnnotatef + +## TestWrap + +## TestWrapOfNil + +## TestWrapf + +## TestWrapfOfNil + +## TestMask + +## TestMaskf + +## TestCause + +## TestDetails + +## TestErrorStack + +## TestFormat + +## Test + +## TestTrimDefaultSet + +## TestTrimSourcePath + +## TestSetSourceTrimPrefix + +## Test + +## TestLockNoContention + +## TestLockBlocks + +## TestTryLock + +## TestTryLockNoBlock + +## TestUnlockedWithTimeout + +## TestLockWithTimeout + +## TestStress + +## TestLockFromOtherProcess + +## TestSpecValidity + +## TestLockNoContention + +## TestLockContentionWithinProcessTimeout + +## TestLockAfterTimeout + +## TestLockContentionWithinProcessCancel + +## TestSecondReleaseFine + +## TestDifferentNamesDontBlock + +## TestLockBlocks + +## TestProcessReleasesWhenDead + +## TestStress + +## TestMutexNotInherited +tests that subprocesses do not inherit mutexes. + +## TestFilePermissions +tests that the file permissions are correct. + +## TestFilePermissionsWithSudoEnvars +TestFilePermissionsWithSudo tests that the file permissions are correct. + +## TestLockFromOtherProcess + +## TestSleepFromOtherProcess + +## Test + ## TestingKicBaseImage will return true if the integraiton test is running against a passed --base-image flag diff --git a/translations/de.json b/translations/de.json index 12ea4e025711..8362c15961e7 100644 --- a/translations/de.json +++ b/translations/de.json @@ -431,6 +431,7 @@ "Mounts the specified directory into minikube": "Mounted das angegebene Verzeichnis in Minikube", "Mounts the specified directory into minikube.": "Mounted das angegebene Verzeichnis in Minikube.", "Multiple errors deleting profiles": "Es sind mehrere Fehler beim Löschen der Profile aufgetreten", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "Es wurden mehrere Minikube Profile gefunden - ", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "NIC Type der fürs Host only Netzwerk verwendet wird. Einer aus Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, oder virtio (nur virtualbox Treiber)", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "NIC Type der fürs NAT Network verwendet wird. Einer aus Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (Nur virtualbox Treiber)", diff --git a/translations/es.json b/translations/es.json index 97c950087b70..46450d57993f 100644 --- a/translations/es.json +++ b/translations/es.json @@ -439,6 +439,7 @@ "Mounts the specified directory into minikube": "", "Mounts the specified directory into minikube.": "", "Multiple errors deleting profiles": "", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", diff --git a/translations/fr.json b/translations/fr.json index af3d6a585897..cdc9bfde88ea 100644 --- a/translations/fr.json +++ b/translations/fr.json @@ -422,6 +422,7 @@ "Mounts the specified directory into minikube": "Monte le répertoire spécifié dans minikube", "Mounts the specified directory into minikube.": "Monte le répertoire spécifié dans minikube.", "Multiple errors deleting profiles": "Plusieurs erreurs lors de la suppression des profils", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "Plusieurs profils minikube ont été trouvés -", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "Type de carte réseau utilisé pour le réseau hôte uniquement. Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM ou virtio (pilote virtualbox uniquement)", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "Type de carte réseau utilisé pour le réseau nat. Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM ou virtio (pilote virtualbox uniquement)", diff --git a/translations/ja.json b/translations/ja.json index 52f00c68eb3b..d5b83966a21d 100644 --- a/translations/ja.json +++ b/translations/ja.json @@ -408,6 +408,7 @@ "Mounts the specified directory into minikube": "minikube に指定されたディレクトリーをマウントします", "Mounts the specified directory into minikube.": "minikube に指定されたディレクトリーをマウントします。", "Multiple errors deleting profiles": "プロファイル削除中に複数のエラーが発生しました", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "複数の minikube プロファイルが見つかりました - ", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "ホストオンリーネットワークに使用する NIC タイプ。Am79C970A、Am79C973、82540EM、82543GC、82545EM、virtio のいずれか (virtualbox ドライバーのみ)", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "NAT ネットワークに使用する NIC タイプ。Am79C970A、Am79C973、82540EM、82543GC、82545EM、virtio のいずれか (virtualbox ドライバーのみ)", diff --git a/translations/ko.json b/translations/ko.json index ec8cca1a296d..27f51e2de0a6 100644 --- a/translations/ko.json +++ b/translations/ko.json @@ -454,6 +454,7 @@ "Mounts the specified directory into minikube": "특정 디렉토리를 minikube 에 마운트합니다", "Mounts the specified directory into minikube.": "", "Multiple errors deleting profiles": "", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", diff --git a/translations/pl.json b/translations/pl.json index c300a992b954..d57e0d027a16 100644 --- a/translations/pl.json +++ b/translations/pl.json @@ -443,6 +443,7 @@ "Mounts the specified directory into minikube": "Montuje podany katalog wewnątrz minikube", "Mounts the specified directory into minikube.": "Montuje podany katalog wewnątrz minikube", "Multiple errors deleting profiles": "Wystąpiło wiele błędów podczas usuwania profili", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "Znaleziono wiele profili minikube - ", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", diff --git a/translations/ru.json b/translations/ru.json index 095293e496b3..9b6d05b04cd5 100644 --- a/translations/ru.json +++ b/translations/ru.json @@ -404,6 +404,7 @@ "Mounts the specified directory into minikube": "", "Mounts the specified directory into minikube.": "", "Multiple errors deleting profiles": "", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", diff --git a/translations/strings.txt b/translations/strings.txt index 953366b390c9..9f2cf97558b1 100644 --- a/translations/strings.txt +++ b/translations/strings.txt @@ -404,6 +404,7 @@ "Mounts the specified directory into minikube": "", "Mounts the specified directory into minikube.": "", "Multiple errors deleting profiles": "", + "Multiple errors encountered:": "", "Multiple minikube profiles were found - ": "", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", "NIC Type used for nat network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "", diff --git a/translations/zh-CN.json b/translations/zh-CN.json index ed7882591ba1..839791d8ab19 100644 --- a/translations/zh-CN.json +++ b/translations/zh-CN.json @@ -518,6 +518,7 @@ "Mounts the specified directory into minikube": "将指定的目录挂载到 minikube", "Mounts the specified directory into minikube.": "将指定的目录挂载到 minikube。", "Multiple errors deleting profiles": "删除配置文件时出现多个错误", + "Multiple errors encountered:": "", "Multiple minikube profiles were found -": "发现了多个 minikube 配置文件 -", "Multiple minikube profiles were found - ": "", "NIC Type used for host only network. One of Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM, or virtio (virtualbox driver only)": "网卡类型仅用于主机网络。Am79C970A, Am79C973, 82540EM, 82543GC, 82545EM 之一,或 virtio(仅限 VirtualBox 驱动程序)", From c55a4caacc027a4e3a80fa1a58dc3aca28ec210f Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 22:07:44 +0100 Subject: [PATCH 28/39] Adds the ownership check as a dependency Otherway I see no obvious way to pass this check during (say) unit testing --- cmd/minikube/cmd/delete.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index b2f8bd2f8abb..76f7acd3102a 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -622,7 +622,7 @@ func killProcess(path string) error { // error handling is done below var errs []error for _, pp := range ppp { - err := trySigKillProcess(pp) + err := trySigKillProcess(pp, doesPIDBelongToMinikube) if err != nil { errs = append(errs, err) } @@ -649,9 +649,11 @@ func killProcess(path string) error { return err } -// trySigKillProcess takes a PID as argument and tries to SIGKILL it -func trySigKillProcess(pid int) error { - itDoes, err := doesPIDBelongToMinikube(pid) +// trySigKillProcess takes a PID as argument and tries to SIGKILL it. +// It performs the OCHECK ownership check of the pid, +// before trying to send a sigkill signal to it +func trySigKillProcess(pid int, ocheck func(int) (bool, error)) error { + itDoes, err := ocheck(pid) if err != nil { return err } From 82abc0dbeeb98a6fd684298c7c5ecd1043aeef61 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 22:08:56 +0100 Subject: [PATCH 29/39] Adapts unit test to new dependency --- cmd/minikube/cmd/delete_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index 82795bad79a4..47a43abe63dc 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -269,7 +269,11 @@ func main() { } pid := processToKill.Process.Pid - err = trySigKillProcess(pid) + mockOwnershipCheck := func(int) (bool, error) { + return true, nil + } + + err = trySigKillProcess(pid, mockOwnershipCheck) if err != nil { t.Fatalf("while trying to kill child proc: %d", pid) } From 595c8eeedaf4d7527c0a18215863df1e4235501b Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 22:09:25 +0100 Subject: [PATCH 30/39] Adapts TestTryKillOne log for failing --- cmd/minikube/cmd/delete_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index 47a43abe63dc..c65fa81b39dc 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -259,13 +259,13 @@ func main() { tmpfile := filepath.Join(td, "waitForSig.go") if err := os.WriteFile(tmpfile, waitForSig, 0o700); err != nil { - t.Fatalf("copying source to %s", tmpfile) + t.Fatalf("copying source to %s: %s\n", tmpfile, err) } processToKill := exec.Command("go", "run", tmpfile) err := processToKill.Start() if err != nil { - t.Fatalf("while execing child process") + t.Fatalf("while execing child process: %s\n", err) } pid := processToKill.Process.Pid @@ -275,11 +275,11 @@ func main() { err = trySigKillProcess(pid, mockOwnershipCheck) if err != nil { - t.Fatalf("while trying to kill child proc: %d", pid) + t.Fatalf("while trying to kill child proc %d: %s\n", pid, err) } // waiting for process to exit if err := processToKill.Wait(); !strings.Contains(err.Error(), "killed") { - t.Fatalf("unable to kill process") + t.Fatalf("unable to kill process: %s\n", err) } } From 8f50ee999a945bf123ade5042dd97c91cbe485cc Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 22:09:53 +0100 Subject: [PATCH 31/39] Corrects mendacious comment --- cmd/minikube/cmd/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 76f7acd3102a..10667b5f14d5 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -677,7 +677,7 @@ func trySigKillProcess(pid int, ocheck func(int) (bool, error)) error { } // doesPIDBelongToMinikube tries to find the process with that PID -// and checks if the executable name is "minikube" +// and checks if the executable name contains the string "minikube" func doesPIDBelongToMinikube(pid int) (bool, error) { entry, err := ps.FindProcess(pid) if err != nil { From 1398988d52f0a16f913f1f66d1167c79fb13400a Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 22:12:49 +0100 Subject: [PATCH 32/39] Regenerates docs --- site/content/en/docs/contrib/tests.en.md | 667 ----------------------- 1 file changed, 667 deletions(-) diff --git a/site/content/en/docs/contrib/tests.en.md b/site/content/en/docs/contrib/tests.en.md index ef9d2b3c94d5..1d3ff3407dca 100644 --- a/site/content/en/docs/contrib/tests.en.md +++ b/site/content/en/docs/contrib/tests.en.md @@ -494,673 +494,6 @@ verifies the docker/podman driver works with a custom subnet ## TestKicStaticIP starts minikube with the static IP flag -## TestPrintablePathCheckHandler - -## TestGet_progress - -## TestBzip2Decompressor - -## TestGzipDecompressor - -## TestTar - -## TestDecompressTarPermissions - -## TestTarBzip2Decompressor - -## TestDecompressor -is a helper function for testing generic decompressors. - -## TestTarGzipDecompressor - -## TestTarXzDecompressor - -## TestTarZstdDecompressor - -## TestXzDecompressor - -## TestZipDecompressor - -## TestDecompressZipPermissions - -## TestZstdDecompressor - -## TestBitBucketDetector - -## TestFileDetector - -## TestFileDetector_noPwd - -## TestFileDetector_relativeSymlink -If a relative symlink is passed in as the pwd to Detect, the resulting URL -can have an invalid path. - -## TestGCSDetector - -## TestGitDetector - -## TestGitHubDetector - -## TestGitLabDetector - -## TestS3Detector - -## TestDetect - -## TestFolderStorage_impl - -## TestFolderStorage - -## TestCopy - -## TestFileGetter_impl - -## TestFileGetter - -## TestFileGetter_sourceFile - -## TestFileGetter_sourceNoExist - -## TestFileGetter_dir - -## TestFileGetter_dirSymlink - -## TestFileGetter_GetFile - -## TestFileGetter_GetFile_Copy - -## TestFileGetter_percent2F -https://github.com/hashicorp/terraform/issues/8418 - -## TestFileGetter_ClientMode_notexist - -## TestFileGetter_ClientMode_file - -## TestFileGetter_ClientMode_dir - -## TestGCSGetter_impl - -## TestGCSGetter - -## TestGCSGetter_subdir - -## TestGCSGetter_GetFile - -## TestGCSGetter_GetGenerationFile - -## TestGCSGetter_GetFile_notfound - -## TestGCSGetter_ClientMode_dir - -## TestGCSGetter_ClientMode_file - -## TestGCSGetter_ClientMode_notfound - -## TestGCSGetter_Url - -## TestGCSGetter_GetFile_OAuthAccessToken - -## TestGitGetter_impl - -## TestGitGetter - -## TestGitGetter_branch - -## TestGitGetter_commitID - -## TestGitGetter_remoteWithoutMaster - -## TestGitGetter_shallowClone - -## TestGitGetter_shallowCloneWithTag - -## TestGitGetter_shallowCloneWithCommitID - -## TestGitGetter_branchUpdate - -## TestGitGetter_tag - -## TestGitGetter_GetFile - -## TestGitGetter_gitVersion - -## TestGitGetter_sshKey - -## TestGitGetter_sshSCPStyle - -## TestGitGetter_sshExplicitPort - -## TestGitGetter_sshSCPStyleInvalidScheme - -## TestGitGetter_submodule - -## TestGitGetter_setupGitEnv_sshKey - -## TestGitGetter_setupGitEnvWithExisting_sshKey - -## TestGitGetter_subdirectory_symlink - -## TestGitGetter_subdirectory - -## TestHgGetter_impl - -## TestHgGetter - -## TestHgGetter_branch - -## TestHgGetter_GetFile - -## TestHgGetter_HgArgumentsNotAllowed - -## TestHttpGetter_impl - -## TestHttpGetter_header - -## TestHttpGetter_requestHeader - -## TestHttpGetter_meta - -## TestHttpGetter_metaSubdir - -## TestHttpGetter_metaSubdirGlob - -## TestHttpGetter_none - -## TestHttpGetter_resume - -## TestHttpGetter_resumeNoRange -The server may support Byte-Range, but has no size for the requested object - -## TestHttpGetter_file - -## TestHttpGetter_http2server -tests that http.Request is not reused -between HEAD & GET, which would lead to race condition in HTTP/2. -This test is only meaningful for the race detector (go test -race). - -## TestHttpGetter_auth - -## TestHttpGetter_authNetrc - -## TestHttpGetter_cleanhttp -verify that the default httpClient no longer comes from http.DefaultClient - -## TestHttpGetter__RespectsContextCanceled - -## TestHttpGetter__XTerraformGetLimit - -## TestHttpGetter__XTerraformGetDisabled - -## TestHttpGetter__XTerraformGetDetected -test a source url with no protocol - -## TestHttpGetter__XTerraformGetProxyBypass - -## TestHttpGetter__XTerraformGetConfiguredGettersBypass - -## TestHttpGetter__endless_body - -## TestHttpGetter_subdirLink - -## TestS3Getter_impl - -## TestS3Getter - -## TestS3Getter_subdir - -## TestS3Getter_GetFile - -## TestS3Getter_GetFile_badParams - -## TestS3Getter_GetFile_notfound - -## TestS3Getter_ClientMode_dir - -## TestS3Getter_ClientMode_file - -## TestS3Getter_ClientMode_notfound - -## TestS3Getter_ClientMode_collision - -## TestS3Getter_Url - -## TestGet_badSchema - -## TestGet_file - -## TestGet_fileDecompressorExt -https://github.com/hashicorp/terraform/issues/11438 - -## TestGet_filePercent2F -https://github.com/hashicorp/terraform/issues/8418 - -## TestGet_fileDetect - -## TestGet_fileForced - -## TestGet_fileSubdir - -## TestGet_archive - -## TestGetAny_archive - -## TestGet_archiveRooted - -## TestGet_archiveSubdirWild - -## TestGet_archiveSubdirWildMultiMatch - -## TestGetAny_file - -## TestGetAny_dir - -## TestGetFile - -## TestGetFile_archive - -## TestGetFile_archiveChecksum - -## TestGetFile_archiveNoUnarchive - -## TestGetFile_checksum - -## TestGetFile_checksum_from_file - -## TestGetFile_checksumURL - -## TestGetFile_filename - -## TestGetFile_filename_path_traversal - -## TestGetFile_checksumSkip - -## TestParse - -## TestAddAuthFromNetrc - -## TestAddAuthFromNetrc_hasAuth - -## TestAddAuthFromNetrc_hasUsername - -## TestSourceDirSubdir - -## TestSourceSubdirGlob - -## TestRedactURL - -## TestDir - -## TestNewConstraint - -## TestConstraintCheck - -## TestConstraintsString - -## TestCollection - -## TestNewVersion - -## TestNewSemver - -## TestVersionCompare - -## TestVersionCompare_versionAndSemver - -## TestVersionEqual_nil - -## TestComparePreReleases - -## TestVersionMetadata - -## TestVersionPrerelease - -## TestVersionSegments - -## TestVersionSegments64 - -## TestVersionString - -## TestEqual - -## TestGreaterThan - -## TestLessThan - -## TestGreaterThanOrEqual - -## TestLessThanOrEqual - -## TestDecode_interface - -## TestDecode_interfaceInline - -## TestDecode_equal - -## TestDecode_flatMap - -## TestDecode_structure - -## TestDecode_structurePtr - -## TestDecode_structureArray - -## TestDecode_sliceExpand - -## TestDecode_structureMap - -## TestDecode_structureMapInvalid - -## TestDecode_interfaceNonPointer - -## TestDecode_intString - -## TestDecode_float32 - -## TestDecode_float64 - -## TestDecode_intStringAliased - -## TestDecode_Node - -## TestDecode_NestedNode - -## TestDecode_topLevelKeys -https://github.com/hashicorp/hcl/issues/60 - -## TestDecode_flattenedJSON - -## TestObjectListFilter - -## TestWalk - -## TestWalkEquality - -## TestWalkRewrite - -## TestIsValidFile - -## TestRunMultiplePaths - -## TestRunSubDirectories - -## TestRunStdin - -## TestRunStdinAndWrite - -## TestRunFileError - -## TestRunNoOptions - -## TestRunList - -## TestRunWrite - -## TestRunDiff - -## TestPosError_impl - -## TestType - -## TestListType - -## TestListOfMaps - -## TestListOfMaps_requiresComma - -## TestListType_leadComment - -## TestListType_lineComment - -## TestObjectType - -## TestObjectKey - -## TestCommentGroup - -## TestParse -Official HCL tests - -## TestParse_inline - -## TestFiles - -## TestFormatValidOutput -TestFormatParsable ensures that the output of Format() is can be parsed again. - -## TestPosition - -## TestNullChar - -## TestComment - -## TestOperator - -## TestBool - -## TestIdent - -## TestString - -## TestNumber - -## TestFloat - -## TestWindowsLineEndings - -## TestRealExample - -## TestScan_crlf - -## TestError - -## TestScanDigitsUnread - -## TestScanHeredocRegexpCompile - -## TestUnquote - -## TestTypeString - -## TestTokenValue - -## TestType - -## TestListType - -## TestObjectType - -## TestFlattenObjects - -## TestObjectKey - -## TestParse -Official HCL tests - -## TestParse_inline - -## TestPosition - -## TestComment - -## TestOperator - -## TestBool - -## TestIdent - -## TestString - -## TestNumber - -## TestFloat - -## TestRealExample - -## TestError - -## TestTypeString - -## TestLexMode - -## TestPackage - -## TestNow - -## TestNow - -## TestAdvanceLogs - -## TestWaitAdvance - -## TestAdvanceWithAfter - -## TestAdvanceWithAfterFunc - -## TestAfterFuncStop - -## TestNewTimerReset - -## TestNewTimerAsyncReset - -## TestNewTimerResetCausesWakeup - -## TestMultipleWaiters - -## TestSlowedAfter - -## TestFastAfter - -## TestSlowedAfterFunc - -## TestFastAfterFunc - -## TestSlowedNow - -## TestFastNow - -## TestAdvance - -## TestAdvanceMulti - -## TestStop - -## TestReset - -## TestStopReset - -## TestAdvanceAlreadyFired - -## TestAdvanceFast - -## TestAdvanceReset - -## TestAdvanceResetRacey - -## TestAll - -## TestErrorString - -## TestNewErr - -## TestNewErrWithCause - -## TestDeferredAnnotatef - -## TestAllErrors - -## TestNew - -## TestErrorf - -## TestTrace - -## TestAnnotate - -## TestAnnotatef - -## TestDeferredAnnotatef - -## TestWrap - -## TestWrapOfNil - -## TestWrapf - -## TestWrapfOfNil - -## TestMask - -## TestMaskf - -## TestCause - -## TestDetails - -## TestErrorStack - -## TestFormat - -## Test - -## TestTrimDefaultSet - -## TestTrimSourcePath - -## TestSetSourceTrimPrefix - -## Test - -## TestLockNoContention - -## TestLockBlocks - -## TestTryLock - -## TestTryLockNoBlock - -## TestUnlockedWithTimeout - -## TestLockWithTimeout - -## TestStress - -## TestLockFromOtherProcess - -## TestSpecValidity - -## TestLockNoContention - -## TestLockContentionWithinProcessTimeout - -## TestLockAfterTimeout - -## TestLockContentionWithinProcessCancel - -## TestSecondReleaseFine - -## TestDifferentNamesDontBlock - -## TestLockBlocks - -## TestProcessReleasesWhenDead - -## TestStress - -## TestMutexNotInherited -tests that subprocesses do not inherit mutexes. - -## TestFilePermissions -tests that the file permissions are correct. - -## TestFilePermissionsWithSudoEnvars -TestFilePermissionsWithSudo tests that the file permissions are correct. - -## TestLockFromOtherProcess - -## TestSleepFromOtherProcess - -## Test - ## TestingKicBaseImage will return true if the integraiton test is running against a passed --base-image flag From b09cd43e1d263bf9b8eac6397dd6ae998193867b Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 22:14:03 +0100 Subject: [PATCH 33/39] Corrects style issue --- cmd/minikube/cmd/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 10667b5f14d5..6d20f55d6ccd 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -659,7 +659,7 @@ func trySigKillProcess(pid int, ocheck func(int) (bool, error)) error { } if !itDoes { - return fmt.Errorf("Stale pid: %d", pid) + return fmt.Errorf("stale pid: %d", pid) } proc, err := os.FindProcess(pid) From 017098156704631e04c82a2b47d6c1deb98510ce Mon Sep 17 00:00:00 2001 From: x7upLime Date: Wed, 22 Mar 2023 23:13:10 +0100 Subject: [PATCH 34/39] Saves us from passing func as parameter :) --- cmd/minikube/cmd/delete.go | 10 +++++----- cmd/minikube/cmd/delete_test.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 6d20f55d6ccd..9ebb64288db4 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -622,7 +622,7 @@ func killProcess(path string) error { // error handling is done below var errs []error for _, pp := range ppp { - err := trySigKillProcess(pp, doesPIDBelongToMinikube) + err := trySigKillProcess(pp) if err != nil { errs = append(errs, err) } @@ -650,10 +650,10 @@ func killProcess(path string) error { } // trySigKillProcess takes a PID as argument and tries to SIGKILL it. -// It performs the OCHECK ownership check of the pid, +// It performs an ownership check of the pid, // before trying to send a sigkill signal to it -func trySigKillProcess(pid int, ocheck func(int) (bool, error)) error { - itDoes, err := ocheck(pid) +func trySigKillProcess(pid int) error { + itDoes, err := doesPIDBelongToMinikube(pid) if err != nil { return err } @@ -678,7 +678,7 @@ func trySigKillProcess(pid int, ocheck func(int) (bool, error)) error { // doesPIDBelongToMinikube tries to find the process with that PID // and checks if the executable name contains the string "minikube" -func doesPIDBelongToMinikube(pid int) (bool, error) { +var doesPIDBelongToMinikube = 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)) diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index c65fa81b39dc..f66bcac3c706 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -269,11 +269,11 @@ func main() { } pid := processToKill.Process.Pid - mockOwnershipCheck := func(int) (bool, error) { + doesPIDBelongToMinikube = func(int) (bool, error) { return true, nil } - err = trySigKillProcess(pid, mockOwnershipCheck) + err = trySigKillProcess(pid) if err != nil { t.Fatalf("while trying to kill child proc %d: %s\n", pid, err) } From b196ee46af14f24215fd49209c8959509c1448df Mon Sep 17 00:00:00 2001 From: x7upLime Date: Thu, 20 Apr 2023 20:31:45 +0200 Subject: [PATCH 35/39] Apply suggestions from code review --- cmd/minikube/cmd/delete.go | 10 +++++----- cmd/minikube/cmd/delete_test.go | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 9ebb64288db4..b4956c18f7fd 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -605,7 +605,7 @@ func killMountProcess() error { // 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 ecnountered, it cleans the pidfile +// 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) { @@ -646,7 +646,7 @@ func killProcess(path string) error { return errors.Wrap(err, "While closing mount-pids file") } - return err + return nil } // trySigKillProcess takes a PID as argument and tries to SIGKILL it. @@ -678,7 +678,7 @@ func trySigKillProcess(pid int) error { // doesPIDBelongToMinikube tries to find the process with that PID // and checks if the executable name contains the string "minikube" -var doesPIDBelongToMinikube = func(pid int) (bool, error) { +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)) @@ -697,9 +697,9 @@ var doesPIDBelongToMinikube = func(pid int) (bool, error) { return true, nil } -// GetPids opens the file at PATH and tries to read +// getPids opens the file at PATH and tries to read // one or more space separated pids -func GetPids(path string) ([]int, error) { +func getPids(path string) ([]int, error) { out, err := os.ReadFile(path) if err != nil { return nil, errors.Wrap(err, "ReadFile") diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index f66bcac3c706..fc69de4960d9 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -237,7 +237,7 @@ import ( "syscall" ) -// This is used to unittest functions that send termination +// 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) @@ -258,14 +258,14 @@ func main() { td := t.TempDir() tmpfile := filepath.Join(td, "waitForSig.go") - if err := os.WriteFile(tmpfile, waitForSig, 0o700); err != nil { - t.Fatalf("copying source to %s: %s\n", tmpfile, err) + 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: %s\n", err) + t.Fatalf("while execing child process: %v\n", err) } pid := processToKill.Process.Pid @@ -275,11 +275,11 @@ func main() { err = trySigKillProcess(pid) if err != nil { - t.Fatalf("while trying to kill child proc %d: %s\n", pid, err) + 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: %s\n", err) + t.Fatalf("unable to kill process: %v\n", err) } } From 3d3e6a14e7daaae751c84293146cc230f97f412d Mon Sep 17 00:00:00 2001 From: x7upLime Date: Thu, 20 Apr 2023 20:32:44 +0200 Subject: [PATCH 36/39] added stuff to reflect suggestions --- cmd/minikube/cmd/delete.go | 4 ++-- cmd/minikube/cmd/delete_test.go | 2 +- test/integration/functional_test_mount_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index b4956c18f7fd..9780a00e743a 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -613,7 +613,7 @@ func killProcess(path string) error { } klog.Infof("Found %s ...", pidPath) - ppp, err := GetPids(pidPath) + ppp, err := getPids(pidPath) if err != nil { return err } @@ -653,7 +653,7 @@ func killProcess(path string) error { // It performs an ownership check of the pid, // before trying to send a sigkill signal to it func trySigKillProcess(pid int) error { - itDoes, err := doesPIDBelongToMinikube(pid) + itDoes, err := isMinikubeProcess(pid) if err != nil { return err } diff --git a/cmd/minikube/cmd/delete_test.go b/cmd/minikube/cmd/delete_test.go index fc69de4960d9..36adc1cb4ead 100644 --- a/cmd/minikube/cmd/delete_test.go +++ b/cmd/minikube/cmd/delete_test.go @@ -269,7 +269,7 @@ func main() { } pid := processToKill.Process.Pid - doesPIDBelongToMinikube = func(int) (bool, error) { + isMinikubeProcess = func(int) (bool, error) { return true, nil } diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index ea920a6781ab..0c6f45a016c3 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -285,7 +285,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no } }) - t.Run("verify-cleanup", func(t *testing.T) { + t.Run("VerifyCleanup", func(t *testing.T) { tempDir := t.TempDir() ctx, cancel := context.WithTimeout(ctx, Minutes(10)) From aab0cb6d2a9197fddbb7ae1d7d0ca50a4a369316 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 22 Apr 2023 00:35:04 +0300 Subject: [PATCH 37/39] Adds mechanism to remove pid from pidfile if mount is sigtermd --- cmd/minikube/cmd/mount.go | 72 +++++++++++++++++++++++++++++++++++++-- translations/de.json | 1 + translations/es.json | 1 + translations/fr.json | 1 + translations/ja.json | 1 + translations/ko.json | 1 + translations/pl.json | 1 + translations/ru.json | 1 + translations/strings.txt | 1 + translations/zh-CN.json | 3 +- 10 files changed, 79 insertions(+), 4 deletions(-) diff --git a/cmd/minikube/cmd/mount.go b/cmd/minikube/cmd/mount.go index 60c8f8aeab0e..bd8f16618954 100644 --- a/cmd/minikube/cmd/mount.go +++ b/cmd/minikube/cmd/mount.go @@ -21,6 +21,7 @@ import ( "net" "os" "os/signal" + "path/filepath" "runtime" "strconv" "strings" @@ -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" ) @@ -202,7 +206,7 @@ var mountCmd = &cobra.Command{ out.Infof("Bind Address: {{.Address}}", out.V{"Address": net.JoinHostPort(bindIP, fmt.Sprint(port))}) var wg sync.WaitGroup - pid := make(chan int) + pidchan := make(chan int) if cfg.Type == nineP { wg.Add(1) go func(pid chan int) { @@ -211,8 +215,9 @@ var mountCmd = &cobra.Command{ ufs.StartServer(net.JoinHostPort(bindIP, strconv.Itoa(port)), debugVal, hostPath) out.Step(style.Stopped, "Userspace file server is shutdown") wg.Done() - }(pid) + }(pidchan) } + pid := <-pidchan // Unmount if Ctrl-C or kill request is received. c := make(chan os.Signal, 1) @@ -224,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, <-pid) + 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) @@ -268,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") + 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") + } + + pids := []int{} + strPids := strings.Fields(string(out)) + for _, p := range strPids { + intPid, err := strconv.Atoi(p) + if err != nil { + return errors.Wrap(err, "while converting pids") + } + + // we skip the pid we're looking for + if intPid == pid { + continue + } + + pids = append(pids, intPid) + } + + // 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)), " "), "[]")) + return lock.WriteFile(pidPath, []byte(newPids), 0o644) +} diff --git a/translations/de.json b/translations/de.json index 8362c15961e7..9ed7e20a7553 100644 --- a/translations/de.json +++ b/translations/de.json @@ -249,6 +249,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "Port, der für das über den Proxy erreichbare Dashboard freigegeben wird. Wenn man 0 angibt, wird ein zufälliger Port ausgewählt.", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "Externer Adapter, auf dem der externe Switch erzeugt wird, wenn kein externer Switch gefunden wurde. (nur hyperv Treiber)", "Fail check if container paused": "Schlägt fehl, wenn der Container pausiert ist", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "Runtime fehlgeschlagen", "Failed to build image": "Bau des Images fehlgeschlagen", "Failed to cache and load images": "Cachen und laden der Images fehlgeschlagen", diff --git a/translations/es.json b/translations/es.json index 46450d57993f..3673d2bbd898 100644 --- a/translations/es.json +++ b/translations/es.json @@ -258,6 +258,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "", "Fail check if container paused": "", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "", "Failed to build image": "No se pudo construir la imagen", "Failed to cache and load images": "", diff --git a/translations/fr.json b/translations/fr.json index cdc9bfde88ea..b3f7635a5a60 100644 --- a/translations/fr.json +++ b/translations/fr.json @@ -245,6 +245,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "Port exposé du tableau de bord proxyfié. Réglez sur 0 pour choisir un port aléatoire.", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "L'adaptateur externe sur lequel un commutateur externe sera créé si aucun commutateur externe n'est trouvé. (pilote hyperv uniquement)", "Fail check if container paused": "Échec de la vérification si le conteneur est en pause", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "Échec de l'exécution", "Failed to build image": "Échec de la création de l'image", "Failed to cache and load images": "Échec de la mise en cache et du chargement des images", diff --git a/translations/ja.json b/translations/ja.json index d5b83966a21d..4fa2db612541 100644 --- a/translations/ja.json +++ b/translations/ja.json @@ -235,6 +235,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "プロキシー化されたダッシュボードの公開ポート。0 に設定すると、ランダムなポートが選ばれます。", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "外部スイッチが見つからない場合に、外部スイッチが作成される外部アダプター (hyperv ドライバーのみ)。", "Fail check if container paused": "コンテナーが一時停止しているかどうかのチェックに失敗しました", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "ランタイムが失敗しました", "Failed to build image": "イメージのビルドに失敗しました", "Failed to cache and load images": "イメージのキャッシュとロードに失敗しました", diff --git a/translations/ko.json b/translations/ko.json index 27f51e2de0a6..9064e493ddbe 100644 --- a/translations/ko.json +++ b/translations/ko.json @@ -267,6 +267,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "", "Fail check if container paused": "", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "런타임이 실패하였습니다", "Failed to build image": "", "Failed to cache ISO": "ISO 캐싱에 실패하였습니다", diff --git a/translations/pl.json b/translations/pl.json index d57e0d027a16..494464de67e3 100644 --- a/translations/pl.json +++ b/translations/pl.json @@ -258,6 +258,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "", "Fail check if container paused": "", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "", "Failed to build image": "", "Failed to cache and load images": "", diff --git a/translations/ru.json b/translations/ru.json index 9b6d05b04cd5..d8540a316741 100644 --- a/translations/ru.json +++ b/translations/ru.json @@ -232,6 +232,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "", "Fail check if container paused": "", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "", "Failed to build image": "", "Failed to cache and load images": "", diff --git a/translations/strings.txt b/translations/strings.txt index 9f2cf97558b1..fa3bed8d79ac 100644 --- a/translations/strings.txt +++ b/translations/strings.txt @@ -232,6 +232,7 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "", "Fail check if container paused": "", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "", "Failed to build image": "", "Failed to cache and load images": "", diff --git a/translations/zh-CN.json b/translations/zh-CN.json index 839791d8ab19..8c794611093d 100644 --- a/translations/zh-CN.json +++ b/translations/zh-CN.json @@ -318,6 +318,7 @@ "Fail check if container paused": "如果容器已挂起,则检查失败", "Failed runtime": "运行时失败", "Failed to build image": "构建镜像失败", + "Failed removing pid from pidfile: {{.error}}": "", "Failed to cache ISO": "缓存ISO 时失败", "Failed to cache and load images": "缓存以及导入镜像失败", "Failed to cache binaries": "缓存二进制文件失败", @@ -1208,4 +1209,4 @@ "{{.profile}} profile is not valid: {{.err}}": "", "{{.type}} is not yet a supported filesystem. We will try anyways!": "", "{{.url}} is not accessible: {{.error}}": "" -} \ No newline at end of file +} From 455bcb19ef4b932eeff33345dcab869fcaf35fb0 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Sat, 22 Apr 2023 13:32:45 +0300 Subject: [PATCH 38/39] refactors removePid logic --- cmd/minikube/cmd/mount.go | 24 +++++++++++------------- pkg/minikube/cluster/mount.go | 2 +- pkg/minikube/node/config.go | 2 +- translations/zh-CN.json | 4 ++-- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/cmd/minikube/cmd/mount.go b/cmd/minikube/cmd/mount.go index bd8f16618954..4125c9ba3673 100644 --- a/cmd/minikube/cmd/mount.go +++ b/cmd/minikube/cmd/mount.go @@ -290,7 +290,7 @@ func removePidFromFile(pid int) error { } for _, path := range paths { - err := removePid(path, pid) + err := removePid(path, strconv.Itoa(pid)) if err != nil { return err } @@ -300,7 +300,7 @@ func removePidFromFile(pid int) error { } // removePid reads the file at PATH and tries to remove PID from it if found -func removePid(path string, pid int) error { +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) { @@ -314,23 +314,21 @@ func removePid(path string, pid int) error { return errors.Wrap(err, "ReadFile") } - pids := []int{} + 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 { - intPid, err := strconv.Atoi(p) - if err != nil { - return errors.Wrap(err, "while converting pids") - } - - // we skip the pid we're looking for - if intPid == pid { + // If we find the PID, we don't add it to the slice + if p == pid { continue } - pids = append(pids, intPid) + // if p doesn't correspond to PID, we add to a list + pids = append(pids, p) } - // 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)), " "), "[]")) + // we write the slice that we obtained back to the mount-pids file + newPids := strings.Join(pids, " ") return lock.WriteFile(pidPath, []byte(newPids), 0o644) } diff --git a/pkg/minikube/cluster/mount.go b/pkg/minikube/cluster/mount.go index 5ac564ab6e23..147729905f30 100644 --- a/pkg/minikube/cluster/mount.go +++ b/pkg/minikube/cluster/mount.go @@ -98,7 +98,7 @@ func Mount(r mountRunner, source string, target string, c *MountConfig, pid int) } 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 { + 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) } diff --git a/pkg/minikube/node/config.go b/pkg/minikube/node/config.go index c0aad6afd3a8..f58839aa5a5c 100644 --- a/pkg/minikube/node/config.go +++ b/pkg/minikube/node/config.go @@ -89,7 +89,7 @@ func configureMounts(wg *sync.WaitGroup, cc config.ClusterConfig) { if err := mountCmd.Start(); err != nil { exit.Error(reason.GuestMount, "Error starting mount", err) } - if err := lock.AppendToFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(fmt.Sprintf("%s ", strconv.Itoa(mountCmd.Process.Pid))), 0o644); err != nil { + if err := lock.AppendToFile(filepath.Join(localpath.Profile(profile), constants.MountProcessFileName), []byte(fmt.Sprintf(" %s", strconv.Itoa(mountCmd.Process.Pid))), 0o644); err != nil { exit.Error(reason.HostMountPid, "Error writing mount pid", err) } } diff --git a/translations/zh-CN.json b/translations/zh-CN.json index 8c794611093d..5eada44e95be 100644 --- a/translations/zh-CN.json +++ b/translations/zh-CN.json @@ -316,9 +316,9 @@ "Exposed port of the proxyfied dashboard. Set to 0 to pick a random port.": "", "External Adapter on which external switch will be created if no external switch is found. (hyperv driver only)": "", "Fail check if container paused": "如果容器已挂起,则检查失败", + "Failed removing pid from pidfile: {{.error}}": "", "Failed runtime": "运行时失败", "Failed to build image": "构建镜像失败", - "Failed removing pid from pidfile: {{.error}}": "", "Failed to cache ISO": "缓存ISO 时失败", "Failed to cache and load images": "缓存以及导入镜像失败", "Failed to cache binaries": "缓存二进制文件失败", @@ -1209,4 +1209,4 @@ "{{.profile}} profile is not valid: {{.err}}": "", "{{.type}} is not yet a supported filesystem. We will try anyways!": "", "{{.url}} is not accessible: {{.error}}": "" -} +} \ No newline at end of file From 8931619b0105bd9ce792ef77975511dda4f769e5 Mon Sep 17 00:00:00 2001 From: x7upLime Date: Tue, 25 Apr 2023 15:11:41 +0300 Subject: [PATCH 39/39] Applied suggestions --- cmd/minikube/cmd/delete.go | 10 +++++----- cmd/minikube/cmd/mount.go | 5 ++--- test/integration/functional_test_mount_test.go | 6 +++--- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/cmd/minikube/cmd/delete.go b/cmd/minikube/cmd/delete.go index 9780a00e743a..74ecf22bf1d4 100644 --- a/cmd/minikube/cmd/delete.go +++ b/cmd/minikube/cmd/delete.go @@ -587,7 +587,7 @@ func deleteMachineDirectories(cc *config.ClusterConfig) { // 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), @@ -643,7 +643,7 @@ func killProcess(path string) error { // 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 errors.Wrap(err, "while closing mount-pids file") } return nil @@ -664,13 +664,13 @@ func trySigKillProcess(pid int) error { proc, err := os.FindProcess(pid) if err != nil { - return errors.Wrap(err, fmt.Sprintf("os.FindProcess: %d", pid)) + 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) - return errors.Wrap(err, fmt.Sprintf("Removing likely stale unkillable pid: %d", pid)) + return errors.Wrapf(err, "removing likely stale unkillable pid: %d", pid) } return nil @@ -681,7 +681,7 @@ func trySigKillProcess(pid int) error { 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)) + return false, errors.Wrapf(err, "ps.FindProcess for %d", pid) } if entry == nil { klog.Infof("Process not found. pid %d", pid) diff --git a/cmd/minikube/cmd/mount.go b/cmd/minikube/cmd/mount.go index 4125c9ba3673..dde841f25977 100644 --- a/cmd/minikube/cmd/mount.go +++ b/cmd/minikube/cmd/mount.go @@ -30,7 +30,6 @@ 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" @@ -283,7 +282,7 @@ func getPort() (int, error) { // 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") + profile := ClusterFlagValue() paths := []string{ localpath.MiniPath(), // legacy mount-process path for backwards compatibility localpath.Profile(profile), @@ -311,7 +310,7 @@ func removePid(path string, pid string) error { // we're reading the pids... out, err := os.ReadFile(pidPath) if err != nil { - return errors.Wrap(err, "ReadFile") + return errors.Wrap(err, "readFile") } pids := []string{} diff --git a/test/integration/functional_test_mount_test.go b/test/integration/functional_test_mount_test.go index 0c6f45a016c3..a04e2a800350 100644 --- a/test/integration/functional_test_mount_test.go +++ b/test/integration/functional_test_mount_test.go @@ -120,7 +120,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no if err := retry.Expo(checkMount, time.Millisecond*500, Seconds(15)); err != nil { // For local testing, allow macOS users to click prompt. If they don't, skip the test. if runtime.GOOS == "darwin" { - t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-codesigned binaries to listen on non-localhost port") + t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-code signed binaries to listen on non-localhost port") } t.Fatalf("/mount-9p did not appear within %s: %v", time.Since(start), err) } @@ -248,7 +248,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no if err := retry.Expo(checkMount, time.Millisecond*500, Seconds(15)); err != nil { // For local testing, allow macOS users to click prompt. If they don't, skip the test. if runtime.GOOS == "darwin" { - t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-codesigned binaries to listen on non-localhost port") + t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-code signed binaries to listen on non-localhost port") } t.Fatalf("/mount-9p did not appear within %s: %v", time.Since(start), err) } @@ -337,7 +337,7 @@ func validateMountCmd(ctx context.Context, t *testing.T, profile string) { // no if err := retry.Expo(checkMount, time.Millisecond*500, Seconds(15)); err != nil { // For local testing, allow macOS users to click prompt. If they don't, skip the test. if runtime.GOOS == "darwin" { - t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-codesigned binaries to listen on non-localhost port") + t.Skip("skipping: mount did not appear, likely because macOS requires prompt to allow non-code signed binaries to listen on non-localhost port") } t.Fatalf("mount was not ready in time: %v", err) }