Mostly LGTM, except the concern about spurious errors. You should have @andrewbaptist have a look at this too, since he's more familiar with the allocator.

Reviewed 7 of 7 files at r1, 4 of 4 files at r2, 4 of 4 files at r3, 3 of 3 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @herkolategan, @kvoli, and @smg260)

pkg/cmd/roachtest/tests/failover.go line 1773 at r3 (raw file):

	var value float64
	conn := c.Conn(ctx, t.L(), node)
	defer conn.Close()

nit: this isn't necessary afaik, because Conn() returns a sql.DB, whose connections are managed automatically. If it returned a sql.Conn then we'd have to return it to the pool.

https://pkg.go.dev/database/sql#DB.Close

pkg/cmd/roachtest/tests/lease_preferences.go line 126 at r3 (raw file):

				// ...
				// dc=N: n2N-1 n2N
				fmt.Sprintf("--locality=region=fake-region,zone=fake-zone,dc=%d", (node-1)/2+1))

nit: consider install.NumRacksOption() which automatically sets up N racks [0,N) with locality rack and round-robins nodes across them.

pkg/cmd/roachtest/tests/lease_preferences.go line 183 at r3 (raw file):

	require.NoError(t, WaitForReplication(ctx, t, conn, spec.replFactor))
	c.Run(ctx, c.Node(numNodes), fmt.Sprintf(
		`./cockroach workload init kv --splits %d {pgurl:%d}`, spec.ranges, numNodes))

May want to do a scatter here too, with --scatter.

pkg/cmd/roachtest/tests/lease_preferences.go line 200 at r3 (raw file):

	// Wait for the preference conformance with some non-live nodes.
	checkLeasePreferenceConformance(ctx)

I don't think there's anything here that causes us to reacquire leases for the ranges that lost their leaseholders. We only do this if there's traffic on the range. Considering doing a select count(*) from kv to ensure we acquire a lease on all ranges (typically acquired on the gateway node, or the replica closest to it).

pkg/cmd/roachtest/tests/lease_preferences.go line 201 at r3 (raw file):

	// Wait for the preference conformance with some non-live nodes.
	checkLeasePreferenceConformance(ctx)
	// Start back up the stopped nodes, before finishing the test.

This isn't necessary if you set SkipPostValidations: registry.PostValidationNoDeadNodes.

pkg/kv/kvserver/replica_proposal.go line 529 at r4 (raw file):

			preferenceStatus := makeLeasePreferenceStatus(st, r.store.StoreID(), r.store.Attrs(),
				r.store.nodeDesc.Attrs, r.store.nodeDesc.Locality, r.mu.conf.LeasePreferences)
			switch preferenceStatus {

Should we have a default clause to guard against new statuses?

pkg/kv/kvserver/replica_range_lease.go line 116 at r4 (raw file):

// configured preferences, it is enqueued in the replicate queue for
// processing.
var LeaseCheckPreferencesOnAcquisitionEnabled = settings.RegisterBoolSetting(

Consider a TODO to remove this in 24.1.

pkg/kv/kvserver/replicate_queue.go line 89 at r4 (raw file):

	// removal, non-voter addition and rebalancing.
	// See allocatorimpl.AllocatorAction.Priority.
	replicateQueueLeasePreferencePriority = 1001

Would it make sense to enqueue this above upreplication, if we're using a non-zero priority anyway? Presumably these operations are cheap, as long as they can't starve out upreplication on failures.

pkg/kv/kvserver/allocator/plan/replicate.go line 884 at r5 (raw file):

		)
		if err != nil {
			return nil, stats, err

This error can bubble up to callers that don't expect it. From a cursory read of the code, looks like it'll hit adminScatter and ReplicateQueueDryRun. The only reason why we return this error up is to get the replica into purgatory -- is there a better way to do that which won't leak errors for other callers?

Thanks for the review. I agree this requires another set of eyes.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @erikgrinaker, @herkolategan, and @smg260)

pkg/cmd/roachtest/tests/failover.go line 1773 at r3 (raw file):

When I don't add this in, I see leaked goroutines after the test passes:

15:58:29 main.go:569: runTests destroying all clusters
15:58:34 leaktest.go:161: Leaked goroutine: goroutine 12 [select, 1 minutes]:
database/sql.(*DB).connectionOpener(0xc004c41d40, {0x84685a8, 0xc004c2e740})
        GOROOT/src/database/sql/sql.go:1224 +0x8d
created by database/sql.OpenDB
        GOROOT/src/database/sql/sql.go:792 +0x18d
Leaked goroutine: goroutine 14 [select, 1 minutes]:
database/sql.(*DB).connectionOpener(0xc003a07d40, {0x84685a8, 0xc001402b00})
        GOROOT/src/database/sql/sql.go:1224 +0x8d
created by database/sql.OpenDB

pkg/cmd/roachtest/tests/lease_preferences.go line 126 at r3 (raw file):

Perhaps premature on my part, I was envisioning some additional tests here which have more interesting topologies. Specifying a non-rack locality, like dc makes this a bit easier.

pkg/cmd/roachtest/tests/lease_preferences.go line 183 at r3 (raw file):

I do want a scatter here. I thought these were scattered by default for workload imports? Added a --scatter here just to be sure.

pkg/cmd/roachtest/tests/lease_preferences.go line 200 at r3 (raw file):

The test asserts on the preference metrics. If there's no valid leaseholder, the violating/undersatisfied metrics won't be bumped - so the test still passes.

Do you think we get more coverage out of doing this? I'm thinking we may want both?

pkg/cmd/roachtest/tests/lease_preferences.go line 201 at r3 (raw file):

Ack - I set that originally but thought it might be preferred to bring up the dead nodes anyway. Thinking again, its not really adding much.

Swapped to SkipPostValidations: registry.PostValidationNoDeadNodes

pkg/kv/kvserver/replica_proposal.go line 529 at r4 (raw file):

Do you mean moving all the statuses, other than leasePreferencesViolating under default?

Added a default catch all here, currently it catches leasePreferencesUnknown.

pkg/kv/kvserver/replica_range_lease.go line 116 at r4 (raw file):

Done.

pkg/kv/kvserver/replicate_queue.go line 89 at r4 (raw file):

Once it gets processed by the queue, the action is recomputed and only when there's no higher priority action or rebalancing will the lease preference stuff happen.

This would be expedient when these ranges don't need any action performed, but there's up-replication required on other ranges.

The risk I see, is that these ranges also require some other higher priority action e.g. decommissioning etc. - and that this would invert the priority queue ordering for up-replication.

pkg/kv/kvserver/allocator/plan/replicate.go line 884 at r5 (raw file):

HavingReplicateQueueDryRun bubble up this error seems useful to me. You wouldn't want to get different behavior when debugging via a dry run.

Good point on scatter. It won't do anything useful by returning this error on scatter. I've updated to swallow the error on scatter and log (v=3) instead.

LGTM, but yeah, let's get Andrew's ✅ as well.

Reviewed 11 of 11 files at r6, 4 of 4 files at r7, 3 of 3 files at r8, 5 of 5 files at r9, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @herkolategan, @kvoli, and @smg260)

pkg/cmd/roachtest/tests/failover.go line 1773 at r3 (raw file):

Hm ok, that's annoying -- thanks!

pkg/cmd/roachtest/tests/lease_preferences.go line 126 at r3 (raw file):

I see, that's fine.

pkg/cmd/roachtest/tests/lease_preferences.go line 183 at r3 (raw file):

I don't believe they are, at least not for a kv init. Other workloads may scatter automatically.

pkg/cmd/roachtest/tests/lease_preferences.go line 200 at r3 (raw file):

I think we'll have to do this, otherwise what are we even testing?

Consider the trivial case, where all leases are on 1 node. If the node dies, and we don't acquire leases anywhere else, then there are 0 valid leases in the cluster, and the test trivially passes.

To test what we want to test here, we have to make sure leases are acquired outside of the preferences, and then moved where we want them to be. We can either do this by running a count(*) on a node outside of the preferences, or explicitly move the leases there with an ALTER RANGE RELOCATE LEASE.

FWIW, on the original issue, I ran some experiments with killing nodes and for some reason this did seem to get leases back to preferred regions more reliably (didn't look into why). When I moved the leases manually, I would always get lease violations that persisted for several minutes. So we may want to have a case for this too, since it appears more reliable, but your call.

pkg/kv/kvserver/replica_proposal.go line 529 at r4 (raw file):

No, I meant having explicit cases for all known statuses, and error out in the default case -- to make sure we don't forget to handle any new statuses that may be added later. Like this:

But we could just check for Violating alone here too, I suppose, since that's the only status we care about. Nbd.

pkg/kv/kvserver/replicate_queue.go line 89 at r4 (raw file):

Ok, I trust your and Andrew's judgement on this.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @herkolategan, @kvoli, and @smg260)

pkg/kv/kvserver/replica_proposal.go line 533 at r9 (raw file):

				// We could also enqueue the lease when we are a less preferred
				// leaseholder, however the replicate queue will eventually get to it and
				// we already satisfy _some_ preference.

It seems best to enqueue this rather than wait in the less preferred state. I could easily see customers specifying every possible node as part of their constraint definition from best to worst. In that case, we will always be in LessPreferred and never see Violating. For a similar reason, it might just be simpler to remove the difference between these two enums (LessPreferred and Violating). It seems like the only harm of queuing is we do additional work.

pkg/kv/kvserver/replicate_queue.go line 89 at r4 (raw file):

I agree with @kvoli. However, consider moving this into constant into allocator.go in the Priority() method (even though its not used there directly). Since it is intimately tied to those priorities it is important that anyone who changes them is aware of it.

pkg/kv/kvserver/replicate_queue.go line 822 at r9 (raw file):

	} else if change.Action == allocatorimpl.AllocatorConsiderRebalance &&
		!change.Replica.LeaseViolatesPreferences(ctx) {

There is a "bad" case here where we finish rebalancing and there is a place to put this that doesn't violate lease preference. We will "busy loop" here waiting for a replica to be created. Take the example with 3 regions, and only 1 node per region. If the preference is set for one region and that region goes down, we will constantly be adding this back. Since this is "low priority" it doesn't block any other actions, but it will potentially stop other rebalancing. Have you tried this case (either manually or in a test?)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @herkolategan, @kvoli, and @smg260)

pkg/kv/kvserver/replicate_queue.go line 822 at r9 (raw file):

Retract: I forgot the purgatory state will handle this. Maybe just add a comment here describing how this relates to purgatory. It is pretty subtle.

Thanks for the review Erik! I'll wait for Andrew to approve before merging.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @erikgrinaker, @herkolategan, and @smg260)

pkg/cmd/roachtest/tests/lease_preferences.go line 200 at r3 (raw file):

I think we'll have to do this, otherwise what are we even testing?

In the testing I've done, there are always num ranges valid leases - so It was being tested. I didn't check the acquisition mechanism - maybe this won't hold for 23.1.

Consider the trivial case, where all leases are on 1 node. If the node dies, and we don't acquire leases anywhere else, then there are 0 valid leases in the cluster, and the test trivially passes.

See above, I'm assuming that (most of) these leases get acquired somewhere even without intervention. Seems like a shaky assumption, so I'll add in a select count(*) and load a small amount of rows.

When I moved the leases manually, I would always get lease violations that persisted for several minutes. So we may want to have a case for this too, since it appears more reliable, but your call.

Good idea, added a test for this case.

pkg/kv/kvserver/replica_proposal.go line 529 at r4 (raw file):

Ah yeah, makes sense. Updated to fatal on the default case, and expclitily enumerate the others.

pkg/kv/kvserver/replicate_queue.go line 89 at r4 (raw file):

ack - I'll see what @andrewbaptist thinks.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @erikgrinaker, @herkolategan, and @smg260)

pkg/kv/kvserver/replica_proposal.go line 533 at r9 (raw file):

It seems best to enqueue this rather than wait in the less preferred state.

The reason I chose not to is that it would also compete with lease preference violation replicas on mass lease acquisition. I am concerned about the additional work. It doesn’t seem worth the risk in a backport, however does seem useful to add on master to bake.

it might just be simpler to remove the difference between these two enums (LessPreferred and Violating).

I think the difference is important. These are ordered preferences, not constraints after all. The difference between satisfying some preference and satisfying none is largely dependent on the user's setup.

pkg/kv/kvserver/replicate_queue.go line 89 at r4 (raw file):

There are many places specifying a priority outside the regular allocatorimpl.AllocatorAction enum.

This would need a specific action associated with it, and there are currently no actions associated with leases. I'd prefer if we could defer introducing new action+priority combos until we have a more coherent story with leases and #90110. This struck me as a reasonable middle ground for now.

pkg/kv/kvserver/replicate_queue.go line 822 at r9 (raw file):

Good idea, added a comment here to describe what would happen, and linking the purgatory error.

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @herkolategan, @kvoli, and @smg260)

pkg/cmd/roachtest/tests/lease_preferences.go line 200 at r3 (raw file):

In the testing I've done, there are always num ranges valid leases - so It was being tested.

Hm ok. We did add eager lease acquisition in #101098, but that only works for unquiesced ranges, so I wouldn't really expect that to fire here. The other possibilities are that we're using expiration leases (this test doesn't, but maybe it should use MetamorphicLeases), that the replicate queue acquired the leases (it does so once per cycle), or that some other background process was accessing the ranges (e.g. SQL stats scans).

Either way, yeah, let's do a scan to make sure.

load a small amount of rows

We don't need any rows for this to work, a select count(*) needs valid leases on all ranges to know that they're empty.

added a test for this case

Thanks!

Thanks for the clarification and changes!