storage: rework AdminChangeReplicas for atomic membership changes

[tbg]

Following the plan laid out in #39485, this adds API support for
general replication changes to AdminChangeReplicasRequest.

(*DB).AtomicChangeReplicas will now accept an arbitrary set
of additions/removals, though only on paper - the changes will
be executed individually.

The compatibility story is straightforward since this request is never
persisted. We simply populate both the deprecated and the new field (and
it isn't safe to emit "mixed" changes until all nodes run 19.2 and it's
not worth plumbing a setting around), and in 20.1 we remove the old
fields. A maybe-snag is that as a result, there are a few months left
in this release in which folks may accidentally mix additions and
removals in a replica change without proper version gating. This wasn't
deemed very likely; to mitigate we could add in-memory state on the
request that fires a panic whenever the changeType changes.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

The compatibility story is straightforward since this request is never
persisted. We simply populate both the deprecated and the new field (and
it isn't safe to emit "mixed" changes until all nodes run 19.2 and it's
not worth plumbing a setting around), and in 20.1 we remove the old
fields.

How are you planning on using these new fields? For an AdminChangeReplicas request that adds and removes replicas, we'll need to break compatibility with existing versions of CRDB, so we'll need a gated cluster version. Maybe I'm missing something.

Reviewed 14 of 14 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/roachpb/api.go, line 1192 at r1 (raw file):

// item for each target.
func MakeReplicationChanges(
	changeType ReplicaChangeType, targets []ReplicationTarget,

nit: consider making the targets arg variadic.

---

pkg/roachpb/api.go, line 1209 at r1 (raw file):
Should we assert that this is consistent across all changes until we can get rid of it? This is what you meant by the following, right?

A maybe-snag is that as a result, there are a few months left
in this release in which folks may accidentally mix additions and
removals in a replica change without proper version gating. This wasn't
deemed very likely; to mitigate we could add in-memory state on the
request that fires a panic whenever the changeType changes.

What we have here seems like a footgun because we're blindly ignoring the change type of all but the first change. If we're not careful at the caller, we could end up performing a replica change that we didn't intend on.

---

pkg/roachpb/api.proto, line 750 at r1 (raw file):

}

message ReplicationChange {

Give this a comment.

[tbg]

We are not using them except for testing. The only production user of atomic replication changes in 19.2 is the replicate queue, but it doesn't use this request type but calls straight into (*Replica).addReplica:

cockroach/pkg/storage/replicate_queue.go

Lines 763 to 765 in dd5053e

	if _, err := repl.addReplica(ctx, target, desc, priority, reason, details); err != nil {
	return err
	}

I should've added that bit of information to the commit message (done now).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/roachpb/api.go, line 1209 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we assert that this is consistent across all changes until we can get rid of it? This is what you meant by the following, right?

A maybe-snag is that as a result, there are a few months left
in this release in which folks may accidentally mix additions and
removals in a replica change without proper version gating. This wasn't
deemed very likely; to mitigate we could add in-memory state on the
request that fires a panic whenever the changeType changes.

What we have here seems like a footgun because we're blindly ignoring the change type of all but the first change. If we're not careful at the caller, we could end up performing a replica change that we didn't intend on.

I want to use this for testing, so I don't want to outright prevent mixing (see TestAtomicMembershipChange). But since I need it only in tests, I added what I feel is an acceptable hack: we look for a "testing" key in the context. If it isn't there, we make sure you don't mix adds and removes. Otherwise, you get to do it. We could also have two flavors of this method, but then we need to worry similarly about the general flavor being called in prod before it's safe. PTAL

[nvanbenschoten]

Reviewed 10 of 10 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

---

pkg/internal/client/db.go, line 538 at r2 (raw file):

		// TODO(tbg): remove this when 19.2 is released.
		var typ *roachpb.ReplicaChangeType
		for _, chg := range chgs {

nit:

for i := range chgs {
    chg := &chgs[i]
    if typ == nil {
        typ = &chg.ChangeType

to avoid the allocation.

[tbg]

TFTR!

Hmm, CI hit a test failure that is probably new, I assume from https://github.com/cockroachdb/cockroach/pulls?q=is%3Apr+author%3Adanhhz+is%3Aclosed

Looks "benign" enough, let's see if I can repro it.

TestReplicateQueueDownReplicate
replicate_queue_test.go:255: expected range log event reason range over-replicated, got abandoned learner replica from info {r21:{m-/Table/SystemConfigSpan/Start} [(n2,s2):1, (n4,s4):2, (n1,s1):5, next=7, gen=16] (n5,s5):6LEARNER abandoned learner replica }

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

[tbg]

I meant #39034.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

[tbg]

make stressrace PKG=./pkg/storage/ TESTS=TestReplicateQueueDownReplicate STRESSFLAGS='-stderr=false -p 12'
2 runs completed, 1 failures, over 3m42s

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten)

[tbg]

Going to PR the fix separately (problem not introduced in this PR).

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)