rpc: nodes fail to connect to peer even after the peer is up

[andreimatei]

I believe I've run into the following race:

• node 1 goes down
• some node 2 code path tries to dial node 1. Let's call it dial attempt 1.
• node 1 comes back, opens its ports
• another code path on node 2 tries to dial. This is dial attempt 2. Dial attempt 1 is still in progress, so dial attempt two blocks on conn.initOnce
• dial attempt one was in the process of failing, and eventually releases that lock
• every subsequent attempt (i.e. dial attempt 2) that was convoyed behind attempt 1 fails with the same message

This is quite unfortunate, particularly in situations where dial attempt 2 is causally related to node 1 coming back. For example, by node 1 having sent a SetupFlow RPC, which asks node 2 to connect back to it. Node 2 failing to connect back is very unfortunate for the respective SQL query, which will wait in vain for a long time.

A suggested fix is by having node 2 consider network availability signals (incoming connections, or successful heartbeats) and making sure that no error from a dial attempt from before a signal is propagated to any dial attempt from after the signal.

cc @ajwerner @bdarnell

Epic: CRDB-8500

Jira issue: CRDB-5255

[ajwerner]

This is a cool issue and would be good to fix at some point. I'm taking it out of my queue, as I haven't thought about it in years.

[tbg]

We could add code here

cockroach/pkg/server/server.go

Lines 251 to 261 in 2dc2da8

	OnIncomingPing: func(ctx context.Context, req *rpc.PingRequest) error {
	// Decommission state is only tracked for the system tenant.
	if tenantID, isTenant := roachpb.TenantFromContext(ctx); isTenant &&
	!roachpb.IsSystemTenantID(tenantID.ToUint64()) {
	return nil
	}
	// Incoming ping will reject requests with codes.PermissionDenied to
	// signal remote node that it is not considered valid anymore and
	// operations should fail immediately.
	return checkPingFor(ctx, req.OriginNodeID, codes.PermissionDenied)
	},

that resets the breaker for the originator of the PingRequest:

diff --git a/pkg/server/server.go b/pkg/server/server.go
index 3f24182238..3ed5616a67 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -233,6 +233,13 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 
 	nodeTombStorage, checkPingFor := getPingCheckDecommissionFn(engines)
 
+	// This will be assigned once we've instantiated the nodeDialer.
+	//
+	// NB: not a fan of this pattern but alternative is a larger
+	// rearrangement. Would need to double-check that this won't
+	// be invoked before it's populated.
+	var resetNodeDialerBreakerTo func(nodeID roachpb.NodeID)
+
 	rpcCtxOpts := rpc.ContextOptions{
 		TenantID:         roachpb.SystemTenantID,
 		NodeID:           cfg.IDContainer,
@@ -257,7 +264,10 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 			// Incoming ping will reject requests with codes.PermissionDenied to
 			// signal remote node that it is not considered valid anymore and
 			// operations should fail immediately.
-			return checkPingFor(ctx, req.OriginNodeID, codes.PermissionDenied)
+			if err := checkPingFor(ctx, req.OriginNodeID, codes.PermissionDenied); err != nil {
+				return err
+			}
+			resetNodeDialerBreakerTo(req.OriginNodeID)
 		},
 	}
 	if knobs := cfg.TestingKnobs.Server; knobs != nil {
@@ -322,6 +332,11 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 
 	nodeDialer := nodedialer.NewWithOpt(rpcContext, gossip.AddressResolver(g),
 		nodedialer.DialerOpt{TestingKnobs: dialerKnobs})
+	resetNodeDialerBreakerTo = func(nodeID roachpb.NodeID) {
+		for class := rpc.ConnectionClass(0); int(class) < rpc.NumConnectionClasses; class++ {
+			nodeDialer.GetCircuitBreaker(nodeID, class).Reset()
+		}
+	}
 
 	runtimeSampler := status.NewRuntimeStatSampler(ctx, clock)
 	registry.AddMetricStruct(runtimeSampler)

This would deal with the "causally related" situations. For example, #87104 (comment) would now read:

• n4 restarts
• n1 has breaker to n4 tripped as a result
• n4 issues SetupFlow to n1
  • in particular, establishes healthy connection to n1
  • which includes a heartbeat n4->n1
  • which resets the breaker n1->n4
• n1 checks breaker, which passes
• n1 sets up the flow to n4

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!