spanconfig: set stage for migration

[irfansharif]

spanconfig: get rid of ReconciliationDependencies interface

It was hollow, simply embedding the spanconfig.Reconciler interface. In
a future commit we end up relying on each pod's span config reconciler
outside of just the reconciliation job. This makes the interface even
awkwarder than it was.

spanconfig/reconciler: export the checkpoint timestamp

We'll make use of it in a future commit.

*kvserver: plumb in a context into (Store).GetConfReader

We'll use it in a future commit.

clusterversion: improve a version comment

Gets rid of a squiggly line in Goland.

---

Set of commits to set the stage for #73876.

[cockroach-teamcity]

This change is 

[irfansharif]

(Gentle bump.)

[arulajmani]

Reviewed 1 of 1 files at r1, 7 of 7 files at r2, 7 of 7 files at r3, 9 of 9 files at r4, 9 of 13 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @irfansharif, and @nvanbenschoten)

---

pkg/migration/migrations/migrate_span_configs.go, line 34 at r5 (raw file):

	for r := retry.StartWithCtx(ctx, base.DefaultRetryOptions()); r.Next(); {
		if !d.SpanConfig.Reconciler.Checkpoint().IsEmpty() {

I'm not very familiar with the migrations framework, but how would things work if this is run on a node that isn't running the reconciliation job?

---

pkg/migration/migrations/migrations.go, line 156 at r5 (raw file):

		insertMissingPublicSchemaNamespaceEntry,
	),
	migration.NewTenantMigration(

Should this be a NewSystemMigration as well?

---

pkg/migration/migrations/migrate_span_configs_test.go, line 43 at r5 (raw file):

	tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
		ServerArgs: base.TestServerArgs{
			EnableSpanConfigs: true,

I took this for a spin, is this failing because we haven't turned on the cluster settings to create the reconciliation job?

[ajwerner]

Reviewed 1 of 1 files at r1, 7 of 7 files at r2, 7 of 7 files at r3, 9 of 9 files at r4, 13 of 13 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @irfansharif, and @nvanbenschoten)

---

pkg/kv/kvserver/queue_helpers_testutil.go, line 55 at r2 (raw file):

// enqueues any that need to be replicated.
func (s *Store) ForceReplicationScanAndProcess() error {
	return forceScanAndProcess(context.TODO(), s, s.replicateQueue.baseQueue)

IIRC elsewhere we've decided that for testing calls like this we just use context.Background.

---

pkg/migration/migrations/migrate_span_configs.go, line 34 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I'm not very familiar with the migrations framework, but how would things work if this is run on a node that isn't running the reconciliation job?

☝️, I think we need to persist the checkpoint as a pre-req to making this migration work. It's also a task we need to do anyway

---

pkg/migration/migrations/migrations.go, line 156 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this be a NewSystemMigration as well?

NewTenantMigration is run on the system tenant too

[irfansharif]

TFTRs! I thought this PR would be cleanly separable from the one enabling everything by default, but it isn't. The migrations here depend on the reconciliation job running, and that in turn depends on the infra being enabled. Given that, here's what I think we should do:

• Review the migration code here, in this PR. Happy to iterate further.
• Once rubber stamped, I'll land these commits as part of *: enable span configs by default #73876.

Does that work?

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

---

pkg/kv/kvserver/queue_helpers_testutil.go, line 55 at r2 (raw file):

Previously, ajwerner wrote…

IIRC elsewhere we've decided that for testing calls like this we just use context.Background.

Everything else in this file is using context.TODO(), just did it for that reason.

---

pkg/migration/migrations/migrate_span_configs.go, line 34 at r5 (raw file):

Previously, ajwerner wrote…

☝️, I think we need to persist the checkpoint as a pre-req to making this migration work. It's also a task we need to do anyway

Done. We're still not actually using the checkpoint, just persisting it. Using it is just slightly more involved (need a few reconciler changes to detect if system.{zones,descriptor} haven't already been GC-ed at the given timestamp). Given this PR is holding up a few others, could we get to it after?

---

pkg/migration/migrations/migrate_span_configs_test.go, line 43 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I took this for a spin, is this failing because we haven't turned on the cluster settings to create the reconciliation job?

Woops, that's right, had pulled it out of the other PR that enabled it by default so it's failing here.

[ajwerner]

I'm cool with that.

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

[irfansharif]

The remaining test failures now are expected, just because the infra is disabled but these last migrations need it. Do take a look at the job checkpointing stuff, I'm not really familiar with those libraries. I'll shift to just #73876 once you do.

[arulajmani]

for me. I have a mild aesthetic preference for reverting the commit that exposed the Checkpoint on the Reconciler now that we don't make use of it in the migration.

Reviewed 32 of 32 files at r6, 7 of 7 files at r7, 7 of 7 files at r8, 9 of 9 files at r9, 16 of 16 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @HonoreDB, and @nvanbenschoten)

---

pkg/migration/migrations/migrations.go, line 156 at r5 (raw file):

Previously, ajwerner wrote…

NewTenantMigration is run on the system tenant too

We don't do any work and return early for secondary tenants though, so maybe we could change this to just run on the system tenant.

[irfansharif]

Thanks! Closing this in favor of #73876 now.

[irfansharif]

Got rid of the beefy migration commit in this PR, just going the land the rest of the tiny ones.

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)