settings: make .Override set the value origin

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",
     ],
 )
 

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

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)
 }

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)
 }

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)
 	}

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)
 }

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)
 }

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)
 		}
 	})

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)
 }

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,111 +93,156 @@ 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 {
 	if ignoreAllUpdates {
 		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 {
 			return err
 		}
 		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
 		// setting, changes to which are propagated through direct RPCs to each
 		// 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)
 }

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
 }