storage: scatter right after split leads to poor balance

[tbg]

Run this script:

#!/bin/bash

set -euxo pipefail

pkill -9 roach || true
rm -rf cockroach-data* || true

./cockroach start --insecure --listen-addr 127.0.0.1 --background
./cockroach start --insecure --http-port 8081 --port 26258 --store cockroach-data2 --join 127.0.0.1:26257 --background
./cockroach start --insecure --http-port 8082 --port 26259 --store cockroach-data3 --join 127.0.0.1:26257 --logtostderr --background

sleep 10
./cockroach sql --insecure -e "create table foo(id int primary key, v string);"
./cockroach sql --insecure -e "SET CLUSTER SETTING kv.range_merge.queue_enabled = false;"
./cockroach sql --insecure -e "ALTER TABLE foo SPLIT AT (SELECT i*10 FROM generate_series(1, 999) AS g(i));"

# sleep 70
./cockroach sql --insecure -e "ALTER TABLE foo SCATTER;"

See this kind of graph:

The expectation is that the SCATTER leaves the leaseholders roughly balanced. The graph shows a >10x difference.

We think that much of the variability in durations of bulk i/o restore/import is due to this phenomenon.

When I looked at this last, I think it was caused by the allocator receiving updated replica counts only at some interval, but I just inserted a 20s sleep before the scatter and it's just as bad. Ditto 70s:

Scatters seems to .... just not be doing the right thing. It seems to drain the local node, giving equal shares of the leases to the other followers.

cc @danhhz this is much worse than I thought 😆

Jira issue: CRDB-4542

[tbg]

@darinpp this could be a baptism-by-fire debugging allocator/replication issue for you -- let's chat about it in 1:1

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
5 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

[knz]

the issue still exists in 21.1

[nvanbenschoten]

I thought this would have been fixed by #75894, but it appears that the issue remains. cc. @aayushshah15.

[aayushshah15]

I looked into this and what's happening here is that we're seeing correlated decisions being made across the different ranges that AdminScatter is called over. This is the reason we're only seeing this when we call AdminScatter when there already is an existing imbalance in lease counts among the nodes (all those ranges will individually try to reconcile this load imbalance, will makes the node with the most leases shed all of its leases away to the other nodes).

In DistSender.Send() we freely split batches containing these AdminScatterRequests across range boundaries and send those requests asynchronously. That combined with the fact that AdminScatter calls processOneChange directly (i.e. without any semaphore limiting these scatters) produces the symptoms we're seeing.

[aayushshah15]

It seems bad that these scatters aren't rate limited somehow, so it seems like calling scatter on a large enough table can lead to kv.dist_sender.concurrency_limit number of snapshots flying around?

During the evaluation of AdminScatter, should we not be calling into replicateQueue.processOneChange() directly? or should we be making the DistSender not send these scatter requests asynchronously? Both these options seem like relatively significant behavioural changes at this point in this release.

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!