kvserver: drained nodes can re-acquire + retain leases

[irfansharif]

Describe the problem

In a recent customer escalation we observed a cluster with v21.1.3 binaries where a node that was drained, when re-drained appeared to have re-acquired range leases. The re-drain attempt should've been a no-op; the node was not restarted in the interim.

$ crdb node drain --host :26256 --cluster-name=<redacted>
node is draining... remaining: 129
node is draining... remaining: 0 (complete)

$ crdb node drain --host :26256 --cluster-name=<redacted>
node is draining... remaining: 2
node is draining... remaining: 1
node is draining... remaining: 0 (complete)

Should this be possible?

To Reproduce

Haven't tried reproducing.

Expected behavior

Drained nodes to not acquire additional leases.

---

+cc @knz for routing/triage.

Jira issue: CRDB-12212

[jtsiros]

We'll likely pick this up during stability and investigate.

[aayushshah15]

Related conversation: https://cockroachlabs.slack.com/archives/C0KB9Q03D/p1648756314828899. At least one of the causes for this is the fact that AdminScatter called with the RandomizeLeases option (as IMPORTs do) will freely transfer leases to draining nodes.

[irfansharif]

Saw this again elsewhere when trying to work through implications of allowing rebalancing snapshots to drained nodes (#87969); was hitting this code path:

cockroach/pkg/kv/kvserver/replica_range_lease.go

Lines 803 to 825 in 2675c7c

	// If we're draining, we'd rather not take any new leases (since we're also
	// trying to move leases away elsewhere). But if we're the leader, we don't
	// really have a choice and we take the lease - there might not be any other
	// replica available to take this lease (perhaps they're all draining).
	if r.store.IsDraining() {
	// NB: Replicas that are not the Raft leader will not take leases anyway
	// (see the check inside propBuf.FlushLockedWithRaftGroup()), so we don't
	// really need any special behavior for draining nodes here. This check
	// serves mostly as a means to get more granular logging and as a defensive
	// precaution.
	if r.raftBasicStatusRLocked().RaftState != raft.StateLeader {
	log.VEventf(ctx, 2, "refusing to take the lease because we're draining")
	return r.mu.pendingLeaseRequest.newResolvedHandle(
	roachpb.NewError(
	newNotLeaseHolderError(
	roachpb.Lease{}, r.store.StoreID(), r.mu.state.Desc,
	"refusing to take the lease; node is draining",
	),
	),
	)
	}
	log.Info(ctx, "trying to take the lease while we're draining since we're the raft leader")
	}

[irfansharif]

I guess this issue has been true as of #55624. What I guess is surprising to me though is that there's no active mechanism to take the leases away from raft leaders on drained nodes when there are valid replicas elsewhere. This is easily rectified in practice with another cockroach node drain but this to me is somewhat surprising.

[irfansharif]

I used the following repro steps (-ish):

$ roachprod create local -n 5
$ roachprod put local ./cockroach # built using https://github.com/cockroachdb/cockroach/pull/87969
$ roachprod start local --racks 5
$ roachprod sql local:1 -- -e "alter range liveness configure zone using num_replicas = 3";
... repeat for range meta, range default, range system, database system
... wait for initial upreplication/lease balancing
$ roachprod run local:1 './cockroach node drain 4 --insecure'
$ roachprod sql local:1 -- -e "alter range liveness configure zone using constraints = '{"+rack=3": 1}';"
... repeat for range meta, range default, range system, database system
... observe replica transfers into n4, as a result of the changes in https://github.com/cockroachdb/cockroach/pull/87969
... also observe some lease transfers into n4 despite the drained state; the leases stay there despite other targets being available

[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!