Merge #150725 #151548 #151559 #151692 #151750 #151752 #151753

150725: pcr: reorder time series dashboards r=alicia-l2 a=alicia-l2

Swapped order of Replication Lag and Logical Bytes graphs in Physical Cluster Replication DB Console

Fixes: #147621 
Release note: none


151548: roachtest: fix overlapping failure injection bug r=Dev-Kyle a=Dev-Kyle

This change fixes a bug regarding failure injections overlapping over each other when not intended. Marks the mutator step itself to also set unavailableNodes so sequential failure injections don't insert into them. Also considers `waitForStableClusterVersionStep` as incompatible when injecting a network partition recovery.

Epic: none

Release note: none

Fixes: #151522 , #151384

151559: kvcoord: fix txnWriteBuffer for batches with limits and Dels r=yuzefovich a=yuzefovich

Previously, the txnWriteBuffer was oblivious to the fact that some transformed requests might be returned incomplete due to limits set on the BatchRequest (either TargetBytes or MaxSpanRequestKeys), so it would incorrectly think that it has acquired locks on some keys when it hasn't. Usage from SQL was only exposed to the bug via special delete-range fast-path where we used point Dels (i.e. a stmt of the form `DELETE FROM t WHERE k IN (<ids>)` where there are gaps between `id`s) since it always sets a key limit of 600. This commit fixes this particular issue for Dels transformed into Gets and adds a couple of assertions that we don't see batches with CPuts and/or Puts with the limits set.

Additionally, it adjusts the comment to indicate which requests are allowed in batches with limits.

Given that this feature is disabled by default and in the private preview AND it's limited to the DELETE fast-path when more than 600 keys are deleted, I decided to omit the release note.

Fixes: #151294.
Fixes: #151649.

Release note: None

151692: log: add KV_EXEC and CHANGEFEED logging channels r=kyle-a-wong a=kyle-a-wong

KV_EXEC is intended to contain KV events that don't fall into the KV_DISTRIBUTION channel

CHANGEFEED is for changefeed events that are currently being logged to the TELEMETRY channel

Part of: CRDB-53411
Epic: CRDB-53410
Release note (ops change): Introduces two new logging channels: KV_EXEC and CHANGEFEED. KV_EXEC will contain kv events that don't fall into the KV_DISTRIBUTION channel and CHANGEFEED will eventually contain changefeed events (This change doesn't add any logic to move existing changefeed logs to this channel).

151750: roachtest: update pgjdbc test for new portal behavior r=rafiss a=rafiss

We recently merged #151153 which makes certain statements invalid when used in a pausable portal. This causes a few PGJDBC tests to fail during the test setup phase.

fixes #151582
Release note: None

151752: roachtest: deflake hibernate timeouts r=rafiss a=rafiss

This patch increases the timeout to avoid hitting spurious failures when the test takes too long.

fixes #151591
fixes #151711
Release note: None

151753: sctestbackupccl: deflake schemachanger tests during BACKUP r=rafiss a=rafiss

Increase the timeout for waiting for a schema change to complete, which can take a longer time if there are concurrent backups.

fixes #151469
fixes #150842
Release note: None

Co-authored-by: Alicia Lu <[email protected]>
Co-authored-by: Kyle <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 6 people

docs/generated/logging.md

@@ -177,3 +177,12 @@ The `KV_DISTRIBUTION` channel is used to report data distribution events, such a
 replicas between stores in the cluster, or adding (removing) replicas to
 ranges.
 
+### `CHANGEFEED`
+
+The `CHANGEFEED` channel is used to report changefeed events
+
+### `KV_EXEC`
+
+The `KV_EXEC` channel is used to report KV execution events that don't fall into the
+KV_DISTRIBUTION channel.
+

pkg/cli/testdata/logflags

@@ -29,7 +29,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,
@@ -66,7 +68,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,
@@ -173,7 +177,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],/pathA/logs,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],/pathA/logs,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],/pathA/logs,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],/pathA/logs,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],/pathA/logs,true,crdb-v2)>,
@@ -212,7 +218,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],/mypath,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],/mypath,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],/mypath,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],/mypath,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],/mypath,true,crdb-v2)>,
@@ -252,7 +260,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],/pathA/logs,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],/pathA/logs,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],/pathA/logs,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],/pathA/logs,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],/pathA/logs,true,crdb-v2)>,
@@ -297,7 +307,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],/mypath,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],/mypath,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],/mypath,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],/mypath,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],/mypath,true,crdb-v2)>,
@@ -335,7 +347,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,
@@ -374,7 +388,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,
@@ -423,7 +439,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,
@@ -495,7 +513,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],/mypath,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],/mypath,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],/mypath,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],/mypath,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],/mypath,true,crdb-v2)>,
@@ -535,7 +555,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],/pathA,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],/pathA,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],/pathA,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],/pathA,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],/pathA,true,crdb-v2)>,
@@ -595,7 +617,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,
@@ -633,7 +657,9 @@ SQL_EXEC,
 SQL_PERF,
 SQL_INTERNAL_PERF,
 TELEMETRY,
-KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
+KV_DISTRIBUTION,
+CHANGEFEED,
+KV_EXEC],<defaultLogDir>,true,crdb-v2)>,
 health: <fileCfg(INFO: [HEALTH],<defaultLogDir>,true,crdb-v2)>,
 kv-distribution: <fileCfg(INFO: [KV_DISTRIBUTION],<defaultLogDir>,true,crdb-v2)>,
 pebble: <fileCfg(INFO: [STORAGE],<defaultLogDir>,true,crdb-v2)>,

pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel

@@ -66,6 +66,7 @@ go_test(
         "//pkg/cmd/roachtest/roachtestutil/task",
         "//pkg/cmd/roachtest/spec",
         "//pkg/roachpb",
+        "//pkg/roachprod/failureinjection/failures",
         "//pkg/roachprod/install",
         "//pkg/roachprod/logger",
         "//pkg/roachprod/vm",

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

@@ -85,6 +85,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/failureinjection/failures"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
@@ -383,8 +384,9 @@ type (
 		// the following are test-only fields, allowing tests to simulate
 		// cluster properties without passing a cluster.Cluster
 		// implementation.
-		_arch    *vm.CPUArch
-		_isLocal *bool
+		_arch      *vm.CPUArch
+		_isLocal   *bool
+		_getFailer func(name string) (*failures.Failer, error)
 	}
 
 	shouldStop chan struct{}
@@ -964,6 +966,7 @@ func (t *Test) plan() (plan *TestPlan, retErr error) {
 			bgChans:        t.bgChans,
 			logger:         t.logger,
 			cluster:        t.cluster,
+			_getFailer:     t._getFailer,
 		}
 		// Let's generate a plan.
 		plan, err = planner.Plan()

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

@@ -488,6 +488,8 @@ func (m panicNodeMutator) Generate(
 			return s == restartStep[0]
 		})
 		failureContextSteps.MarkNodesUnavailable(true, false)
+		addPanicStep[0].hasUnavailableSystemNodes = true
+		addRestartStep[0].hasUnavailableSystemNodes = true
 
 		mutations = append(mutations, addPanicStep...)
 		mutations = append(mutations, addRestartStep...)
@@ -496,14 +498,20 @@ func (m panicNodeMutator) Generate(
 	return mutations, nil
 }
 
+func GetFailer(planner *testPlanner, name string) (*failures.Failer, error) {
+	if planner._getFailer != nil {
+		return planner._getFailer(name)
+	}
+
+	return planner.cluster.GetFailer(planner.logger, planner.cluster.CRDBNodes(), name, false)
+}
+
 type networkPartitionMutator struct{}
 
 func (m networkPartitionMutator) Name() string { return failures.IPTablesNetworkPartitionName }
 
 func (m networkPartitionMutator) Probability() float64 {
-	// Temporarily set to 0 while we investigate a better way to handle
-	// intersecting failures.
-	return 0
+	return 0.3
 }
 
 func (m networkPartitionMutator) Generate(
@@ -514,8 +522,7 @@ func (m networkPartitionMutator) Generate(
 	idx := newStepIndex(plan)
 	nodeList := planner.currentContext.System.Descriptor.Nodes
 
-	failure := failures.GetFailureRegistry()
-	f, err := failure.GetFailer(planner.cluster.Name(), failures.IPTablesNetworkPartitionName, planner.logger, false)
+	f, err := GetFailer(planner, failures.IPTablesNetworkPartitionName)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get failer for %s: %w", failures.IPTablesNetworkPartitionName, err)
 	}
@@ -557,6 +564,10 @@ func (m networkPartitionMutator) Generate(
 			// Many hook steps require communication between specific nodes, so we
 			// should recover the network partition before running them.
 			_, runHook := s.impl.(runHookStep)
+			// Waiting for stable cluster version requires communication between
+			// all nodes in the cluster, so we should recover the network partition
+			// before running it.
+			_, waitForStable := s.impl.(waitForStableClusterVersionStep)
 
 			if idx.IsConcurrent(s) {
 				if firstStepInConcurrentBlock == nil {
@@ -574,7 +585,7 @@ func (m networkPartitionMutator) Generate(
 			} else {
 				unavailableNodes = s.context.System.hasUnavailableNodes
 			}
-			return unavailableNodes || restartTenant || restartSystem || runHook
+			return unavailableNodes || restartTenant || restartSystem || runHook || waitForStable
 		}
 
 		_, validStartStep := upgrade.CutAfter(func(s *singleStep) bool {
@@ -619,6 +630,10 @@ func (m networkPartitionMutator) Generate(
 		})
 
 		failureContextSteps.MarkNodesUnavailable(true, true)
+		addPartition[0].hasUnavailableSystemNodes = true
+		addPartition[0].hasUnavailableTenantNodes = true
+		addRecoveryStep[0].hasUnavailableSystemNodes = true
+		addRecoveryStep[0].hasUnavailableTenantNodes = true
 
 		mutations = append(mutations, addPartition...)
 		mutations = append(mutations, addRecoveryStep...)

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

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/failureinjection/failures"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
@@ -100,6 +101,9 @@ type (
 
 		// State variables updated as the test plan is generated.
 		usingFixtures bool
+
+		// Unit test only fields.
+		_getFailer func(name string) (*failures.Failer, error)
 	}
 
 	// UpgradeStage encodes in what part of an upgrade a test step is in
@@ -151,6 +155,12 @@ type (
 		reference *singleStep
 		impl      singleStepProtocol
 		op        mutationOp
+		// unavailableNodes are marked for each step during the `Generate`
+		// method, but the mutator steps themselves are not created until
+		// `applyMutations` is called. These booleans denote whether
+		// the mutator sets any nodes to unavailable.
+		hasUnavailableSystemNodes bool
+		hasUnavailableTenantNodes bool
 	}
 
 	// stepSelector provides a high level API for mutator
@@ -1537,6 +1547,10 @@ func (plan *TestPlan) applyMutations(rng *rand.Rand, mutations []mutation) {
 					impl:    mut.impl,
 					rng:     rngFromRNG(rng),
 				}
+				newSingleStep.context.System.hasUnavailableNodes = mut.hasUnavailableSystemNodes
+				if newSingleStep.context.Tenant != nil {
+					newSingleStep.context.Tenant.hasUnavailableNodes = mut.hasUnavailableTenantNodes
+				}
 			}
 
 			switch mut.op {

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

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/failureinjection/failures"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
@@ -353,6 +354,76 @@ func Test_maxNumPlanSteps(t *testing.T) {
 	require.Nil(t, plan)
 }
 
+// TestNoConcurrentFailureInjections tests that failure injection
+// steps properly manage node availability. Specifically:
+// - Failure injection steps should only run if no other failure is currently injected.
+// - Failure recovery steps can only occur if there is an active failure injected.
+// - We can only bump the cluster version if no failures are currently injected.
+func TestNoConcurrentFailureInjections(t *testing.T) {
+	const numIterations = 500
+	rngSource := rand.NewSource(randutil.NewPseudoSeed())
+	// Set all failure injection mutator probabilities to 1.
+	var opts []CustomOption
+	for _, mutator := range failureInjectionMutators {
+		opts = append(opts, WithMutatorProbability(mutator.Name(), 1.0))
+	}
+	opts = append(opts, NumUpgrades(3))
+	getFailer := func(name string) (*failures.Failer, error) {
+		return nil, nil
+	}
+
+	for range numIterations {
+		mvt := newTest(opts...)
+		mvt._getFailer = getFailer
+		mvt.InMixedVersion("test hook", dummyHook)
+		// Use different seed for each iteration
+		mvt.prng = rand.New(rngSource)
+
+		plan, err := mvt.plan()
+		require.NoError(t, err)
+
+		isFailureInjected := false
+
+		var checkSteps func(steps []testStep)
+		checkSteps = func(steps []testStep) {
+			for _, step := range steps {
+				switch s := step.(type) {
+				case *singleStep:
+					switch s.impl.(type) {
+					case panicNodeStep:
+						require.False(t, isFailureInjected, "there should be no active failure when panicNodeStep runs")
+						isFailureInjected = true
+					case networkPartitionInjectStep:
+						require.False(t, isFailureInjected, "there should be no active failure when networkPartitionInjectStep runs")
+						isFailureInjected = true
+					case restartNodeStep:
+						require.True(t, isFailureInjected, "there is no active failure to recover from")
+						isFailureInjected = false
+					case networkPartitionRecoveryStep:
+						require.True(t, isFailureInjected, "there is no active failure to recover from")
+						isFailureInjected = false
+					case waitForStableClusterVersionStep:
+						require.False(t, isFailureInjected, "waitForStableClusterVersionStep cannot run under failure injection")
+					}
+				case sequentialRunStep:
+					checkSteps(s.steps)
+				case concurrentRunStep:
+					// Failure injection steps should never run concurrently with other steps, so treat concurrent
+					// steps as sequential for simplicity.
+					for _, delayedStepInterface := range s.delayedSteps {
+						ds := delayedStepInterface.(delayedStep)
+						checkSteps([]testStep{ds.step})
+					}
+				}
+			}
+		}
+
+		checkSteps(plan.Steps())
+
+		require.False(t, isFailureInjected, "all failure injections should be cleaned up at the end of the test")
+	}
+}
+
 // setDefaultVersions overrides the test's view of the current build
 // as well as the oldest supported version. This allows the test
 // output to remain stable as new versions are released and/or we bump