roachtest: loqrecovery/half-online/workload=tpcc/rangeSize=16mb failed

[cockroach-teamcity]

roachtest.loqrecovery/half-online/workload=tpcc/rangeSize=16mb failed with artifacts on master @ a4ad94ae1379daa20b467c89d09de27ba8e2356d:

(cluster.go:2495).Run: full command output in run_125336.240786697_n6_cockroach-workload-r.log: COMMAND_PROBLEM: exit status 1
test artifacts and logs in: /artifacts/loqrecovery/half-online/workload=tpcc/rangeSize=16mb/run_1

Parameters:

• arch=amd64
• cloud=gce
• coverageBuild=false
• cpu=4
• encrypted=false
• fs=ext4
• localSSD=true
• metamorphicLeases=default
• runtimeAssertionsBuild=false
• ssd=0

Help

See: roachtest README

See: How To Investigate (internal)

See: Grafana

Same failure on other branches

• roachtest: loqrecovery/half-online/workload=tpcc/rangeSize=16mb failed #151276 roachtest: loqrecovery/half-online/workload=tpcc/rangeSize=16mb failed [C-test-failure O-roachtest O-robot T-kv branch-release-25.2]

/cc @cockroachdb/kv-triage

This test on roachdash | Improve this report!

Jira issue: CRDB-53168

[pav-kv]

Error: error printed in newOrder:
ERROR: split failed while applying backpressure to
Put [/Table/113/1/99/4518/0],
Put [/Table/113/1/99/14422/0],
Put [/Table/113/1/99/14582/0],
Put [/Table/113/1/99/15524/0],
Put [/Table/113/1/99/16462/0],
Put [/Table/113/1/99/24818/0],
Put [/Table/113/1/99/64692/0],
Put [/Table/113/1/99/73950/0],
Put [/Table/113/1/99/77014/0],
Put [/Table/113/1/99/81617/0],
[txn: e8168be5]
on range r409:/Table/11{3/1/98/15324-4/1} [(n4,s4):1, (n3,s3):2, (n5,s5):3, (n2,s2):4LEARNER, next=5, gen=35]: needs lease, not adding: repl=(n5,s5):3 seq=5 start=1754225636.522207193,0 exp=1754225642.604787993,0 pro=1754225636.604787993,0 acq=Request (SQLSTATE XXUUU)

[pav-kv]

Same as #146186 (comment)

[pav-kv]

@tbg

Note the acquireLeaseIfNeeded==false which is a prereq for hitting the error). I'm actually not sure why we don't try to acquire the lease here. Either way, if the lease had really changed hands at this point, we wouldn't have been able to acquire it and the split would've failed in much the same way; but it would've been better to let the request pass anyway, so that it can catch a proper NotLeaseholderError and retry at the leaseholder.

Agree that this seems weird. The only place where acquireLeaseIfNeeded==true is when the queue decided to process the replica.

cockroach/pkg/kv/kvserver/queue.go

Line 961 in 0d291f9

conf, err := bq.replicaCanBeProcessed(ctx, repl, true /* acquireLeaseIfNeeded */)

which comes after all the pre-checks such as in maybeAdd and the one you linked. So it seems like in a typical case the queue tries to re-acquire the lease only if it has seen a moment ago that it had one? I see that all 3 callers of processReplica check replicaCanBeProcessed(..., acquireLeaseIfNeeded==false) first.

Which means this flag is almost a no-op at the moment.

[wenyihu6]

I'm actually not sure why we don't try to acquire the lease here.

I also ran into this when looking at decommission stall.

This seems like a behaviour change from c9cf068 which looks unintended.

Below was my question:

Is it intentional to call replicaCanBeProcessed here
cockroach/pkg/kv/kvserver/queue.go

Line 911 in 18dfffd

if _, err := bq.replicaCanBeProcessed(ctx, repl, false /*acquireLeaseIfNeeded */); err != nil {
. processReplica calls into it again
cockroach/pkg/kv/kvserver/queue.go

Line 922 in 18dfffd

err := bq.processReplica(ctx, repl)
. This also seems like a behaviour change from c9cf068 which looks unintended.

[wenyihu6]

Had a question while looking at above - is there anything fundamental from

cockroach/pkg/kv/kvserver/replica_command.go

Line 1207 in 2a0c14d

func (r *Replica) changeReplicasImpl(

that made the replicate queue require the lease? We could lose the lease anytime right before we call process, right? Is it just to coordinate things better at a high level so that replicas on the same range wouldn't be performing actions at the same time?

[pav-kv]

is there anything fundamental ... that made the replicate queue require the lease? We could lose the lease anytime right before we call process, right? Is it just to coordinate things better at a high level so that replicas on the same range wouldn't be performing actions at the same time?

It's the split queue. Yes, it's probably just to avoid thundering herd.

cockroach/pkg/kv/kvserver/queue.go

Lines 300 to 305 in 0d291f9

	// needsLease controls whether this queue requires the range lease to operate
	// on a replica. If so, one will be acquired if necessary. Many queues set
	// needsLease not because they literally need a lease, but because they work
	// on a range level and use it to ensure that only one node in the cluster
	// processes that range.
	needsLease bool

This seems like a behaviour change from c9cf068 which looks unintended.

@wenyihu6 Do you have a specific path in mind which you suspect? I found only one path in the old code which did acquire the lease, which then corresponds to replicaCanBeProcessed(..., true) here after the change. So it seems like the PR hasn't removed any lease acquisitions, and just added more places where the lease is required/checked. Are you saying there is a new replicaCanBeProcessed(..., false) somewhere which causes the queue to give up earlier?