Fix unit test failures for TestTryKillOne #21426

bobsira · 2025-08-26T12:56:02Z

k8s-ci-robot · 2025-08-26T12:56:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bobsira
Once this PR has been reviewed and has the lgtm label, please assign medyagh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-08-26T12:56:12Z

Hi @bobsira. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

minikube-bot · 2025-08-26T12:57:03Z

Can one of the admins verify this patch?

cmd/minikube/cmd/delete.go

cmd/minikube/cmd/delete_test.go

pkg/util/utils_test.go

bobsira · 2025-08-26T17:56:24Z

creating an issue to track all the windows failures here -> #21427

cmd/minikube/cmd/delete.go

medyagh · 2025-08-28T04:08:36Z

@bobsira plz see nirs comments and also rebase

k8s-ci-robot · 2025-08-28T04:08:44Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

nirs

Squashing the commits will make this easier to review.

nirs · 2025-09-02T16:37:27Z

pkg/minikube/process/process.go

@@ -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) {


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.

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.

nirs · 2025-09-02T16:51:21Z

cmd/minikube/cmd/delete_test.go

+		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)


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:

Kill the process (async)

We start a timer to fail test after a short timeout

Wait of the process

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.

In the current change,

I've kill the process with trySigKillProcess(PID)

Then I've created a channel and spawn goroutine to Wait()

I then wait for either process exit or timeout

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

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.

- Add exported ExistsPID(pid int) to pkg/minikube/process - Update cmd/minikube/cmd/delete_test.go to wait & use ExistsPID - Remove runtime.GOOS branching and brittle Wait() string checks

nirs · 2025-09-04T15:56:49Z

cmd/minikube/cmd/delete_test.go

+		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)


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.

nirs · 2025-09-04T16:04:29Z

pkg/minikube/process/process.go

+func ExistsPID(pid int) (bool, error) {
+	if pid <= 0 {
+		return false, nil
+	}


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

nirs · 2025-09-04T16:05:28Z

pkg/minikube/process/process.go

+	if err != nil {
+		return true, err
+	}
+	return entry != nil, nil


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

nirs · 2025-09-04T16:08:13Z

pkg/minikube/process/process.go

@@ -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) {


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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2025

k8s-ci-robot requested review from nirs and prezha August 26, 2025 12:56

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 26, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 26, 2025

nirs reviewed Aug 26, 2025

View reviewed changes

djeada reviewed Aug 27, 2025

View reviewed changes

cmd/minikube/cmd/delete.go Outdated Show resolved Hide resolved

bobsira changed the title ~~test: make chown tests cross-platform and relax TryKill test on Windows~~ Fix for TestTryKillOne unit test failure on windows Aug 27, 2025

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2025

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 28, 2025

bobsira changed the title ~~Fix for TestTryKillOne unit test failure on windows~~ Fix unit test failures for TestTryKillOne Sep 2, 2025

nirs reviewed Sep 2, 2025

View reviewed changes

process: add ExistsPID and make TestTryKillOne platform-agnostic

d00df82

- Add exported ExistsPID(pid int) to pkg/minikube/process - Update cmd/minikube/cmd/delete_test.go to wait & use ExistsPID - Remove runtime.GOOS branching and brittle Wait() string checks

bobsira force-pushed the fit-windows-unit-tests-chown-trykill branch from 906b6c5 to d00df82 Compare September 3, 2025 14:24

nirs mentioned this pull request Sep 3, 2025

test: Remove duplicate kubernetes versions in tests #21483

Open

added a short timout before checking if process exists

8a64f7d

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 3, 2025

nirs reviewed Sep 4, 2025

View reviewed changes

Fix unit test failures for TestTryKillOne #21426

Are you sure you want to change the base?

Fix unit test failures for TestTryKillOne #21426

Conversation

bobsira commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 26, 2025

Uh oh!

k8s-ci-robot commented Aug 26, 2025

Uh oh!

minikube-bot commented Aug 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bobsira commented Aug 26, 2025

Uh oh!

Uh oh!

medyagh commented Aug 28, 2025

Uh oh!

k8s-ci-robot commented Aug 28, 2025

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bobsira commented Aug 26, 2025 •

edited

Loading