Prevent uptimeFunc from being called everytime CheckHealth is called

[mcshooter]

There is currently an issue for Windows nodes where NPD is causing the CPU to hit and persist at 100%. After investigating, it looks like there was a change that moved uptimeFunc to be called every time we checked the health through health checker. We shouldn't be calling uptimeFunc unless we know it has been determined that one of the services is unhealthy and there are no patterns to be checking. For Windows, because the query to get and calculate the uptime is pretty heavy, having it be called every 10s or so for each of the separate services would unnecessarily increase usage. This change will revert part of how we CheckHealth and move the loopBackTime logic inside of the logPatternHealthCheck function to only be done if there are log patterns that need to be checked.

[k8s-ci-robot]

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 /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/test-infra repository.

[mcshooter]

/hold testing in progress

[mcshooter]

/cc @smileusd

[k8s-ci-robot]

@mcshooter: GitHub didn't allow me to request PR reviews from the following users: smileusd.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @smileusd

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.

[mcshooter]

/cc @pjh

[mcshooter]

/cc @ibabou

[mcshooter]

cc/ @Random-Liu

[jeremyje]

/sig node
/sig windows
/priority important-soon
/ok-to-test

[pjh]

I reviewed but don't have much to offer. At a quick glance it's challenging to read the code here (existing and modified) since I'm not sure what loopBackTime means.

But +1 to the goal of not calling the uptimeFunc as often to reduce CPU utilization. We'll still need to work on further changes to avoid starting numerous Powershell sessions on Windows.

[mcshooter]

/unhold

[mcshooter]

Was able to test locally on a kubernetes node

[Random-Liu]

@liyanhui1228

[Random-Liu]

In this case, do we still intend to use log pattern or repair on windows?

If not, can we just declare it as not properly supported, and return error if users try to use log pattern or repair on windows?

In that way, we can no-op those functions for windows instead.

The current code makes me feel like we will never use it, because it has serious performance issue. In that case, I prefer we disable the feature and cleanup the corresponding code, instead of moving the code around to make the code never run just for us. Because others may still hit the issue if they try to use it.

[mcshooter]

In this case, do we still intend to use log pattern or repair on windows?

If not, can we just declare it as not properly supported, and return error if users try to use log pattern or repair on windows?

In that way, we can no-op those functions for windows instead.

The current code makes me feel like we will never use it, because it has serious performance issue. In that case, I prefer we disable the feature and cleanup the corresponding code, instead of moving the code around to make the code never run just for us. Because others may still hit the issue if they try to use it.

We still do use the repair functionality. So it doesn't make sense to remove this as a windows functionality. The code will still run to repair and call uptime but only in the case when it expects it to.

[Random-Liu]

Offline discussed, there is no good way to get the last start time when the service is down.

So the current fix makes sense to me

[Random-Liu]

/lgtm
/approve

[k8s-ci-robot]

[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:

• OWNERS [Random-Liu]

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