From 6a737cd871f1831e249140d2cbf03bffd557effc Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 20 Sep 2023 23:48:36 +0200 Subject: [PATCH 01/10] settingswatcher: simplify Release note: None --- pkg/server/settingswatcher/settings_watcher.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/server/settingswatcher/settings_watcher.go b/pkg/server/settingswatcher/settings_watcher.go index 720a400cb6e2..1a589a6cf128 100644 --- a/pkg/server/settingswatcher/settings_watcher.go +++ b/pkg/server/settingswatcher/settings_watcher.go @@ -345,8 +345,6 @@ type settingsValue struct { tombstone bool } -const versionSettingKey = "version" - // set the current value of a setting. func (s *SettingsWatcher) setLocked( ctx context.Context, @@ -361,7 +359,7 @@ func (s *SettingsWatcher) setLocked( // bootstrap the initial cluster version on tenant startup. In all other // instances, this code should no-op (either because we're in the system // tenant, or because the new version <= old version). - if key == versionSettingKey && !s.codec.ForSystemTenant() { + if key == clusterversion.KeyVersionSetting && !s.codec.ForSystemTenant() { var newVersion clusterversion.ClusterVersion oldVersion := s.settings.Version.ActiveVersionOrEmpty(ctx) if err := protoutil.Unmarshal([]byte(val.Value), &newVersion); err != nil { @@ -409,7 +407,7 @@ func (s *SettingsWatcher) updateOverrides(ctx context.Context) (updateCh <-chan defer s.mu.Unlock() for key, val := range newOverrides { - if key == versionSettingKey { + if key == clusterversion.KeyVersionSetting { var newVersion clusterversion.ClusterVersion if err := protoutil.Unmarshal([]byte(val.Value), &newVersion); err != nil { log.Warningf(ctx, "ignoring invalid cluster version: %s - %v\n"+ From 3877ccf1cedef1162a7fac39999cfaa369e95ac2 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sun, 17 Sep 2023 17:29:10 +0200 Subject: [PATCH 02/10] server: move settings tests to new package settings/integration_tests Release note: None --- pkg/BUILD.bazel | 2 ++ pkg/server/BUILD.bazel | 1 - pkg/settings/integration_tests/BUILD.bazel | 23 ++++++++++++++ pkg/settings/integration_tests/main_test.go | 30 +++++++++++++++++++ .../integration_tests}/settings_test.go | 22 ++++---------- 5 files changed, 61 insertions(+), 17 deletions(-) create mode 100644 pkg/settings/integration_tests/BUILD.bazel create mode 100644 pkg/settings/integration_tests/main_test.go rename pkg/{server => settings/integration_tests}/settings_test.go (91%) diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index a7b490394804..9ad8388e6454 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -316,6 +316,7 @@ ALL_TESTS = [ "//pkg/server/tenantsettingswatcher:tenantsettingswatcher_test", "//pkg/server/tracedumper:tracedumper_test", "//pkg/server:server_test", + "//pkg/settings/integration_tests:integration_tests_test", "//pkg/settings/lint:lint_test", "//pkg/settings/rulebasedscanner:rulebasedscanner_test", "//pkg/settings:settings_test", @@ -1597,6 +1598,7 @@ GO_TARGETS = [ "//pkg/server:server", "//pkg/server:server_test", "//pkg/settings/cluster:cluster", + "//pkg/settings/integration_tests:integration_tests_test", "//pkg/settings/lint:lint_test", "//pkg/settings/rulebasedscanner:rulebasedscanner", "//pkg/settings/rulebasedscanner:rulebasedscanner_test", diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index c8303ad2f9b7..47794696e324 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -455,7 +455,6 @@ go_test( "server_systemlog_gc_test.go", "server_test.go", "settings_cache_test.go", - "settings_test.go", "span_stats_server_test.go", "span_stats_test.go", "statements_test.go", diff --git a/pkg/settings/integration_tests/BUILD.bazel b/pkg/settings/integration_tests/BUILD.bazel new file mode 100644 index 000000000000..7bbc72b18446 --- /dev/null +++ b/pkg/settings/integration_tests/BUILD.bazel @@ -0,0 +1,23 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") + +go_test( + name = "integration_tests_test", + srcs = [ + "main_test.go", + "settings_test.go", + ], + args = ["-test.timeout=295s"], + deps = [ + "//pkg/base", + "//pkg/security/securityassets", + "//pkg/security/securitytest", + "//pkg/server", + "//pkg/settings", + "//pkg/testutils", + "//pkg/testutils/serverutils", + "//pkg/testutils/sqlutils", + "//pkg/util/leaktest", + "//pkg/util/log", + "@com_github_cockroachdb_errors//:errors", + ], +) diff --git a/pkg/settings/integration_tests/main_test.go b/pkg/settings/integration_tests/main_test.go new file mode 100644 index 000000000000..62df1fa103cf --- /dev/null +++ b/pkg/settings/integration_tests/main_test.go @@ -0,0 +1,30 @@ +// 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 ( + "os" + "testing" + + "github.com/cockroachdb/cockroach/pkg/security/securityassets" + "github.com/cockroachdb/cockroach/pkg/security/securitytest" + "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" +) + +func TestMain(m *testing.M) { + securityassets.SetLoader(securitytest.EmbeddedAssets) + serverutils.InitTestServerFactory(server.TestServerFactory) + + os.Exit(m.Run()) +} + +//go:generate ../util/leaktest/add-leaktest.sh *_test.go diff --git a/pkg/server/settings_test.go b/pkg/settings/integration_tests/settings_test.go similarity index 91% rename from pkg/server/settings_test.go rename to pkg/settings/integration_tests/settings_test.go index 86429e8dcd2e..8b1910849d21 100644 --- a/pkg/server/settings_test.go +++ b/pkg/settings/integration_tests/settings_test.go @@ -8,7 +8,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. -package server_test +package integration_tests import ( "context" @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/settings" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" @@ -67,12 +66,10 @@ func TestSettingsRefresh(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // Set up some additional cluster settings to play around with. Note that we - // need to do this before starting the server, or there will be data races. - st := cluster.MakeTestingClusterSettings() - s, rawDB, _ := serverutils.StartServer(t, base.TestServerArgs{Settings: st}) + s, rawDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) defer s.Stopper().Stop(context.Background()) + st := s.ApplicationLayer().ClusterSettings() db := sqlutils.MakeSQLRunner(rawDB) insertQ := `UPSERT INTO system.settings (name, value, "lastUpdated", "valueType") @@ -192,12 +189,9 @@ func TestSettingsRefresh(t *testing.T) { func TestSettingsSetAndShow(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // Set up some additional cluster settings to play around with. Note that we - // need to do this before starting the server, or there will be data races. - st := cluster.MakeTestingClusterSettings() - s, rawDB, _ := serverutils.StartServer(t, base.TestServerArgs{Settings: st}) + s, rawDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) defer s.Stopper().Stop(context.Background()) - + st := s.ApplicationLayer().ClusterSettings() db := sqlutils.MakeSQLRunner(rawDB) // TODO(dt): add placeholder support to SET and SHOW. @@ -265,11 +259,7 @@ func TestSettingsShowAll(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - // Set up some additional cluster settings to play around with. Note that we - // need to do this before starting the server, or there will be data races. - st := cluster.MakeTestingClusterSettings() - - s, rawDB, _ := serverutils.StartServer(t, base.TestServerArgs{Settings: st}) + s, rawDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) defer s.Stopper().Stop(context.Background()) db := sqlutils.MakeSQLRunner(rawDB) From 8d4a09890ad851fcebafb7b5015745a6d16bb6c6 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 25 Sep 2023 15:31:45 +0200 Subject: [PATCH 03/10] server: add a context.Context parameter to certain APIs Release note: None --- pkg/server/node.go | 8 +++--- .../tenantsettingswatcher/overrides_store.go | 15 ++++++++--- .../overrides_store_test.go | 26 ++++++++++--------- pkg/server/tenantsettingswatcher/watcher.go | 25 +++++++++--------- .../tenantsettingswatcher/watcher_test.go | 16 ++++++------ 5 files changed, 49 insertions(+), 41 deletions(-) diff --git a/pkg/server/node.go b/pkg/server/node.go index ba48911f5617..c1d9bd5fc0d5 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -2227,7 +2227,7 @@ func (n *Node) TenantSettings( // Send the setting overrides for one precedence level. const firstPrecedenceLevel = kvpb.TenantSettingsEvent_ALL_TENANTS_OVERRIDES - allOverrides, allCh := settingsWatcher.GetAllTenantOverrides() + allOverrides, allCh := settingsWatcher.GetAllTenantOverrides(ctx) // Inject the current storage logical version as an override; as the // tenant server needs this to start up. @@ -2246,7 +2246,7 @@ func (n *Node) TenantSettings( // Then send the initial setting overrides for the other precedence // level. This is the payload that will let the tenant client // connector signal readiness. - tenantOverrides, tenantCh := settingsWatcher.GetTenantOverrides(args.TenantID) + tenantOverrides, tenantCh := settingsWatcher.GetTenantOverrides(ctx, args.TenantID) if err := sendSettings(kvpb.TenantSettingsEvent_TENANT_SPECIFIC_OVERRIDES, tenantOverrides, false /* incremental */); err != nil { return err } @@ -2274,7 +2274,7 @@ func (n *Node) TenantSettings( // All-tenant overrides have changed, send them again. // TODO(multitenant): We can optimize this by only sending the delta since the last // update, with Incremental set to true. - allOverrides, allCh = settingsWatcher.GetAllTenantOverrides() + allOverrides, allCh = settingsWatcher.GetAllTenantOverrides(ctx) if err := sendSettings(kvpb.TenantSettingsEvent_ALL_TENANTS_OVERRIDES, allOverrides, false /* incremental */); err != nil { return err } @@ -2283,7 +2283,7 @@ func (n *Node) TenantSettings( // Tenant-specific overrides have changed, send them again. // TODO(multitenant): We can optimize this by only sending the delta since the last // update, with Incremental set to true. - tenantOverrides, tenantCh = settingsWatcher.GetTenantOverrides(args.TenantID) + tenantOverrides, tenantCh = settingsWatcher.GetTenantOverrides(ctx, args.TenantID) if err := sendSettings(kvpb.TenantSettingsEvent_TENANT_SPECIFIC_OVERRIDES, tenantOverrides, false /* incremental */); err != nil { return err } diff --git a/pkg/server/tenantsettingswatcher/overrides_store.go b/pkg/server/tenantsettingswatcher/overrides_store.go index dd8afb7d1b7a..72ea2ad00650 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store.go +++ b/pkg/server/tenantsettingswatcher/overrides_store.go @@ -11,6 +11,7 @@ package tenantsettingswatcher import ( + "context" "sort" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" @@ -65,7 +66,7 @@ func (s *overridesStore) Init() { s.mu.tenants = make(map[roachpb.TenantID]*tenantOverrides) } -// SetAll initializes the overrides for all tenants. Any existing overrides are +// setAll initializes the overrides for all tenants. Any existing overrides are // replaced. // // The store takes ownership of the overrides slices in the map (the caller can @@ -73,7 +74,9 @@ func (s *overridesStore) Init() { // // This method is called once we complete a full initial scan of the // tenant_setting table. -func (s *overridesStore) SetAll(allOverrides map[roachpb.TenantID][]kvpb.TenantSetting) { +func (s *overridesStore) setAll( + ctx context.Context, allOverrides map[roachpb.TenantID][]kvpb.TenantSetting, +) { s.mu.Lock() defer s.mu.Unlock() @@ -102,7 +105,9 @@ func (s *overridesStore) SetAll(allOverrides map[roachpb.TenantID][]kvpb.TenantS // // The caller can listen for closing of changeCh, which is guaranteed to happen // if the tenant's overrides change. -func (s *overridesStore) GetTenantOverrides(tenantID roachpb.TenantID) *tenantOverrides { +func (s *overridesStore) GetTenantOverrides( + ctx context.Context, tenantID roachpb.TenantID, +) *tenantOverrides { s.mu.RLock() res, ok := s.mu.tenants[tenantID] s.mu.RUnlock() @@ -128,7 +133,9 @@ func (s *overridesStore) GetTenantOverrides(tenantID roachpb.TenantID) *tenantOv // SetTenantOverride changes an override for the given tenant. If the setting // has an empty value, the existing override is removed; otherwise a new // override is added. -func (s *overridesStore) SetTenantOverride(tenantID roachpb.TenantID, setting kvpb.TenantSetting) { +func (s *overridesStore) SetTenantOverride( + ctx context.Context, tenantID roachpb.TenantID, setting kvpb.TenantSetting, +) { s.mu.Lock() defer s.mu.Unlock() var before []kvpb.TenantSetting diff --git a/pkg/server/tenantsettingswatcher/overrides_store_test.go b/pkg/server/tenantsettingswatcher/overrides_store_test.go index 8ea8d99ae381..80a960eb56e0 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store_test.go +++ b/pkg/server/tenantsettingswatcher/overrides_store_test.go @@ -11,6 +11,7 @@ package tenantsettingswatcher import ( + "context" "fmt" "strings" "testing" @@ -25,6 +26,7 @@ import ( func TestOverridesStore(t *testing.T) { defer leaktest.AfterTest(t)() + ctx := context.Background() var s overridesStore s.Init() t1 := roachpb.MustMakeTenantID(1) @@ -55,36 +57,36 @@ func TestOverridesStore(t *testing.T) { t.Fatalf("channel did not close") } } - o1 := s.GetTenantOverrides(t1) + o1 := s.GetTenantOverrides(ctx, t1) expect(o1, "") - s.SetAll(map[roachpb.TenantID][]kvpb.TenantSetting{ + s.setAll(ctx, map[roachpb.TenantID][]kvpb.TenantSetting{ t1: {st("a", "aa"), st("b", "bb"), st("d", "dd")}, t2: {st("x", "xx")}, }) expectChange(o1) - o1 = s.GetTenantOverrides(t1) + o1 = s.GetTenantOverrides(ctx, t1) expect(o1, "a=aa b=bb d=dd") - o2 := s.GetTenantOverrides(t2) + o2 := s.GetTenantOverrides(ctx, t2) expect(o2, "x=xx") - s.SetTenantOverride(t1, st("b", "changed")) + s.SetTenantOverride(ctx, t1, st("b", "changed")) expectChange(o1) - o1 = s.GetTenantOverrides(t1) + o1 = s.GetTenantOverrides(ctx, t1) expect(o1, "a=aa b=changed d=dd") - s.SetTenantOverride(t1, st("b", "")) + s.SetTenantOverride(ctx, t1, st("b", "")) expectChange(o1) - o1 = s.GetTenantOverrides(t1) + o1 = s.GetTenantOverrides(ctx, t1) expect(o1, "a=aa d=dd") - s.SetTenantOverride(t1, st("c", "cc")) + s.SetTenantOverride(ctx, t1, st("c", "cc")) expectChange(o1) - o1 = s.GetTenantOverrides(t1) + o1 = s.GetTenantOverrides(ctx, t1) expect(o1, "a=aa c=cc d=dd") // Set an override for a tenant that has no existing data. t3 := roachpb.MustMakeTenantID(3) - s.SetTenantOverride(t3, st("x", "xx")) - o3 := s.GetTenantOverrides(t3) + s.SetTenantOverride(ctx, t3, st("x", "xx")) + o3 := s.GetTenantOverrides(ctx, t3) expect(o3, "x=xx") } diff --git a/pkg/server/tenantsettingswatcher/watcher.go b/pkg/server/tenantsettingswatcher/watcher.go index 83aadc5effa6..c0fb1af62f81 100644 --- a/pkg/server/tenantsettingswatcher/watcher.go +++ b/pkg/server/tenantsettingswatcher/watcher.go @@ -40,13 +40,13 @@ import ( // if err := w.Start(ctx); err != nil { ... } // // // Get overrides and keep them up to date. -// all, allCh := w.AllTenantOverrides() -// tenant, tenantCh := w.TenantOverrides(tenantID) +// all, allCh := w.GetAllTenantOverrides() +// tenant, tenantCh := w.GetTenantOverrides(ctx,tenantID) // select { // case <-allCh: -// all, allCh = w.AllTenantOverrides() +// all, allCh = w.GetAllTenantOverrides() // case <-tenantCh: -// tenant, tenantCh = w.TenantOverrides(tenantID) +// tenant, tenantCh = w.GetTenantOverrides(ctx,tenantID) // case <-ctx.Done(): // ... // } @@ -152,7 +152,7 @@ func (w *Watcher) startRangeFeed( allOverrides[tenantID] = append(allOverrides[tenantID], setting) } else { // We are processing incremental changes. - w.store.SetTenantOverride(tenantID, setting) + w.store.SetTenantOverride(ctx, tenantID, setting) } return nil } @@ -162,7 +162,7 @@ func (w *Watcher) startRangeFeed( // The CompleteUpdate indicates that the table scan is complete. // Henceforth, all calls to translateEvent will be incremental changes, // until we hit an error and have to restart the rangefeed. - w.store.SetAll(allOverrides) + w.store.setAll(ctx, allOverrides) allOverrides = nil if !initialScan.done { @@ -249,9 +249,9 @@ func (w *Watcher) TestingRestart() { // // The caller must not modify the returned overrides slice. func (w *Watcher) GetTenantOverrides( - tenantID roachpb.TenantID, + ctx context.Context, tenantID roachpb.TenantID, ) (overrides []kvpb.TenantSetting, changeCh <-chan struct{}) { - o := w.store.GetTenantOverrides(tenantID) + o := w.store.GetTenantOverrides(ctx, tenantID) return o.overrides, o.changeCh } @@ -260,9 +260,8 @@ func (w *Watcher) GetTenantOverrides( // have an override for the same setting. // // The caller must not modify the returned overrides slice. -func (w *Watcher) GetAllTenantOverrides() ( - overrides []kvpb.TenantSetting, - changeCh <-chan struct{}, -) { - return w.GetTenantOverrides(allTenantOverridesID) +func (w *Watcher) GetAllTenantOverrides( + ctx context.Context, +) (overrides []kvpb.TenantSetting, changeCh <-chan struct{}) { + return w.GetTenantOverrides(ctx, allTenantOverridesID) } diff --git a/pkg/server/tenantsettingswatcher/watcher_test.go b/pkg/server/tenantsettingswatcher/watcher_test.go index fec2e1fcdec5..36df4c0482cd 100644 --- a/pkg/server/tenantsettingswatcher/watcher_test.go +++ b/pkg/server/tenantsettingswatcher/watcher_test.go @@ -77,16 +77,16 @@ func TestWatcher(t *testing.T) { t1 := roachpb.MustMakeTenantID(1) t2 := roachpb.MustMakeTenantID(2) t3 := roachpb.MustMakeTenantID(3) - all, allCh := w.GetAllTenantOverrides() + all, allCh := w.GetAllTenantOverrides(ctx) expect(all, "foo=foo-all") - t1Overrides, t1Ch := w.GetTenantOverrides(t1) + t1Overrides, t1Ch := w.GetTenantOverrides(ctx, t1) expect(t1Overrides, "bar=bar-t1 foo=foo-t1") - t2Overrides, _ := w.GetTenantOverrides(t2) + t2Overrides, _ := w.GetTenantOverrides(ctx, t2) expect(t2Overrides, "baz=baz-t2") - t3Overrides, t3Ch := w.GetTenantOverrides(t3) + t3Overrides, t3Ch := w.GetTenantOverrides(ctx, t3) expect(t3Overrides, "") expectClose := func(ch <-chan struct{}) { @@ -100,24 +100,24 @@ func TestWatcher(t *testing.T) { // Add an all-tenant override. r.Exec(t, "INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (0, 'bar', 'bar-all', 's')") expectClose(allCh) - all, allCh = w.GetAllTenantOverrides() + all, allCh = w.GetAllTenantOverrides(ctx) expect(all, "bar=bar-all foo=foo-all") // Modify a tenant override. r.Exec(t, "UPSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (1, 'bar', 'bar-t1-updated', 's')") expectClose(t1Ch) - t1Overrides, _ = w.GetTenantOverrides(t1) + t1Overrides, _ = w.GetTenantOverrides(ctx, t1) expect(t1Overrides, "bar=bar-t1-updated foo=foo-t1") // Remove an all-tenant override. r.Exec(t, "DELETE FROM system.tenant_settings WHERE tenant_id = 0 AND name = 'foo'") expectClose(allCh) - all, _ = w.GetAllTenantOverrides() + all, _ = w.GetAllTenantOverrides(ctx) expect(all, "bar=bar-all") // Add an override for a tenant that has no overrides. r.Exec(t, "INSERT INTO system.tenant_settings (tenant_id, name, value, value_type) VALUES (3, 'qux', 'qux-t3', 's')") expectClose(t3Ch) - t3Overrides, _ = w.GetTenantOverrides(t3) + t3Overrides, _ = w.GetTenantOverrides(ctx, t3) expect(t3Overrides, "qux=qux-t3") } From f08cdab82f06aceea0bdb536261e7c9cd63c0b03 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 25 Sep 2023 15:33:29 +0200 Subject: [PATCH 04/10] settings,server: add missing comments Release note: None --- pkg/server/tenantsettingswatcher/overrides_store.go | 2 +- pkg/settings/updater.go | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/server/tenantsettingswatcher/overrides_store.go b/pkg/server/tenantsettingswatcher/overrides_store.go index 72ea2ad00650..2f264dc86bf8 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store.go +++ b/pkg/server/tenantsettingswatcher/overrides_store.go @@ -47,7 +47,7 @@ type overridesStore struct { // tenantOverrides stores the current overrides for a tenant (or the current // all-tenant overrides). It is an immutable data structure. type tenantOverrides struct { - // overrides, ordered by Name. + // overrides, ordered by InternalKey. overrides []kvpb.TenantSetting // changeCh is a channel that is closed when the tenant overrides change (in diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index 30b3723d1117..5ed2a3680384 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -113,8 +113,8 @@ func NewUpdater(sv *Values) Updater { } } -// getSetting determines whether the target setting can -// be set or overridden. +// getSetting determines whether the target setting can be set or +// overridden. func (u updater) getSetting(key InternalKey, value EncodedValue) (internalSetting, error) { d, ok := registry[key] if !ok { @@ -130,7 +130,7 @@ func (u updater) getSetting(key InternalKey, value EncodedValue) (internalSettin return d, nil } -// Set attempts to parse and update a setting and notes that it was updated. +// Set attempts update a setting and notes that it was updated. func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) error { d, err := u.getSetting(key, value) if err != nil || d == nil { @@ -140,6 +140,9 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e return u.setInternal(ctx, key, value, d, OriginExplicitlySet) } +// setInternal is the shared code between Set() and SetFromStorage(). +// It propagates the given value to the in-RAM store and keeps track +// of the origin of the value. func (u updater) setInternal( ctx context.Context, key InternalKey, value EncodedValue, d internalSetting, origin ValueOrigin, ) error { From e6cfd67b9304a84c25a18b9a9a1a3fe91ddb98bd Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 25 Sep 2023 15:34:38 +0200 Subject: [PATCH 05/10] kvtenant: add some extra tracing for troubleshooting Release note: None --- pkg/kv/kvclient/kvtenant/setting_overrides.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/kv/kvclient/kvtenant/setting_overrides.go b/pkg/kv/kvclient/kvtenant/setting_overrides.go index 0c27fe99b5a8..f534bae0f45b 100644 --- a/pkg/kv/kvclient/kvtenant/setting_overrides.go +++ b/pkg/kv/kvclient/kvtenant/setting_overrides.go @@ -230,8 +230,10 @@ func (c *connector) processSettingsEvent( for _, o := range e.Overrides { if o.Value == (settings.EncodedValue{}) { // Empty value indicates that the override is removed. + log.VEventf(ctx, 1, "removing %v override for %q", e.Precedence, o.InternalKey) delete(m, o.InternalKey) } else { + log.VEventf(ctx, 1, "adding %v override for %q = %q", e.Precedence, o.InternalKey, o.Value.Value) m[o.InternalKey] = o.Value } } From 42c5af87051ec034cfb07d8446032e18d3081619 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 25 Sep 2023 15:37:59 +0200 Subject: [PATCH 06/10] tenantsettingswatcher: add some logging/tracing for troubleshooting Release note: None --- pkg/server/tenantsettingswatcher/BUILD.bazel | 2 ++ .../tenantsettingswatcher/overrides_store.go | 25 +++++++++++++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/pkg/server/tenantsettingswatcher/BUILD.bazel b/pkg/server/tenantsettingswatcher/BUILD.bazel index 12b6a05ba239..0b2286525bcf 100644 --- a/pkg/server/tenantsettingswatcher/BUILD.bazel +++ b/pkg/server/tenantsettingswatcher/BUILD.bazel @@ -26,12 +26,14 @@ go_library( "//pkg/sql/rowenc/valueside", "//pkg/sql/sem/tree", "//pkg/sql/types", + "//pkg/util", "//pkg/util/hlc", "//pkg/util/log", "//pkg/util/startup", "//pkg/util/stop", "//pkg/util/syncutil", "@com_github_cockroachdb_errors//:errors", + "@com_github_cockroachdb_redact//:redact", ], ) diff --git a/pkg/server/tenantsettingswatcher/overrides_store.go b/pkg/server/tenantsettingswatcher/overrides_store.go index 2f264dc86bf8..f4283d08fbe4 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store.go +++ b/pkg/server/tenantsettingswatcher/overrides_store.go @@ -17,7 +17,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings" + "github.com/cockroachdb/cockroach/pkg/util" + "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/syncutil" + "github.com/cockroachdb/errors" + "github.com/cockroachdb/redact" ) // overridesStore is the data structure that maintains all the tenant overrides @@ -55,7 +59,18 @@ type tenantOverrides struct { changeCh chan struct{} } -func newTenantOverrides(overrides []kvpb.TenantSetting) *tenantOverrides { +func newTenantOverrides( + ctx context.Context, tenID roachpb.TenantID, overrides []kvpb.TenantSetting, +) *tenantOverrides { + if log.V(1) { + var buf redact.StringBuilder + buf.Printf("loaded overrides for tenant %d (%s)\n", tenID.InternalValue, util.GetSmallTrace(2)) + for _, v := range overrides { + buf.Printf("%v = %+v", v.InternalKey, v.Value) + buf.SafeRune('\n') + } + log.VEventf(ctx, 1, "%v", buf) + } return &tenantOverrides{ overrides: overrides, changeCh: make(chan struct{}), @@ -94,10 +109,10 @@ func (s *overridesStore) setAll( // Sanity check. for i := 1; i < len(overrides); i++ { if overrides[i].InternalKey == overrides[i-1].InternalKey { - panic("duplicate setting") + panic(errors.AssertionFailedf("duplicate setting: %s", overrides[i].InternalKey)) } } - s.mu.tenants[tenantID] = newTenantOverrides(overrides) + s.mu.tenants[tenantID] = newTenantOverrides(ctx, tenantID, overrides) } } @@ -125,7 +140,7 @@ func (s *overridesStore) GetTenantOverrides( if res, ok = s.mu.tenants[tenantID]; ok { return res } - res = newTenantOverrides(nil /* overrides */) + res = newTenantOverrides(ctx, tenantID, nil /* overrides */) s.mu.tenants[tenantID] = res return res } @@ -159,5 +174,5 @@ func (s *overridesStore) SetTenantOverride( } // 3. Append all settings after setting.InternalKey. after = append(after, before...) - s.mu.tenants[tenantID] = newTenantOverrides(after) + s.mu.tenants[tenantID] = newTenantOverrides(ctx, tenantID, after) } From 35bacc1f87dde4fadb6f6ced4e21ecb90f427926 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 25 Sep 2023 15:47:27 +0200 Subject: [PATCH 07/10] settingswatcher: simplify a test Release note: None --- pkg/server/settingswatcher/settings_watcher_external_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/server/settingswatcher/settings_watcher_external_test.go b/pkg/server/settingswatcher/settings_watcher_external_test.go index f88b6b757373..5e92467ec235 100644 --- a/pkg/server/settingswatcher/settings_watcher_external_test.go +++ b/pkg/server/settingswatcher/settings_watcher_external_test.go @@ -530,7 +530,7 @@ func TestStaleRowsDoNotCauseSettingsToRegress(t *testing.T) { // The tenant prefix, if one exists, will have been stripped from the // key. getSettingKVForFakeSetting := func(t *testing.T) roachpb.KeyValue { - codec := s.ExecutorConfig().(sql.ExecutorConfig).Codec + codec := s.Codec() k := codec.TablePrefix(keys.SettingsTableID) rows, err := s.DB().Scan(ctx, k, k.PrefixEnd(), 0 /* maxRows */) require.NoError(t, err) From ab71d0ff9aedcd48d9d53cdb2d36d4120b9989b0 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 25 Sep 2023 16:24:42 +0200 Subject: [PATCH 08/10] tenantsettingswatcher: unexport an API Release note: None --- .../tenantsettingswatcher/overrides_store.go | 8 +++---- .../overrides_store_test.go | 22 +++++++++---------- pkg/server/tenantsettingswatcher/watcher.go | 4 ++-- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/pkg/server/tenantsettingswatcher/overrides_store.go b/pkg/server/tenantsettingswatcher/overrides_store.go index f4283d08fbe4..e3581bba7aaa 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store.go +++ b/pkg/server/tenantsettingswatcher/overrides_store.go @@ -116,11 +116,11 @@ func (s *overridesStore) setAll( } } -// GetTenantOverrides retrieves the overrides for a given tenant. +// getTenantOverrides retrieves the overrides for a given tenant. // // The caller can listen for closing of changeCh, which is guaranteed to happen // if the tenant's overrides change. -func (s *overridesStore) GetTenantOverrides( +func (s *overridesStore) getTenantOverrides( ctx context.Context, tenantID roachpb.TenantID, ) *tenantOverrides { s.mu.RLock() @@ -145,10 +145,10 @@ func (s *overridesStore) GetTenantOverrides( return res } -// SetTenantOverride changes an override for the given tenant. If the setting +// setTenantOverride changes an override for the given tenant. If the setting // has an empty value, the existing override is removed; otherwise a new // override is added. -func (s *overridesStore) SetTenantOverride( +func (s *overridesStore) setTenantOverride( ctx context.Context, tenantID roachpb.TenantID, setting kvpb.TenantSetting, ) { s.mu.Lock() diff --git a/pkg/server/tenantsettingswatcher/overrides_store_test.go b/pkg/server/tenantsettingswatcher/overrides_store_test.go index 80a960eb56e0..aea1c0628bd6 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store_test.go +++ b/pkg/server/tenantsettingswatcher/overrides_store_test.go @@ -57,36 +57,36 @@ func TestOverridesStore(t *testing.T) { t.Fatalf("channel did not close") } } - o1 := s.GetTenantOverrides(ctx, t1) + o1 := s.getTenantOverrides(ctx, t1) expect(o1, "") s.setAll(ctx, map[roachpb.TenantID][]kvpb.TenantSetting{ t1: {st("a", "aa"), st("b", "bb"), st("d", "dd")}, t2: {st("x", "xx")}, }) expectChange(o1) - o1 = s.GetTenantOverrides(ctx, t1) + o1 = s.getTenantOverrides(ctx, t1) expect(o1, "a=aa b=bb d=dd") - o2 := s.GetTenantOverrides(ctx, t2) + o2 := s.getTenantOverrides(ctx, t2) expect(o2, "x=xx") - s.SetTenantOverride(ctx, t1, st("b", "changed")) + s.setTenantOverride(ctx, t1, st("b", "changed")) expectChange(o1) - o1 = s.GetTenantOverrides(ctx, t1) + o1 = s.getTenantOverrides(ctx, t1) expect(o1, "a=aa b=changed d=dd") - s.SetTenantOverride(ctx, t1, st("b", "")) + s.setTenantOverride(ctx, t1, st("b", "")) expectChange(o1) - o1 = s.GetTenantOverrides(ctx, t1) + o1 = s.getTenantOverrides(ctx, t1) expect(o1, "a=aa d=dd") - s.SetTenantOverride(ctx, t1, st("c", "cc")) + s.setTenantOverride(ctx, t1, st("c", "cc")) expectChange(o1) - o1 = s.GetTenantOverrides(ctx, t1) + o1 = s.getTenantOverrides(ctx, t1) expect(o1, "a=aa c=cc d=dd") // Set an override for a tenant that has no existing data. t3 := roachpb.MustMakeTenantID(3) - s.SetTenantOverride(ctx, t3, st("x", "xx")) - o3 := s.GetTenantOverrides(ctx, t3) + s.setTenantOverride(ctx, t3, st("x", "xx")) + o3 := s.getTenantOverrides(ctx, t3) expect(o3, "x=xx") } diff --git a/pkg/server/tenantsettingswatcher/watcher.go b/pkg/server/tenantsettingswatcher/watcher.go index c0fb1af62f81..9b4d1892dfd8 100644 --- a/pkg/server/tenantsettingswatcher/watcher.go +++ b/pkg/server/tenantsettingswatcher/watcher.go @@ -152,7 +152,7 @@ func (w *Watcher) startRangeFeed( allOverrides[tenantID] = append(allOverrides[tenantID], setting) } else { // We are processing incremental changes. - w.store.SetTenantOverride(ctx, tenantID, setting) + w.store.setTenantOverride(ctx, tenantID, setting) } return nil } @@ -251,7 +251,7 @@ func (w *Watcher) TestingRestart() { func (w *Watcher) GetTenantOverrides( ctx context.Context, tenantID roachpb.TenantID, ) (overrides []kvpb.TenantSetting, changeCh <-chan struct{}) { - o := w.store.GetTenantOverrides(ctx, tenantID) + o := w.store.getTenantOverrides(ctx, tenantID) return o.overrides, o.changeCh } From 84ae0d468109db72ca3db0415b4d806a67fb2b8e Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 26 Sep 2023 12:50:44 +0200 Subject: [PATCH 09/10] settings: update a comment Release note: None --- pkg/settings/updater.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index 5ed2a3680384..4851446a2cda 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -74,8 +74,8 @@ type Updater interface { // SetToDefault resets the setting to its default value. SetToDefault(ctx context.Context, key InternalKey) error - // ResetRemaining loads the default values for all settings that - // were not updated by Set(). + // ResetRemaining loads the values from build-time defaults for all + // settings that were not updated by Set() and SetFromStorage(). ResetRemaining(ctx context.Context) // SetFromStorage is called by the settings watcher to update the From 17f42ed27c2a9b321369d350f94164ed8f95f25d Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 26 Sep 2023 12:56:48 +0200 Subject: [PATCH 10/10] settings: check the origin is set via Override Release note: None --- pkg/settings/settings_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go index 293ef65009a4..a19183429406 100644 --- a/pkg/settings/settings_test.go +++ b/pkg/settings/settings_test.go @@ -800,14 +800,24 @@ func TestOverride(t *testing.T) { sv := &settings.Values{} sv.Init(ctx, settings.TestOpaque) + // Check the origin before an override. + require.Equal(t, settings.OriginDefault, overrideBool.ValueOrigin(ctx, sv)) + // Test override for bool setting. require.Equal(t, true, overrideBool.Get(sv)) overrideBool.Override(ctx, sv, false) require.Equal(t, false, overrideBool.Get(sv)) + + // Override changes the origin. + require.Equal(t, settings.OriginExplicitlySet, overrideBool.ValueOrigin(ctx, sv)) + u := settings.NewUpdater(sv) u.ResetRemaining(ctx) require.Equal(t, false, overrideBool.Get(sv)) + // ResetRemaining does not change the origin for overridden settings. + require.Equal(t, settings.OriginExplicitlySet, overrideBool.ValueOrigin(ctx, sv)) + // Test override for int setting. require.Equal(t, int64(0), overrideInt.Get(sv)) overrideInt.Override(ctx, sv, 42)