Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi-tenant: Relax restrictions on supported zone configuration changes from secondary tenants #75569

Closed
ajstorm opened this issue Jan 26, 2022 · 2 comments · Fixed by #111387
Assignees
Labels
A-multitenancy Related to multi-tenancy A-unification C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-multitenant Issues owned by the multi-tenant virtual team

Comments

@ajstorm
Copy link
Collaborator

ajstorm commented Jan 26, 2022

Originally we were of the opinion that zone configurations in secondary tenants would only be updated by the multi-region semantics. As a result, we restricted their use to only specify constraints which referenced the region or zone tiers:

for _, constraint := range toValidate {
switch constraint.Key {
case "zone":
_, found := zones[constraint.Value]
if !found {
return pgerror.Newf(
pgcode.CheckViolation,
"zone %q not found",
constraint.Value,
)
}
case "region":
_, found := regions[constraint.Value]
if !found {
return pgerror.Newf(
pgcode.CheckViolation,
"region %q not found",
constraint.Value,
)
}
default:
return errors.WithHint(pgerror.Newf(
pgcode.CheckViolation,
"invalid constraint attribute: %q",
constraint.Key,
),
`only "zone" and "region" are allowed`,
)
}
}

The potential problem with this approach is that if we're going to make multi-tenant deployments the default deployment pattern for CRDB (something which is still being debated), this restriction will break some users. We'll need to relax this restriction if/when we go down that route.

Jira issue: CRDB-12730

Epic CRDB-26686

@ajstorm ajstorm added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-multitenancy Related to multi-tenancy T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Jan 26, 2022
@arulajmani
Copy link
Collaborator

Originally we were of the opinion that zone configurations in secondary tenants would only be updated by the multi-region semantics. As a result, we restricted their use to only specify constraints which referenced the region or zone tiers:

It might be just me, but I found this phrasing slightly ambiguous. To clarify, we only restrict secondary tenants' to reference zone/region tiers in the constraints/voter constraints/lease preferences fields. However, they are allowed to alter all other fields, such as num_voters, gc.ttl etc.

The reason for this limitation is that secondary tenants do not have access to the NodeStatusServer, and in turn, the underlying store descriptors. So they can't validate constraints such as hardware or other locality tiers. It's an open question if secondary tenants should have a wider view than what the RegionServer offers today, but if we were to support this functionality, we'd need to expose more things on the RegionServer.

@yuzefovich yuzefovich self-assigned this Aug 16, 2023
@rafiss rafiss added T-multitenant Issues owned by the multi-tenant virtual team and removed T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Sep 5, 2023
@yuzefovich
Copy link
Member

@knz suggested that this issue has been effectively superseded by #100787 which is next on Jeff's plate. I'll make some minor adjustments to the code to highlight that issue in error messages.

craig bot pushed a commit that referenced this issue Sep 27, 2023
111321: sql: remove references to closed multi-tenancy issues r=yuzefovich a=yuzefovich

This commit updates a few places to remove now-closed multi-tenancy
issues. Two of those places are gated behind the system tenant, so the
issue doesn't matter, and they now use existing issue (even though it
doesn't really relate to the features in question). In one place we
choose to return "tenant cluster setting" not enabled error to make it
more clear (I was just bitten by this because we returned opaque
"unimplemented" error). In two other places in comments we update the
references to existing issues tracking the corresponding work.

Informs: #75569.
Epic: None

Release note: None

111375: enginepb: use struct equality comparison in MVCCValueHeader.IsEmpty r=RaduBerinde a=jbowens

Now that we're on Go 1.20, the regression is gone.

```
goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                                 │   old.txt   │              new.txt               │
                                                                 │   sec/op    │   sec/op     vs base               │
EncodeMVCCValue/header=empty/value=tombstone-24                    4.797n ± 0%   4.809n ± 0%  +0.26% (p=0.000 n=10)
EncodeMVCCValue/header=empty/value=short-24                        4.796n ± 0%   4.809n ± 0%  +0.27% (p=0.000 n=10)
EncodeMVCCValue/header=empty/value=long-24                         4.797n ± 0%   4.809n ± 0%  +0.26% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime/value=tombstone-24           54.16n ± 1%   53.56n ± 0%  -1.10% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime/value=short-24               56.71n ± 1%   56.02n ± 1%  -1.22% (p=0.001 n=10)
EncodeMVCCValue/header=local_walltime/value=long-24                1.371µ ± 3%   1.314µ ± 1%  -4.16% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=long-24        1.346µ ± 2%   1.323µ ± 4%       ~ (p=0.160 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=tombstone-24   57.56n ± 2%   57.09n ± 1%  -0.83% (p=0.029 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=short-24       60.61n ± 1%   60.05n ± 1%  -0.92% (p=0.001 n=10)
geomean                                                            50.62n        50.10n       -1.02%
```

Epic: none
Resolves #89199.
Release note: None

111377: go.mod: bump Pebble to 725ebe297867 r=itsbilal a=jbowens

```
725ebe29 db: fix excise test
d925b88b db: add SkipPoint iterator option
b5677d86 db: propagate Comparer into levelIter
3a1391d6 metamorphic: Ignore ErrCancelledCompaction in background errors
da739ee7 db: adds tests for virtual sstable async table stats calculation
02b87adf db: improvements to data_test.go
5500da0a .*: support async stats for virtual sstables
17c625cf db: Fix FormatMajorVersion check in version_set for VSSTs
4e353e51 metamorphic: fix block property collectors integration
75888967 Revert "db: add SkipPoint iterator option"
4df2ce80 metamorphic: Add testing for ingest splits
```

Epic: none
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
@craig craig bot closed this as completed in e8f27c3 Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy A-unification C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-multitenant Issues owned by the multi-tenant virtual team
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants