-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
server,settings: properly cascade defaults for TenantReadOnly
TLDR: this patch ensures that virtual cluster servers observe changes made to TenantReadOnly settings via SET CLUSTER SETTING in the system interface, even when there is no override set via ALTER VIRTUAL CLUSTER SET CLUSTER SETTING. For example, after `SET CLUSTER SETTING kv.closed_timestamp.target_duration = '10s'` in the system interface, this value will show up via `SHOW CLUSTER SETTING` in a virtual cluster SQL session. This changes the way that settings are picked up in virtual cluster, as follows: 1. if there is an override specifically for this tenant's ID (in `tenant_settings`), use that. 2. otherwise, if there is an override for the pseudo-ID 0 (in `tenant_settings` still, set via `ALTER TENANT ALL SET CLUSTER SETTING`), then use that. 3. **NEW** otherwise, if the class is TenantReadOnly and there is a custom value in `system.settings`, set via a regular `SET CLUSTER SETTING` in the system tenant, then use that. 4. otherwise, use the global default set via the setting's `Register()` call. ---- Prior to this patch, TenantReadOnly settings as observed from virtual clusters were defined as the following priority order: 1. if there is an override specifically for this tenant's ID (in `tenant_settings`), use that. 2. otherwise, if there is an override for the pseudo-ID 0 (in `tenant_settings` still, set via `ALTER TENANT ALL SET CLUSTER SETTING`), then use that. 3. otherwise, use the global default set via the setting's `Register()` call. Remarkably, this did not pick up any changes made via a plain `SET CLUSTER SETTING` statement via the system interface, which only modifies this setting's value in `system.settings` (thus not `tenant_settings`). This situation was problematic in two ways. To start, settings like `kv.closed_timestamp.target_duration` cannot be set solely in `system.tenant_settings`; they are also used in the storage layer and so must also be picked up from changes in `system.settings`. For these settings, it is common for operators to just issue the plain `SET CLUSTER SETTING` statement (to update `system.settings`) and simply forget to _also_ run `ALTER TENANT ALL SET CLUSTER SETTING`. This mistake is nearly unavoidable and would result in incoherent behavior, where the storage layer would use the customized value and virtual clusters would use the registered global default. The second problem is in mixed-version configurations, where the storage layer runs version N+1 and the SQL service runs version N of the executable. If the registered global default changes from version N to N+1, the SQL service would not properly pick up the new default defined in version N+1 of the storage layer. This patch fixes both problems as follows: - it integrates changes to TenantReadOnly settings observed in `system.settings`, to the watcher that tracks changes to `system.tenant_settings`. When a TenantReadOnly setting is present in the former but not the latter, a synthetic override is added. - it also initializes synthetic overrides for all the TenantReadOnly settings upon server initialization, from the registered global default, so that virtual cluster servers always pick up the storage layer's default as override. Release note: None
- Loading branch information
Showing
8 changed files
with
359 additions
and
14 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,206 @@ | ||
| // Copyright 2023 The Cockroach Authors. | ||
| // | ||
| // Use of this software is governed by the Business Source License | ||
| // included in the file licenses/BSL.txt. | ||
| // | ||
| // As of the Change Date specified in that file, in accordance with | ||
| // the Business Source License, use of this software will be governed | ||
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package integration_tests | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/cockroachdb/cockroach/pkg/base" | ||
| "github.com/cockroachdb/cockroach/pkg/settings" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/skip" | ||
| "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" | ||
| "github.com/cockroachdb/cockroach/pkg/util/leaktest" | ||
| "github.com/cockroachdb/cockroach/pkg/util/log" | ||
| "github.com/cockroachdb/errors" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestSettingDefaultPropagationReadOnly1 runs one of 4 invocations of | ||
| // `RunSettingDefaultPropagationTest`. The test is split 4-ways to | ||
| // avoid timeouts in CI and increased test parallelism. | ||
| func TestSettingDefaultPropagationReadOnly1(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
|
|
||
| runSettingDefaultPropagationTest(t, roS, true) | ||
| } | ||
|
|
||
| // TestSettingDefaultPropagationReadOnly2 runs one of 4 invocations of | ||
| // `RunSettingDefaultPropagationTest`. The test is split 4-ways to | ||
| // avoid timeouts in CI and increased test parallelism. | ||
| func TestSettingDefaultPropagationReadOnly2(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
|
|
||
| runSettingDefaultPropagationTest(t, roS, false) | ||
| } | ||
|
|
||
| // TestSettingDefaultPropagationReadWrite1 runs one of 4 invocations | ||
| // of `RunSettingDefaultPropagationTest`. The test is split 4-ways to | ||
| // avoid timeouts in CI and increased test parallelism. | ||
| func TestSettingDefaultPropagationReadWrite1(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
|
|
||
| runSettingDefaultPropagationTest(t, rwS, true) | ||
| } | ||
|
|
||
| // TestSettingDefaultPropagationReadWrite2 runs one of 4 invocations | ||
| // of `RunSettingDefaultPropagationTest`. The test is split 4-ways to | ||
| // avoid timeouts in CI and increased test parallelism. | ||
| func TestSettingDefaultPropagationReadWrite2(t *testing.T) { | ||
| defer leaktest.AfterTest(t)() | ||
|
|
||
| runSettingDefaultPropagationTest(t, rwS, false) | ||
| } | ||
|
|
||
| var roS = settings.RegisterStringSetting(settings.TenantReadOnly, "tenant.read.only", "desc", "initial") | ||
| var rwS = settings.RegisterStringSetting(settings.TenantWritable, "tenant.writable", "desc", "initial") | ||
|
|
||
| // runSettingDefaultPropagationTest is a test helper. | ||
| func runSettingDefaultPropagationTest( | ||
| t *testing.T, setting *settings.StringSetting, setSystemBefore bool, | ||
| ) { | ||
| defer log.Scope(t).Close(t) | ||
|
|
||
| // This test currently takes >1 minute. | ||
| // | ||
| // TODO(multi-tenant): make it faster. Currently most of the | ||
| // overhead is in tenant service initialization and shutdown | ||
| // (strangely, especially shutdown). | ||
| skip.UnderShort(t) | ||
|
|
||
| skip.UnderRace(t, "slow test") | ||
| skip.UnderStress(t, "slow test") | ||
|
|
||
| ctx := context.Background() | ||
| s := serverutils.StartServerOnly(t, base.TestServerArgs{ | ||
| DefaultTestTenant: base.TestControlsTenantsExplicitly, | ||
| }) | ||
| defer s.Stopper().Stop(ctx) | ||
|
|
||
| sysDB := sqlutils.MakeSQLRunner(s.SystemLayer().SQLConn(t, "")) | ||
| sysDB.Exec(t, "SELECT crdb_internal.create_tenant($1, 'test')", serverutils.TestTenantID().ToUint64()) | ||
| // Speed up the tests. | ||
| sysDB.Exec(t, "SET CLUSTER SETTING kv.closed_timestamp.target_duration = '10ms'") | ||
|
|
||
| expectation := func(setting settings.Setting, sysOverride, tenantOverride, tenantAllOverride string) string { | ||
| if tenantOverride != "" { | ||
| // ALTER TENANT xxx SET CLUSTER SETTING -> highest precedence. | ||
| return tenantOverride | ||
| } | ||
| if tenantAllOverride != "" { | ||
| // ALTER TENANT ALL SET CLUSTER SETTING, override is used | ||
| // only if there is no tenant-specific override. | ||
| return tenantAllOverride | ||
| } | ||
| // No tenant override. What is the default? | ||
| // For TenantReadOnly, if there is a custom value in the | ||
| // system interface, that becomes the default. | ||
| if setting.Class() == settings.TenantReadOnly && sysOverride != "" { | ||
| return sysOverride | ||
| } | ||
| // Otherwise, fall back to the default. | ||
| return "initial" | ||
| } | ||
|
|
||
| key := setting.InternalKey() | ||
| testutils.RunTrueAndFalse(t, "set-override-before", func(t *testing.T, setOverrideBefore bool) { | ||
| testutils.RunTrueAndFalse(t, "set-override-all-before", func(t *testing.T, setOverrideAllBefore bool) { | ||
| testutils.RunTrueAndFalse(t, "set-system-after", func(t *testing.T, setSystemAfter bool) { | ||
| testutils.RunTrueAndFalse(t, "set-override-after", func(t *testing.T, setOverrideAfter bool) { | ||
| testutils.RunTrueAndFalse(t, "set-override-all-after", func(t *testing.T, setOverrideAllAfter bool) { | ||
| var sysOverride string | ||
| if setSystemBefore { | ||
| t.Logf("before start: system custom value via SET CLUSTER SETTING") | ||
| sysOverride = "before-sys" | ||
| sysDB.Exec(t, fmt.Sprintf("SET CLUSTER SETTING %s = '%s'", key, sysOverride)) | ||
| } else { | ||
| sysDB.Exec(t, fmt.Sprintf("RESET CLUSTER SETTING %s", key)) | ||
| } | ||
| var tenantAllOverride string | ||
| if setOverrideAllBefore { | ||
| t.Logf("before start: override via ALTER VIRTUAL CLUSTER ALL SET CLUSTER SETTING") | ||
| tenantAllOverride = "before-all" | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER ALL SET CLUSTER SETTING %s = '%s'", key, tenantAllOverride)) | ||
| } else { | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER ALL RESET CLUSTER SETTING %s", key)) | ||
| } | ||
| var tenantOverride string | ||
| if setOverrideBefore { | ||
| t.Logf("before start: override via ALTER VIRTUAL CLUSTER test SET CLUSTER SETTING") | ||
| tenantOverride = "before-specific" | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER test SET CLUSTER SETTING %s = '%s'", key, tenantOverride)) | ||
| } else { | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER test RESET CLUSTER SETTING %s", key)) | ||
| } | ||
|
|
||
| t.Logf("starting secondary tenant service") | ||
| ten, err := s.TenantController().StartTenant(ctx, base.TestTenantArgs{ | ||
| TenantID: serverutils.TestTenantID(), | ||
| }) | ||
| require.NoError(t, err) | ||
| defer ten.AppStopper().Stop(ctx) | ||
|
|
||
| expected := expectation(setting, sysOverride, tenantOverride, tenantAllOverride) | ||
| t.Logf("expecting setting set to %q", expected) | ||
| val := setting.Get(&ten.ClusterSettings().SV) | ||
| t.Logf("setting currenly at %q", val) | ||
| if val != expected { | ||
| t.Errorf("expected %q, got %q", expected, val) | ||
| } | ||
|
|
||
| t.Logf("changing configuration") | ||
| if setSystemAfter { | ||
| t.Logf("after start: setting system custom value via SET CLUSTER SETTING") | ||
| sysOverride = "after-sys" | ||
| sysDB.Exec(t, fmt.Sprintf("SET CLUSTER SETTING %s = '%s'", key, sysOverride)) | ||
| } else { | ||
| t.Logf("after start: resetting system custom value with RESET CLUSTER SETTING") | ||
| sysOverride = "" | ||
| sysDB.Exec(t, fmt.Sprintf("RESET CLUSTER SETTING %s", key)) | ||
| } | ||
| if setOverrideAllAfter { | ||
| t.Logf("after start: override via ALTER VIRTUAL CLUSTER ALL SET CLUSTER SETTING") | ||
| tenantAllOverride = "after-all" | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER ALL SET CLUSTER SETTING %s = '%s'", key, tenantAllOverride)) | ||
| } else { | ||
| t.Logf("after start: resetting system custom value with ALTER VIRTUAL CLUSTER ALL RESET CLUSTER SETTING") | ||
| tenantAllOverride = "" | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER ALL RESET CLUSTER SETTING %s", key)) | ||
| } | ||
| if setOverrideAfter { | ||
| t.Logf("after start: override via ALTER VIRTUAL CLUSTER test SET CLUSTER SETTING") | ||
| tenantOverride = "after-specific" | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER test SET CLUSTER SETTING %s = '%s'", key, tenantOverride)) | ||
| } else { | ||
| t.Logf("after start: resetting system custom value with ALTER VIRTUAL CLUSTER test RESET CLUSTER SETTING") | ||
| tenantOverride = "" | ||
| sysDB.Exec(t, fmt.Sprintf("ALTER VIRTUAL CLUSTER test RESET CLUSTER SETTING %s", key)) | ||
| } | ||
|
|
||
| expected = expectation(setting, sysOverride, tenantOverride, tenantAllOverride) | ||
| t.Logf("expecting setting set to %q", expected) | ||
| testutils.SucceedsSoon(t, func() error { | ||
| val := setting.Get(&ten.ClusterSettings().SV) | ||
| t.Logf("setting currenly at %q", val) | ||
| if val != expected { | ||
| return errors.Newf("expected %q, got %q", expected, val) | ||
| } | ||
| return nil | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.