improvement health-checker #539

smileusd · 2021-03-10T09:35:07Z

/kind bug

add loopbacktime to reduce time of journalctl call
If use start time of kubelet as the journalctl, it could be take too much time to call.
I add loopkbacktime to set loop back time for journalctl, if not set or set = 0, it uses start time directly, else it use loopkbacktime.
fix timelayout for all timezones
Different nodes usally use different time zone:
[root@kubernetes-master-505-pkshx-4589367 software]# systemctl show kubelet --property=InactiveExitTimestamp InactiveExitTimestamp=Wed 2021-03-10 06:05:51 PST
I think npd doesn't need to care about the timezone if can ensure timezone is agreement with runtime/kubelet and journald.

this point has been fixed by #558

add npd health-checker deploy file
Many users reflect that can not use health-checker because use the wrong specs: !439, !540, !482. I thinks this commit can fix these issues.

k8s-ci-robot · 2021-03-10T09:35:15Z

Hi @smileusd. 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.

smileusd · 2021-03-11T09:41:22Z

@Random-Liu PTAL

Random-Liu · 2021-03-13T00:09:29Z

@abansal4032 Hey archit, can you take a look at this change?

Random-Liu · 2021-03-13T00:10:23Z

/assign @@abansal4032

Random-Liu · 2021-03-13T01:01:09Z

/ok-to-test

smileusd · 2021-03-15T09:43:31Z

/test pull-npd-e2e-test

abansal4032 · 2021-03-15T18:33:17Z

config/health-checker-kubelet.json

@@ -25,6 +25,7 @@
        "--component=kubelet",
        "--enable-repair=true",
        "--cooldown-time=1m",
+        "--loopback-time=5m",


Maybe let the default config use the default loopback time to keep the same behavior as it was before this change?

Default loopback time is zero, so if not this set. It is the same behavior as it was before. Here is a example to ues.

I set to 0.

abansal4032 · 2021-03-15T18:33:22Z

pkg/healthchecker/health_checker.go

-	logPatternHealthy, err := logPatternHealthCheck(hc.systemdService, hc.logPatternsToCheck)
+	uptime, err := hc.uptimeFunc()
+	if err != nil {
+		return false, err


This will return the service as unhealthy in case of parsing errors in uptime. Might lead to unnecessary restarts of the service. We can log the error here and return true as we are not sure if the service was unhealthy at this point.

abansal4032 · 2021-03-15T18:49:56Z

deployment/node-problem-detector-healthchecker.yaml

+          name: machine-id
+          readOnly: true
+        - mountPath: /run/systemd/system
+          name: systemd


Were you able to run the deployment and get the health signals without errors using this configuration?

Yes, this deployment has been ran in our productions.

abansal4032 · 2021-04-19T17:16:48Z

/lgtm

k8s-ci-robot · 2021-04-19T17:16:57Z

@abansal4032: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

Random-Liu · 2021-04-19T17:21:53Z

pkg/healthchecker/health_checker.go

@@ -82,7 +82,8 @@ func getUptimeFunc(service string) func() (time.Duration, error) {
 		if len(val) < 2 {
 			return time.Duration(0), errors.New("could not parse the service uptime time correctly")
 		}
-		t, err := time.Parse(types.UptimeTimeLayout, val[1])
+		timeEndIndex := strings.LastIndex(val[1], " ")
+		t, err := time.Parse(types.UptimeTimeLayout, val[1][:timeEndIndex])


IIUC, if you run NPD in a container, it may not be in the same time zone with the host, in that case would this cause any problem?

What is the problem if we don't make this change? I don't quite get it after reading the PR description.

The resoult for the cmd out is like this:

InactiveExitTimestamp=Wed 2021-04-14 01:11:39 -07

But types.UptimeTimeLayout is UptimeTimeLayout = "Mon 2006-01-02 15:04:05"
It is failed to parse time

The UptimeTimeLayout was "Mon 2006-01-02 15:04:05 UTC" before your change.

And in your PR description, the systemctl output was:

[root@kubernetes-master-505-pkshx-4589367 software]# systemctl show kubelet --property=InactiveExitTimestamp InactiveExitTimestamp=Wed 2021-03-10 06:05:51 PST

And on my node it is also:

$ systemctl show docker --property=InactiveExitTimestamp InactiveExitTimestamp=Thu 2021-04-15 17:20:37 PDT

Why would the parsing fail?

I rollback to old version and retry on my node, it is still wrong and reture error:

I0425 22:54:26.604844 18356 plugin.go:87] Add check result {Rule:0xc4200bd9d0 ExitStatus:2 Message:systemctl show: Sun 2021-04-25 11:22:50 PDT error checking kubelet health: pars}

Then I code a test case like this:

import ( "fmt" "time" ) const UptimeTimeLayout = "Mon 2006-01-02 15:04:05 UTC" func main() { strs := []string{ "Wed 2021-03-10 03:39:13 PST", "Sun 2021-04-25 11:22:50 PDT", } t, err := time.Parse(UptimeTimeLayout, strs[0]) if err != nil { fmt.Println("error: ", err) } t, err = time.Parse(UptimeTimeLayout, strs[1]) if err != nil { fmt.Println("error: ", err) } fmt.Println(t.String()) }

It is also wrong:

error: parsing time "Wed 2021-03-10 03:39:13 PST" as "Mon 2006-01-02 15:04:05 UTC": cannot parse "PST" as " UTC" error: parsing time "Sun 2021-04-25 11:22:50 PDT" as "Mon 2006-01-02 15:04:05 UTC": cannot parse "PDT" as " UTC" 0001-01-01 00:00:00 +0000 UTC Exiting.

I think Golang don't recognize UTC is a time zoom, but only a suffix.

I see. In that case, we probably need a better fix.

package main import ( "fmt" "time" ) const UptimeTimeLayout = "Mon 2006-01-02 15:04:05 MST" func main() { strs := []string{ "Wed 2021-03-10 03:39:13 PST", "Sun 2021-04-25 11:22:50 PDT", } t, err := time.Parse(UptimeTimeLayout, strs[0]) if err != nil { fmt.Println("error: ", err) } fmt.Println(t.String()) t, err = time.Parse(UptimeTimeLayout, strs[1]) if err != nil { fmt.Println("error: ", err) } fmt.Println(t.String()) }

Can you change it to MST?

When parsing a time with a zone abbreviation like MST, if the zone abbreviation has a defined offset in the current location, then that offset is used.

Random-Liu · 2021-05-10T23:41:11Z

Hi @smileusd Can you rebase your change and fix the timestamp format?

We plan to cut a release this week. :)

smileusd · 2021-05-19T05:59:10Z

Hi @smileusd Can you rebase your change and fix the timestamp format?

We plan to cut a release this week. :)

Sorry to late see this.
Done

smileusd · 2021-05-19T06:06:24Z

/test pull-npd-e2e-test

smileusd · 2021-05-19T08:29:58Z

@Random-Liu @abansal4032 PTAL

smileusd · 2021-06-21T09:30:21Z

@Random-Liu Any updates?

Random-Liu

/lgtm
/approve

k8s-ci-robot · 2021-06-25T16:25:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abansal4032, Random-Liu, smileusd

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 10, 2021

k8s-ci-robot requested review from andyxning and wangzhen127 March 10, 2021 09:35

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

smileusd changed the title ~~add loopbacktime to reduce time of journalctl call~~ improvement health-checker Mar 11, 2021

smileusd force-pushed the health_check branch from e0d37b5 to 7ecdfbd Compare March 11, 2021 09:40

Random-Liu self-assigned this Mar 13, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 13, 2021

abansal4032 suggested changes Mar 15, 2021

View reviewed changes

smileusd force-pushed the health_check branch from 7ecdfbd to 8acefa8 Compare April 5, 2021 04:10

Random-Liu reviewed Apr 19, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2021

smileusd added 2 commits May 19, 2021 13:55

add loopbacktime to reduce time of journalctl call

a3b9284

add npd health-ckecker deploy file

b409875

smileusd force-pushed the health_check branch from 8acefa8 to b409875 Compare May 19, 2021 05:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021

smileusd requested a review from Random-Liu May 27, 2021 02:59

Random-Liu reviewed Jun 25, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2021

k8s-ci-robot merged commit e349323 into kubernetes:master Jun 25, 2021

smileusd mentioned this pull request Nov 26, 2024

REQUEST: New membership for smileusd kubernetes/org#5274

Closed

11 tasks

improvement health-checker #539

improvement health-checker #539

Uh oh!

Conversation

smileusd commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 10, 2021

Uh oh!

smileusd commented Mar 11, 2021

Uh oh!

Random-Liu commented Mar 13, 2021

Uh oh!

Random-Liu commented Mar 13, 2021

Uh oh!

Random-Liu commented Mar 13, 2021

Uh oh!

smileusd commented Mar 15, 2021

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!

abansal4032 commented Apr 19, 2021

Uh oh!

k8s-ci-robot commented Apr 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smileusd Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Random-Liu commented May 10, 2021

Uh oh!

smileusd commented May 19, 2021

Uh oh!

smileusd commented May 19, 2021

Uh oh!

smileusd commented May 19, 2021

Uh oh!

smileusd commented Jun 21, 2021

Uh oh!

Random-Liu left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 25, 2021

Uh oh!

Uh oh!

smileusd commented Mar 10, 2021 •

edited

Loading

Random-Liu Apr 20, 2021 •

edited

Loading

smileusd Apr 26, 2021 •

edited

Loading