Skip to content

Fix unit test failures for TestTryKillOne #21426

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix unit test failures for TestTryKillOne #21426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d00df82
process: add ExistsPID and make TestTryKillOne platform-agnostic
bobsira Sep 3, 2025
8a64f7d
added a short timout before checking if process exists
bobsira Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 21 additions & 6 deletions cmd/minikube/cmd/delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import (
"os"
"os/exec"
"path/filepath"
"strings"
"testing"
"time"

"github.com/docker/machine/libmachine"
"github.com/google/go-cmp/cmp"
Expand All @@ -32,6 +32,7 @@ import (
cmdcfg "k8s.io/minikube/cmd/minikube/cmd/config"
"k8s.io/minikube/pkg/minikube/config"
"k8s.io/minikube/pkg/minikube/localpath"
"k8s.io/minikube/pkg/minikube/process"
)

// exclude returns a list of strings, minus the excluded ones
Expand Down Expand Up @@ -242,7 +243,7 @@ func main() {
done := make(chan struct{})
defer close(ch)

signal.Notify(ch, syscall.SIGHUP)
signal.Notify(ch, syscall.SIGTERM)
defer signal.Stop(ch)

go func() {
Expand All @@ -263,7 +264,7 @@ func main() {
processToKill := exec.Command("go", "run", tmpfile)
err := processToKill.Start()
if err != nil {
t.Fatalf("while execing child process: %v\n", err)
t.Fatalf("while executing child process: %v\n", err)
}
pid := processToKill.Process.Pid

Expand All @@ -276,8 +277,22 @@ func main() {
t.Fatalf("while trying to kill child proc %d: %v\n", pid, err)
}

// waiting for process to exit
if err := processToKill.Wait(); !strings.Contains(err.Error(), "killed") {
t.Fatalf("unable to kill process: %v\n", err)
done := make(chan error, 1)
go func() { done <- processToKill.Wait() }()

var waitErr error
select {
case waitErr = <-done:
t.Logf("child process wait result: %v", waitErr)
case <-time.After(1 * time.Second):
t.Fatalf("timed out waiting for process %d to exit", pid)
}

exists, err := process.ExistsPID(pid)
if err != nil {
t.Fatalf("error checking process existence for pid %d: %v", pid, err)
}
if exists {
t.Fatalf("process %d still exists after trySigKillProcess; waitErr=%v", pid, waitErr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sending SIGKILL (or any other signal is asynchronous. It can return before the process was killed.

So checking if the pid exist in the processes table is racy and incorrect. The way to check the process status and detect if it was killed is to wait() for the process:
https://pkg.go.dev/os#Process.Wait

and get the system dependent exist status:
https://pkg.go.dev/os#ProcessState.Sys

On posix we can get check if the process was terminated by signal and get the signal from
https://pkg.go.dev/syscall#WaitStatus

In process_test.go we test Kill() little differently:

  1. Kill the process (async)
  2. We start a timer to fail test after a short timeout
  3. Wait of the process
  4. Check that Exists() return false

We don't verify the exit status since our test process can be configured to block SIGTERM, and we know that the only reason for termination can be SIGKILL.

If we need ugly platform specific code to check the exit status we can put it in the process package to make this easy to use everywhere. The package already have build tags to make it easy to do the right thing on windows and other platforms.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current change,

  1. I've kill the process with trySigKillProcess(PID)
  2. Then I've created a channel and spawn goroutine to Wait()
  3. I then wait for either process exit or timeout
  4. Finally I check process table for PID presence.

I've avoided adding any platform-dependent code checks for exit status to make a short and clean change.

any thoughts, @nirs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on for Wait() with t timeout looks correct and better than the way we do this in process_test.go, since we know we will stop waiting on timeout. I'm not sure the wait is intrrupted in current code in process_test.go.

The check for pid exists seems unneeded - if the the wait completed we know the process terminated so there is nothing to check. And if there is nothing to check there is no need to add the process.ExistPID().

I agree that we don't have to verify that the process was terminated by signal, but this seems like a useful utility to have.

}
}
14 changes: 14 additions & 0 deletions pkg/minikube/process/process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,20 @@ func Exists(pid int, executable string) (bool, error) {
return entry.Executable() == executable, nil
}

// ExistsPID reports whether a process with the given pid exists.
// This is a PID-only check (no executable name matching).
func ExistsPID(pid int) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we call this? if we have a child process we know that the pid exist and we don't need to look it up since the pid cannot be reused until we wait() for the child.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this PIDExist() - since it is a little bit confusing with Exists() which is about a process, using a pidfile. We already have a function pidExists() so this function can simply expose it, or we can renamse pidExists() to PIDExists() to make it useful outside of the package.

if pid <= 0 {
return false, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems unneeded - why would you call with invalid pid?


entry, err := ps.FindProcess(pid)
if err != nil {
return true, err
}
return entry != nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect - we have pidExist() helper that does the right thing on windows and posix.

}

// Terminate a process with pid and matching name. Returns os.ErrProcessDone if
// the process does not exist, or nil if termination was requested. Caller need
// to wait until the process disappears.
Expand Down