-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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 pleg relist time #45496
fix pleg relist time #45496
Conversation
/assign @derekwaynecarr PTAL. |
@k8s-bot ok to test |
@andyxning the original intention of the check is to indicate that kubelet is not function normally -- it's not updating its view of the containers/pods in the time frame. When that happens, kubelet will not react to changes in container states and will not be able to start new pods. That's why I wanted to surface it to the node status. What's the reason of changing the logic? |
Just for docker hung for example.
In However, just as what What we should do is to update the latest relist time only when we successfully relist, i.e., without docker hung. If we encounter docker hung, the latest relist time may longer than TL;DR: |
friendly ping @yujuhong :) |
/cc @yujuhong @derekwaynecarr @dchen1107 :) |
pkg/kubelet/pleg/generic.go
Outdated
glog.Errorf("GenericPLEG: Unable to retrieve pods: %v", err) | ||
return | ||
} | ||
|
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.
You need to move timestamp := g.clock.Now()
and the defer block for PLEGRelistLatency
to before the GetPods
. Otherwise, we won't be recording the right metrics.
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.
@yujuhong Right. Will do.
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.
Done. PTAL.
BTW, @yujuhong Does this PR can be cherry picked to 1.3/1.4/1.5/1.6?
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.
/ping @yujuhong
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.
@andyxning Why do you need to cherry pick this to 1.3? Are you still using it?
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.
Yes, some clusters are still using it.
0f738bc
to
af6c040
Compare
I think we've stopped patching and cutting releases for 1.3. Cherrypicks for 1.4 or 1.5 are usually critical fixes. I understand this bug might be annoying, but kubelet also has other docker health checks. On top of that, on most platforms, the babysitter on your node will run |
I have checked the source code for v1.3.3. There is a timeout for rlist operation. I think if this pr is ok, maybe we could accept it first. As for backport to 1.3, this is just my thought. If we do not do it in the main repo, i can do this for our own.
We use systemd to supervisor docker instead of supervisord. For systemd, it is not so easy to make a babysister.:( |
/lgtm
For cherrypicking, you'd need a unit test to convince the branch manager that this PR actually fixes an issue. If you can add a test, we can at least try to cherrypick this to 1.6 (and 1.5).
It's unlikely we'll do this, but I don't have time to verify whether this is possible. You can try asking in the dev mailing list and/or release slack channels, but I'd suggest you patch manually.
On COS (the image we used for GKE), we started a separate systemd unit to monitor docker, just FYI: https://github.com/kubernetes/kubernetes/blob/v1.7.0-alpha.0/cluster/gce/gci/node.yaml#L43 |
Please add a release note. |
Release note: |
@yujuhong Done. |
@andyxning you need to update your original comment with the "```release-note" block. I edited that for you. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyxning, yujuhong
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@yujuhong Many thanks. Can we remove the |
/ping @yujuhong |
@andyxning: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue |
@sjenning -- can you pick this to our origin 1.6 release? |
@derekwaynecarr yes, i'll do it |
👍 |
This PR fix pleg reslist time. According to current implementation, we have a
Healthy
method periodically check the relist time. If current timestamp subtracts latest relist time is longer thanrelistThreshold
(default is 3 minutes), we should return an error to indicate the error of runtime.relist
method is also called periodically. If runtime(docker) hung, the relist method should return immediately without updating the latest relist time. If we update latest relist time no matter runtime(docker) hung(default timeout is 2 minutes), theHealthy
method will never return an error./cc @yujuhong @Random-Liu @dchen1107