Fix for TestTryKillOne unit test failure on windows #21426
Conversation
bobsira
commented
Aug 26, 2025
bobsira
commented
- Made chown-related tests portable: removed compile-time Unix-only assumptions, closed temp files, and used runtime checks + reflection so ownership assertions run only on platforms that expose Uid/Gid. This prevents Windows build failures and TempDir cleanup errors.
- Adjusted behavior expectations for Windows SIDs (non-numeric Uid) so tests don't call strconv.Atoi on a SID.
- Consolidated duplicated test loops to run each case once and avoid flaky/misleading failures.
- Relaxed the process-kill test (TestTryKillOne) to treat any process termination as success and stop asserting on platform-specific Wait() error strings (Windows vs POSIX differences).
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bobsira The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
size/L
|
Can one of the admins verify this patch? |
| if err := proc.Kill(); err != nil { | ||
| klog.Infof("Kill failed with %v - removing probably stale pid...", err) | ||
| return errors.Wrapf(err, "removing likely stale unkillable pid: %d", pid) | ||
| } |
There was a problem hiding this comment.
If we want to do graceful termination,we should not add platform specific code in this file. We have a process package supporting both posix and windows. Can we use it here?
| klog.Infof("Kill failed with %v - removing probably stale pid...", err) | ||
| return errors.Wrapf(err, "removing likely stale unkillable pid: %d", pid) | ||
| // On non-Windows try to send SIGUP first (graceful), fallback to kill if signaling fails. | ||
| // Use numeric syscall.Signal(1) (SIGUP) to avoid referencing platform-specific constant names. |
There was a problem hiding this comment.
This is not graceful shutdown on posix. Most processes do not handle SIGHUP but they expect SIGTERM as gracefull termination. Previous code did not do graceful termination, why do we ned to do this now?
| // On POSIX we signal SIGHUP — child should exit cleanly. | ||
| if waitErr != nil { | ||
| t.Fatalf("unable to kill process: %v\n", waitErr) | ||
| } |
There was a problem hiding this comment.
We should not have platform specific code in this file. If we need different code for different platforms it should be implemented once in minikube. Can we add this code to the process package?
| t.Fatalf("expected non-nil Wait error on windows, got nil") | ||
| } | ||
| } else { | ||
| // On POSIX we signal SIGHUP — child should exit cleanly. |
There was a problem hiding this comment.
SIGHUP is not graceful termination, we expect SIGTERM.
|
creating an issue to track all the windows failures here -> #21427 |
| return errors.Wrapf(err, "removing likely stale unkillable pid: %d", pid) | ||
| // On non-Windows try to send SIGUP first (graceful), fallback to kill if signaling fails. | ||
| // Use numeric syscall.Signal(1) (SIGUP) to avoid referencing platform-specific constant names. | ||
| if runtime.GOOS != "windows" { |
There was a problem hiding this comment.
you can just call err := process.Terminate(pid, "") and then handle it with if err != nil { ... }. the package is k8s.io/minikube/pkg/minikube/process
There was a problem hiding this comment.
I would not use the process package if we don't have pid file. But maybe we an extend it to handle child processes where you know the pid and we don't need to verify that the pid exists.
bobsira
changed the title
k8s-ci-robot
added
size/M
|
@bobsira plz see nirs comments and also rebase |
|
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. |
k8s-ci-robot
added
the
needs-rebase
