Repository navigation
client: reconnect when the address is reused by another service - #11326
bufferflies wants to merge 4 commits into
Conversation
When a stale DNS record points to a recycled Pod IP that now belongs to another kind of service (e.g., a scheduling server), the client keeps reusing a READY gRPC connection to the wrong service and every request fails with "unknown service pdpb.PD" until the process restarts. Add a client interceptor that closes the connection once the remote endpoint reports that it does not provide the service expected by the connection, i.e., pdpb.PD, tsopb.TSO, routerpb.Router or resource_manager.ResourceManager. Other unknown services or methods are ignored to stay compatible with older servers and other deployment modes. Recreate the closed connections: GetOrCreateGRPCConn drops a cached connection in the Shutdown state, and the resource manager and router service discovery reconnect even if the service URLs are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Detect unknown-service responses client/pkg/utils/grpcutil/grpcutil.go, client/pkg/utils/grpcutil/expected_service_test.go |
Unary and stream interceptors close a connection when an error is Unimplemented with the exact message unknown service <service>. Cached connections in the Shutdown state are replaced. Tests cover matching and non-matching errors and cached connection replacement. |
Apply options and recreate discovery connections client/servicediscovery/expected_service.go, client/servicediscovery/service_discovery.go, client/servicediscovery/tso_service_discovery.go, client/servicediscovery/router_service_discovery.go, client/servicediscovery/resource_manager_service_discovery.go, client/servicediscovery/expected_service_test.go, tests/integrations/client/stale_address_test.go |
PD, TSO, Router, and ResourceManager discovery apply the corresponding expected-service options. ResourceManager and Router discovery recreate shut-down connections when discovery data is unchanged. Tests cover connection replacement, leader switching, and client recovery after a stale address redirects dials. |
Resolve rejected stream sends client/pkg/utils/grpcutil/grpcutil.go, client/clients/router/client.go, client/clients/router/expected_service_test.go, client/resource_manager_client.go, client/resource_manager_client_test.go |
When stream Send returns io.EOF, Router and ResourceManager request processing attempt Recv and use the resolved error. Tests cover rejected streams and error delivery. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Suggested labels: type/cherry-pick-for-release-8.5
Suggested reviewers: rleungx, lhy1024
Merge Risk: 
ec906
-
No actionable issue is established from the supplied evidence. The change is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main client reconnection change caused by an address being reused by another service. Description check The description covers the issue, issue reference, implementation, unit and integration tests, side effects, release note, and cherry-pick requirement. The omitted code-changes checklist is non-critic… Docstring Coverage Docstring coverage is inconclusive: files from earlier reviews still need to be rechecked within the check's analysis limits. Not blocking. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description covers the issue, issue reference, implementation, unit and integration tests, side effects, release note, and cherry-pick requirement. The omitted code-changes checklist is non-critical because the description identifies no configuration, API, or persistent-data changes.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11326 +/- ##
==========================================
+ Coverage 84.89% 84.94% +0.04%
==========================================
Files 554 555 +1
Lines 71688 71746 +58
==========================================
+ Hits 60862 60943 +81
+ Misses 10825 10802 -23
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| return false | ||
| } | ||
| // The message is generated by the gRPC server, see `grpc.Server.handleStream`. | ||
| return s.Message() == "unknown service "+service |
There was a problem hiding this comment.
This matches an internal gRPC error-message string rather than a stable contract, so a grpc-go version bump that rewords it would silently disable the reconnect. Prefer checking the grpc.UnknownServiceError type instead, and add a test pinning the current format so a dependency bump fails CI.
There was a problem hiding this comment.
Confirmed, a reworded message would silently disable the reconnect. I pinned the format in 5866c4268, but kept matching the message instead of checking a type, because grpc-go doesn't expose one: there is no grpc.UnknownServiceError in v1.82.1. The server writes status.New(codes.Unimplemented, "unknown service <S>") in handleStream, so the client only gets the code and the message. The code alone isn't enough either. An unknown method of a registered service also returns Unimplemented, and treating that as a wrong endpoint would drop valid connections to older servers that lack a newer RPC.
The prefix is now unknownServiceErrorPrefix, and the new TestUnknownServiceErrorFormat checks the exact message from a real gRPC server for both unary and stream calls, and that an unknown method doesn't match. When the prefix doesn't match the server's message, the test fails and says to update unknownServiceErrorPrefix. So a grpc-go bump that changes the wording fails CI instead of quietly turning off the reconnect.
isUnknownServiceError matches the message generated by the gRPC server, since gRPC exposes no typed error for an unregistered service. Move the prefix into unknownServiceErrorPrefix and add TestUnknownServiceErrorFormat to fail CI if a gRPC upgrade changes the format. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
| } | ||
|
|
||
| // RecvMsg implements grpc.ClientStream. | ||
| func (s *expectedServiceClientStream) RecvMsg(m any) error { |
There was a problem hiding this comment.
When a pre-opened Router QueryRegion stream is rejected for a missing service, the next Send can return io.EOF; processRequestsInner returns on that error without calling Recv, so this RecvMsg hook never observes the Unimplemented status and the underlying connection remains READY. With unchanged registry URLs, discovery skips reconnecting, and the background connection daemon can keep pre-opening rejected streams on the same cached connection after DNS recovers, causing repeated failures for sparse Router requests.
There was a problem hiding this comment.
Good catch, fixed in ec906d57f: processRequestsInner and the resource manager's processTokenRequests now pass a send error through the new grpcutil.ResolveStreamSendError, which calls Recv once when Send returns io.EOF and returns the stream status instead. That Recv goes through the expected-service stream wrapper, so an unknown service rejection closes the connection, and the discoveries recreate it as for the unary path. Other send errors are returned unchanged. The token path also now routes the resolved error through gRPCErrorHandler / resourceManagerErrorHandler, consistent with the Recv failure path. Added TestProcessRequestsClosesConnOnRejectedStream and TestProcessTokenRequestsClosesConnOnRejectedStream, which pre-open a stream to a server without the service and wait for Send to fail. Both fail without the fix: they get io.EOF and the connection stays open. TSO is unaffected since its recvLoop already receives the status.
Signed-off-by: bufferflies <1045931706@qq.com>
A pre-opened Router QueryRegion stream or resource manager token stream rejected for an unknown service only returns io.EOF on Send, the status is reported on Recv. Receive the status on io.EOF so the expected service check closes the connection to be recreated, and fix the modernize lint in the tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
|
@bufferflies: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
What problem does this PR solve?
Issue Number: ref #9894
When a stale DNS record points to a recycled Pod IP that now belongs to another kind of service (e.g., a scheduling server reusing an old PD Pod's IP), the client keeps reusing the READY gRPC connection to the wrong service. Every request fails with
rpc error: code = Unimplemented desc = unknown service pdpb.PDuntil TiDB restarts, even after the DNS record is refreshed. See #9894 (comment).The callee ID check added in #10600 cannot catch this case, because the request is rejected by gRPC routing before reaching any handler.
What is changed and how does it work?
Check List
Tests
Side effects
Related changes
Release note
🤖 Generated with Claude Code
Summary by CodeRabbit