From 2f5d717178a87b42fa94c20a226cc491d185455d Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 23 Sep 2023 00:02:45 +0200 Subject: [PATCH 1/4] settings: move the 'modified' map to valueContainer In a later commit, we will want to ensure that `.Override()` sets the modified bit. This is not possible if the modified bits are in the Updater. (The `Override` method is on the setting itself and has no access to the updater.) Release note: None --- pkg/settings/settings_test.go | 14 +++++--------- pkg/settings/updater.go | 16 +++++++--------- pkg/settings/values.go | 5 +++++ 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go index e48d1fbd6336..293ef65009a4 100644 --- a/pkg/settings/settings_test.go +++ b/pkg/settings/settings_test.go @@ -574,23 +574,19 @@ func TestCache(t *testing.T) { if expected, actual := true, boolFA.Get(sv); expected != actual { t.Fatalf("expected %v, got %v", expected, actual) } - // If the updater doesn't have a key, e.g. if the setting has been deleted, - // Resetting it from the cache. + // The updated status remains in sv. A new updater is able to pick + // it up. settings.NewUpdater(sv).ResetRemaining(ctx) - if expected, actual := 2, changes.boolTA; expected != actual { + if expected, actual := 1, changes.boolTA; expected != actual { t.Fatalf("expected %d, got %d", expected, actual) } - if expected, actual := 2, changes.i1A; expected != actual { + if expected, actual := 1, changes.i1A; expected != actual { t.Fatalf("expected %d, got %d", expected, actual) } - if expected, actual := false, boolFA.Get(sv); expected != actual { - t.Fatalf("expected %v, got %v", expected, actual) - } - - if expected, actual := false, boolFA.Get(sv); expected != actual { + if expected, actual := true, boolFA.Get(sv); expected != actual { t.Fatalf("expected %v, got %v", expected, actual) } }) diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index e5023381573a..5efbcf6fbf96 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -60,7 +60,6 @@ func EncodeProtobuf(p protoutil.Message) string { type updater struct { sv *Values - m map[InternalKey]struct{} } // Updater is a helper for updating the in-memory settings. @@ -92,7 +91,6 @@ func NewUpdater(sv *Values) Updater { return NoopUpdater{} } return updater{ - m: make(map[InternalKey]struct{}, len(registry)), sv: sv, } } @@ -108,7 +106,7 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e return errors.Errorf("unknown setting '%s'", key) } - u.m[key] = struct{}{} + u.sv.container.modified[d.getSlot()] = true if expected := d.Typ(); value.Type != expected { return errors.Errorf("setting '%s' defined as type %s, not %s", d.Name(), expected, value.Type) @@ -167,19 +165,19 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e // ResetRemaining sets all settings not updated by the updater to their default values. func (u updater) ResetRemaining(ctx context.Context) { - for k, v := range registry { - - if _, hasOverride := u.m[k]; hasOverride { - u.sv.setValueOrigin(ctx, v.getSlot(), OriginExplicitlySet) + for _, v := range registry { + slot := v.getSlot() + if hasOverride := u.sv.container.modified[slot]; hasOverride { + u.sv.setValueOrigin(ctx, slot, OriginExplicitlySet) } else { - u.sv.setValueOrigin(ctx, v.getSlot(), OriginDefault) + u.sv.setValueOrigin(ctx, slot, OriginDefault) } if u.sv.SpecializedToVirtualCluster() && v.Class() == SystemOnly { // Don't try to reset system settings on a non-system tenant. continue } - if _, ok := u.m[k]; !ok { + if m := u.sv.container.modified[slot]; !m { v.setToDefault(ctx, u.sv) } } diff --git a/pkg/settings/values.go b/pkg/settings/values.go index 09683e472b11..bfc1a4bd1fa5 100644 --- a/pkg/settings/values.go +++ b/pkg/settings/values.go @@ -87,6 +87,11 @@ type valuesContainer struct { forbidden [numSlots]bool hasValue [numSlots]uint32 + + // modified is set when a setting is explictly set via Set() + // in the updater. + // TODO(knz): Check if this can be merged with hasValue above. + modified [numSlots]bool } func (c *valuesContainer) setGenericVal(slot slotIdx, newVal interface{}) { From 3bc6d7ac95a1059d3bf20a54220aee4de6d0cdb6 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 23 Sep 2023 00:12:19 +0200 Subject: [PATCH 2/4] settings,settingswatcher: simplify interfaces This commit simplifies the Updater interface as follows: - it provides a new `SetFromStorage()` method which combines the effect of `Set()` and `SetValueOrigin()` into one method. The settings watcher is also modified to use this new method. - it provides a new `SetToDefault()` method which resets a setting to its registered default. This is then used instead of `Set(s.EncodedDefault())` in the setting watcher. The benefit here is that `SetToDefault()` properly clears the 'modified' bit, which we plan to use in a later commit. Release note: None --- pkg/server/settingswatcher/BUILD.bazel | 1 - .../settingswatcher/settings_watcher.go | 17 +--- pkg/settings/updater.go | 95 ++++++++++++++++--- 3 files changed, 84 insertions(+), 29 deletions(-) diff --git a/pkg/server/settingswatcher/BUILD.bazel b/pkg/server/settingswatcher/BUILD.bazel index ec423c35cbb8..4b0daac6a1e6 100644 --- a/pkg/server/settingswatcher/BUILD.bazel +++ b/pkg/server/settingswatcher/BUILD.bazel @@ -35,7 +35,6 @@ go_library( "//pkg/util/stop", "//pkg/util/syncutil", "@com_github_cockroachdb_errors//:errors", - "@com_github_cockroachdb_redact//:redact", ], ) diff --git a/pkg/server/settingswatcher/settings_watcher.go b/pkg/server/settingswatcher/settings_watcher.go index c5c51702617b..720a400cb6e2 100644 --- a/pkg/server/settingswatcher/settings_watcher.go +++ b/pkg/server/settingswatcher/settings_watcher.go @@ -34,7 +34,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/errors" - "github.com/cockroachdb/redact" ) // SettingsWatcher is used to watch for cluster settings changes with a @@ -388,24 +387,16 @@ func (s *SettingsWatcher) setLocked( return } - if err := s.mu.updater.Set(ctx, key, val); err != nil { - log.Warningf(ctx, "failed to set setting %s to %s: %v", redact.Safe(key), val.Value, err) + if err := s.mu.updater.SetFromStorage(ctx, key, val, origin); err != nil { + log.Warningf(ctx, "failed to set setting %s to %s: %v", key, val.Value, err) } - s.mu.updater.SetValueOrigin(ctx, key, origin) } // setDefaultLocked sets a setting to its default value. func (s *SettingsWatcher) setDefaultLocked(ctx context.Context, key settings.InternalKey) { - setting, ok := settings.LookupForLocalAccessByKey(key, s.codec.ForSystemTenant()) - if !ok { - log.Warningf(ctx, "failed to find setting %s, skipping update", redact.Safe(key)) - return - } - val := settings.EncodedValue{ - Value: setting.EncodedDefault(), - Type: setting.Typ(), + if err := s.mu.updater.SetToDefault(ctx, key); err != nil { + log.Warningf(ctx, "failed to set setting %s to default: %v", key, err) } - s.setLocked(ctx, key, val, settings.OriginDefault) } // updateOverrides updates the overrides map and updates any settings diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index 5efbcf6fbf96..1b9f748b9626 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -69,9 +69,19 @@ type updater struct { // wrapped atomic settings values as we go and note which settings were updated, // then set the rest to default in ResetRemaining(). type Updater interface { + // Set is used by tests to configure overrides in a generic fashion. Set(ctx context.Context, key InternalKey, value EncodedValue) error + // 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(ctx context.Context) - SetValueOrigin(ctx context.Context, key InternalKey, origin ValueOrigin) + + // SetFromStorage is called by the settings watcher to update the + // settings from either the rangefeed over system.settings, or + // overrides coming in over the network from the system tenant. + SetFromStorage(ctx context.Context, key InternalKey, value EncodedValue, origin ValueOrigin) error } // A NoopUpdater ignores all updates. @@ -83,7 +93,15 @@ func (u NoopUpdater) Set(ctx context.Context, key InternalKey, value EncodedValu // ResetRemaining implements Updater. It is a no-op. func (u NoopUpdater) ResetRemaining(context.Context) {} -func (u NoopUpdater) SetValueOrigin(ctx context.Context, key InternalKey, origin ValueOrigin) {} +// SetToDefault implements Updater. It is a no-op. +func (u NoopUpdater) SetToDefault(ctx context.Context, key InternalKey) error { return nil } + +// SetFromStorage implements Updater. It is a no-op. +func (u NoopUpdater) SetFromStorage( + ctx context.Context, key InternalKey, value EncodedValue, origin ValueOrigin, +) error { + return nil +} // NewUpdater makes an Updater. func NewUpdater(sv *Values) Updater { @@ -95,32 +113,52 @@ func NewUpdater(sv *Values) Updater { } } -// Set attempts to parse and update a setting and notes that it was updated. -func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) error { +// 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 { if _, ok := retiredSettings[key]; ok { - return nil + return nil, nil } // Likely a new setting this old node doesn't know about. - return errors.Errorf("unknown setting '%s'", key) + return nil, errors.Errorf("unknown setting '%s'", key) } - - u.sv.container.modified[d.getSlot()] = true - if expected := d.Typ(); value.Type != expected { - return errors.Errorf("setting '%s' defined as type %s, not %s", d.Name(), expected, value.Type) + return nil, errors.Errorf("setting '%s' defined as type %s, not %s", d.Name(), expected, value.Type) + } + return d, nil +} + +// Set attempts to parse and 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 { + return err } + return u.setInternal(ctx, key, value, d, OriginExplicitlySet) +} + +func (u updater) setInternal( + ctx context.Context, key InternalKey, value EncodedValue, d internalSetting, origin ValueOrigin, +) error { + // Mark the setting as modified, such that + // (updater).ResetRemaining() does not touch it. + u.sv.container.modified[d.getSlot()] = true + u.sv.setValueOrigin(ctx, d.getSlot(), origin) + switch setting := d.(type) { case *StringSetting: return setting.set(ctx, u.sv, value.Value) + case *ProtobufSetting: p, err := setting.DecodeValue(value.Value) if err != nil { return err } return setting.set(ctx, u.sv, p) + case *BoolSetting: b, err := setting.DecodeValue(value.Value) if err != nil { @@ -128,30 +166,35 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e } setting.set(ctx, u.sv, b) return nil + case numericSetting: i, err := setting.DecodeValue(value.Value) if err != nil { return err } return setting.set(ctx, u.sv, i) + case *FloatSetting: f, err := setting.DecodeValue(value.Value) if err != nil { return err } return setting.set(ctx, u.sv, f) + case *DurationSetting: d, err := setting.DecodeValue(value.Value) if err != nil { return err } return setting.set(ctx, u.sv, d) + case *DurationSettingWithExplicitUnit: d, err := setting.DecodeValue(value.Value) if err != nil { return err } return setting.set(ctx, u.sv, d) + case *VersionSetting: // We intentionally avoid updating the setting through this code path. // The specific setting backed by VersionSetting is the cluster version @@ -159,7 +202,25 @@ func (u updater) Set(ctx context.Context, key InternalKey, value EncodedValue) e // node in the cluster instead of gossip. This is done using the // BumpClusterVersion RPC. return nil + + default: + return errors.AssertionFailedf("unhandled type: %T", d) } +} + +func (u updater) SetToDefault(ctx context.Context, key InternalKey) error { + d, ok := registry[key] + if !ok { + if _, ok := retiredSettings[key]; ok { + return nil + } + // Likely a new setting this old node doesn't know about. + return errors.Errorf("unknown setting '%s'", key) + } + + u.sv.container.modified[d.getSlot()] = false + u.sv.setValueOrigin(ctx, d.getSlot(), OriginDefault) + d.setToDefault(ctx, u.sv) return nil } @@ -183,10 +244,14 @@ func (u updater) ResetRemaining(ctx context.Context) { } } -// SetValueOrigin sets the origin of the value of a given setting. -func (u updater) SetValueOrigin(ctx context.Context, key InternalKey, origin ValueOrigin) { - d, ok := registry[key] - if ok { - u.sv.setValueOrigin(ctx, d.getSlot(), origin) +// SetFromStorage loads the stored value into the setting. +func (u updater) SetFromStorage( + ctx context.Context, key InternalKey, value EncodedValue, origin ValueOrigin, +) error { + d, err := u.getSetting(key, value) + if err != nil || d == nil { + return err } + + return u.setInternal(ctx, key, value, d, origin) } From e94de87bc6ecf7f6a0fb5d15a8ec02f3bc9363bf Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 23 Sep 2023 00:31:43 +0200 Subject: [PATCH 3/4] settings: simplify the container This commit uses the "value origin" field now throughout to decide whether or not a setting was modified. Release note: None --- pkg/settings/updater.go | 11 +---------- pkg/settings/values.go | 6 +----- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index 1b9f748b9626..30b3723d1117 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -145,7 +145,6 @@ func (u updater) setInternal( ) error { // Mark the setting as modified, such that // (updater).ResetRemaining() does not touch it. - u.sv.container.modified[d.getSlot()] = true u.sv.setValueOrigin(ctx, d.getSlot(), origin) switch setting := d.(type) { @@ -218,7 +217,6 @@ func (u updater) SetToDefault(ctx context.Context, key InternalKey) error { return errors.Errorf("unknown setting '%s'", key) } - u.sv.container.modified[d.getSlot()] = false u.sv.setValueOrigin(ctx, d.getSlot(), OriginDefault) d.setToDefault(ctx, u.sv) return nil @@ -227,18 +225,11 @@ func (u updater) SetToDefault(ctx context.Context, key InternalKey) error { // ResetRemaining sets all settings not updated by the updater to their default values. func (u updater) ResetRemaining(ctx context.Context) { for _, v := range registry { - slot := v.getSlot() - if hasOverride := u.sv.container.modified[slot]; hasOverride { - u.sv.setValueOrigin(ctx, slot, OriginExplicitlySet) - } else { - u.sv.setValueOrigin(ctx, slot, OriginDefault) - } - if u.sv.SpecializedToVirtualCluster() && v.Class() == SystemOnly { // Don't try to reset system settings on a non-system tenant. continue } - if m := u.sv.container.modified[slot]; !m { + if u.sv.getValueOrigin(ctx, v.getSlot()) == OriginDefault { v.setToDefault(ctx, u.sv) } } diff --git a/pkg/settings/values.go b/pkg/settings/values.go index bfc1a4bd1fa5..38284bc930dc 100644 --- a/pkg/settings/values.go +++ b/pkg/settings/values.go @@ -86,12 +86,8 @@ type valuesContainer struct { // tenant). Reading or writing such a setting causes panics in test builds. forbidden [numSlots]bool + // hasValue contains the origin of the current value of the setting. hasValue [numSlots]uint32 - - // modified is set when a setting is explictly set via Set() - // in the updater. - // TODO(knz): Check if this can be merged with hasValue above. - modified [numSlots]bool } func (c *valuesContainer) setGenericVal(slot slotIdx, newVal interface{}) { From dc361468f8d5feee13710fcc71c6a75759a8ae47 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Sat, 23 Sep 2023 00:36:22 +0200 Subject: [PATCH 4/4] settings: make `.Override` set the value origin Prior to this patch, setting overrides in tests did not have their value origin set properly. This patch fixes it. Release note: None --- pkg/settings/bool.go | 1 + pkg/settings/duration.go | 1 + pkg/settings/float.go | 1 + pkg/settings/int.go | 1 + pkg/settings/protobuf.go | 1 + pkg/settings/string.go | 1 + 6 files changed, 6 insertions(+) diff --git a/pkg/settings/bool.go b/pkg/settings/bool.go index 242e347b332f..7a0f58a00bb7 100644 --- a/pkg/settings/bool.go +++ b/pkg/settings/bool.go @@ -76,6 +76,7 @@ var _ = (*BoolSetting).Default // // For testing usage only. func (b *BoolSetting) Override(ctx context.Context, sv *Values, v bool) { + sv.setValueOrigin(ctx, b.slot, OriginExplicitlySet) b.set(ctx, sv, v) sv.setDefaultOverride(b.slot, v) } diff --git a/pkg/settings/duration.go b/pkg/settings/duration.go index 59038e418818..e1cd27bccfc5 100644 --- a/pkg/settings/duration.go +++ b/pkg/settings/duration.go @@ -102,6 +102,7 @@ func (d *DurationSetting) Validate(v time.Duration) error { // // For testing usage only. func (d *DurationSetting) Override(ctx context.Context, sv *Values, v time.Duration) { + sv.setValueOrigin(ctx, d.slot, OriginExplicitlySet) sv.setInt64(ctx, d.slot, int64(v)) sv.setDefaultOverride(d.slot, v) } diff --git a/pkg/settings/float.go b/pkg/settings/float.go index aa3fda1dbadc..f7b17c1a106f 100644 --- a/pkg/settings/float.go +++ b/pkg/settings/float.go @@ -80,6 +80,7 @@ var _ = (*FloatSetting).Default // // For testing usage only. func (f *FloatSetting) Override(ctx context.Context, sv *Values, v float64) { + sv.setValueOrigin(ctx, f.slot, OriginExplicitlySet) if err := f.set(ctx, sv, v); err != nil { panic(err) } diff --git a/pkg/settings/int.go b/pkg/settings/int.go index 750615262b48..debf2098b62e 100644 --- a/pkg/settings/int.go +++ b/pkg/settings/int.go @@ -89,6 +89,7 @@ func (i *IntSetting) Validate(v int64) error { // // For testing usage only. func (i *IntSetting) Override(ctx context.Context, sv *Values, v int64) { + sv.setValueOrigin(ctx, i.slot, OriginExplicitlySet) sv.setInt64(ctx, i.slot, v) sv.setDefaultOverride(i.slot, v) } diff --git a/pkg/settings/protobuf.go b/pkg/settings/protobuf.go index cde455487001..cf460a3b494f 100644 --- a/pkg/settings/protobuf.go +++ b/pkg/settings/protobuf.go @@ -107,6 +107,7 @@ func (s *ProtobufSetting) Validate(sv *Values, p protoutil.Message) error { // Override sets the setting to the given value, assuming it passes validation. func (s *ProtobufSetting) Override(ctx context.Context, sv *Values, p protoutil.Message) { + sv.setValueOrigin(ctx, s.slot, OriginExplicitlySet) _ = s.set(ctx, sv, p) sv.setDefaultOverride(s.slot, p) } diff --git a/pkg/settings/string.go b/pkg/settings/string.go index 5c1d2d185bef..7eb518a753ac 100644 --- a/pkg/settings/string.go +++ b/pkg/settings/string.go @@ -81,6 +81,7 @@ func (s *StringSetting) Validate(sv *Values, v string) error { // Override sets the setting to the given value, assuming // it passes validation. func (s *StringSetting) Override(ctx context.Context, sv *Values, v string) { + sv.setValueOrigin(ctx, s.slot, OriginExplicitlySet) _ = s.set(ctx, sv, v) sv.setDefaultOverride(s.slot, v) }