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/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/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/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) } diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index e5023381573a..30b3723d1117 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. @@ -70,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. @@ -84,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 { @@ -92,37 +109,55 @@ func NewUpdater(sv *Values) Updater { return NoopUpdater{} } return updater{ - m: make(map[InternalKey]struct{}, len(registry)), sv: sv, } } -// 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.m[key] = struct{}{} - 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.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 { @@ -130,30 +165,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 @@ -161,34 +201,48 @@ 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.setValueOrigin(ctx, d.getSlot(), OriginDefault) + d.setToDefault(ctx, u.sv) return nil } // 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) - } else { - u.sv.setValueOrigin(ctx, v.getSlot(), OriginDefault) - } - + for _, v := range registry { 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 u.sv.getValueOrigin(ctx, v.getSlot()) == OriginDefault { v.setToDefault(ctx, u.sv) } } } -// 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) } diff --git a/pkg/settings/values.go b/pkg/settings/values.go index 09683e472b11..38284bc930dc 100644 --- a/pkg/settings/values.go +++ b/pkg/settings/values.go @@ -86,6 +86,7 @@ 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 }