kvserver: abstract away system config span usage in kv

[irfansharif]

Part of #67679. We'll hide config.SystemConfig behind the
spanconfig.StoreReader interface, and use that instead in the various
queues that need access to the system config span. In future PRs we'll
introduce a data structure that maintains a mapping between spans and
configs that implements this same interface, except it will be powered
by a view over system.span_configurations that was introduced in
#69047.

When we do make that switch, i.e. have KV consult this new thing for
splits, merges, GC and replication, instead of the gossip backed system
config span, ideally it'd be as easy as swapping the source. This PR
helps pave the way for just that.

In #66348 we described how zonepb.ZoneConfigs going forward were going
to be an exclusively SQL-level construct. Consequently we purge[*] all
usages of it in KV, storing on each replica a roachpb.SpanConfig
instead.

[*]: The only remaining use is what powers our replication reports,
which does not extend well to multi-tenancy in its current form and
needs replacing.

Release note: None

Release justification: low risk, high benefit changes to existing functionality

[cockroach-teamcity]

This change is 

[irfansharif]

Actually the last commit is reviewable as is.

[ajwerner]

Most of this change feels mechanical. When you can, it's nice to separate out mechanical changes from implementation changes to help the reviewer.

I don't think this is a particularly controversial change. I'd like Nathan to take a pass as it's the KV team which will end up owning this stuff.

[irfansharif]

Woops, the shadowConfReader was not meant to be in this commit (it wasn't wired up to anything anyway -- we'll use it in a later PR when we have another implementation of spanconfig.StoreReader). @nvanbenschoten, mind taking a look at the last commit when you get the chance? It's entirely mechanical.

[irfansharif]

(gentle bump, @nvanbenschoten)

[nvanbenschoten]

Reviewed 59 of 66 files at r7, 77 of 77 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @irfansharif)

---

pkg/kv/kvserver/allocator_scorer_test.go, line 620 at r8 (raw file):

				{
					Constraints: []zonepb.Constraint{
						{Value: "a", Type: zonepb.Constraint_DEPRECATED_POSITIVE},

This raised a red flag for me and I now see that Constraint_DEPRECATED_POSITIVE is not handled in ZoneConfig.toSpanConfig. Should it be? Do we have a guarantee that we will never see an existing zone with a Constraint_DEPRECATED_POSITIVE constraint type?

---

pkg/kv/kvserver/merge_queue.go, line 389 at r8 (raw file):

	}
	if testingAggressiveConsistencyChecks {
		if _, err := mq.store.consistencyQueue.process(ctx, lhsRepl, nil); err != nil {

Are we intentionally passing nil here?

---

pkg/kv/kvserver/replicate_queue.go, line 322 at r8 (raw file):

			if testingAggressiveConsistencyChecks {
				if _, err := rq.store.consistencyQueue.process(ctx, repl, nil); err != nil {

Same question here. Is this intentional?

---

pkg/kv/kvserver/store.go, line 2982 at r8 (raw file):

}

// TestingDefaultSpanConfig // XXX:

Were you planning to remove/rewrite this?

---

pkg/roachpb/span_config.proto, line 92 at r8 (raw file):

  // GCTTL is the number of seconds overwritten values will be retained before
  // garbage collection. A value <= 0 means older versions are never GC-ed.
  int32 gc_ttl = 3 [(gogoproto.customname) = "GCTTL"];

Out of curiosity, why did we decide to inline this instead of keeping it split out in a separate policy struct? It feel like we may have lost some future-proofing with that decision, as extensions to the GC policy along multiple dimensions (e.g. retain all deletion tombstones for incremental backup, but eagerly collect duplicate values) will now be more invasive. I wonder how @ajwerner feels about this.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @irfansharif, and @nvanbenschoten)

---

pkg/roachpb/span_config.proto, line 92 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Out of curiosity, why did we decide to inline this instead of keeping it split out in a separate policy struct? It feel like we may have lost some future-proofing with that decision, as extensions to the GC policy along multiple dimensions (e.g. retain all deletion tombstones for incremental backup, but eagerly collect duplicate values) will now be more invasive. I wonder how @ajwerner feels about this.

In some ways this feels like the most invasive part of this entire change. I also did not see the motivation. I let it go because I didn't see any strong reason to oppose it. I think your point about policy changes in the future is sound.

[irfansharif]

TFTRs! Will merge on green.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @nvanbenschoten)

---

pkg/kv/kvserver/allocator_scorer_test.go, line 620 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This raised a red flag for me and I now see that Constraint_DEPRECATED_POSITIVE is not handled in ZoneConfig.toSpanConfig. Should it be? Do we have a guarantee that we will never see an existing zone with a Constraint_DEPRECATED_POSITIVE constraint type?

We're guaranteed, yes. We validate against this when users set zone configs. The only remaining uses of this were in tests.

cockroach/pkg/config/zonepb/zone.go

Lines 368 to 371 in b296bac

	if constraint.Type == Constraint_DEPRECATED_POSITIVE {
	return fmt.Errorf("constraints must either be required (prefixed with a '+') or " +
	"prohibited (prefixed with a '-')")
	}

Even looking back to the change that deprecated it, it seems they were never officially supported or documented.
73466fd

I suppose it's possible for some on-prem customer running v1.x to have this lying around, and an upgrade to 21.2 would break it cause we never blocked the upgrade with this validation. Do we care about that at all?

---

pkg/kv/kvserver/merge_queue.go, line 389 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are we intentionally passing nil here?

It was, we passed in nil elsewhere for queues that don't depend on the system config span, and these queues don't either, but meh I'll pass in the available conf reader.

cockroach/pkg/kv/kvserver/store.go

Line 2879 in 56afa9f

processed, err := s.replicaGCQueue.process(ctx, repl, nil)

---

pkg/kv/kvserver/replicate_queue.go, line 322 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Same question here. Is this intentional?

See above.

---

pkg/kv/kvserver/store.go, line 2982 at r8 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Were you planning to remove/rewrite this?

No, this will be here for now, but I did forget to add a proper comment. Done.

---

pkg/roachpb/span_config.proto, line 92 at r8 (raw file):

Previously, ajwerner wrote…

In some ways this feels like the most invasive part of this entire change. I also did not see the motivation. I let it go because I didn't see any strong reason to oppose it. I think your point about policy changes in the future is sound.

Hm, I was inclined to simplify it given the struct in its original form was introduced with the single field back in 2014 and hadn't seen any change since. But I don't feel strongly about it either way -- changed to a similar looking policy struct.

[irfansharif]

bors r+

[irfansharif]

bors r-

[craig]

Canceled.

[irfansharif]

bors r+

[nvanbenschoten]

Reviewed 8 of 10 files at r9, 2 of 2 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, and @nvanbenschoten)

[craig]

Build failed:

• GitHub CI (Cockroach)

[irfansharif]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)