Merge cockroachdb#156088

156088: kvnemesis: address fault mode todos r=stevendanna a=miraradeva

This commit addresses TODOs left after introducing network partitions and node restarts to kvnemesis:

- All fault mode tests run with 4 nodes to ensure that liveness tests can always maintain a majority quorum of replicas on nodes 1 and 2. If the tests run with 5 nodes, we need to take extra care to ensure 3 replicas are healthy for all RF=5 ranges (e.g. the systems ranges).

- Splits are now allowed in both partition and restart liveness modes. Previously, if r3 was unavailable, the range ID allocator would get stuck retrying the increment operation that generates a new range ID. This commit ensures all systems ranges are available in liveness mode.

- Lease transafers are now allowed in partition liveness mode. It's not clear what changed but the failure doesn't repro anymore. Will investigate more if it fails in CI.

Part of: cockroachdb#64828
Part of: cockroachdb#114814

Release note: None

Co-authored-by: Mira Radeva <[email protected]>

Author: craig[bot]

pkg/kv/kvnemesis/kvnemesis_test.go

@@ -470,7 +470,7 @@ func TestKVNemesisMultiNode_Partition_Safety(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	testKVNemesisImpl(t, kvnemesisTestCfg{
-		numNodes:                     5,
+		numNodes:                     4,
 		numSteps:                     defaultNumSteps,
 		concurrency:                  5,
 		seedOverride:                 0,
@@ -481,16 +481,6 @@ func TestKVNemesisMultiNode_Partition_Safety(t *testing.T) {
 		testGeneratorConfig: func(cfg *GeneratorConfig) {
 			cfg.Ops.Fault.AddNetworkPartition = 1
 			cfg.Ops.Fault.RemoveNetworkPartition = 1
-			// TODO(mira): DeleteRangeUsingTombstone and AddSSTable are always
-			// non-transactional, and as such are susceptible to double-application.
-			// The cluster setting kvcoord.NonTransactionalWritesNotIdempotent is
-			// enabled for this test to protect against double-application, but these
-			// requests don't propagate the flag AmbiguousReplayProtection to ensure
-			// the second application fails. We should fix this.
-			cfg.Ops.DB.DeleteRangeUsingTombstone = 0
-			cfg.Ops.DB.AddSSTable = 0
-			// The same issue above occurs for non-transactional DeleteRange requests.
-			cfg.Ops.DB.DeleteRange = 0
 		},
 	})
 }
@@ -500,7 +490,7 @@ func TestKVNemesisMultiNode_Partition_Liveness(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	testKVNemesisImpl(t, kvnemesisTestCfg{
-		numNodes:                     5,
+		numNodes:                     4,
 		numSteps:                     defaultNumSteps,
 		concurrency:                  5,
 		seedOverride:                 0,
@@ -516,20 +506,9 @@ func TestKVNemesisMultiNode_Partition_Liveness(t *testing.T) {
 			// constraints (at least one replica on nodes 1 and 2).
 			cfg.Ops.ChangeReplicas = ChangeReplicasConfig{}
 			// Epoch leases can experience indefinite unavailability in the case of a
-			// leader-leaseholder split and a network partition, so only leader leases
-			// are allowed.
+			// leader-leaseholder split and a network partition. This test starts off
+			// with leader leases, and lease type changes are disallowed.
 			cfg.Ops.ChangeSetting = ChangeSettingConfig{}
-			// TODO(mira): Transfers can result in RUEs because in the intermediate
-			// expiration-lease state, a request can get stuck holding latches until
-			// the replica circuit breaker trips and poisons the latches. This results
-			// in RUEs returned to the client. The behavior is expected; we can enable
-			// this setting if we allow the test to tolerate these RUEs.
-			cfg.Ops.ChangeLease = ChangeLeaseConfig{}
-			// TODO(mira): We should investigate splits more. So far I've seen then
-			// fail for two reasons: (1) r1 can become uvavailable (we can fix this by
-			// setting the right zone configs), and (2) if a partition races with the
-			// split, the range ID allocator can get stuck waiting for a response.
-			cfg.Ops.Split = SplitConfig{}
 		},
 	})
 }
@@ -559,9 +538,6 @@ func TestKVNemesisMultiNode_Restart_Liveness(t *testing.T) {
 			// Disallow replica changes because they interfere with the zone config
 			// constraints (at least one replica on nodes 1 and 2).
 			cfg.Ops.ChangeReplicas = ChangeReplicasConfig{}
-			// TODO(mira): Similar issue to Partition_Liveness, except the failure
-			// mode (2) here looks like "could not allocate ID; system is draining".
-			cfg.Ops.Split = SplitConfig{}
 		},
 	})
 }
@@ -817,22 +793,17 @@ func setAndVerifyZoneConfigs(
 			voter_constraints = '{"+node=n1": 1, "+node=n2": 1}'`,
 	)
 
-	// Ensure the liveness and meta ranges are also constrained appropriately.
-	sqlRunner.Exec(
-		t, `ALTER RANGE meta CONFIGURE ZONE USING
-			num_replicas = 3,
-			num_voters = 3,
-			constraints = '{"+node=n1": 1, "+node=n2": 1}',
-			voter_constraints = '{"+node=n1": 1, "+node=n2": 1}'`,
-	)
-
-	sqlRunner.Exec(
-		t, `ALTER RANGE liveness CONFIGURE ZONE USING
-			num_replicas = 3,
-			num_voters = 3,
-			constraints = '{"+node=n1": 1, "+node=n2": 1}',
-			voter_constraints = '{"+node=n1": 1, "+node=n2": 1}'`,
-	)
+	// Ensure the liveness, meta and system ranges are also constrained.
+	systemRanges := []string{"meta", "liveness", "system"}
+	for _, r := range systemRanges {
+		sqlRunner.Exec(
+			t, fmt.Sprintf(`ALTER RANGE %s CONFIGURE ZONE USING
+				num_replicas = 3,
+				num_voters = 3,
+				constraints = '{"+node=n1": 1, "+node=n2": 1}',
+				voter_constraints = '{"+node=n1": 1, "+node=n2": 1}'`, r),
+		)
+	}
 
 	// Wait for zone configs to propagate to all span config subscribers.
 	require.NoError(t, tc.WaitForZoneConfigPropagation())
@@ -853,7 +824,7 @@ func setAndVerifyZoneConfigs(
 							Key:    desc.StartKey.AsRawKey(),
 							EndKey: desc.EndKey.AsRawKey(),
 						}
-						if replicaSpan.Overlaps(dataSpan) || desc.RangeID <= 2 {
+						if replicaSpan.Overlaps(dataSpan) || desc.RangeID <= 3 {
 							overlappingReplicas = append(overlappingReplicas, replica)
 						}
 						return true // continue

pkg/kv/kvnemesis/validator.go

@@ -489,11 +489,13 @@ func (v *validator) processOp(op Operation) {
 			break
 		}
 		readObservation := &observedRead{Key: t.Key}
+		var shouldObserveRead bool
 		writeObservation := &observedWrite{
 			Key:   t.Key,
 			Seq:   t.Seq,
 			Value: roachpb.MakeValueFromString(t.Value()),
 		}
+		var shouldObserveWrite bool
 		// Consider two cases based on whether the CPut hit a ConditionFailedError.
 		err := errorFromResult(t.Result)
 		if e := (*kvpb.ConditionFailedError)(nil); errors.As(err, &e) {
@@ -504,7 +506,7 @@ func (v *validator) processOp(op Operation) {
 				observedVal.RawBytes = e.ActualValue.RawBytes
 			}
 			readObservation.Value = observedVal
-			v.curObservations = append(v.curObservations, readObservation)
+			shouldObserveRead = true
 		} else {
 			// If the CPut succeeded, the expected value is observed, and the CPut's
 			// write is also observed.
@@ -517,11 +519,21 @@ func (v *validator) processOp(op Operation) {
 					observedVal = roachpb.MakeValueFromBytes(t.ExpVal)
 				}
 				readObservation.Value = observedVal
-				v.curObservations = append(v.curObservations, readObservation)
+				shouldObserveRead = true
 			}
 			if sv, ok := v.tryConsumeWrite(t.Key, t.Seq); ok {
 				writeObservation.Timestamp = sv.Timestamp
 			}
+			shouldObserveWrite = true
+		}
+		// The read observation should be added before the write observation, since
+		// that's the order in which the CPut executed. Moreover, the CPut read is
+		// always non-locking, so if the observation filter is observeLocking, we
+		// won't be adding it.
+		if shouldObserveRead && v.observationFilter != observeLocking {
+			v.curObservations = append(v.curObservations, readObservation)
+		}
+		if shouldObserveWrite {
 			v.curObservations = append(v.curObservations, writeObservation)
 		}
 		if v.buffering == bufferingSingle {

pkg/kv/kvserver/idalloc/BUILD.bazel

@@ -8,6 +8,7 @@ go_library(
     deps = [
         "//pkg/base",
         "//pkg/kv",
+        "//pkg/kv/kvpb",
         "//pkg/roachpb",
         "//pkg/util/log",
         "//pkg/util/retry",
@@ -28,6 +29,7 @@ go_test(
     deps = [
         "//pkg/keys",
         "//pkg/kv/kvclient/kvcoord",
+        "//pkg/kv/kvpb",
         "//pkg/roachpb",
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",

pkg/kv/kvserver/idalloc/id_alloc.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/kv"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -84,7 +85,7 @@ func (ia *Allocator) Allocate(ctx context.Context) (int64, error) {
 	case id := <-ia.ids:
 		// when the channel is closed, the zero value is returned.
 		if id == 0 {
-			return id, errors.Errorf("could not allocate ID; system is draining")
+			return id, &kvpb.NodeUnavailableError{}
 		}
 		return id, nil
 	case <-ctx.Done():

pkg/kv/kvserver/idalloc/id_alloc_test.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/idalloc"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	// Import to set ZoneConfigHook.
@@ -232,9 +233,9 @@ func TestAllocateWithStopper(t *testing.T) {
 	s, idAlloc := newTestAllocator(t)
 	s.Stop() // not deferred.
 
-	if _, err := idAlloc.Allocate(context.Background()); !testutils.IsError(err, "system is draining") {
-		t.Errorf("unexpected error: %+v", err)
-	}
+	_, err := idAlloc.Allocate(context.Background())
+	require.Error(t, err)
+	require.ErrorIs(t, err, &kvpb.NodeUnavailableError{})
 }
 
 // TestLostWriteAssertion makes sure that the Allocator performs a best-effort