roachtest: schemachange/during/kv failed

[cockroach-teamcity]

roachtest.schemachange/during/kv failed with artifacts on master @ 748b89a3fbef294f4b0f930c9dbdf88294b3deeb:

test artifacts and logs in: /artifacts/schemachange/during/kv/run_1
	schemachange.go:49,monitor.go:105,errgroup.go:74: pq: importing 688 ranges: splitting key /Table/111/1/31834467: change replicas of r105 failed: descriptor changed: [expected] r105:/Table/111/1/{26192423-47979489} [(n2,s2):1VOTER_DEMOTING_LEARNER, (n5,s5):2, (n4,s4):3, (n3,s3):4VOTER_INCOMING, next=5, gen=18, sticky=1659433859.194661639,0] != [actual] r105:/Table/111/1/2{6192423-7379106} [(n3,s3):4VOTER_DEMOTING_LEARNER, (n5,s5):2, (n4,s4):3, (n1,s1):5VOTER_INCOMING, next=6, gen=23, sticky=1659433859.194661639,0]

	monitor.go:127,schemachange.go:53,test_runner.go:896: monitor failure: monitor task failed: t.Fatal() was called
		(1) attached stack trace
		  -- stack trace:
		  | main.(*monitorImpl).WaitE
		  | 	main/pkg/cmd/roachtest/monitor.go:115
		  | main.(*monitorImpl).Wait
		  | 	main/pkg/cmd/roachtest/monitor.go:123
		  | github.com/cockroachdb/cockroach/pkg/cmd/roachtest/tests.registerSchemaChangeDuringKV.func1
		  | 	github.com/cockroachdb/cockroach/pkg/cmd/roachtest/tests/schemachange.go:53
		  | main.(*testRunner).runTest.func2
		  | 	main/pkg/cmd/roachtest/test_runner.go:896
		Wraps: (2) monitor failure
		Wraps: (3) attached stack trace
		  -- stack trace:
		  | main.(*monitorImpl).wait.func2
		  | 	main/pkg/cmd/roachtest/monitor.go:171
		Wraps: (4) monitor task failed
		Wraps: (5) attached stack trace
		  -- stack trace:
		  | main.init
		  | 	main/pkg/cmd/roachtest/monitor.go:80
		  | runtime.doInit
		  | 	GOROOT/src/runtime/proc.go:6222
		  | runtime.main
		  | 	GOROOT/src/runtime/proc.go:233
		  | runtime.goexit
		  | 	GOROOT/src/runtime/asm_amd64.s:1571
		Wraps: (6) t.Fatal() was called
		Error types: (1) *withstack.withStack (2) *errutil.withPrefix (3) *withstack.withStack (4) *errutil.withPrefix (5) *withstack.withStack (6) *errutil.leafError

Parameters: ROACHTEST_cloud=gce , ROACHTEST_cpu=4 , ROACHTEST_ssd=0

Help

See: roachtest README

See: How To Investigate (internal)

/cc @cockroachdb/sql-schema

This test on roachdash | Improve this report!

Jira issue: CRDB-18251

Epic CRDB-19172

[ajwerner]

It feels like adding robustness to transient failures to split inside of SplitAndScatter falls on @cockroachdb/kv-distribution or some KV team.

[nvanbenschoten]

This is surprising because we do have a retry loop in executeAdminCommandWithDescriptor that shouldn't allow such an error to escape.

I think the key is that this is coming from "change replicas", so it must be returned by maybeLeaveAtomicChangeReplicas before running the split. We should retry on those errors as well.

[nvanbenschoten]

I think I understand why this is failing to be retried. executeAdminCommandWithDescriptor should be retrying on a ConditionFailedError:

cockroach/pkg/kv/kvserver/replica_command.go

Line 551 in f4b491f

if !errors.HasType(lastErr, (*roachpb.ConditionFailedError)(nil)) &&

To do that, it uses errors.HasType.

However, when we construct this error, we use errors.WithSecondaryError, which deliberately shifts the ConditionFailedError to a "secondary" error position.

cockroach/pkg/kv/kvserver/replica_command.go

Lines 2427 to 2432 in f4b491f

	if ok, actualDesc := maybeDescriptorChangedError(referenceDesc, err); ok {
	// We do not include the original error as cause in this case -
	// the caller should not observe the cause. We still include it
	// as "secondary payload", in case the error object makes it way
	// to logs or telemetry during a crash.
	err = errors.WithSecondaryError(newDescChangedError(referenceDesc, actualDesc), err)

This is not checked by errors.HasType.

I don't yet understand the comment here, but this would be fixed by #85405.