Merge cockroachdb#107075 cockroachdb#107967 cockroachdb#107984

craig[bot] · wenyihu6 · kvoli · craig[bot] · commit 46255c89775a · 2023-08-02T20:22:37.000Z
107075: asim: add random predefined cluster config selection r=kvoli a=wenyihu6 This patch takes the first step towards a randomized framework by enabling asim testing to randomly select a cluster information configuration from a set of predefined choices. These choices are hardcoded and represent common cluster configurations. TestRandomized now takes in `true` for randOptions.cluster to enable random cluster config selection. This provides two modes for cluster generation: 1. Default: currently set to 3 nodes and 1 store per node 2. Random: randomly select a predefined cluster info Part of: cockroachdb#106311 Release note: None 107967: kvserver: ignore lease validity when checking lease preferences r=erikgrinaker,andrewbaptist a=kvoli In cockroachdb#107507, we began eagerly enqueuing into the replicate queue, when acquiring a replica acquired a new lease which violated lease preferences. Lease preferences were only considered violated when the lease itself was valid. In cockroachdb#107507, we saw that it is uncommon, but possible for an invalid lease to be acquired, violate lease preferences and not be enqueued as a result. The end result was a leaseholder violating the applied lease preferences which would not be resolved until the next scanner cycle. Update the eager enqueue check to only check that the replica is the incoming leaseholder when applying the lease, and that the replica violates the applied lease preferences. Note the purgatory error introduced in cockroachdb#107507, still checks that the lease is valid and owned by the store before proceeding. It is a condition that the lease must be valid+owned by the store to have a change planned, so whilst it is possible the lease becomes invalid somewhere in-between planning, when the replica applies a valid lease, it will still be enqueued so purgatory is unnecessary. Fixes: cockroachdb#107862 Release note: None 107984: roachtest: bugfix testeng grafana link missing cluster name r=renatolabs a=annrpom Previously, the testeng grafana link generated after a roachtest failure directed the user to cockroachlabs.com due to a missing cluster name from the link. This patch should fix this issue by getting the cluster name from a `*githubIssues.cluster.name` instead of the `clusterName` from roachtest/cluster.go. Fixes: cockroachdb#107894 Release note (bug fix): The link to testeng grafana that is generated on a roachtest failure should now properly direct to grafana. Co-authored-by: wenyihu6 <wenyi.hu@cockroachlabs.com> Co-authored-by: Austen McClernon <austen@cockroachlabs.com> Co-authored-by: Annie Pompa <annie@cockroachlabs.com>
diff --git a/pkg/cmd/roachtest/github.go b/pkg/cmd/roachtest/github.go
@@ -60,10 +60,13 @@ func generateHelpCommand(
 			"How To Investigate (internal)",
 			"https://cockroachlabs.atlassian.net/l/c/SSSBr8c7",
 		)(renderer)
-		issues.HelpCommandAsLink(
-			"Grafana",
-			fmt.Sprintf("https://go.crdb.dev/p/roachfana/%s/%d/%d", clusterName, start.UnixMilli(), end.UnixMilli()),
-		)(renderer)
+		// An empty clusterName corresponds to a cluster creation failure
+		if clusterName != "" {
+			issues.HelpCommandAsLink(
+				"Grafana",
+				fmt.Sprintf("https://go.crdb.dev/p/roachfana/%s/%d/%d", clusterName, start.UnixMilli(), end.UnixMilli()),
+			)(renderer)
+		}
 	}
 }
 
@@ -131,6 +134,7 @@ func (g *githubIssues) createPostRequest(
 
 	issueOwner := spec.Owner
 	issueName := testName
+	issueClusterName := ""
 
 	messagePrefix := ""
 	var infraFlake bool
@@ -202,6 +206,7 @@ func (g *githubIssues) createPostRequest(
 			// Hence, we only emit when arch was unspecified.
 			clusterParams[roachtestPrefix("arch")] = string(g.cluster.arch)
 		}
+		issueClusterName = g.cluster.name
 	}
 
 	issueMessage := messagePrefix + message
@@ -218,7 +223,7 @@ func (g *githubIssues) createPostRequest(
 		Artifacts:       artifacts,
 		ExtraLabels:     labels,
 		ExtraParams:     clusterParams,
-		HelpCommand:     generateHelpCommand(clusterName, start, end),
+		HelpCommand:     generateHelpCommand(issueClusterName, start, end),
 	}, nil
 }
 
diff --git a/pkg/cmd/roachtest/github_test.go b/pkg/cmd/roachtest/github_test.go
@@ -184,11 +184,13 @@ func TestCreatePostRequest(t *testing.T) {
 			}
 
 			ti := &testImpl{
-				spec: testSpec,
-				l:    nilLogger(),
+				spec:  testSpec,
+				l:     nilLogger(),
+				start: time.Date(2023, time.July, 21, 16, 34, 3, 817, time.UTC),
+				end:   time.Date(2023, time.July, 21, 16, 42, 13, 137, time.UTC),
 			}
 
-			testClusterImpl := &clusterImpl{spec: clusterSpec, arch: vm.ArchAMD64}
+			testClusterImpl := &clusterImpl{spec: clusterSpec, arch: vm.ArchAMD64, name: "foo"}
 			vo := vm.DefaultCreateOpts()
 			vmOpts := &vo
 
@@ -220,6 +222,11 @@ func TestCreatePostRequest(t *testing.T) {
 				req, err := github.createPostRequest("github_test", ti.start, ti.end, testSpec, c.failure, "message")
 				assert.NoError(t, err, "Expected no error in createPostRequest")
 
+				r := &issues.Renderer{}
+				req.HelpCommand(r)
+				file := fmt.Sprintf("help_command_createpost_%d.txt", idx+1)
+				echotest.Require(t, r.String(), filepath.Join("testdata", file))
+
 				if c.expectedParams != nil {
 					require.Equal(t, c.expectedParams, req.ExtraParams)
 				}
diff --git a/pkg/cmd/roachtest/testdata/help_command_createpost_1.txt b/pkg/cmd/roachtest/testdata/help_command_createpost_1.txt
@@ -0,0 +1,17 @@
+echo
+----
+----
+
+
+See: [roachtest README](https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md)
+
+
+
+See: [How To Investigate \(internal\)](https://cockroachlabs.atlassian.net/l/c/SSSBr8c7)
+
+
+
+See: [Grafana](https://go.crdb.dev/p/roachfana/foo/1689957243000/1689957733000)
+
+----
+----
diff --git a/pkg/cmd/roachtest/testdata/help_command_createpost_2.txt b/pkg/cmd/roachtest/testdata/help_command_createpost_2.txt
@@ -0,0 +1,17 @@
+echo
+----
+----
+
+
+See: [roachtest README](https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md)
+
+
+
+See: [How To Investigate \(internal\)](https://cockroachlabs.atlassian.net/l/c/SSSBr8c7)
+
+
+
+See: [Grafana](https://go.crdb.dev/p/roachfana/foo/1689957243000/1689957733000)
+
+----
+----
diff --git a/pkg/cmd/roachtest/testdata/help_command_createpost_3.txt b/pkg/cmd/roachtest/testdata/help_command_createpost_3.txt
@@ -0,0 +1,13 @@
+echo
+----
+----
+
+
+See: [roachtest README](https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md)
+
+
+
+See: [How To Investigate \(internal\)](https://cockroachlabs.atlassian.net/l/c/SSSBr8c7)
+
+----
+----
diff --git a/pkg/cmd/roachtest/testdata/help_command_createpost_5.txt b/pkg/cmd/roachtest/testdata/help_command_createpost_5.txt
@@ -0,0 +1,17 @@
+echo
+----
+----
+
+
+See: [roachtest README](https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md)
+
+
+
+See: [How To Investigate \(internal\)](https://cockroachlabs.atlassian.net/l/c/SSSBr8c7)
+
+
+
+See: [Grafana](https://go.crdb.dev/p/roachfana/foo/1689957243000/1689957733000)
+
+----
+----
diff --git a/pkg/kv/kvserver/allocator/plan/replicate.go b/pkg/kv/kvserver/allocator/plan/replicate.go
@@ -963,7 +963,8 @@ func (rp ReplicaPlanner) shedLeaseTarget(
 		liveVoters, _ := rp.storePool.LiveAndDeadReplicas(
 			existingVoters, false /* includeSuspectAndDrainingStores */)
 		preferred := rp.allocator.PreferredLeaseholders(rp.storePool, conf, liveVoters)
-		if len(preferred) > 0 && repl.LeaseViolatesPreferences(ctx) {
+		if len(preferred) > 0 &&
+			repl.LeaseViolatesPreferences(ctx) {
 			return nil, CantTransferLeaseViolatingPreferencesError{RangeID: desc.RangeID}
 		}
 		return nil, nil
diff --git a/pkg/kv/kvserver/asim/tests/BUILD.bazel b/pkg/kv/kvserver/asim/tests/BUILD.bazel
@@ -36,6 +36,7 @@ go_library(
         "assert.go",
         "default_settings.go",
         "rand_framework.go",
+        "rand_gen.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvserver/asim/tests",
     visibility = ["//visibility:public"],
diff --git a/pkg/kv/kvserver/asim/tests/rand_framework.go b/pkg/kv/kvserver/asim/tests/rand_framework.go
@@ -50,7 +50,7 @@ func (f randTestingFramework) getCluster() gen.ClusterGen {
 	if !f.s.randOptions.cluster {
 		return defaultBasicClusterGen()
 	}
-	return gen.BasicCluster{}
+	return f.randomClusterInfoGen(f.s.randSource)
 }
 
 func (f randTestingFramework) getRanges() gen.RangeGen {
diff --git a/pkg/kv/kvserver/asim/tests/rand_gen.go b/pkg/kv/kvserver/asim/tests/rand_gen.go
@@ -0,0 +1,25 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package tests
+
+import (
+	"math/rand"
+
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/asim/gen"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/asim/state"
+)
+
+// randomClusterInfoGen returns a randomly picked predefined configuration.
+func (f randTestingFramework) randomClusterInfoGen(randSource *rand.Rand) gen.LoadedCluster {
+	chosenIndex := randSource.Intn(len(state.ClusterOptions))
+	chosenType := state.ClusterOptions[chosenIndex]
+	return loadClusterInfo(chosenType)
+}
diff --git a/pkg/kv/kvserver/asim/tests/rand_test.go b/pkg/kv/kvserver/asim/tests/rand_test.go
@@ -70,7 +70,7 @@ func defaultSettings(randOptions testRandOptions) testSettings {
 // default
 func TestRandomized(t *testing.T) {
 	randOptions := testRandOptions{
-		cluster:        false,
+		cluster:        true,
 		ranges:         false,
 		load:           false,
 		staticSettings: false,
diff --git a/pkg/kv/kvserver/replica_metrics.go b/pkg/kv/kvserver/replica_metrics.go
@@ -149,8 +149,8 @@ func calcReplicaMetrics(d calcReplicaMetricsInput) ReplicaMetrics {
 		validLeaseType = d.leaseStatus.Lease.Type()
 		if validLeaseOwner {
 			livenessLease = keys.NodeLivenessSpan.Overlaps(d.desc.RSpan().AsRawSpanWithNoLocals())
-			switch makeLeasePreferenceStatus(
-				d.leaseStatus, d.storeID, d.storeAttrs, d.nodeAttrs,
+			switch checkStoreAgainstLeasePreferences(
+				d.storeID, d.storeAttrs, d.nodeAttrs,
 				d.nodeLocality, d.conf.LeasePreferences) {
 			case leasePreferencesViolating:
 				violatingLeasePreferences = true
diff --git a/pkg/kv/kvserver/replica_proposal.go b/pkg/kv/kvserver/replica_proposal.go
@@ -517,25 +517,27 @@ func (r *Replica) leasePostApplyLocked(
 		})
 	}
 
-	// If we acquired a new lease, and it violates the lease preferences, enqueue
-	// it in the replicate queue.
-	if leaseChangingHands && iAmTheLeaseHolder {
-		if LeaseCheckPreferencesOnAcquisitionEnabled.Get(&r.store.cfg.Settings.SV) {
-			preferenceStatus := makeLeasePreferenceStatus(st, r.store.StoreID(), r.store.Attrs(),
-				r.store.nodeDesc.Attrs, r.store.nodeDesc.Locality, r.mu.conf.LeasePreferences)
-			switch preferenceStatus {
-			case leasePreferencesOK, leasePreferencesLessPreferred, leasePreferencesUnknown:
-				// We could also enqueue the lease when we are a less preferred
-				// leaseholder, however the replicate queue will eventually get to it and
-				// we already satisfy _some_ preference.
-			case leasePreferencesViolating:
-				log.VEventf(ctx, 2,
-					"acquired lease violates lease preferences, enqueueing for transfer [lease=%v preferences=%v]",
-					newLease, r.mu.conf.LeasePreferences)
-				r.store.replicateQueue.AddAsync(ctx, r, replicateQueueLeasePreferencePriority)
-			default:
-				log.Fatalf(ctx, "unknown lease preferences status: %v", preferenceStatus)
-			}
+	// If we acquired a lease, and it violates the lease preferences, enqueue it
+	// in the replicate queue. NOTE: We don't check whether the lease is valid,
+	// it is possible that the lease being applied is invalid due to replication
+	// lag, or previously needing a snapshot. The replicate queue will ensure the
+	// lease is valid and owned by the replica before processing.
+	if iAmTheLeaseHolder && leaseChangingHands &&
+		LeaseCheckPreferencesOnAcquisitionEnabled.Get(&r.store.cfg.Settings.SV) {
+		preferenceStatus := checkStoreAgainstLeasePreferences(r.store.StoreID(), r.store.Attrs(),
+			r.store.nodeDesc.Attrs, r.store.nodeDesc.Locality, r.mu.conf.LeasePreferences)
+		switch preferenceStatus {
+		case leasePreferencesOK, leasePreferencesLessPreferred:
+			// We could also enqueue the lease when we are a less preferred
+			// leaseholder, however the replicate queue will eventually get to it and
+			// we already satisfy _some_ preference.
+		case leasePreferencesViolating:
+			log.VEventf(ctx, 2,
+				"acquired lease violates lease preferences, enqueuing for transfer [lease=%v preferences=%v]",
+				newLease, r.mu.conf.LeasePreferences)
+			r.store.replicateQueue.AddAsync(ctx, r, replicateQueueLeasePreferencePriority)
+		default:
+			log.Fatalf(ctx, "unknown lease preferences status: %v", preferenceStatus)
 		}
 	}
 
diff --git a/pkg/kv/kvserver/replica_range_lease.go b/pkg/kv/kvserver/replica_range_lease.go
@@ -1556,16 +1556,14 @@ func (r *Replica) hasCorrectLeaseTypeRLocked(lease roachpb.Lease) bool {
 type leasePreferencesStatus int
 
 const (
-	// leasePreferencesUnknown indicates the preferences status cannot be
-	// determined.
-	leasePreferencesUnknown leasePreferencesStatus = iota
-	// leasePreferencesViolating indicates the leaseholder does not
-	// satisfy any lease preference applied.
+	_ leasePreferencesStatus = iota
+	// leasePreferencesViolating indicates the checked store does not satisfy any
+	// lease preference applied.
 	leasePreferencesViolating
-	// leasePreferencesLessPreferred indicates the leaseholder satisfies _some_
+	// leasePreferencesLessPreferred indicates the checked store satisfies _some_
 	// preference, however not the most preferred.
 	leasePreferencesLessPreferred
-	// leasePreferencesOK indicates the lease satisfies the first
+	// leasePreferencesOK indicates the checked store satisfies the first
 	// preference, or no lease preferences are applied.
 	leasePreferencesOK
 )
@@ -1577,32 +1575,34 @@ func (r *Replica) LeaseViolatesPreferences(ctx context.Context) bool {
 	storeID := r.store.StoreID()
 	now := r.Clock().NowAsClockTimestamp()
 	r.mu.RLock()
-	leaseStatus := r.leaseStatusAtRLocked(ctx, now)
 	preferences := r.mu.conf.LeasePreferences
+	leaseStatus := r.leaseStatusAtRLocked(ctx, now)
 	r.mu.RUnlock()
 
+	if !leaseStatus.IsValid() || !leaseStatus.Lease.OwnedBy(storeID) {
+		// We can't determine if the lease preferences are being conformed to or
+		// not, as the store either doesn't own the lease, or doesn't own a valid
+		// lease.
+		return false
+	}
+
 	storeAttrs := r.store.Attrs()
 	nodeAttrs := r.store.nodeDesc.Attrs
 	nodeLocality := r.store.nodeDesc.Locality
-	preferenceStatus := makeLeasePreferenceStatus(
-		leaseStatus, storeID, storeAttrs, nodeAttrs, nodeLocality, preferences)
-
+	preferenceStatus := checkStoreAgainstLeasePreferences(
+		storeID, storeAttrs, nodeAttrs, nodeLocality, preferences)
 	return preferenceStatus == leasePreferencesViolating
 }
 
-func makeLeasePreferenceStatus(
-	leaseStatus kvserverpb.LeaseStatus,
+// checkStoreAgainstLeasePreferences returns whether the given store would
+// violate, be less preferred or ok, leaseholder, according the the lease
+// preferences.
+func checkStoreAgainstLeasePreferences(
 	storeID roachpb.StoreID,
 	storeAttrs, nodeAttrs roachpb.Attributes,
 	nodeLocality roachpb.Locality,
 	preferences []roachpb.LeasePreference,
 ) leasePreferencesStatus {
-	if !leaseStatus.IsValid() || !leaseStatus.Lease.OwnedBy(storeID) {
-		// We can't determine if the lease preferences are being conformed to or
-		// not, as the store either doesn't own the lease, or doesn't own a valid
-		// lease.
-		return leasePreferencesUnknown
-	}
 	if len(preferences) == 0 {
 		return leasePreferencesOK
 	}

-Original file line number
+Diff line change
@@ @@ -0,0 +1,13 @@ @@
 +echo
 +----
 +----
++
++
 +See: [roachtest README](https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/README.md)
++
++
++
 +See: [How To Investigate \(internal\)](https://cockroachlabs.atlassian.net/l/c/SSSBr8c7)
++
 +----
 +----
Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ func (f randTestingFramework) getCluster() gen.ClusterGen {`
`50`	`50`	`if !f.s.randOptions.cluster {`
`51`	`51`	`return defaultBasicClusterGen()`
`52`	`52`	`}`
`53`		`- return gen.BasicCluster{}`
	`53`	`+ return f.randomClusterInfoGen(f.s.randSource)`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	`func (f randTestingFramework) getRanges() gen.RangeGen {`