kvserver: TestReplicateRestartAfterTruncationWithRemoveAndReAdd is flaky

[mgartner]

I get a failure on master @ 24bd59b in ~5-20 iterations:

./dev test pkg/kv/kvserver -f TestReplicateRestartAfterTruncationWithRemoveAndReAdd --stress

I haven't been able to reproduce on release-25.4 nor release-25.3.

Jira issue: CRDB-56116

[pav-kv]

    client_raft_test.go:2373: could not add [n2,s2] replicas to range containing /System/"aaa-testing": AdminChangeReplicas error: change replicas of r80 failed: range r80:/System{\x00aaa-testing-/NodeLiveness} [(n1,s1):1, (n3,s3):3, (n2,s2):4LEARNER, next=5, gen=9, sticky=9223372036.854775807,2147483647] cannot make progress with proposed changes add=[(n2,s2):4LEARNER] del=[] based on live replicas [(n1,s1):1]
        (1) AdminChangeReplicas error: change replicas of r80 failed: range r80:/System{\x00aaa-testing-/NodeLiveness} [(n1,s1):1, (n3,s3):3, (n2,s2):4LEARNER, next=5, gen=9, sticky=9223372036.854775807,2147483647] cannot make progress with proposed changes add=[(n2,s2):4LEARNER] del=[] based on live replicas [(n1,s1):1]
          | -- cause hidden behind barrier
          | AdminChangeReplicas error: change replicas of r80 failed: range r80:/System{\x00aaa-testing-/NodeLiveness} [(n1,s1):1, (n3,s3):3, (n2,s2):4LEARNER, next=5, gen=9, sticky=9223372036.854775807,2147483647] cannot make progress with proposed changes add=[(n2,s2):4LEARNER] del=[] based on live replicas [(n1,s1):1]
          | (1) attached stack trace
          |   -- stack trace:
          |   | github.com/cockroachdb/cockroach/pkg/testutils/testcluster.(*TestCluster).changeReplicas
          |   |         pkg/testutils/testcluster/testcluster.go:902
          |   | github.com/cockroachdb/cockroach/pkg/testutils/testcluster.(*TestCluster).addReplica
          |   |         pkg/testutils/testcluster/testcluster.go:912
          |   | github.com/cockroachdb/cockroach/pkg/testutils/testcluster.(*TestCluster).AddVoters
          |   |         pkg/testutils/testcluster/testcluster.go:930
          |   | github.com/cockroachdb/cockroach/pkg/testutils/testcluster.(*TestCluster).AddVotersOrFatal
          |   |         pkg/testutils/testcluster/testcluster.go:1054
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver_test.runReplicateRestartAfterTruncation
          |   |         pkg/kv/kvserver/client_raft_test.go:2373
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver_test.TestReplicateRestartAfterTruncationWithRemoveAndReAdd
          |   |         pkg/kv/kvserver/client_raft_test.go:2269
          |   | testing.tRunner
          |   |         GOROOT/src/testing/testing.go:1934
          |   | runtime.goexit
          |   |         src/runtime/asm_arm64.s:1268
          | Wraps: (2) AdminChangeReplicas error
          | Wraps: (3)
          |   | (opaque error wrapper)
          |   | type name: github.com/cockroachdb/errors/withstack/*withstack.withStack
          |   | reportable 0:
          |   |
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.execChangeReplicasTxn
          |   |         pkg/kv/kvserver/replica_command.go:2645
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).initializeRaftLearners
          |   |         pkg/kv/kvserver/replica_command.go:1962
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).changeReplicasImpl
          |   |         pkg/kv/kvserver/replica_command.go:1277
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).ChangeReplicas
          |   |         pkg/kv/kvserver/replica_command.go:1204
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).executeAdminBatch
          |   |         pkg/kv/kvserver/replica_send.go:972
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Replica).SendWithWriteBytes
          |   |         pkg/kv/kvserver/replica_send.go:192
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Store).SendWithWriteBytes
          |   |         pkg/kv/kvserver/store_send.go:188
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvserver.(*Stores).SendWithWriteBytes
          |   |         pkg/kv/kvserver/stores.go:199
          |   | github.com/cockroachdb/cockroach/pkg/server.(*Node).batchInternal
          |   |         node.go:1667
          |   | github.com/cockroachdb/cockroach/pkg/server.(*Node).Batch
          |   |         node.go:1860
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func1
          |   |         pkg/rpc/context.go:761
          |   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.ServerInterceptor.func12
          |   |         pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:60
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.chainUnaryServerInterceptors.bindUnaryServerInterceptorToHandler.func5
          |   |         pkg/rpc/context.go:872
          |   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func3
          |   |         pkg/rpc/context.go:181
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.chainUnaryServerInterceptors.bindUnaryServerInterceptorToHandler.func5
          |   |         pkg/rpc/context.go:872
          |   | github.com/cockroachdb/cockroach/pkg/rpc.kvAuth.unaryInterceptor
          |   |         pkg/rpc/auth.go:97
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.chainUnaryServerInterceptors.bindUnaryServerInterceptorToHandler.func5
          |   |         pkg/rpc/context.go:872
          |   | github.com/cockroachdb/cockroach/pkg/rpc.gatewayRequestRecoveryInterceptor
          |   |         pkg/rpc/metrics.go:514
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.chainUnaryServerInterceptors.bindUnaryServerInterceptorToHandler.func5
          |   |         pkg/rpc/context.go:872
          |   | github.com/cockroachdb/cockroach/pkg/server.newGRPCServer.NewRequestMetricsInterceptor.func4
          |   |         pkg/rpc/metrics.go:467
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.chainUnaryServerInterceptors.bindUnaryServerInterceptorToHandler.func5
          |   |         pkg/rpc/context.go:872
          |   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1.1
          |   |         pkg/rpc/context.go:138
          |   | github.com/cockroachdb/cockroach/pkg/util/stop.(*Stopper).RunTaskWithErr
          |   |         pkg/util/stop/stopper.go:351
          |   | github.com/cockroachdb/cockroach/pkg/rpc.NewServerEx.func1
          |   |         pkg/rpc/context.go:136
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.chainUnaryServerInterceptors.bindUnaryServerInterceptorToHandler.func5
          |   |         pkg/rpc/context.go:872
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func2
          |   |         pkg/rpc/context.go:771
          |   | github.com/cockroachdb/cockroach/pkg/rpc.NewContext.ClientInterceptor.func9
          |   |         pkg/util/tracing/grpcinterceptor/grpc_interceptor.go:185
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.getChainUnaryInvoker.func4
          |   |         pkg/rpc/context.go:956
          |   | github.com/cockroachdb/cockroach/pkg/rpc.makeInternalClientAdapter.func3
          |   |         pkg/rpc/context.go:841
          |   | github.com/cockroachdb/cockroach/pkg/rpc.internalClientAdapter.Batch
          |   |         pkg/rpc/context.go:964
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*grpcTransport).sendBatch
          |   |         pkg/kv/kvclient/kvcoord/transport.go:211
          |   | github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord.(*grpcTransport).SendNext
          |   |         pkg/kv/kvclient/kvcoord/transport.go:189
          | Wraps: (4) change replicas of r80 failed
          | Wraps: (5) range r80:/System{\x00aaa-testing-/NodeLiveness} [(n1,s1):1, (n3,s3):3, (n2,s2):4LEARNER, next=5, gen=9, sticky=9223372036.854775807,2147483647] cannot make progress with proposed changes add=[(n2,s2):4LEARNER] del=[] based on live replicas [(n1,s1):1]
          |   |
          |   | (opaque error leaf)
          |   | type name: github.com/cockroachdb/cockroach/pkg/kv/kvserver/*kvserver.quorumError
          | Error types: (1) *withstack.withStack (2) *errutil.withPrefix (3) *errbase.opaqueWrapper (4) *errutil.withPrefix (5) *errbase.opaqueLeaf
        Error types: (1) *barriers.barrierErr
    panic.go:615: -- test log scope end --

@mgartner Does this match the failure you were seeing?

[pav-kv]

So the test wants to add n2 as a voter, and one of the first steps there (which fails) is adding it as a learner.

The range had 2 voters: [(n1,s1):1, (n3,s3):3, (n2,s2):4LEARNER, next=5, gen=9, sticky=9223372036.854775807,2147483647]

And for some reason, we see only n1 being alive here (concluding that there is no quorum alive):

cockroach/pkg/kv/kvserver/replica_command.go

Lines 2559 to 2573 in 91b1736

	liveReplicas, _ := args.liveAndDeadReplicas(replicas.Descriptors(),
	true /* includeSuspectAndDrainingStores */)
	if !replicas.CanMakeProgress(
	func(rDesc roachpb.ReplicaDescriptor) bool {
	for _, inner := range liveReplicas {
	if inner.ReplicaID == rDesc.ReplicaID {
	return true
	}
	}
	return false
	}) {
	// NB: we use newQuorumError which is recognized by the replicate queue.
	return newQuorumError("range %s cannot make progress with proposed changes add=%v del=%v "+
	"based on live replicas %v", crt.Desc, crt.Added(), crt.Removed(), liveReplicas)
	}

live replicas [(n1,s1):1]

[pav-kv]

The test stops and restarts n2, but not other nodes (notably, it doesn't touch n3, so quorum shouldn't be lost).

cockroach/pkg/kv/kvserver/client_raft_test.go

Lines 2352 to 2357 in 91b1736

	// Re-add the store and restart it.
	// TODO(dt): ben originally suggested we also attempt this in the other order.
	// This currently hits an NPE in mtc.replicateRange though when it tries to
	// read the Ident.NodeID field in the specified store, and will become
	// impossible after streaming snapshots.
	require.NoError(t, tc.RestartServer(1))

[pav-kv]

With enhanced logging, we see that both n2 and n3 are considered dead:

cannot make progress with proposed changes add=[(n2,s2):4LEARNER] del=[] based on live replicas [(n1,s1):1], dead [(n3,s3):3 (n2,s2):4LEARNER]

[mgartner]

@mgartner Does this match the failure you were seeing?

yes, that looks like it.

[pav-kv]

Bisect points at e00f221

CC @tbg does this flake look like a possible effect of this change? Looks like: we are now propagating an error higher up, which previously was masked?

But the flake is in how the allocator sees liveness. Sometimes it's too slow to realize that n2/n3 are alive. So perhaps it's a test-only issue, could be fixed by waiting for the liveness to stabilize, or wrapping the config change into a SucceedsSoon.

[tbg]

That definitely looks like it. I'm surprised it takes >10s for nodes to be marked as live here, that's weird, isn't it? Does this only happen when stressing? Probably nodes are actually losing liveness then.

We have an allowUnsafeReplicationChanges knob that we can set in cases like this. But I worry this will happen in more tests. Maybe the right solution is to not stress tests that carry out replication changes on a real cluster? Or maybe this knob should be "default on" in tests.