Skip to content
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

clientv3: close streams after use in lessor keepAliveOnce method #14357

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

amdprophet
Copy link

We use client.KeepAliveOnce heavily in Sensu Go. Since upgrading to Etcd 3.5 we've seen an excessive amount of logs containing failed to receive lease keepalive request from gRPC stream when logging at debug level. I believe this is happening as there's no attempt to close the stream after the keepAliveOnce method has completed. The stream is instead closed by the deferred call to cancel the context.

The log lines no longer appear after attempting to close the stream using the code in this PR.

@ahrtr
Copy link
Member

ahrtr commented Aug 18, 2022

Thanks @amdprophet for the PR, but please submit a PR for main firstly, and cherry pick back to 3.5 afterwards.

Streams are now closed after being used in the lessor `keepAliveOnce` method.
This prevents the "failed to receive lease keepalive request from gRPC stream"
message from being logged by the server after the context is cancelled by the
client.

Signed-off-by: Justin Kolberg <amd.prophet@gmail.com>
@amdprophet amdprophet changed the base branch from release-3.5 to main August 18, 2022 01:02
@amdprophet
Copy link
Author

@ahrtr updated

@codecov-commenter
Copy link

Codecov Report

Merging #14357 (295044f) into main (ba0c7c3) will decrease coverage by 0.26%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##             main   #14357      +/-   ##
==========================================
- Coverage   75.40%   75.14%   -0.27%     
==========================================
  Files         457      457              
  Lines       37139    37144       +5     
==========================================
- Hits        28006    27913      -93     
- Misses       7376     7461      +85     
- Partials     1757     1770      +13     
Flag Coverage Δ
all 75.14% <42.85%> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/v3/lease.go 90.74% <42.85%> (-1.29%) ⬇️
server/etcdserver/api/rafthttp/peer_status.go 87.87% <0.00%> (-12.13%) ⬇️
server/proxy/grpcproxy/register.go 69.76% <0.00%> (-9.31%) ⬇️
server/auth/simple_token.go 80.00% <0.00%> (-8.47%) ⬇️
server/etcdserver/api/rafthttp/peer.go 87.01% <0.00%> (-8.45%) ⬇️
client/pkg/v3/fileutil/lock_linux.go 72.22% <0.00%> (-8.34%) ⬇️
client/pkg/v3/tlsutil/tlsutil.go 83.33% <0.00%> (-8.34%) ⬇️
client/v3/leasing/util.go 91.66% <0.00%> (-6.67%) ⬇️
client/v3/concurrency/mutex.go 61.64% <0.00%> (-5.48%) ⬇️
server/lease/lease.go 94.87% <0.00%> (-5.13%) ⬇️
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you @amdprophet

@ahrtr
Copy link
Member

ahrtr commented Aug 18, 2022

cc @spzala

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
Thanks @amdprophet

@spzala spzala merged commit 12d63d1 into etcd-io:main Aug 18, 2022
@amdprophet amdprophet deleted the close-keepalive-stream branch August 18, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants