spanconfigreconciler{ccl}: apply system span config diffs to the store #76948

adityamaru · 2022-02-23T21:54:32Z

This change teaches the reconciler about system span configs. Concretely,
we make the following changes:

A full reconciliation when checking for existing span configurations now
asks for SpanConfigs corresponding to the SystemTargets relevant to the tenant.

For the host tenant this includes the SystemTarget for the entire-keyspace as
well as the SystemTarget for span configs installed by the host tenant on its
tenant keyspace, and on other secondary tenant keyspaces.

For secondary tenants this only includes the SystemTarget for span configs installed
by it on its own tenant keyspace.

During incremental reconciliation, before applying our updates to the Store,
we now also check for "missing protected timestamp system targets". These correspond
to protected timestamp records that target a Cluster or a Tenant but no longer
exist in the system.protected_ts_records table as they have been released by the client.
For every such unique missing system target we apply a spanconfig.Deletion to the Store.

In order to make the above possible, this change moves the ptsStateReader from the
spanconfigsqltranslator package, to the top level spanconfig package.

Informs: #73727

Release note: None

cockroach-teamcity · 2022-02-23T21:54:40Z

This change is

arulajmani

This should be good to go once we've enhanced the datadriven tests to let secondary tenants set PTS records on their "logical" cluster as well. Exciting to see this entire thing so close to being wired up end to end!

Reviewed 17 of 18 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @irfansharif)

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/protectedts, line 15 at r1 (raw file):

----

mutations

nit: you could reconcile, discard mutations, and then do the protection you do above to make this a bit more readable.

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/protectedts, line 128 at r1 (raw file):

release record-id=5
----

nit: Let's print out the mutations here as well before releasing record-id=1?

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/multitenant/protectedts, line 116 at r1 (raw file):

...

# Release all the protected timestamp records from the host tenant.

In addition to setting/releasing records from the host tenant, we should extend this test to do so as a tenant as well. How about adding an optional tenant arg to both protect and release?

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/datadriven_test.go, line 219 at r1 (raw file):

				return spanconfigtestutils.MaybeLimitAndOffset(t, d, "...", lines)

			case "protect":

nit: we should add this thing to the comment explaining the datadriven language above as well.

pkg/spanconfig/protectedts_state_reader.go, line 78 at r1 (raw file):

// GetProtectionPoliciesForTenant returns all the protected timestamps that
// apply to a particular tenant's keyspace.
func (p *ProtectedTimestampStateReader) GetProtectionPoliciesForTenant(

Looks like we usage of this is of the form if len(GetProtectionPolicies) == 0 -- consider changing this thing to something like ProtectionExistsForTenant(roachpb.TenantID) bool

adityamaru

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

pkg/ccl/spanconfigccl/spanconfigreconcilerccl/testdata/protectedts, line 15 at r1 (raw file):