spanconfig: teach the KVAccessor about system span configurations #76414

arulajmani · 2022-02-11T01:17:21Z

First 3 commits are from #76219, this one's quite small -- mostly just tests.

This patch teaches the KVAccessor to update and get system span
configurations.

Release note: None

cockroach-teamcity · 2022-02-11T01:17:31Z

This change is

irfansharif

LGTM (looked at only the last commit here).

irfansharif · 2022-02-14T20:41:55Z

pkg/spanconfig/spanconfigtestutils/utils.go

@@ -32,6 +32,10 @@ import (
 // and "end" keys.
 var spanRe = regexp.MustCompile(`^\[(\w+),\s??(\w+)\)$`)

+// systemTargetRe matches strings of the form
+// "{source=(<id>|system),target=(<id>|system)}".


Simplify the grammar to drop "system". There are already too many "system" in our vocabulary and here used for two different meanings ("system tenant" and "system target"); there's no difference between tenant-id==1 and system, and the test cases render them differently. It would help simplify your parser below. I'd be ok exporting it now too (ParseSystemTarget) like we do for everything else -- we are going to use something like it in spanconfig.store tests shortly anyway.

irfansharif · 2022-02-14T20:44:58Z

pkg/spanconfig/spanconfigtestutils/utils.go

+	default:
+		t.Fatalf("expected %s to match span or system target regex", target)
+	}
+	return spanconfig.Target{}


Could panic instead of this confusing return.

irfansharif · 2022-02-14T20:45:40Z

pkg/spanconfig/spanconfigtestutils/utils.go

+	case target.IsSystemTarget():
+		return printSystemTarget(target.GetSystemTarget())
+	default:
+		t.Fatalf("unknown traget type")


Typo: traget. Could panic below instead of the empty string return.

irfansharif · 2022-02-14T20:48:27Z

pkg/spanconfig/spanconfigkvaccessor/validation_test.go

+			// Duplicate in toDelete with some span targets.
+			toDelete: []spanconfig.Target{
+				spanconfig.MakeTargetFromSpan(roachpb.Span{Key: roachpb.Key("a"), EndKey: roachpb.Key("c")}),
+				makeTenantClusterTarget(roachpb.SystemTenantID.InternalValue),


(Applies to earlier commits as well I think) Should we be using InternalValue? The name and surrounding comment suggests we shouldn't:

cockroach/pkg/roachpb/data.proto

Lines 752 to 753 in 5a18c3a

// InternalValue should not be set or accessed directly; use ToUint64.

uint64 id = 1 [(gogoproto.customname) = "InternalValue"];

Could also make our helper take the TenantID type instead. If we got rid of type casting errors like suggested in the other review, some of this code could be simplified/inlined given there'd be no error handling needed.

irfansharif · 2022-02-14T20:51:36Z

pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs

+ok
+
+kvaccessor-get
+system-target {source=system,target=10}


How exactly are we expecting the reconciler to get all system targets down the line? The API here suggests that each key is to be retrieved by naming it one-by-one. Will that continue to be the case?

arulajmani

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

pkg/spanconfig/spanconfigtestutils/utils.go, line 36 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Simplify the grammar to drop "system". There are already too many "system" in our vocabulary and here used for two different meanings ("system tenant" and "system target"); there's no difference between tenant-id==1 and system, and the test cases render them differently. It would help simplify your parser below. I'd be ok exporting it now too (ParseSystemTarget) like we do for everything else -- we are going to use something like it in spanconfig.store tests shortly anyway.

I agree, done. I only added it because that's how we stringify tenant IDs, but I instead ended up changing the String method on system targets instead. As for making it public, I agree we'll need it very soon, but I'll just do it then.

pkg/spanconfig/spanconfigtestutils/utils.go, line 98 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Could panic instead of this confusing return.

Done.

pkg/spanconfig/spanconfigtestutils/utils.go, line 263 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Typo: traget. Could panic below instead of the empty string return.

Done.

pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs, line 72 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

How exactly are we expecting the reconciler to get all system targets down the line? The API here suggests that each key is to be retrieved by naming it one-by-one. Will that continue to be the case?

I was going to follow this PR with one that adds another method to the KVAccessor, something of the form GetAllHostTenantInstalledSystemSpanConfigs which wouldn't be accessible on the connector like we talked offline.

pkg/spanconfig/spanconfigkvaccessor/validation_test.go, line 155 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Could also make our helper take the TenantID type instead. If we got rid of type casting errors like suggested in the other review, some of this code could be simplified/inlined given there'd be no error handling needed.

This just goes away once I rebased from the last PR. Good call on the internal value stuff -- tacked on a commit to fix that.

irfansharif · 2022-02-15T17:39:39Z

pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs

+
+# Test with an empty slate.
+kvaccessor-get
+system-target {cluster}


I'd recommend getting rid of this part of the grammar too. It's not adding anything that isn't represented by {source=1}, something our grammar can already represent. I also have misgivings about spanconfig.MakeClusterTarget. For one, despite the same name, it's wholly different conceptually to ptpb.MakeClusterTarget. We chatted a bit offline about this with @adityamaru, regardless of where we land, simplifying the testdata grammar itself is a good thing IMO.

irfansharif · 2022-02-15T17:41:45Z

pkg/spanconfig/spanconfigtestutils/utils.go

+			line = strings.TrimPrefix(line, systemTargetPrefix)
+		default:
+			t.Fatalf(
+				"malformed line %q, expected to find spanPrefix %q or system target prefix %q",


[nit] "expected to find %q or %q prefix" would avoid stuttering in your error message.

irfansharif · 2022-02-15T17:44:42Z

pkg/spanconfig/systemtarget.go

@@ -43,15 +43,36 @@ type SystemTarget struct {

 // MakeSystemTarget constructs, validates, and returns a new SystemTarget.
 func MakeSystemTarget(
-	sourceTenantID roachpb.TenantID, targetTenantID *roachpb.TenantID,
+	sourceTenantID roachpb.TenantID, targetTenantID roachpb.TenantID,


For future PRs: worth reconsidering IMO these constructors, in addition to MakeClusterTarget. My preference:

MakeClusterTarget(roachpb.TenantID) SystemTarget // takes in what you call today "source" MakeTenantsTarget([]roachpb.TenantID) []SystemTarget // doesn't take in the source, has to be system tenant, but takes in dests

Pick whatever names you want for these.

What we ended up with for the time being is:

MakeClusterTarget() SystemTarget // returns a system target with only the sourceTarget set to the system tenant ID MakeTenantTarget(source, target roachpb.TenantID)

Then the SQLTranslator will:

// Aggregate cluster target protections for the tenant. clusterProtections := ptsStateReader.getProtectionPoliciesForCluster() if len(clusterProtections) != 0 { var systemTarget spanconfig.SystemTarget var err error if sourceTenantID == roachpb.SystemTenantID { systemTarget = spanconfig.MakeClusterTarget() } else { systemTarget, err = spanconfig.MakeTargetTenant(sourceTenantID, sourceTenantID) if err != nil { return nil, err } } clusterSystemRecord := spanconfig.Record{ Target: spanconfig.MakeTargetFromSystemTarget(systemTarget), Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ProtectionPolicies: clusterProtections}}} records = append(records, clusterSystemRecord) } // Aggregate tenant target protections. tenantProtections := ptsStateReader.getProtectionPoliciesForTenants() for _, protection := range tenantProtections { tenantProtection := protection tenantSystemRecord := spanconfig.Record{ Target: spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeTenantTarget( sourceTenantID, tenantProtection.tenantID)), Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ ProtectionPolicies: tenantProtection.protections}}, } records = append(records, tenantSystemRecord) }

But we could make it simpler without complicating the client side:

// Aggregate cluster target protections for the tenant. clusterProtections := ptsStateReader.getProtectionPoliciesForCluster() if len(clusterProtections) != 0 { clusterSystemRecord := spanconfig.Record{ Target: spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeClusterTarget(sourceTenantID)), Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ProtectionPolicies: clusterProtections}}} records = append(records, clusterSystemRecord) } // Aggregate tenant target protections. tenantProtections := ptsStateReader.getProtectionPoliciesForTenants() for _, protection := range tenantProtections { tenantProtection := protection tenantSystemRecord := spanconfig.Record{ Target: spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeTenantTarget(tenantProtection.tenantID)), Config: roachpb.SpanConfig{GCPolicy: roachpb.GCPolicy{ ProtectionPolicies: tenantProtection.protections}}, } records = append(records, tenantSystemRecord) }

If we want to simplify the client side logic, it might be worth considering changing the ptsStateReader to return ptpb.Targets instead of protection policies like it does today. Then, we could write conversions to/from ptpb.Targets to spanconfig.Targets.

It's not about simplifying client code, it's about concepts. We have a cluster concept in backup/pts, and we have another cluster concept here that means a wholly different thing. They both have the same MakeClusterTarget signature too. It's a bit difficult to see why.

This patch teaches the KVAccessor to update and get system span configurations. Release note: None

Release note: None

arulajmani

TFTR!

bors r+

Dismissed @irfansharif from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @irfansharif, and @nvanbenschoten)

pkg/spanconfig/spanconfigtestutils/utils.go, line 157 at r8 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] "expected to find %q or %q prefix" would avoid stuttering in your error message.

Done.

pkg/spanconfig/spanconfigkvaccessor/testdata/system_span_configs, line 5 at r8 (raw file):

Previously, irfansharif (irfan sharif) wrote…

I'd recommend getting rid of this part of the grammar too. It's not adding anything that isn't represented by {source=1}, something our grammar can already represent. I also have misgivings about spanconfig.MakeClusterTarget. For one, despite the same name, it's wholly different conceptually to ptpb.MakeClusterTarget. We chatted a bit offline about this with @adityamaru, regardless of where we land, simplifying the testdata grammar itself is a good thing IMO.

I agree that the cluster target is just {source=1}, but I think having this {cluster} here does a bit for readability.

@adityamaru captured where the discussion around MakeClusterTarget went above.

craig · 2022-02-15T19:42:19Z

Build succeeded:

GitHub CI (Cockroach)

arulajmani requested review from ajwerner, nvanbenschoten, irfansharif and adityamaru February 11, 2022 01:17

arulajmani requested a review from a team as a code owner February 11, 2022 01:17

arulajmani requested a review from a team February 11, 2022 01:17

arulajmani force-pushed the protectedts-kvaccessor branch from da946c9 to d8d71c1 Compare February 11, 2022 01:22

arulajmani mentioned this pull request Feb 11, 2022

spanconfig: modify existing RPCs to work with system span configurations #76219

Merged

arulajmani force-pushed the protectedts-kvaccessor branch from d8d71c1 to 44c2d1f Compare February 11, 2022 04:22

arulajmani mentioned this pull request Feb 11, 2022

protectedts: rework protected timestamp storage for multi-tenant model #73727

Closed

23 tasks

irfansharif approved these changes Feb 14, 2022

View reviewed changes

arulajmani force-pushed the protectedts-kvaccessor branch from 44c2d1f to 66916bf Compare February 15, 2022 15:57

arulajmani commented Feb 15, 2022

View reviewed changes

arulajmani force-pushed the protectedts-kvaccessor branch from 66916bf to 51246a3 Compare February 15, 2022 16:41

irfansharif approved these changes Feb 15, 2022

View reviewed changes

arulajmani force-pushed the protectedts-kvaccessor branch from 51246a3 to fd5aa86 Compare February 15, 2022 18:08

arulajmani added 2 commits February 15, 2022 13:14

spanconfig: teach the KVAccessor about system span configurations

c034e26

This patch teaches the KVAccessor to update and get system span configurations. Release note: None

spanconfig: replace all usages of raw InternalValue with ToUint64

f78be51

Release note: None

arulajmani force-pushed the protectedts-kvaccessor branch from fd5aa86 to f78be51 Compare February 15, 2022 18:14

arulajmani commented Feb 15, 2022

View reviewed changes

adityamaru mentioned this pull request Feb 15, 2022

spanconfigsqltranslator: emit all SystemTarget span configs when required #76606

Merged

craig bot merged commit 3cb7eb0 into cockroachdb:master Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spanconfig: teach the KVAccessor about system span configurations #76414

spanconfig: teach the KVAccessor about system span configurations #76414

arulajmani commented Feb 11, 2022 •

edited

Loading

cockroach-teamcity commented Feb 11, 2022

irfansharif left a comment

irfansharif Feb 14, 2022

irfansharif Feb 14, 2022

irfansharif Feb 14, 2022

irfansharif Feb 14, 2022

irfansharif Feb 14, 2022

irfansharif Feb 14, 2022

arulajmani left a comment

irfansharif Feb 15, 2022

irfansharif Feb 15, 2022

irfansharif Feb 15, 2022 •

edited

Loading

adityamaru Feb 15, 2022 •

edited

Loading

irfansharif Feb 15, 2022

arulajmani Feb 15, 2022

irfansharif Feb 15, 2022

arulajmani left a comment

craig bot commented Feb 15, 2022

	// InternalValue should not be set or accessed directly; use ToUint64.
	uint64 id = 1 [(gogoproto.customname) = "InternalValue"];

spanconfig: teach the KVAccessor about system span configurations #76414

spanconfig: teach the KVAccessor about system span configurations #76414

Conversation

arulajmani commented Feb 11, 2022 • edited Loading

cockroach-teamcity commented Feb 11, 2022

irfansharif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irfansharif Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

adityamaru Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

craig bot commented Feb 15, 2022

arulajmani commented Feb 11, 2022 •

edited

Loading

irfansharif Feb 15, 2022 •

edited

Loading

adityamaru Feb 15, 2022 •

edited

Loading