Merge #118265 #118441 #118559

118265: rangefeed: improve assertions r=erikgrinaker a=erikgrinaker

**rangefeed: use `log.Fatal` instead of panic**

This will include the log tags from the context, aiding with debugging. Involves a bunch of context plumbing, and removing some assertion tests.

A couple of panics were left in, where it didn't appear natural to plumb through a context.

**rangefeed: use pointer receiver in MVCCLogicalOp assertion**

`String()` is only implemented for a pointer receiver, so the formatting breaks otherwise.

Epic: none
Release note: None

118441: mixedversion: add public API to disable individual mutators and rename `finalize` step r=DarrylWong a=renatolabs

With this API, tests are now able to disable individual mutators if
they are incompatible with the test (e.g., setting a cluster setting
that conflicts with what the test is trying to do). Tests should be
able to disable mutators with something like the following:

```go
mvt := mixedversion.NewTest(...,
    mixedversion.DisableMutators(mixedversion.ClusterSetting("foo")),
)
```

Note: this is just an example -- individual mutator implementatios are not
yet available.

**mixedversion: rename `finalizeUpgrade` step to `allowUpgrade`**
In the near future, we will randomly change the timing of this step so
that it is not immediately followed by migrations, as the name
previously suggested.

Epic: none

Release note: None

118559: opt/exec: try deflaking distsql_tenant_locality again r=yuzefovich a=michae2

In #118168 we tried a retry, but that didn't work. Let's check whether the sql instances are alive. I think this is what
`crdb_internal.sql_liveness_is_alive` is for.`

Fixes: #117005
Epic: None
Release note: None

Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Michael Erickson <[email protected]>

pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go

@@ -154,12 +154,13 @@ var (
 	defaultTestOptions = testOptions{
 		// We use fixtures more often than not as they are more likely to
 		// detect bugs, especially in migrations.
-		useFixturesProbability:  0.7,
-		upgradeTimeout:          clusterupgrade.DefaultUpgradeTimeout,
-		minUpgrades:             1,
-		maxUpgrades:             4,
-		minimumSupportedVersion: OldestSupportedVersion,
-		predecessorFunc:         randomPredecessorHistory,
+		useFixturesProbability:         0.7,
+		upgradeTimeout:                 clusterupgrade.DefaultUpgradeTimeout,
+		minUpgrades:                    1,
+		maxUpgrades:                    4,
+		minimumSupportedVersion:        OldestSupportedVersion,
+		predecessorFunc:                randomPredecessorHistory,
+		overriddenMutatorProbabilities: make(map[string]float64),
 	}
 
 	// OldestSupportedVersion is the oldest cockroachdb version
@@ -249,13 +250,14 @@ type (
 	// testOptions contains some options that can be changed by the user
 	// that expose some control over the generated test plan and behaviour.
 	testOptions struct {
-		useFixturesProbability  float64
-		upgradeTimeout          time.Duration
-		minUpgrades             int
-		maxUpgrades             int
-		minimumSupportedVersion *clusterupgrade.Version
-		predecessorFunc         predecessorFunc
-		settings                []install.ClusterSettingOption
+		useFixturesProbability         float64
+		upgradeTimeout                 time.Duration
+		minUpgrades                    int
+		maxUpgrades                    int
+		minimumSupportedVersion        *clusterupgrade.Version
+		predecessorFunc                predecessorFunc
+		settings                       []install.ClusterSettingOption
+		overriddenMutatorProbabilities map[string]float64
 	}
 
 	CustomOption func(*testOptions)
@@ -392,6 +394,23 @@ func AlwaysUseLatestPredecessors(opts *testOptions) {
 	opts.predecessorFunc = latestPredecessorHistory
 }
 
+// WithMutatorProbability allows tests to override the default
+// probability that a mutator will be applied to a test plan.
+func WithMutatorProbability(name string, probability float64) CustomOption {
+	return func(opts *testOptions) {
+		opts.overriddenMutatorProbabilities[name] = probability
+	}
+}
+
+// DisableMutators disables all mutators with the names passed.
+func DisableMutators(names ...string) CustomOption {
+	return func(opts *testOptions) {
+		for _, name := range names {
+			WithMutatorProbability(name, 0)(opts)
+		}
+	}
+}
+
 // NewTest creates a Test struct that users can use to create and run
 // a mixed-version roachtest.
 func NewTest(
@@ -841,21 +860,21 @@ func (s restartWithNewBinaryStep) Run(
 	)
 }
 
-// finalizeUpgradeStep resets the `preserve_downgrade_option` cluster
+// allowUpgradeStep resets the `preserve_downgrade_option` cluster
 // setting, allowing the upgrade migrations to run and the cluster
 // version to eventually reach the binary version on the nodes.
-type finalizeUpgradeStep struct {
+type allowUpgradeStep struct {
 	crdbNodes option.NodeListOption
 	prng      *rand.Rand
 }
 
-func (s finalizeUpgradeStep) Background() shouldStop { return nil }
+func (s allowUpgradeStep) Background() shouldStop { return nil }
 
-func (s finalizeUpgradeStep) Description() string {
-	return "finalize upgrade by resetting `preserve_downgrade_option`"
+func (s allowUpgradeStep) Description() string {
+	return "allow upgrade to happen by resetting `preserve_downgrade_option`"
 }
 
-func (s finalizeUpgradeStep) Run(
+func (s allowUpgradeStep) Run(
 	ctx context.Context, l *logger.Logger, c cluster.Cluster, h *Helper,
 ) error {
 	node, db := h.RandomDB(s.prng, s.crdbNodes)

pkg/cmd/roachtest/roachtestutil/mixedversion/planner.go

@@ -98,7 +98,9 @@ type (
 		// Probability returns the probability that this mutator will run
 		// in any given test run. Every mutator should run under some
 		// probability. Making this an explicit part of the interface
-		// makes that more prominent.
+		// makes that more prominent. Note that tests are able to override
+		// this probability for specific mutator implementations as
+		// needed.
 		Probability() float64
 		// Generate takes a test plan and a RNG and returns the list of
 		// mutations that should be applied to the plan.
@@ -242,7 +244,7 @@ func (p *testPlanner) Plan() *TestPlan {
 	// Probabilistically enable some of of the mutators on the base test
 	// plan generated above.
 	for _, mut := range planMutators {
-		if p.prng.Float64() < mut.Probability() {
+		if p.mutatorEnabled(mut) {
 			mutations := mut.Generate(p.prng, testPlan)
 			testPlan.applyMutations(p.prng, mutations)
 			testPlan.enabledMutators = append(testPlan.enabledMutators, mut)
@@ -408,7 +410,7 @@ func (p *testPlanner) finalizeUpgradeSteps(
 	p.currentContext.Finalizing = true
 	p.currentContext.Stage = RunningUpgradeMigrationsStage
 	runMigrations := p.newSingleStep(
-		finalizeUpgradeStep{prng: p.newRNG(), crdbNodes: p.crdbNodes},
+		allowUpgradeStep{prng: p.newRNG(), crdbNodes: p.crdbNodes},
 	)
 	var mixedVersionStepsDuringMigrations []testStep
 	if scheduleHooks {
@@ -460,6 +462,15 @@ func (p *testPlanner) newRNG() *rand.Rand {
 	return rngFromRNG(p.prng)
 }
 
+func (p *testPlanner) mutatorEnabled(mut mutator) bool {
+	probability := mut.Probability()
+	if p, ok := p.options.overriddenMutatorProbabilities[mut.Name()]; ok {
+		probability = p
+	}
+
+	return p.prng.Float64() < probability
+}
+
 func newUpgradePlan(from, to *clusterupgrade.Version) *upgradePlan {
 	return &upgradePlan{
 		from: from,

pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go

@@ -57,11 +57,22 @@ var (
 const seed = 12345 // expectations are based on this seed
 
 func TestTestPlanner(t *testing.T) {
-	reset := setBuildVersion()
-	defer reset()
+	// Tests run from an empty list of mutators; the only way to add
+	// mutators is by using the `add-mutators` directive in the
+	// test. This allows the output to remain stable when new mutators
+	// are added, and also allows us to test mutators explicitly and in
+	// isolation.
+	resetMutators := func() { planMutators = nil }
+	resetBuildVersion := setBuildVersion()
+	defer func() {
+		resetBuildVersion()
+		resetMutators()
+	}()
 
 	datadriven.Walk(t, datapathutils.TestDataPath(t, "planner"), func(t *testing.T, path string) {
+		resetMutators()
 		mvt := newTest()
+
 		datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
 			if d.Cmd == "plan" {
 				plan, err := mvt.plan()
@@ -76,6 +87,20 @@ func TestTestPlanner(t *testing.T) {
 			}
 
 			switch d.Cmd {
+			case "add-mutators":
+				for _, arg := range d.CmdArgs {
+					var mut mutator
+					switch mutatorName := arg.Key; mutatorName {
+					case "concurrent_user_hooks_mutator":
+						mut = concurrentUserHooksMutator{}
+					case "remove_user_hooks_mutator":
+						mut = removeUserHooksMutator{}
+					default:
+						t.Fatalf("unknown mutator: %s", mutatorName)
+					}
+
+					planMutators = append(planMutators, mut)
+				}
 			case "mixed-version-test":
 				mvt = createDataDrivenMixedVersionTest(t, d.CmdArgs)
 			case "on-startup":
@@ -353,6 +378,24 @@ func createDataDrivenMixedVersionTest(t *testing.T, args []datadriven.CmdArg) *T
 			require.NoError(t, err)
 			isLocal = boolP(b)
 
+		case "mutator_probabilities":
+			if len(arg.Vals)%2 != 0 {
+				t.Fatalf("even number of values required for %s directive", arg.Key)
+			}
+
+			var j int
+			for j < len(arg.Vals) {
+				name, probStr := arg.Vals[j], arg.Vals[j+1]
+				prob, err := strconv.ParseFloat(probStr, 64)
+				require.NoError(t, err)
+				opts = append(opts, WithMutatorProbability(name, prob))
+
+				j += 2
+			}
+
+		case "disable_mutator":
+			opts = append(opts, DisableMutators(arg.Vals[0]))
+
 		default:
 			t.Errorf("unknown mixed-version-test option: %s", arg.Key)
 		}
@@ -684,6 +727,42 @@ NEXT_STEP:
 	return fmt.Errorf("no concurrent step that includes: %#v", names)
 }
 
+// concurrentUserHooksMutator is a test mutator that inserts a step
+// concurrently with every user-provided hook.
+type concurrentUserHooksMutator struct{}
+
+func (concurrentUserHooksMutator) Name() string         { return "concurrent_user_hooks_mutator" }
+func (concurrentUserHooksMutator) Probability() float64 { return 0.5 }
+
+func (concurrentUserHooksMutator) Generate(rng *rand.Rand, plan *TestPlan) []mutation {
+	// Insert our `testSingleStep` implementation concurrently with every
+	// user-provided function.
+	return plan.
+		newStepSelector().
+		Filter(func(s *singleStep) bool {
+			_, ok := s.impl.(runHookStep)
+			return ok
+		}).
+		InsertConcurrent(&testSingleStep{})
+}
+
+// removeUserHooksMutator is a test mutator that removes every
+// user-provided hook from the plan.
+type removeUserHooksMutator struct{}
+
+func (removeUserHooksMutator) Name() string         { return "remove_user_hooks_mutator" }
+func (removeUserHooksMutator) Probability() float64 { return 0.5 }
+
+func (removeUserHooksMutator) Generate(rng *rand.Rand, plan *TestPlan) []mutation {
+	return plan.
+		newStepSelector().
+		Filter(func(s *singleStep) bool {
+			_, ok := s.impl.(runHookStep)
+			return ok
+		}).
+		Remove()
+}
+
 func dummyHook(context.Context, *logger.Logger, *rand.Rand, *Helper) error {
 	return nil
 }

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/basic_test_mixed_version_hooks

@@ -43,6 +43,6 @@ mixed-version test plan for upgrading from "v22.2.8" to "<current>":
    │   ├── restart node 1 with binary version <current> (13)
    │   ├── run "mixed-version 1" (14)
    │   └── restart node 3 with binary version <current> (15)
-   ├── finalize upgrade by resetting `preserve_downgrade_option` (16)
+   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (16)
    ├── run "mixed-version 2" (17)
    └── wait for nodes :1-4 to reach cluster version <current> (18)

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/conflicting_mutators

@@ -0,0 +1,47 @@
+# Tests that we are able to deal with mutators that insert relative to
+# steps that are later removed by subsequent mutations. The initial
+# insertion should create a concurrent step with user-hooks (see
+# `mutator_probabilities` test) and the second mutator removes user
+# hooks, flattening that concurrent run.
+
+add-mutators concurrent_user_hooks_mutator remove_user_hooks_mutator
+----
+ok
+
+# ensure both mutators are always applied
+mixed-version-test num_upgrades=1 mutator_probabilities=(concurrent_user_hooks_mutator, 1, remove_user_hooks_mutator, 1)
+----
+ok
+
+in-mixed-version name=(my mixed version feature)
+----
+ok
+
+plan debug=true
+----
+mixed-version test plan for upgrading from "v22.2.8" to "<current>" with mutators {concurrent_user_hooks_mutator, remove_user_hooks_mutator}:
+├── install fixtures for version "v22.2.8" (1) [stage=cluster-setup]
+├── start cluster at version "v22.2.8" (2) [stage=cluster-setup]
+├── wait for nodes :1-4 to reach cluster version '22.2' (3) [stage=cluster-setup]
+└── upgrade cluster from "v22.2.8" to "<current>"
+   ├── prevent auto-upgrades by setting `preserve_downgrade_option` (4) [stage=init]
+   ├── upgrade nodes :1-4 from "v22.2.8" to "<current>"
+   │   ├── restart node 4 with binary version <current> (5) [stage=temporary-upgrade]
+   │   ├── restart node 2 with binary version <current> (6) [stage=temporary-upgrade]
+   │   ├── restart node 3 with binary version <current> (7) [stage=temporary-upgrade]
+   │   ├── testSingleStep (8) [stage=temporary-upgrade]
+   │   └── restart node 1 with binary version <current> (9) [stage=temporary-upgrade]
+   ├── downgrade nodes :1-4 from "<current>" to "v22.2.8"
+   │   ├── restart node 2 with binary version v22.2.8 (10) [stage=rollback-upgrade]
+   │   ├── restart node 4 with binary version v22.2.8 (11) [stage=rollback-upgrade]
+   │   ├── testSingleStep (12) [stage=rollback-upgrade]
+   │   ├── restart node 1 with binary version v22.2.8 (13) [stage=rollback-upgrade]
+   │   └── restart node 3 with binary version v22.2.8 (14) [stage=rollback-upgrade]
+   ├── upgrade nodes :1-4 from "v22.2.8" to "<current>"
+   │   ├── restart node 2 with binary version <current> (15) [stage=last-upgrade]
+   │   ├── restart node 4 with binary version <current> (16) [stage=last-upgrade]
+   │   ├── testSingleStep (17) [stage=last-upgrade]
+   │   ├── restart node 1 with binary version <current> (18) [stage=last-upgrade]
+   │   └── restart node 3 with binary version <current> (19) [stage=last-upgrade]
+   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (20) [stage=running-upgrade-migrations]
+   └── wait for nodes :1-4 to reach cluster version <current> (21) [stage=running-upgrade-migrations]

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/local_runs_reduced_wait_time

@@ -49,7 +49,7 @@ mixed-version test plan for upgrading from "v22.2.3" to "v23.1.4" to "v23.2.0" t
 │   │   ├── wait for 30s (15)
 │   │   ├── restart node 4 with binary version v23.1.4 (16)
 │   │   └── restart node 2 with binary version v23.1.4 (17)
-│   ├── finalize upgrade by resetting `preserve_downgrade_option` (18)
+│   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (18)
 │   └── wait for nodes :1-4 to reach cluster version '23.1' (19)
 ├── run "initialize bank workload" (20)
 ├── start background hooks concurrently
@@ -65,7 +65,7 @@ mixed-version test plan for upgrading from "v22.2.3" to "v23.1.4" to "v23.2.0" t
 │   │   ├── restart node 1 with binary version v23.2.0 (27)
 │   │   ├── restart node 2 with binary version v23.2.0 (28)
 │   │   └── restart node 3 with binary version v23.2.0 (29)
-│   ├── finalize upgrade by resetting `preserve_downgrade_option` (30)
+│   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (30)
 │   ├── wait for nodes :1-4 to reach cluster version '23.2' (31)
 │   └── run "validate upgrade" (32)
 └── upgrade cluster from "v23.2.0" to "<current>"
@@ -91,7 +91,7 @@ mixed-version test plan for upgrading from "v22.2.3" to "v23.1.4" to "v23.2.0" t
    │   ├── run "mixed-version 2" (49)
    │   ├── restart node 2 with binary version <current> (50)
    │   └── restart node 3 with binary version <current> (51)
-   ├── finalize upgrade by resetting `preserve_downgrade_option` (52)
+   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (52)
    ├── run "mixed-version 2" (53)
    ├── wait for nodes :1-4 to reach cluster version <current> (54)
    └── run "validate upgrade" (55)

pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/minimum_supported_version

@@ -49,7 +49,7 @@ mixed-version test plan for upgrading from "v21.2.11" to "v22.1.8" to "v22.2.3"
 │   │   ├── wait for 5m0s (15)
 │   │   ├── restart node 4 with binary version v22.1.8 (16)
 │   │   └── restart node 2 with binary version v22.1.8 (17)
-│   ├── finalize upgrade by resetting `preserve_downgrade_option` (18)
+│   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (18)
 │   └── wait for nodes :1-4 to reach cluster version '22.1' (19)
 ├── upgrade cluster from "v22.1.8" to "v22.2.3"
 │   ├── prevent auto-upgrades by setting `preserve_downgrade_option` (20)
@@ -59,7 +59,7 @@ mixed-version test plan for upgrading from "v21.2.11" to "v22.1.8" to "v22.2.3"
 │   │   ├── restart node 1 with binary version v22.2.3 (23)
 │   │   ├── restart node 2 with binary version v22.2.3 (24)
 │   │   └── restart node 3 with binary version v22.2.3 (25)
-│   ├── finalize upgrade by resetting `preserve_downgrade_option` (26)
+│   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (26)
 │   └── wait for nodes :1-4 to reach cluster version '22.2' (27)
 ├── upgrade cluster from "v22.2.3" to "v23.1.4"
 │   ├── prevent auto-upgrades by setting `preserve_downgrade_option` (28)
@@ -69,7 +69,7 @@ mixed-version test plan for upgrading from "v21.2.11" to "v22.1.8" to "v22.2.3"
 │   │   ├── restart node 3 with binary version v23.1.4 (31)
 │   │   ├── restart node 4 with binary version v23.1.4 (32)
 │   │   └── wait for 10m0s (33)
-│   ├── finalize upgrade by resetting `preserve_downgrade_option` (34)
+│   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (34)
 │   └── wait for nodes :1-4 to reach cluster version '23.1' (35)
 ├── run "initialize bank workload" (36)
 ├── start background hooks concurrently
@@ -84,7 +84,7 @@ mixed-version test plan for upgrading from "v21.2.11" to "v22.1.8" to "v22.2.3"
 │   │   ├── restart node 2 with binary version v23.2.0 (43)
 │   │   ├── run "mixed-version 2" (44)
 │   │   └── restart node 3 with binary version v23.2.0 (45)
-│   ├── finalize upgrade by resetting `preserve_downgrade_option` (46)
+│   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (46)
 │   ├── wait for nodes :1-4 to reach cluster version '23.2' (47)
 │   └── run "validate upgrade" (48)
 └── upgrade cluster from "v23.2.0" to "<current>"
@@ -111,7 +111,7 @@ mixed-version test plan for upgrading from "v21.2.11" to "v22.1.8" to "v22.2.3"
    │   ├── run "mixed-version 1" (65)
    │   ├── restart node 4 with binary version <current> (66)
    │   └── restart node 1 with binary version <current> (67)
-   ├── finalize upgrade by resetting `preserve_downgrade_option` (68)
+   ├── allow upgrade to happen by resetting `preserve_downgrade_option` (68)
    ├── run "mixed-version 1" (69)
    ├── wait for nodes :1-4 to reach cluster version <current> (70)
    └── run "validate upgrade" (71)