Merge #111008

111008: settings: fix the override behavior for string and proto settings r=yuzefovich a=knz Needed for #110758. Epic: CRDB-6671 Somehow no test was setting overrides on string and protobuf settings. It would not have worked. With this patch, it would. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Sep 21, 2023 · 49eef74 · 49eef74
2 parents afed367 + 0894f4e
commit 49eef74
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 3 deletions.
diff --git a/pkg/settings/BUILD.bazel b/pkg/settings/BUILD.bazel
@@ -60,6 +60,7 @@ go_test(
         "//pkg/util/protoutil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_redact//:redact",
+        "@com_github_gogo_protobuf//proto",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
     ],

diff --git a/pkg/settings/protobuf.go b/pkg/settings/protobuf.go
@@ -108,6 +108,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) {
 	_ = s.set(ctx, sv, p)
+	sv.setDefaultOverride(s.slot, p)
 }
 
 func (s *ProtobufSetting) set(ctx context.Context, sv *Values, p protoutil.Message) error {
@@ -121,6 +122,13 @@ func (s *ProtobufSetting) set(ctx context.Context, sv *Values, p protoutil.Messa
 }
 
 func (s *ProtobufSetting) setToDefault(ctx context.Context, sv *Values) {
+	// See if the default value was overridden.
+	if val := sv.getDefaultOverride(s.slot); val != nil {
+		// As per the semantics of override, these values don't go through
+		// validation.
+		_ = s.set(ctx, sv, val.(protoutil.Message))
+		return
+	}
 	if err := s.set(ctx, sv, s.defaultValue); err != nil {
 		panic(err)
 	}

diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
+	proto "github.com/gogo/protobuf/proto"
 	"github.com/stretchr/testify/require"
 )
 
@@ -36,6 +37,10 @@ type dummyVersion struct {
 	growsbyone string
 }
 
+func init() {
+	proto.RegisterType((*dummyVersion)(nil), "cockroach.dummyVersion")
+}
+
 var _ settings.ClusterVersionImpl = &dummyVersion{}
 
 func (d *dummyVersion) ClusterVersionImpl() {}
@@ -145,6 +150,7 @@ var changes = struct {
 	duA      int
 	eA       int
 	byteSize int
+	pA       int
 }{}
 
 var boolTA = settings.RegisterBoolSetting(settings.SystemOnly, "bool.t", "desc", true)
@@ -156,6 +162,7 @@ var i2A = settings.RegisterIntSetting(settings.TenantWritable, "i.2", "desc", 5)
 var fA = settings.RegisterFloatSetting(settings.TenantReadOnly, "f", "desc", 5.4)
 var dA = settings.RegisterDurationSetting(settings.TenantWritable, "d", "desc", time.Second)
 var duA = settings.RegisterDurationSettingWithExplicitUnit(settings.TenantWritable, "d_with_explicit_unit", "desc", time.Second, settings.NonNegativeDuration, settings.WithPublic)
+var pA = settings.RegisterProtobufSetting(settings.TenantWritable, "p", "desc", &dummyVersion{msg1: "foo"})
 var _ = settings.RegisterDurationSetting(settings.TenantWritable, "d_with_maximum", "desc", time.Second, settings.NonNegativeDurationWithMaximum(time.Hour))
 var eA = settings.RegisterEnumSetting(settings.SystemOnly, "e", "desc", "foo", map[int64]string{1: "foo", 2: "bar", 3: "baz"})
 var byteSize = settings.RegisterByteSizeSetting(settings.TenantWritable, "zzz", "desc", mb)
@@ -235,6 +242,7 @@ func TestCache(t *testing.T) {
 	dA.SetOnChange(sv, func(context.Context) { changes.dA++ })
 	duA.SetOnChange(sv, func(context.Context) { changes.duA++ })
 	eA.SetOnChange(sv, func(context.Context) { changes.eA++ })
+	pA.SetOnChange(sv, func(context.Context) { changes.pA++ })
 	byteSize.SetOnChange(sv, func(context.Context) { changes.byteSize++ })
 
 	t.Run("VersionSetting", func(t *testing.T) {
@@ -340,6 +348,10 @@ func TestCache(t *testing.T) {
 		if expected, actual := int64(1), eA.Get(sv); expected != actual {
 			t.Fatalf("expected %v, got %v", expected, actual)
 		}
+		{
+			expected, actual := (&dummyVersion{msg1: "foo"}), pA.Get(sv)
+			require.Equal(t, expected, actual)
+		}
 		// Note that we don't test the state-machine setting for a default, since it
 		// doesn't have one and it would crash.
 	})
@@ -462,6 +474,20 @@ func TestCache(t *testing.T) {
 			u.Set(ctx, "e", v("notAValidValue", "e")); !testutils.IsError(err, expected) {
 			t.Fatalf("expected '%s' != actual error '%s'", expected, err)
 		}
+
+		if expected, actual := 0, changes.pA; expected != actual {
+			t.Fatalf("expected %d, got %d", expected, actual)
+		}
+		testVal := &dummyVersion{msg1: "bar"}
+		encoded, err := testVal.Marshal()
+		require.NoError(t, err)
+		if err := u.Set(ctx, "p", v(string(encoded), "p")); err != nil {
+			t.Fatal(err)
+		}
+		if expected, actual := 1, changes.pA; expected != actual {
+			t.Fatalf("expected %d, got %d", expected, actual)
+		}
+
 		defaultDummyV := dummyVersion{msg1: "default", growsbyone: "AB"}
 		if err := setDummyVersion(defaultDummyV, mA, sv); err != nil {
 			t.Fatal(err)
@@ -510,6 +536,10 @@ func TestCache(t *testing.T) {
 		if expected, actual := "default.AB", mA.Encoded(sv); expected != actual {
 			t.Fatalf("expected %v, got %v", expected, actual)
 		}
+		{
+			expected, actual := (&dummyVersion{msg1: "bar"}), pA.Get(sv)
+			require.Equal(t, expected, actual)
+		}
 
 		// We didn't change this one, so should still see the default.
 		if expected, actual := "bar", strBarA.Get(sv); expected != actual {
@@ -766,6 +796,8 @@ var overrideBool = settings.RegisterBoolSetting(settings.SystemOnly, "override.b
 var overrideInt = settings.RegisterIntSetting(settings.TenantReadOnly, "override.int", "desc", 0)
 var overrideDuration = settings.RegisterDurationSetting(settings.TenantWritable, "override.duration", "desc", time.Second)
 var overrideFloat = settings.RegisterFloatSetting(settings.TenantWritable, "override.float", "desc", 1.0)
+var overrideString = settings.RegisterStringSetting(settings.TenantWritable, "override.string", "desc", "foo")
+var overrideProto = settings.RegisterProtobufSetting(settings.TenantWritable, "override.proto", "desc", &dummyVersion{msg1: "foo"})
 
 func TestOverride(t *testing.T) {
 	ctx := context.Background()
@@ -800,6 +832,21 @@ func TestOverride(t *testing.T) {
 	require.Equal(t, 42.0, overrideFloat.Get(sv))
 	u.ResetRemaining(ctx)
 	require.Equal(t, 42.0, overrideFloat.Get(sv))
+
+	// Test override for string setting.
+	require.Equal(t, "foo", overrideString.Get(sv))
+	overrideString.Override(ctx, sv, "bar")
+	require.Equal(t, "bar", overrideString.Get(sv))
+	u.ResetRemaining(ctx)
+	require.Equal(t, "bar", overrideString.Get(sv))
+
+	// Test override for proto setting.
+	require.Equal(t, &dummyVersion{msg1: "foo"}, overrideProto.Get(sv))
+	overrideProto.Override(ctx, sv, &dummyVersion{msg1: "bar"})
+	require.Equal(t, &dummyVersion{msg1: "bar"}, overrideProto.Get(sv))
+	u.ResetRemaining(ctx)
+	require.Equal(t, &dummyVersion{msg1: "bar"}, overrideProto.Get(sv))
+
 }
 
 func TestSystemOnlyDisallowedOnVirtualCluster(t *testing.T) {

diff --git a/pkg/settings/string.go b/pkg/settings/string.go
@@ -82,6 +82,7 @@ func (s *StringSetting) Validate(sv *Values, v string) error {
 // it passes validation.
 func (s *StringSetting) Override(ctx context.Context, sv *Values, v string) {
 	_ = s.set(ctx, sv, v)
+	sv.setDefaultOverride(s.slot, v)
 }
 
 func (s *StringSetting) set(ctx context.Context, sv *Values, v string) error {
@@ -95,6 +96,13 @@ func (s *StringSetting) set(ctx context.Context, sv *Values, v string) error {
 }
 
 func (s *StringSetting) setToDefault(ctx context.Context, sv *Values) {
+	// See if the default value was overridden.
+	if val := sv.getDefaultOverride(s.slot); val != nil {
+		// As per the semantics of override, these values don't go through
+		// validation.
+		_ = s.set(ctx, sv, val.(string))
+		return
+	}
 	if err := s.set(ctx, sv, s.defaultValue); err != nil {
 		panic(err)
 	}

diff --git a/pkg/settings/version.go b/pkg/settings/version.go
@@ -174,9 +174,6 @@ func (v *VersionSetting) SetInternal(ctx context.Context, sv *Values, newVal int
 // setToDefault is part of the extendingSetting interface. This is a no-op for
 // VersionSetting. They don't have defaults that they can go back to at any
 // time.
-//
-// TODO(irfansharif): Is this true? Shouldn't the default here just the the
-// version we initialize with?
 func (v *VersionSetting) setToDefault(ctx context.Context, sv *Values) {}
 
 // RegisterVersionSetting adds the provided version setting to the global