lease: Fix incorrect gRPC Unavailable on client cancel during LeaseKeepAlive forwarding #21122

zhijun42 · 2026-01-13T09:42:06Z

Follow-up to the previous reproduction PR #21050. Refer there for full context.

k8s-ci-robot · 2026-01-13T09:42:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhijun42
Once this PR has been reviewed and has the lgtm label, please assign spzala for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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 · 2026-01-13T09:42:17Z

Hi @zhijun42. Thanks for your PR.

I'm waiting for a etcd-io 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.

Details

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-sigs/prow repository.

serathius · 2026-01-13T10:46:31Z

/ok-to-test

serathius · 2026-01-13T10:48:07Z

Have you read the original motivation for returning NoLeader? #7630 #7275

Could we add a test for WithRequireLeader?

Maybe we could ask @heyitsanthony @xiang90

codecov · 2026-01-13T11:19:02Z

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.39%. Comparing base (c492848) to head (d899fa7).

Files with missing lines	Patch %	Lines
server/etcdserver/v3_server.go	81.81%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdserver/api/v3rpc/lease.go	`76.62% <ø> (-5.66%)`	⬇️
server/etcdserver/api/v3rpc/util.go	`67.74% <ø> (ø)`
server/lease/leasehttp/http.go	`62.16% <100.00%> (ø)`
server/etcdserver/v3_server.go	`75.66% <81.81%> (+0.12%)`	⬆️

... and 21 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #21122      +/-   ##
==========================================
- Coverage   68.40%   68.39%   -0.02%     
==========================================
  Files         429      429              
  Lines       35242    35248       +6     
==========================================
  Hits        24109    24109              
- Misses       9737     9746       +9     
+ Partials     1396     1393       -3

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c492848...d899fa7. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zhijun42 · 2026-01-14T07:40:24Z

@serathius I think this is the story: In the original PR, when the server stream is canceled due to no leader, the author converts the error code from Canceled to NoLeader because he incorrectly assumed that the error code from stream is Canceled, which is in fact not true - it's already NoLeader.

We can prove this by commenting out these lines on main branch and will see that relevant tests still pass.

func (ls *LeaseServer) LeaseKeepAlive(stream pb.Lease_LeaseKeepAliveServer) (err error) {
	errc := make(chan error, 1)
	go func() {
	    errc <- ls.leaseKeepAlive(stream)
	}()
	select {
	case err = <-errc:
	case <-stream.Context().Done():
        err = stream.Context().Err()
        //if errors.Is(err, context.Canceled) {
        //	err = rpctypes.ErrGRPCNoLeader
        //}
	}
	return err
}

As for WithRequireLeader, it's already covered by tests TestLeaseWithRequireLeader and TestV3LeaseRequireLeader in that original PR.

But yeah you make a good point ,so I do add a new test case almost identical to TestV3LeaseRequireLeader (except that I don't use WithRequireLeader) to showcase that we will always receive NoLeader error regardless of WithRequireLeader.

zhijun42 · 2026-01-14T09:37:18Z

On a separate yet related note, I find an opportunity for improvement. If a lease client sets this withRequireLeader and closes its receive channel due to no leader, it should also stop sending more keepAlive requests to the server (it wouldn't have a channel to receive response anyway). Opened a new PR #21126 for this.

serathius · 2026-01-14T09:50:25Z

tests/integration/v3_lease_test.go

 		}, 3*time.Second, 100*time.Millisecond)
 	})
+
+	// This is almost identical to TestV3LeaseRequireLeader, except that it


Instead of having a comment saying those tests are identical, could you just migrate TestV3LeaseRequireLeader to be a test case here? Having it as separate PR will help showing the current behavior.

Good idea! Created a separate PR #21127

Rebased. Now that my test function has 5 test cases and only 2 of them require gofail. I refactor a bit to move the make gofail-enable check from function level into test case level.

zhijun42 · 2026-01-15T06:33:28Z

Note about grpcproxy behavior: During testing, I found that the grpcproxy doesn't propagate NoLeader errors when WithRequireLeader is not used. The keepAliveLoop in server/proxy/grpcproxy/lease.go (lines 263-265) discards errors and only calls cancel(), resulting in context.Canceled instead of the actual server error.

Not sure if this is intentional design or an actual bug, but since this code has been stable since 2017, I decided not to touch it. Instead, I modified my test cases to handle such difference.

serathius · 2026-01-15T10:00:08Z

tests/integration/clientv3/watch/v3_watch_test.go

-	if len(gofail.List()) == 0 {
-		t.Skip("please run 'make gofail-enable' before running the test")
-	}
+	integration.GofailSkipCheck(t)


Could you move such refactor to separate PR?

Moved to PR #21136

serathius · 2026-01-15T10:02:26Z

server/etcdserver/api/v3rpc/lease.go

+		// We end up here due to:
+		// 1. Client cancellation
+		// 2. Server cancellation: the client ctx is wrapped with WithRequireLeader,
+		//		monitorLeader() detects no leader and thus cancels this stream with ErrGRPCNoLeader.
+		// 3. Server cancellation: the server is shutting down.


This is a useful comment. Most other are not.

Will keep raising my standard for comments going forward!

serathius · 2026-01-15T10:03:55Z

server/etcdserver/v3_server.go

+	// This is best-effort until the context times out. For now all
+	// error codes are considered retryable except ErrLeaseNotFound.


This comment is not useful.

Removed it and other bad comments.

tests/integration/v3_lease_test.go

Signed-off-by: Zhijun <[email protected]>

zhijun42 · 2026-01-16T01:50:32Z

Rebased. Great idea moving the SkipGoFail and additional test case into separate PRs!

k8s-ci-robot added area/testing needs-ok-to-test labels Jan 13, 2026

k8s-ci-robot added the size/M label Jan 13, 2026

zhijun42 force-pushed the fix-lease-keep-alive-unavailable branch from 430f664 to c1b8ad3 Compare January 13, 2026 09:45

zhijun42 mentioned this pull request Jan 13, 2026

lease: Reproduce incorrect gRPC Unavailable on client cancel during LeaseKeepAlive forwarding #21050

Merged

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jan 13, 2026

k8s-ci-robot added size/L size/M and removed size/M size/L labels Jan 14, 2026

serathius reviewed Jan 14, 2026

View reviewed changes

zhijun42 mentioned this pull request Jan 14, 2026

lease: Migrate TestV3LeaseRequireLeader into TestV3LeaseKeepAliveForwardingCatchError #21127

Merged

k8s-ci-robot added the needs-rebase label Jan 15, 2026

zhijun42 force-pushed the fix-lease-keep-alive-unavailable branch from b74031b to 7ba114f Compare January 15, 2026 03:13

k8s-ci-robot added size/L and removed needs-rebase size/M labels Jan 15, 2026

serathius reviewed Jan 15, 2026

View reviewed changes

tests/integration/v3_lease_test.go Show resolved Hide resolved

This was referenced Jan 15, 2026

lease: Add new test to catch NoLeader error when client not using WithRequireLeader #21135

Merged

Consolidate gofail check in integration tests #21136

Merged

zhijun42 force-pushed the fix-lease-keep-alive-unavailable branch from 21dc2a9 to c9f280e Compare January 15, 2026 12:10

k8s-ci-robot added size/M and removed size/L labels Jan 15, 2026

Helps solve the leaseKeepAlive grpc Unavailable issue

d899fa7

Signed-off-by: Zhijun <[email protected]>

zhijun42 force-pushed the fix-lease-keep-alive-unavailable branch from c9f280e to d899fa7 Compare January 16, 2026 00:33

		// This is best-effort until the context times out. For now all
		// error codes are considered retryable except ErrLeaseNotFound.

lease: Fix incorrect gRPC Unavailable on client cancel during LeaseKeepAlive forwarding #21122

Are you sure you want to change the base?

lease: Fix incorrect gRPC Unavailable on client cancel during LeaseKeepAlive forwarding #21122

Conversation

zhijun42 commented Jan 13, 2026

Uh oh!

k8s-ci-robot commented Jan 13, 2026

Uh oh!

k8s-ci-robot commented Jan 13, 2026

Uh oh!

serathius commented Jan 13, 2026

Uh oh!

serathius commented Jan 13, 2026

Uh oh!

codecov bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zhijun42 commented Jan 14, 2026

Uh oh!

zhijun42 commented Jan 14, 2026

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!

zhijun42 commented Jan 15, 2026

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!

Uh oh!

zhijun42 commented Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 13, 2026 •

edited

Loading