spanconfig: introduce new read-only system target #76721
Conversation
arulajmani
requested review from
ajwerner,
nvanbenschoten,
irfansharif and
adityamaru
|
This change is |
arulajmani
force-pushed
the
protectedts-kvaccessor-read-only-targets
branch
from
766ba8b to
245dc6a
Compare
There was a problem hiding this comment.
I agree this is a marked improvement over #76704 that tries to access these special records by referring to them through the underlying span encodings. I felt that PR was leaning for too much into the underlying storage encodings by disguising requests as spans despite having these explicit proto types that are unaware of the storage encoding. I felt we should either use the span encodings all throughout for these system targets (something I'm totally fine with!), or not at all.
Focusing on this PR, I'm not fully understanding the type being proposed. To remind myself, we have a few kinds of targets we want to set:
a. Host tenant setting a record that encompasses all tenants;
b. Host tenant setting records that encompasses specific, named tenants;
c. Secondary tenant setting a record that encompasses only the tenant itself.
We also want the ability to get these records. We're interested in:
- Host tenant getting a specific record (a or b);
- Host tenant getting all tenant-specific records (all b's);
- Secondary tenant getting their own record (c).
This PR tries to extend the Target proto representation to let us refer to a-c and 1-3. I see a few downsides in our current approach:
- It's using this "cluster" terminology, which is entirely different to the "cluster" terminology we're using in adjacent systems (pts, backups, where cluster refers to a tenant's logical cluster -- here it's more physical). I think "cluster" is just too ambiguous and best avoided.
- For a secondary tenant, an
EverythingTargetingTenantsandSpecificTenantare two way to get to the same thing. To me that indicates that the typing is not as distinguished as it could be. - We have these two caveats around constructing the proto itself:
- For
target_tenant_id: "This field can only be set in conjunction with the type targeting a SpecificTenant; it must be left unset for all other system target types". - For
EntireCluster: "Only the system tenant is allowed to target the entire cluster".
- For
These feel a bit confusing to me. Could we rework things to make use explicit union types instead of code comments enforcing that it be the right superposition of fields?
Here's my concrete suggestion that IMO addresses all of the above while still keeping things equally explicit and harder to hold wrong:
// SpanConfigTarget specifies the target of an associated span configuration.
message SpanConfigTarget {
oneof union {
Span span = 1;
SystemSpanConfigTarget system_span_config_target = 2;
}
}
message HostTarget {
oneof target {
TenantID tenant_id = 1; // settable only by host, to get/set (b).
bool all_tenants = 2; // settable only by host, to get (all b's)
}
}
// SystemSpanConfigTarget specifies the target of system span configurations.
message SystemSpanConfigTarget {
// non-optional, used to set/get (a) and (c), distinguished by id == host-tenant-id.
TenantID tenant_id = 1 [(gogoproto.nullable) = false];
// optional, settable only by host, to get/set (b) or (all b's).
HostTarget host_target = 2;
}Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @nvanbenschoten)
There was a problem hiding this comment.
Thanks for spelling your suggestion out. I think the difference in our preference boils down to the discussion we had when this type was introduced -- I wanted secondary tenants to explicitly say they're targeting themselves whereas you wanted to infer this information on the other side of the KV RPC.
If I'm understanding your stub correctly, if the host tenant constructs a SystemSpanConfigTarget without filling in the HostTarget field, it targets the entire physical cluster. When a secondary tenant constructs a SystemSpanConfigTarget with its tenant ID filled in it is targeting its logical cluster. Distinguishing this special casing in KV was something I wanted to deliberately avoid.
This also matters because these RPCs inform the spanconfig.SystemTarget type which is used in KV when all a,b,c system targets are present. There's value in conceptualizing this from the point of view of KV, instead of inferring cluster vs. tenant keyspace based on the tenantID field.
I'm replying in-line to some of the concerns you raised below. I think the alternative sacrifices some explicitness. The oneof type is neat for the RPC, but I think once you boil it down to the spanconfig.SystemTarget struct it is no easier to hold.
It's using this "cluster" terminology, which is entirely different to the "cluster" terminology we're using in adjacent systems (pts, backups, where cluster refers to a tenant's logical cluster -- here it's more physical). I think "cluster" is just too ambiguous and best avoided.
I don't view using referring to the physical cluster here is bad. Our span configuration targets are defined from the point of view of KV, and a "cluster" isn't ambiguous in that context. As an example, for regular span targets, we encode the tenant prefix on the client before issuing the request. Translating from a tenant's understanding of a "cluster" from an adjacent subsystem (logical) to KVs understanding (tenant vs. cluster) seems reasonable.
Speaking of adjacent subsystems, "cluster" doesn't mean the tenant's logical cluster to them -- it means a logical cluster for secondary tenants but a physical cluster for the system tenant (eg. Full cluster backup). The ambiguity in another subsystem's use of a term shouldn't detract us from using it in package spanconfig, considering it's the right one to use.
For a secondary tenant, an EverythingTargetingTenants and SpecificTenant are two way to get to the same thing. To me that indicates that the typing is not as distinguished as it could be.
What you call "typing is not as distinguished as it could be" I call "parity".
We have these two caveat around constructing the proto itself:
For target_tenant_id: "This field can only be set in conjunction with the type targeting a SpecificTenant; it must be left unset for all other system target types".
For EntireCluster: "Only the system tenant is allowed to target the entire cluster".
We have similar caveats in your suggestion too -- in the RPC you can get by with this oneof, but when representing it in the go type you've to add the same caveat "only one should be set" in a comment. You do have the "only the host tenant can set the host target" in the RPC.
Reviewable status:
|
Worth getting second opinions I suppose. I'm happy to retract my suggestion other reviewers feel differently. I'd rather err on the side of getting the proto type right because it's harder to change and appears prominently in our RPCs. With sanitized constructors for the Go type, I'm not as worried about not having native union types. I'm also happy
Perhaps where we differ is that this doesn't seem to me all that bad. This is what our semantics are for full cluster backups. Where are we "special casing in KV"? I'm not fully following your outline for logical vs. physical clusters either, but that's probably just be me. |
|
I agree it's worth bottoming out on the proto type and getting it right as these things are hard to change. It's worth noting that we only ever construct the proto from a |
arulajmani
force-pushed
the
protectedts-kvaccessor-read-only-targets
branch
from
245dc6a to
bc6eaa7
Compare
There was a problem hiding this comment.
Capturing discussions that @irfansharif and I had for others. (Thanks for exploring various alternatives possible here, btw!)
We spent some time exploring a few different ways to represent these RPCs and settled on what I've updated the PR with. We explored the initial suggestion that Irfan had above, but we discarded it because, as written, a target of the form:
SystemSpanConfigTarget{TenantID = 1}
would imply targeting the entire keyspace because it originated from the host, whereas a target of the form:
SystemSpanConfigTarget{TenantID = 2}
would imply targeting tenant 2's keyspace because it originated from a secondary tenant.
This special case was slightly surprising and something I wanted to deliberately avoid with the "cluster" and "tenant" target verbiage above. Considering this verbiage isn't particularly popular, we explored coming up with different names for these concepts. We agreed on changing "cluster target" to "entire keyspace target" and "tenant target" to "tenant keyspace target". I'm quite happy with where we've landed with these and made the change.
The new RPCs use a union type to represent the different types of system span configuration targets as opposed to commenting dependencies above fields. This captures the essence of the initial suggestion that Irfan had. PTAL!
Reviewable status:
There was a problem hiding this comment.
Thanks for the iteration here. LGTM.
We explored the initial suggestion that Irfan had above, but we discarded it
because, as written, a target of the form:
SystemSpanConfigTarget{TenantID = 1}would imply targeting the entire keyspace because it originated from the
host, whereas a target of the form:
SystemSpanConfigTarget{TenantID = 2}would imply targeting tenant 2's keyspace because it originated from a
secondary tenant.This special case was slightly surprising and something I wanted to
deliberately avoid
I'm not going to press my case any further here because these protos aren't persisted and it doesn't tie our hands going forward. That said I'll disagree that this is surprising and worth avoiding. This is the conception FULL CLUSTER BACKUPs have -- if issued by the host tenant, it encompasses everything. If issued by the secondary tenant, it encompasses just the tenant's keyspace. That feels an entirely coherent model to me -- certainly one good enough to extend to pkg/spanconfig. Everyone's virtualized except the host. The host tenant is special, and I feel it's fine to view it as such. As we unify our non-multitenant and multitenant architectures, I imagine us moving towards a world where there are no user tables in the host tenant keyspace; what the "host tenant" is today breaks into two -- a "truly host" tenant and just a vanilla secondary tenant (an "non-multitenant" cluster would have just a single secondary tenant). In that world, full cluster backups or other things issued by the "truly host" tenant would necessarily bleed across the entire keyspace. It's "admin" in every way. This conception is why I would've found it acceptable to switching on the tenant type (distinguished today only by ID) in the way this PR seems opposed.
pkg/roachpb/span_config.proto
Outdated
| // keyspace. | ||
| // | ||
| // Additionally, we also want each tenant to be able to fetch all system span | ||
| // configurations that it has installed. This is required during reconciliation |
There was a problem hiding this comment.
The detail about reconciliation, querying KV state, upsert/delete requests, etc. are not relevant to the type itself and can be confusing if unfamiliar. Perhaps just focusing on the read-only system target for the set of 2's is sufficient.
pkg/roachpb/span_config.proto
Outdated
| option (gogoproto.equal) = true; | ||
|
|
||
| oneof type { | ||
| TenantKeyspaceTarget specific_tenant_keyspace_target=1; |
There was a problem hiding this comment.
[nit] Spacing around '='.
pkg/roachpb/span_config.proto
Outdated
| // EntireKeyspaceTarget targets the entire keyspace (all ranges, including | ||
| // those belonging to secondary tenants). Only the host tenant is allowed | ||
| // to target the entire keyspace. | ||
| bool entire_keyspace_target=2; |
There was a problem hiding this comment.
Should these be booleans or actual message types? It could be confusing to have a non-nil entire_keyspace_target that's actually false.
pkg/roachpb/span_config.proto
Outdated
| // AllTenantKeyspaceTargetsSet is a read-only system target that | ||
| // encompasses all system targets that have been installed by the source | ||
| // tenant on specific tenant's keyspaces. | ||
| bool all_tenant_keyspace_targets_set=3; |
There was a problem hiding this comment.
How do you feel about reducing the amount of stuttering here and above. With SystemSpanConfigTarget and then SystemTargetType and then TenantKeyspaceTarget, it makes for a mouthful. Given these are nested messages anyway, we could s/SystemTargetType/Type.
We could also use the following names for the union types: EntireKeyspace | AllTenantKeyspaces | TenantKeyspace. I also don't mind EntireTenantKeyspace which just re-uses the same verbiage.
pkg/rpc/auth_tenant.go
Outdated
| @@ -315,11 +315,12 @@ func validateSpanConfigTarget( | |||
| return nil | |||
| } | |||
|
|
|||
| if target.TargetTenantID == nil { | |||
| return authErrorf("secondary tenants must explicitly target themselves") | |||
| if target.SystemTargetType.GetEntireKeyspaceTarget() { | |||
There was a problem hiding this comment.
If we step away from booleans messages, we could add a .IsEntireKeyspaceTarget helper. Ditto below.
| // system-target {source=1,target=20} | ||
| // system-target {source=1,target=1} | ||
| // system-target {source=20,target=20} | ||
| // system-target {source=1,everything-installed-on-tenants} |
There was a problem hiding this comment.
Reminder to update these based on new type naming.
| SourceTenantID: roachpb.SystemTenantID, | ||
| TargetTenantID: nil, | ||
| }) | ||
| clusterTarget := spanconfig.MakeTargetFromSystemTarget(spanconfig.MakeEntireKeyspaceTarget()) |
There was a problem hiding this comment.
Should we drop the "cluster" verbiage?
| spanconfig.MakeAllTenantKeyspaceTargetsSet(roachpb.SystemTenantID), | ||
| ), | ||
| }, | ||
| expErr: "cannot use read only system target .* as an update argument", |
There was a problem hiding this comment.
Should this error message say "delete"?
| func TestingEntireSpanConfigurationStateTargets() []Target { | ||
| return Targets{ | ||
| Target{ | ||
| span: keys.EverythingSpan, |
There was a problem hiding this comment.
Should we instead break this down to system targets and span targets instead of implicitly relying on the system target encodings? If we care about being explicit, we should add assertions in KV accessor to not allow spans to straddle the "special" keyspace, or explicitly drop those records from the results.
This patch is motivated by the host tenant's desire to fetch all system span configurations it has installed over secondary tenants without explicitly constructing targets for all tenant IDs in the system. This is handy for the host tenant's reconciliation job, which populates an in-memory view of all system span configurations to diff against the state implied by the schema. This inturn allows the host tenant to issue targeted updates/deletes. We introduce a new system target type to achieve this which allows a tenant to request all system span configurations it has installed that target tenant keyspaces. This is only ever consequential in the context of the host as only the host tenant can install system span configurations on other tenants. We also make this target read-only, in that, we disallow persisting its implied encoding in `system.span_configurations`. We add validation to the KVAccessor to this effect. While here, we also disallow creating span targets that overlap with the reserved system span configuration keyspace. We also improve the concepts around these system targets to talk about "tenant keyspaces' and "entire keyspace" as opposed to "tenant" and "cluster". Release note: None
arulajmani
force-pushed
the
protectedts-kvaccessor-read-only-targets
branch
from
bc6eaa7 to
02c3370
Compare
There was a problem hiding this comment.
You know how I feel about this special casing in full cluster backup and my desire to not let those concepts seep through to this subsystem. I hear you on the future where we might want to break up what the host tenant into two -- except, if anyone asks me, I'd still say backing up the entire keyspace should be called something other than "full cluster backup" that vanilla tenants do. Given the state of the world today, decoupling these concepts and avoiding special cases at this layer makes me happy.
TFTR! bors r+
Dismissed @irfansharif from 7 discussions.
Reviewable status:
pkg/roachpb/span_config.proto, line 188 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
The detail about reconciliation, querying KV state, upsert/delete requests, etc. are not relevant to the type itself and can be confusing if unfamiliar. Perhaps just focusing on the read-only system target for the set of 2's is sufficient.
Done.
pkg/roachpb/span_config.proto, line 217 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
[nit] Spacing around '='.
Done.
pkg/roachpb/span_config.proto, line 221 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Should these be booleans or actual message types? It could be confusing to have a non-nil entire_keyspace_target that's actually false.
I agree, a false value here is slightly confusing, switched.
pkg/roachpb/span_config.proto, line 225 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
How do you feel about reducing the amount of stuttering here and above. With SystemSpanConfigTarget and then SystemTargetType and then TenantKeyspaceTarget, it makes for a mouthful. Given these are nested messages anyway, we could s/SystemTargetType/Type.
We could also use the following names for the union types:
EntireKeyspace | AllTenantKeyspaces | TenantKeyspace. I also don't mind EntireTenantKeyspace which just re-uses the same verbiage.
Done.
pkg/rpc/auth_tenant.go, line 318 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
If we step away from booleans messages, we could add a
.IsEntireKeyspaceTargethelper. Ditto below.
Done.
pkg/spanconfig/target.go, line 277 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Should we instead break this down to system targets and span targets instead of implicitly relying on the system target encodings? If we care about being explicit, we should add assertions in KV accessor to not allow spans to straddle the "special" keyspace, or explicitly drop those records from the results.
It's a testing function, so I'm fine implicitly relying on internal encoding. If you were to compose this using system targets you'd need something that indicates "all tenant keyspace records installed by all secondary tenants". Considering we're never going to use such a target in non-test code, I'd rather rely on the internal encoding in this testing function instead of adding that concept.
Also, I already have assertions like you mentioned -- not in the KVAccessor but when constructing targets. For example, you can't make a span target that overlaps with our reserved keyspace.
pkg/spanconfig/spanconfigkvaccessor/kvaccessor_test.go, line 41 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Reminder to update these based on new type naming.
Done.
pkg/spanconfig/spanconfigkvaccessor/validation_test.go, line 28 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Should we drop the "cluster" verbiage?
Detritus, fixed.
pkg/spanconfig/spanconfigkvaccessor/validation_test.go, line 190 at r2 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Should this error message say "delete"?
We don't distinguish which list (upsert vs. delete) a malformed target is coming from when updating system span configs, I'm oaky as is for this error message.
|
bors r+ |
|
Build succeeded: |
This patch is motivated by the host tenant's desire to fetch all system
span configurations it has installed over secondary tenants without
explicitly constructing targets for all tenant IDs in the system. This
is handy for the host tenant's reconciliation job, which populates an
in-memory view of all system span configurations to diff against the
state implied by the schema. This in-turn allows the host tenant to issue
targeted updates/deletes.
We introduce a new system target type to achieve this which allows a
tenant to request all system span configurations it has installed that
target tenants. This is only ever consequential in the context of the
host as only the host tenant can install system span configurations on
other tenants. We also make this target read-only, in that, we disallow
persisting its implied encoding in
system.span_configurations. We addvalidation to the KVAccessor to this effect.
While here, we also disallow creating span targets that overlap with
the reserved system span configuration keyspace.
Release note: None
Alternative approach to #76704