diff --git a/pkg/server/tenantsettingswatcher/BUILD.bazel b/pkg/server/tenantsettingswatcher/BUILD.bazel index 0b2286525bcf..a116c5b02b83 100644 --- a/pkg/server/tenantsettingswatcher/BUILD.bazel +++ b/pkg/server/tenantsettingswatcher/BUILD.bazel @@ -27,6 +27,7 @@ go_library( "//pkg/sql/sem/tree", "//pkg/sql/types", "//pkg/util", + "//pkg/util/buildutil", "//pkg/util/hlc", "//pkg/util/log", "//pkg/util/startup", @@ -58,6 +59,7 @@ go_test( "//pkg/settings", "//pkg/sql", "//pkg/sql/catalog", + "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", diff --git a/pkg/server/tenantsettingswatcher/overrides_store.go b/pkg/server/tenantsettingswatcher/overrides_store.go index e3581bba7aaa..552f9e5dca45 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store.go +++ b/pkg/server/tenantsettingswatcher/overrides_store.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/util" + "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/errors" @@ -45,6 +46,16 @@ type overridesStore struct { // this ever becomes a problem, we can periodically purge entries with no // overrides. tenants map[roachpb.TenantID]*tenantOverrides + + // alternateDefaults defines values to use for the + // allTenantOverrides settings when there is no override set via + // tenant_settings. + // + // The slice is sorted by InternalKey. + // + // At the time of this writing, this is used for TenantReadOnly + // settings. + alternateDefaults []kvpb.TenantSetting } } @@ -54,26 +65,44 @@ type tenantOverrides struct { // overrides, ordered by InternalKey. overrides []kvpb.TenantSetting + // explicitKeys contains override keys that got their value from the + // rangefeed over system.tenant_settings, i.e. NOT those inherited + // from alternateDefaults. + // + // This map is used when an alternate default is changed + // asynchronously after the overrides have been loaded from the + // rangefeed already, to detect which overrides need to be updated + // from alternate defaults vs those that need to stay as-is (because + // they come from an override in .tenant_settings). + explicitKeys map[settings.InternalKey]struct{} + // changeCh is a channel that is closed when the tenant overrides change (in // which case a new tenantOverrides object will contain the updated settings). changeCh chan struct{} } func newTenantOverrides( - ctx context.Context, tenID roachpb.TenantID, overrides []kvpb.TenantSetting, + ctx context.Context, + tenID roachpb.TenantID, + overrides []kvpb.TenantSetting, + explicitKeys map[settings.InternalKey]struct{}, ) *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) + if _, exists := explicitKeys[v.InternalKey]; exists { + buf.Printf(" (explicit)") + } buf.SafeRune('\n') } log.VEventf(ctx, 1, "%v", buf) } return &tenantOverrides{ - overrides: overrides, - changeCh: make(chan struct{}), + overrides: overrides, + explicitKeys: explicitKeys, + changeCh: make(chan struct{}), } } @@ -106,13 +135,31 @@ func (s *overridesStore) setAll( sort.Slice(overrides, func(i, j int) bool { return overrides[i].InternalKey < overrides[j].InternalKey }) - // Sanity check. - for i := 1; i < len(overrides); i++ { - if overrides[i].InternalKey == overrides[i-1].InternalKey { - panic(errors.AssertionFailedf("duplicate setting: %s", overrides[i].InternalKey)) - } + + providedKeys := make(map[settings.InternalKey]struct{}, len(overrides)) + for _, v := range overrides { + providedKeys[v.InternalKey] = struct{}{} + } + // If we are setting the all-tenant overrides, ensure there is a + // pseudo-override for every TenantReadOnly setting with an + // alternate default. + if tenantID == allTenantOverridesID && len(s.mu.alternateDefaults) > 0 { + // We can set copyOverrides==false because we took ownership + // over the incoming allOverrides map. + overrides = spliceOverrideDefaults(providedKeys, overrides, s.mu.alternateDefaults, false /* copyOverrides */) + } + s.mu.tenants[tenantID] = newTenantOverrides(ctx, tenantID, overrides, providedKeys) + } +} + +func checkSortedByKey(a []kvpb.TenantSetting) { + if !buildutil.CrdbTestBuild { + return + } + for i := 1; i < len(a); i++ { + if a[i].InternalKey <= a[i-1].InternalKey { + panic(errors.AssertionFailedf("duplicate setting: %s", a[i].InternalKey)) } - s.mu.tenants[tenantID] = newTenantOverrides(ctx, tenantID, overrides) } } @@ -140,7 +187,13 @@ func (s *overridesStore) getTenantOverrides( if res, ok = s.mu.tenants[tenantID]; ok { return res } - res = newTenantOverrides(ctx, tenantID, nil /* overrides */) + var overrides []kvpb.TenantSetting + if tenantID == allTenantOverridesID { + // Inherit alternate defaults. + overrides = make([]kvpb.TenantSetting, len(s.mu.alternateDefaults)) + copy(overrides, s.mu.alternateDefaults) + } + res = newTenantOverrides(ctx, tenantID, overrides, nil /* explicitKeys */) s.mu.tenants[tenantID] = res return res } @@ -154,10 +207,15 @@ func (s *overridesStore) setTenantOverride( s.mu.Lock() defer s.mu.Unlock() var before []kvpb.TenantSetting + var providedKeys map[settings.InternalKey]struct{} if existing, ok := s.mu.tenants[tenantID]; ok { before = existing.overrides + providedKeys = existing.explicitKeys close(existing.changeCh) } + if providedKeys == nil { + providedKeys = make(map[settings.InternalKey]struct{}) + } after := make([]kvpb.TenantSetting, 0, len(before)+1) // 1. Add all settings up to setting.InternalKey. for len(before) > 0 && before[0].InternalKey < setting.InternalKey { @@ -167,6 +225,19 @@ func (s *overridesStore) setTenantOverride( // 2. Add the after setting, unless we are removing it. if setting.Value != (settings.EncodedValue{}) { after = append(after, setting) + providedKeys[setting.InternalKey] = struct{}{} + } else { + // The override is being removed. If we have an alternate default, + // use it instead. + delete(providedKeys, setting.InternalKey) + if tenantID == allTenantOverridesID { + if defaultVal, ok := findAlternateDefault(setting.InternalKey, s.mu.alternateDefaults); ok { + after = append(after, kvpb.TenantSetting{ + InternalKey: setting.InternalKey, + Value: defaultVal, + }) + } + } } // Skip any existing setting for this setting key. if len(before) > 0 && before[0].InternalKey == setting.InternalKey { @@ -174,5 +245,199 @@ func (s *overridesStore) setTenantOverride( } // 3. Append all settings after setting.InternalKey. after = append(after, before...) - s.mu.tenants[tenantID] = newTenantOverrides(ctx, tenantID, after) + + // Sanity check. + checkSortedByKey(after) + + s.mu.tenants[tenantID] = newTenantOverrides(ctx, tenantID, after, providedKeys) +} + +// setAlternateDefaults defines alternate defaults, to use +// when there is no stored default in .tenant_settings. +// +// At the time of this writing, this is called when a change is made +// to a TenantReadOnly setting in the system tenant's system.settings +// table. Values set this way serve as default value if there is no +// override in system.tenant_settings. +// +// The input slice must be sorted by key already. +func (s *overridesStore) setAlternateDefaults( + ctx context.Context, alternateDefaultSlice []kvpb.TenantSetting, +) { + // Sanity check. + checkSortedByKey(alternateDefaultSlice) + + s.mu.Lock() + defer s.mu.Unlock() + + alternateDefaults := s.mu.alternateDefaults + alternateDefaults = updateDefaults(alternateDefaults, alternateDefaultSlice) + s.mu.alternateDefaults = alternateDefaults + + // Inject the new read-only default into the all-tenants + // override map. + var overrides []kvpb.TenantSetting + var explicitKeys map[settings.InternalKey]struct{} + copyOverrides := false + if existing, ok := s.mu.tenants[allTenantOverridesID]; ok { + explicitKeys = existing.explicitKeys + overrides = existing.overrides + // Need to pass copyOverrides==true because we don't want to + // modify the overrides slice in-place -- there may be listeners + // that are consuming the slice asynchronously. + copyOverrides = true + close(existing.changeCh) + } + + overrides = spliceOverrideDefaults(explicitKeys, overrides, alternateDefaults, copyOverrides) + s.mu.tenants[allTenantOverridesID] = newTenantOverrides(ctx, allTenantOverridesID, overrides, explicitKeys) +} + +// findAlternateDefault searches the value associated with the given key +// in the alternate default slice, which is assumed to be sorted. +func findAlternateDefault( + key settings.InternalKey, defaults []kvpb.TenantSetting, +) (val settings.EncodedValue, found bool) { + idx := sort.Search(len(defaults), func(i int) bool { + return defaults[i].InternalKey >= key + }) + if idx >= len(defaults) { + return val, false + } + if defaults[idx].InternalKey != key { + return val, false + } + return defaults[idx].Value, true +} + +// spliceOverrideDefaults adds overrides into the 'overrides' slice +// for each key that doesn't have an override yet but is present in +// alternateDefaults. +// +// If the key already has an override, but wasn't set explicitly via +// the rangefeed from tenant_settings (as informed by explicitKeys), +// this means the override was already a fallback to the alternate +// default before; in which case we update it to the new value of the +// alternate default. +// +// The alternateDefaults slice must be sorted already. +// +// If the copyOverrides flag is set, the overrides slice is modified +// in-place. If false, it is copied first. This is necessary when +// updating the overrides slice that was already stored in the tenants +// map before (it can be accessed concurrently). +func spliceOverrideDefaults( + explicitKeys map[settings.InternalKey]struct{}, + overrides []kvpb.TenantSetting, + alternateDefaults []kvpb.TenantSetting, + copyOverrides bool, +) []kvpb.TenantSetting { + if copyOverrides && len(overrides) > 0 { + dst := make([]kvpb.TenantSetting, len(overrides)) + copy(dst, overrides) + overrides = dst + } + aIter, bIter := 0, 0 + for aIter < len(overrides) && bIter < len(alternateDefaults) { + if overrides[aIter].InternalKey == alternateDefaults[bIter].InternalKey { + if _, ok := explicitKeys[overrides[aIter].InternalKey]; !ok { + // The key is not explicitly set (via the rangefeed), this means + // that we were relying on the default to start with. + // Update the override from the new value of the default. + overrides[aIter] = alternateDefaults[bIter] + } + aIter++ + bIter++ + } else if overrides[aIter].InternalKey < alternateDefaults[bIter].InternalKey { + if _, ok := explicitKeys[overrides[aIter].InternalKey]; !ok { + // The key is not explicitly set (via the rangefeed), this + // means that we were relying on the default to start with. + // But now, there is no default any more. Remove the override. + copy(overrides[aIter:], overrides[aIter+1:]) + overrides = overrides[:len(overrides)-1] + } else { + aIter++ + } + } else { + // The following is an optimization, to append as many missing elements + // from the alternateDefaults slice as possible in one go. + // This can be implemented also (albeit less efficiently) as: + // tail := overrides[aIter:] + // dst = append(overrides[:aIter:aIter], alternateDefaults[bIter]) + // overrides = append(overrides, tail...) + // aIter++ + // bIter++ + // continue + numOverrides := 0 + for ; (bIter+numOverrides) < len(alternateDefaults) && + overrides[aIter].InternalKey > alternateDefaults[bIter+numOverrides].InternalKey; numOverrides++ { + } + tail := overrides[aIter:] + overrides = append(overrides[:aIter:aIter], alternateDefaults[bIter:bIter+numOverrides]...) + overrides = append(overrides, tail...) + aIter += numOverrides + bIter += numOverrides + } + } + for aIter < len(overrides) { + if _, ok := explicitKeys[overrides[aIter].InternalKey]; !ok { + // The key is not explicitly set (via the rangefeed), this + // means that we were relying on the default to start with. + // But now, there is no default any more. Remove the override. + copy(overrides[aIter:], overrides[aIter+1:]) + overrides = overrides[:len(overrides)-1] + } else { + aIter++ + } + } + if bIter < len(alternateDefaults) { + overrides = append(overrides, alternateDefaults[bIter:]...) + } + // Sanity check. + checkSortedByKey(overrides) + return overrides +} + +// updateDefaults extends the slice in the first argument with the +// elements in the second argument. If the same setting key is present +// in both, the first slice is updated from the second. +// +// NB: the dst slice is modified in-place. +func updateDefaults(dst, src []kvpb.TenantSetting) []kvpb.TenantSetting { + aIter, bIter := 0, 0 + for aIter < len(dst) && bIter < len(src) { + if dst[aIter].InternalKey == src[bIter].InternalKey { + dst[aIter] = src[bIter] + aIter++ + bIter++ + } else if dst[aIter].InternalKey < src[bIter].InternalKey { + aIter++ + } else { + // The following is an optimization, to append as many missing elements + // from the src slice as possible in one go. + // This can be implemented also (albeit less efficiently) as: + // tail := dst[aIter:] + // dst = append(dst[:aIter:aIter], src[bIter]) + // dst = append(dst, tail...) + // aIter++ + // bIter++ + // continue + numDst := 0 + for ; (bIter+numDst) < len(src) && + dst[aIter].InternalKey > src[bIter+numDst].InternalKey; numDst++ { + } + tail := dst[aIter:] + dst = append(dst[:aIter:aIter], src[bIter:bIter+numDst]...) + dst = append(dst, tail...) + aIter += numDst + bIter += numDst + } + } + if bIter < len(src) { + dst = append(dst, src[bIter:]...) + } + + // Sanity check. + checkSortedByKey(dst) + return dst } diff --git a/pkg/server/tenantsettingswatcher/overrides_store_test.go b/pkg/server/tenantsettingswatcher/overrides_store_test.go index aea1c0628bd6..51cd9e1d63fc 100644 --- a/pkg/server/tenantsettingswatcher/overrides_store_test.go +++ b/pkg/server/tenantsettingswatcher/overrides_store_test.go @@ -20,7 +20,9 @@ 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/testutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/stretchr/testify/require" ) func TestOverridesStore(t *testing.T) { @@ -31,14 +33,6 @@ func TestOverridesStore(t *testing.T) { s.Init() t1 := roachpb.MustMakeTenantID(1) t2 := roachpb.MustMakeTenantID(2) - st := func(key settings.InternalKey, val string) kvpb.TenantSetting { - return kvpb.TenantSetting{ - InternalKey: key, - Value: settings.EncodedValue{ - Value: val, - }, - } - } expect := func(o *tenantOverrides, expected string) { t.Helper() var vals []string @@ -89,4 +83,184 @@ func TestOverridesStore(t *testing.T) { s.setTenantOverride(ctx, t3, st("x", "xx")) o3 := s.getTenantOverrides(ctx, t3) expect(o3, "x=xx") + + // Verify that overrides also work for the special "all tenants" ID. + expect(s.getTenantOverrides(ctx, allTenantOverridesID), "") + s.setTenantOverride(ctx, allTenantOverridesID, st("a", "aa")) + s.setTenantOverride(ctx, allTenantOverridesID, st("d", "dd")) + expect(s.getTenantOverrides(ctx, allTenantOverridesID), "a=aa d=dd") + + // Set an alternate default. + s.setAlternateDefaults(ctx, []kvpb.TenantSetting{st("d", "d2")}) + + // If a custom override is provided, the alternate default is + // hidden. + expect(s.getTenantOverrides(ctx, allTenantOverridesID), "a=aa d=dd") + + // Alternate defaults do not show up for regular tenant IDs. + expect(s.getTenantOverrides(ctx, t3), "x=xx") + + // If the custom override is removed, the alternate appears. + s.setTenantOverride(ctx, allTenantOverridesID, kvpb.TenantSetting{InternalKey: "d"}) + expect(s.getTenantOverrides(ctx, allTenantOverridesID), "a=aa d=d2") + + // If there was no override to start with, the new alternate is + // added as pseudo-override. If not mentioned in new calls to + // `setAlternateDefaults()`, existing alternate defaults are + // preserved. + s.setAlternateDefaults(ctx, []kvpb.TenantSetting{st("e", "ee")}) + expect(s.getTenantOverrides(ctx, allTenantOverridesID), "a=aa d=d2 e=ee") + + // If a custom override is added again, the alternate default gets + // hidden again. + s.setTenantOverride(ctx, allTenantOverridesID, st("d", "dd")) + expect(s.getTenantOverrides(ctx, allTenantOverridesID), "a=aa d=dd e=ee") +} + +func st(key settings.InternalKey, val string) kvpb.TenantSetting { + return kvpb.TenantSetting{ + InternalKey: key, + Value: settings.EncodedValue{ + Value: val, + }, + } +} + +func TestFindAlternateDefault(t *testing.T) { + defer leaktest.AfterTest(t)() + + type kts = kvpb.TenantSetting + + for idx, tc := range []struct { + defaults []kvpb.TenantSetting + key settings.InternalKey + + expectedVal string + }{ + {[]kts{st("a", "aa"), st("c", "cc")}, "d", ""}, + {[]kts{st("a", "aa"), st("c", "cc")}, "b", ""}, + {[]kts{st("c", "cc"), st("d", "dd")}, "a", ""}, + {[]kts{st("a", "aa"), st("c", "cc")}, "a", "aa"}, + {[]kts{st("a", "aa"), st("c", "cc")}, "c", "cc"}, + } { + t.Run(fmt.Sprint(idx), func(t *testing.T) { + val, found := findAlternateDefault(tc.key, tc.defaults) + require.Equal(t, tc.expectedVal != "", found) + require.Equal(t, tc.expectedVal, val.Value) + }) + } +} + +func TestUpdateDefaults(t *testing.T) { + defer leaktest.AfterTest(t)() + + type kts = kvpb.TenantSetting + + for idx, tc := range []struct { + defaults []kvpb.TenantSetting + moreDefaults []kvpb.TenantSetting + + expected []kvpb.TenantSetting + }{ + // Empty case as sanity check. + {nil, nil, nil}, + // No defaults to start with: produce at least the new defaults. + {nil, []kts{st("a", "aa")}, []kts{st("a", "aa")}}, + // Some defaults already known. + // update from the new defaults. + {[]kts{st("a", "aa")}, []kts{st("a", "bb")}, []kts{st("a", "bb")}}, + + // Some defaults known. No new defaults. + {[]kts{st("a", "aa")}, nil, []kts{st("a", "aa")}}, + + // Alternatives to the above, with more elements. + {[]kts{st("a", "aa"), st("c", "cc")}, + []kts{st("b", "bb")}, + []kts{st("a", "aa"), st("b", "bb"), st("c", "cc")}}, + {[]kts{st("a", "aa"), st("c", "cc")}, + []kts{st("b", "bb"), st("c", "c2"), st("d", "dd")}, + []kts{st("a", "aa"), st("b", "bb"), st("c", "c2"), st("d", "dd")}}, + {[]kts{st("b", "bb"), st("d", "dd")}, + // Edge case: the number of alternate defaults before the first + // explicit value, is larger than the number of explicit values + // to start with. This test catches a pitfall in the splice + // logic. + []kts{st("a", "aa"), st("a2", "aa"), st("a3", "aa"), + st("b", "b2"), st("c", "cc"), st("c2", "cc"), st("d", "d2"), st("e", "ee")}, + []kts{st("a", "aa"), st("a2", "aa"), st("a3", "aa"), + st("b", "b2"), st("c", "cc"), st("c2", "cc"), st("d", "d2"), st("e", "ee")}}, + } { + t.Run(fmt.Sprint(idx), func(t *testing.T) { + result := updateDefaults(tc.defaults, tc.moreDefaults) + require.Equal(t, tc.expected, result) + }) + } +} + +func TestSpliceOverrideDefaults(t *testing.T) { + defer leaktest.AfterTest(t)() + + mexp := func(keys ...settings.InternalKey) map[settings.InternalKey]struct{} { + m := make(map[settings.InternalKey]struct{}, len(keys)) + for _, k := range keys { + m[k] = struct{}{} + } + return m + } + + type kts = kvpb.TenantSetting + + for idx, tc := range []struct { + explicitKeys map[settings.InternalKey]struct{} + overrides []kvpb.TenantSetting + alternateDefaults []kvpb.TenantSetting + + expected []kvpb.TenantSetting + }{ + // Empty case as sanity check. + {nil, nil, nil, nil}, + // No overrides to start with: produce at least the alternate defaults. + {nil, nil, []kts{st("a", "aa")}, []kts{st("a", "aa")}}, + // Overrides previously set from alternate defaults (no explicitKeys): + // update from the new alternate defaults. + {nil, []kts{st("a", "aa")}, []kts{st("a", "bb")}, []kts{st("a", "bb")}}, + // Overrides previously set from rangefeed (key in explicitKeys): + // keep the override. + {mexp("a"), []kts{st("a", "aa")}, []kts{st("a", "bb")}, []kts{st("a", "aa")}}, + + // Override previously set from alternate default (no explicitKeys) + // and the alternate default is removed. Remove the override. + {nil, []kts{st("a", "aa")}, nil, []kts{}}, + + // Alternatives to the above, with more elements. + {nil, + []kts{st("a", "aa"), st("c", "cc")}, + []kts{st("b", "bb")}, + []kts{st("b", "bb")}}, + {mexp("a", "c"), + []kts{st("a", "aa"), st("c", "cc")}, + []kts{st("b", "bb")}, + []kts{st("a", "aa"), st("b", "bb"), st("c", "cc")}}, + {mexp("a"), + []kts{st("a", "aa"), st("c", "cc")}, + []kts{st("b", "bb"), st("c", "c2"), st("d", "dd")}, + []kts{st("a", "aa"), st("b", "bb"), st("c", "c2"), st("d", "dd")}}, + {mexp("b", "d"), + []kts{st("b", "bb"), st("d", "dd")}, + // Edge case: the number of alternate defaults before the first + // explicit value, is larger than the number of explicit values + // to start with. This test catches a pitfall in the splice + // logic. + []kts{st("a", "aa"), st("a2", "aa"), st("a3", "aa"), + st("b", "b2"), st("c", "cc"), st("c2", "cc"), st("d", "d2"), st("e", "ee")}, + []kts{st("a", "aa"), st("a2", "aa"), st("a3", "aa"), + st("b", "bb"), st("c", "cc"), st("c2", "cc"), st("d", "dd"), st("e", "ee")}}, + } { + testutils.RunTrueAndFalse(t, "copy", func(t *testing.T, copy bool) { + t.Run(fmt.Sprint(idx), func(t *testing.T) { + result := spliceOverrideDefaults(tc.explicitKeys, tc.overrides, tc.alternateDefaults, copy) + require.Equal(t, tc.expected, result) + }) + }) + } }