-
Notifications
You must be signed in to change notification settings - Fork 606
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
Update powershell command for uptime to help efficiency #612
Conversation
Hi @mcshooter. 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/test-infra repository. |
cc/ @ibabou |
cc/ @pjh |
/sig node |
@mcshooter: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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/test-infra repository. |
/ok-to-test |
/lgtm |
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.
LGTM, with one question.
28cd197
to
dd0d0d7
Compare
/retest |
@@ -33,12 +33,19 @@ import ( | |||
// getUptimeFunc returns the time for which the given service has been running. | |||
func getUptimeFunc(service string) func() (time.Duration, error) { | |||
return func() (time.Duration, error) { | |||
// Using the WinEvent Log Objects to find the Service logs' time when the Service last entered running state. |
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.
Do we need uptime
if we already know that the service is not running?
Maybe we can just return a known error or uptime value to indicate that the service is not running?
And from the caller side, it should treat the service as not healthy in that case.
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.
With that, we may not even need the first fix.
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.
Offline discussed, the uptime is still needed to calculate the cooldown even if the service is not running.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ibabou, mcshooter, Random-Liu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
When uptime function is called, we make a powershell query to trace through all the WinEvents. To attempt to make this call a little efficient, we will attempt to grab the process ID of the service (if it is running) and query the starttime based off of that. This will prevent the extensive querying that we do when calling Get-WinEvent. This only takes care of the case if the service is still running and an issue is detected.
If the Process ID does not exist and the service seems to have stopped running, then we will result to using the Get-WinEvent querying approach with an additional filter. I added an additional filter to filter not only by the logname=system but to also filter on event id=7036 to reduce the number of entries the next command
Where-Object
will have to look through. It seems that all messages indicating a stopped or running service will have the event id=7036.