storage: replicas not balanced across multiple stores in same node

[cuongdo]

When running a cluster where nodes have multiple stores, one store often seems to get a disproportionate number of replicas allocated to it.

Here's an example cluster with 5 nodes, each with 5 stores:

This is happening on 3 of 5 nodes in this cluster. We currently do not rebalance between multiple stores in the same node, but we may want to consider it for performance reasons. For example, maybe each store is backed by a different SSD. Spreading our I/O evenly across stores could benefit performance.

[bdarnell]

Moving replicas between stores on the same node is tracked in #2067 (and i'll add some more comments there), but I think this is a separate issue. Those ranges should be moved somewhere, even if it's to an underloaded store on another node.

[tbg]

Closing as stale.

[a-robinson]

I get that we're very unlikely to work on this anytime soon, but it's a legitimate flaw in the product, not just a feature request. Do we not keep that sort of thing around anymore?

[tbg]

Oh, I missed that facet. I thought this was just a long-fixed replication bug. Sorry and thanks for complaining. Lots of issues to look into.

[knz]

Update from @tbg: with the recent changes to allow atomic replication changes, we're moving closer to resolving this issue. However:

This doesn’t work yet, but only because it’s out of scope. The work here is to remove the check that prevents us from doing it, and making sure there aren’t any other places where we’ve since bought into the “no two replicas of a range on a node” pattern
I think for the second part an audit on anyone who accesses NodeID should be enough. We want to make sure nobody tries to resolve a replica via its NodeID

Also a casual search also reveals related issue #39415 which we need to solve (maybe via PR #39497 from @nvanbenschoten) before we can consider multi-store nodes safe for use.

(cc @johnrk as this is an important long-standing issue which we're aiming to solve soon-ish)

[nvanbenschoten]

@lunevalex this is the issue I was thinking about. This has been unblocked by #12768.

Most of the work here will be in the kvserver.Allocator, where we need to lift the restriction about rebalancing between stores on the same node if we can rebalance atomically between them. I haven't personally touched this code in a long time, so @tbg might be better able to advise you on the details and subtleties that will go into addressing this.

[tbg]

I would start here by trying out a smoke test that proves that #12768 really fundamentally does allow the lateral rebalance. Concretely, I think this would be a test with boilerplate similar to

cockroach/pkg/kv/kvserver/client_atomic_membership_change_test.go

Line 34 in f80ec9c

func TestAtomicReplicationChange(t *testing.T) {

except that we want to start only a single server, with two stores:

cockroach/pkg/base/test_server_args.go

Line 66 in 7b7fcb0

StoreSpecs []StoreSpec

and then

	desc = runChange(desc, []roachpb.ReplicationChange{
		{ChangeType: roachpb.ADD_REPLICA, Target: roachpb.ReplicationTarge{NodeID:1 StoreID: 2},
		{ChangeType: roachpb.REMOVE_REPLICA, Target: roachpb.ReplicationTarge{NodeID:1 StoreID: 1},
	})

This should work (after removing some checks that probably refuse this for legacy reasons).

Assuming this is in the bag, we can turn our attention to the replicate queue. Rebalances generally start out here:

cockroach/pkg/kv/kvserver/replicate_queue.go

Line 412 in 76c63de

return rq.considerRebalance(ctx, repl, voterReplicas, canTransferLease, dryRun)

We should ultimately be able to end up here with replication targets similar to the test:

cockroach/pkg/kv/kvserver/allocator.go

Lines 748 to 755 in f291061

	addTarget := roachpb.ReplicationTarget{
	NodeID: target.store.Node.NodeID,
	StoreID: target.store.StoreID,
	}
	removeTarget := roachpb.ReplicationTarget{
	NodeID: removeReplica.NodeID,
	StoreID: removeReplica.StoreID,
	}

I browsed through the code and the only code in the allocator that rules out lateral rebalances now is here:

cockroach/pkg/kv/kvserver/allocator_scorer.go

Lines 613 to 621 in 4cf5a97

	// Nodes that already have a replica on one of their stores aren't valid
	// rebalance targets. We do include stores that currently have a replica
	// because we want them to be considered as valid stores in the
	// ConvergesOnMean calculations below. This is subtle but important.
	if nodeHasReplica(store.Node.NodeID, existingReplicas) &&
	!storeHasReplica(store.StoreID, existingReplicas) {
	log.VEventf(ctx, 2, "nodeHasReplica(n%d, %v)=true",
	store.Node.NodeID, existingReplicas)
	continue

We can't just remove that - we want to allow a second replica on the same node only if we are in the context of a lateral move (we would not want to add a second replica to the node without atomically removing the first one). This will need to be established and unit tested.

Finally, we should have some sort of high-level validation that this works end-to-end. For example, we could start a three-node cluster on which the first node has two stores and ensure that we organically see replicas move to that store (which means there must have been lateral rebalances).

I'll point out that there are likely deficiencies in how the system tries to balance the load. For example, if n1 has 100 stores and n2 and n3 have one, then effectively n1 will end up with 100/102 percent of data (I think) which may not be what we want (n1 might be CPU bound long before that). But for mostly symmetric configurations, hopefully it would do the right thing. While we're looking at that code, we should document as much as possible the expected behavior and its strengths. I believe we have some simulated rebalancing unit tests, we should update those.

[johnrk-zz]

We have access to a VMware test environment until September. We should consider using that environment to test with vSAN. cc @a-entin