-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 timeout flooding issue after containerd restart #2749
Conversation
Hi @hanlins. Thanks for your PR. I'm waiting for a google 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. |
The fix has been manually verified. You can use this branch to build a binary for verification. |
Cc @bobbypage |
/ok-to-test |
Thanks for the fix and the investigation here! I'm curious, it looks this dialer was used for a long time now in cAdvisor. The last change was 4cf2f01#diff-ad4070b5c5ba5108a8790984457dd485f6363be34312edf452967359bb1a195dR75 which changed from using from Looking at containerd dialer (https://github.com/containerd/containerd/blob/master/pkg/dialer/dialer.go), Also interesting -- the official containerd client is using Just trying to understand the issue, is #2748 tied to a specific cAdvisor / k8s release? I'm curious if you think this might be a problem with containerd's |
Hi @bobbypage, I think the issue becomes obvious when k8s directly talk to containerd instead of docker shim (I tried to just restart docker and there's no such issue, here's my findings). Also there's CVE for containerd recently so people have to restart containerd, so more people are experiencing the issue just recently. The issue was reported for
No, it seems I can reproduce it on multiple k8s versions.
I'm not confident on that, I'm leaning on there's a context misuse in I would say it's not realistic to change the containerd client from singleton to per-connection based, and there might not be a convincing reason to make the change in |
Thanks @hanlins for all your hard work on this! For context, we chatted offline on slack to debug this further. Definitely an fun and interesting bug, requiring some deep diving into grpc backoff logic :) @hanlins can you please update the PR description with the new findings we discussed offline and the updated fix? If you can squash the outdated commits, it would be great as well. Thanks again for all your efforts on hunting down this issue! |
Originally multipilier is default to 0, which will cause flodding if backoff happens. Now setting multiplier to 1.6 which is the default multiplier value in grpc. Signed-off-by: Hanlin Shi <shihanlin9@gmail.com>
4f4a378
to
d231f6c
Compare
@bobbypage thanks! It's great that we root caused this "gotcha", really worthy to share it with other folks, thanks for your helps! |
I've cut new cAdvisor releases ( We should get this fix back into kubernetes. We'll need 3 PRs to k/k with following updates:
|
Originally the gRPC backoff multiplier is not set so it got defaulted as zero. Once a connection timeout happens, gRPC exponential backoff will be triggered which results in setting the backoff durating to zero, causing retries flooding. Setting gRPC multiplier to 1.6 which is the default one from gRPC package will solve the issue.
Closes #2748