-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
Fix unit test failures for TestTryKillOne #21426
Conversation
[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.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
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.
Can one of the admins verify this patch?
creating an issue to track all the windows failures here -> #21427
@bobsira plz see nirs comments and also rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashing the commits will make this easier to review.
pkg/minikube/process/process.go
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nirs is there anything in specific that you'd want me to further address in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@medyagh I believe this PR can be checked in now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bob plz check my comment bellow
- 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
906b6c5 to
d00df82
Compare
There was a problem hiding this comment.
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.
pkg/minikube/process/process.go
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nirs I've centralize the pid-existence logic and exposed PIDExists functionality
pkg/minikube/process/process.go
Outdated
There was a problem hiding this comment.
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.
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the way. Previously we have 2 simple implementations using the os package without dependency on any library. Now both of them depend on a 3rd party library which is archived and not maintained for new years. We should use this library only when we cannot use the standard library.
The right change would be to remove this function, and rename pidExists() to PIDExists().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this function as is, and rename it to PIDExists()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobsira I agree please just rename the func to be PIDExists
no need for the private func to call the public func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this function as is, and rename it to PIDExists()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobsira I agree please just rename the func to be PIDExists
no need for the private func to call the public func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobsira, this is not a comment about the actual diff and nature of the changes but an observation about the commits in this PR:
I noticed that there are three commits in this PR thar are there twice:
refactored pidExists functionalityadded a short timout before checking if process existsprocess: add ExistsPID and make TestTryKillOne platform-agnostic
I am not quite sure how this can have happened but I guess it was not your intention.
Would you consider cleaning the series of commits?
@bobsira, this is not a comment about the actual diff and nature of the changes but an observation about the commits in this PR:
...
Would you consider cleaning the series of commits?
I agree. Unfortunately clean commit history is not popular in this project. I hope we can improve in this area.
I would split it to 2 commits:
- Exporting process.PIDExist
- Fixing TestTryKillOne on windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bob plz check my comment bellow
Uh oh!
There was an error while loading. Please reload this page.