tenantsettingswatcher: implement watcher and integrate into server by RaduBerinde · Pull Request #76445 · cockroachdb/cockroach

RaduBerinde · 2022-02-11T20:33:12Z

~~For now, we hardcode 50 as the system table ID. I plan to work on that and add a commit to this PR. Adding do-not-merge label until then.~~

tenantsettingswatcher: in-memory data structure

This commit introduces the overrides store, the in-memory data
structure that will hold all tenant overrides.

The data structure is designed for efficient read access and to allow
multiple goroutines to get notified of changes (via a channel close).

Release note: None

tenantsettingswatcher: implement watcher

This commit implements the tenantsettingswatcher, which monitors the
tenant_settings table and allows callers to access the current
overrides and listen for changes.

For now, we hardcode the tenant_settings system table ID to 50 (which
will be the case in fresh clusters). We will fix this in a follow-up
change.

Release note: None

tenantsettingswatcher: integrate into server

This commit integrates the tenant settings watcher into the server and
implements the TenantSettings API.

Release note: None

logictestccl: add tenant_settings test

This commit adds functionality to logic tests to allow running SQL
against the host cluster in a tenant configuration. This allows us to
manipulate the tenant_settings table and observe the settings changing
on the tenant.

We add a tenant_settings test that verifies basic overrides behavior.
This test can become more comprehensive once we implement the new
ALTER TENANT settings.

Release note: None

catalog: add SystemTableIDResolver

This commit adds the SystemTableIDResolver interface and its
implementation, and modifies tenantsettingswatcher to use it for the
tenant_settings table.

Release note: None

cockroach-teamcity · 2022-02-11T20:33:22Z

This change is

RaduBerinde · 2022-02-12T19:26:25Z

@ajwerner I added a commit that adds SystemTableIDResolver (both interface and implementation).

RaduBerinde · 2022-02-13T04:02:36Z

I am looking at TestTenantUpgrade:

// TestTenantUpgrade exercises the case where a system tenant is in a
// non-finalized version state and creates a tenant. The test ensures
// that that newly created tenant begins in that same version.
//
// The first subtest creates the tenant in the mixed version state,
// then upgrades the system tenant, then upgrades the secondary tenant,
// and ensures everything is happy. It then restarts the tenant and ensures
// that the cluster version is properly set.

It doesn't work with this PR because the tenant requests the settings regardless of its version, and the host can't satisfy that request until it is upgraded.

I wonder what the right thing to do is here. Threading the tenant cluster version through to the kvtenantccl.Connector code seems a bit strange. I could make the host side return empty overrides if the host version is old, but that won't solve the case where the host is running older code (maybe that doesn't matter since that's not how we do multi-tenant updates).

RaduBerinde · 2022-02-13T04:28:17Z

I could make the host side return empty overrides if the host version is old,

I did that, I think that makes the most sense. The overrides will automatically start getting updated once the version is upgraded.

This commit introduces the overrides store, the in-memory data structure that will hold all tenant overrides. The data structure is designed for efficient read access and to allow multiple goroutines to get notified of changes (via a channel close). Release note: None

ajwerner

LGTM

ajwerner · 2022-02-16T00:04:44Z

pkg/sql/catalog/descs/system_table.go

+) (descpb.ID, error) {
+	var id descpb.ID
+	err := r.collectionFactory.Txn(ctx, r.ie, r.db, func(ctx context.Context, txn *kv.Txn, descriptors *Collection) error {
+		_, desc, err := descriptors.GetImmutableTableByName(


Now that you're here, you can use the lower-level call to get what you need. This will also avoid the lookup against kv.

var id descpb.ID if err := r.collectionFactory.Txn(ctx, r.ie, r.db, func( ctx context.Context, txn *kv.Txn, descriptors *Collection, ) (err error) { id, err = descriptors.kv.lookupName( ctx, txn, nil /* maybeDatabase */, keys.SystemDatabaseID, keys.PublicSchemaID, tableName, ) return err }); err != nil { return 0, err } return id, nil

ajwerner · 2022-02-16T00:08:37Z

pkg/server/tenantsettingswatcher/overrides_store.go

+//
+// The caller can listen for closing of changeCh, which is guaranteed to happen
+// if the tenant's overrides change.
+func (s *overridesStore) GetTenantOverrides(tenantID roachpb.TenantID) *tenantOverrides {


why export this?

ajwerner · 2022-02-16T00:08:45Z

pkg/server/tenantsettingswatcher/overrides_store.go

+//
+// This method is called once we complete a full initial scan of the
+// tenant_setting table.
+func (s *overridesStore) SetAll(allOverrides map[roachpb.TenantID][]roachpb.TenantSetting) {


why export this?

ajwerner · 2022-02-16T00:08:49Z

pkg/server/tenantsettingswatcher/overrides_store.go

+	}
+}
+
+func (s *overridesStore) Init() {


why export this?

ajwerner · 2022-02-16T00:08:59Z

pkg/server/tenantsettingswatcher/overrides_store.go

+// SetTenantOverride changes an override for the given tenant. If the setting
+// has an empty value, the existing override is removed; otherwise a new
+// override is added.
+func (s *overridesStore) SetTenantOverride(


why export this?

ajwerner · 2022-02-16T00:09:44Z

pkg/server/tenantsettingswatcher/watcher.go

+
+// GetTenantOverrides returns the current overrides for the given tenant, and a
+// channel that will be closed when the overrides for this tenant change.
+func (w *Watcher) GetTenantOverrides(


Note that the returned slice is shared and must not be modified

ajwerner · 2022-02-16T00:09:46Z

pkg/server/tenantsettingswatcher/watcher.go

+// GetAllTenantOverrides returns the current overrides for all tenants.
+// Each of these overrides apply to any tenant, as long as that tenant doesn't
+// have an override for the same setting.
+func (w *Watcher) GetAllTenantOverrides() (


Note that the returned slice is shared and must not be modified

This commit implements the tenantsettingswatcher, which monitors the tenant_settings table and allows callers to access the current overrides and listen for changes. For now, we hardcode the tenant_settings system table ID to 50 (which will be the case in fresh clusters). We will fix this in a follow-up change. Release note: None

This commit integrates the tenant settings watcher into the server and implements the TenantSettings API. Release note: None

This commit adds functionality to logic tests to allow running SQL against the host cluster in a tenant configuration. This allows us to manipulate the tenant_settings table and observe the settings changing on the tenant. We add a tenant_settings test that verifies basic overrides behavior. This test can become more comprehensive once we implement the new ALTER TENANT settings. Release note: None

This commit adds the SystemTableIDResolver interface and its implementation, and modifies tenantsettingswatcher to use it for the `tenant_settings` table. Release note: None

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @dt)

pkg/server/tenantsettingswatcher/overrides_store.go, line 63 at r10 (raw file):

Previously, ajwerner wrote…

why export this?

It's not really, since the type itself is not exported. It's just meant to differentiate the "API" of the datastructure from the internals.

pkg/server/tenantsettingswatcher/watcher.go, line 271 at r10 (raw file):

Previously, ajwerner wrote…

Note that the returned slice is shared and must not be modified

Done.

pkg/server/tenantsettingswatcher/watcher.go, line 281 at r10 (raw file):

Previously, ajwerner wrote…

Note that the returned slice is shared and must not be modified

Done.

pkg/sql/catalog/descs/system_table.go, line 49 at r10 (raw file):

Previously, ajwerner wrote…

Now that you're here, you can use the lower-level call to get what you need. This will also avoid the lookup against kv.

var id descpb.ID
if err := r.collectionFactory.Txn(ctx, r.ie, r.db, func(
    ctx context.Context, txn *kv.Txn, descriptors *Collection,
) (err error) {
		id, err = descriptors.kv.lookupName(
    		ctx, txn, nil /* maybeDatabase */, keys.SystemDatabaseID, keys.PublicSchemaID, tableName,
		)
		return err
}); err != nil {
    return 0, err
}
return id, nil

Done.

RaduBerinde

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @dt)

RaduBerinde · 2022-02-16T04:19:02Z

bors r+

craig · 2022-02-16T06:00:29Z

Build succeeded:

GitHub CI (Cockroach)

RaduBerinde added the do-not-merge bors won't merge a PR with this label. label Feb 11, 2022

RaduBerinde requested review from ajwerner and dt February 11, 2022 20:33

RaduBerinde force-pushed the tenant-settings-watcher branch 2 times, most recently from 1d5f1af to a4c22c5 Compare February 11, 2022 23:51

RaduBerinde requested review from a team as code owners February 12, 2022 19:25

RaduBerinde requested review from a team and erikgrinaker and removed request for a team February 12, 2022 19:25

RaduBerinde removed the do-not-merge bors won't merge a PR with this label. label Feb 12, 2022

RaduBerinde force-pushed the tenant-settings-watcher branch 2 times, most recently from 9e86e0c to 050c62d Compare February 13, 2022 03:19

RaduBerinde force-pushed the tenant-settings-watcher branch from 050c62d to 0d2d1ca Compare February 13, 2022 04:27

erikgrinaker removed their request for review February 13, 2022 10:33

RaduBerinde force-pushed the tenant-settings-watcher branch from 0d2d1ca to c8aac71 Compare February 14, 2022 22:24

RaduBerinde requested a review from ajstorm February 15, 2022 16:51

RaduBerinde force-pushed the tenant-settings-watcher branch 2 times, most recently from ba2ff40 to 16e7d5c Compare February 15, 2022 19:06

RaduBerinde force-pushed the tenant-settings-watcher branch 2 times, most recently from c1e2148 to 348cdc7 Compare February 15, 2022 23:20

ajwerner approved these changes Feb 16, 2022

View reviewed changes

RaduBerinde added 3 commits February 15, 2022 18:05

tenantsettingswatcher: integrate into server

baa41f3

This commit integrates the tenant settings watcher into the server and implements the TenantSettings API. Release note: None

catalog: add SystemTableIDResolver

f6dbccc

This commit adds the SystemTableIDResolver interface and its implementation, and modifies tenantsettingswatcher to use it for the `tenant_settings` table. Release note: None

RaduBerinde force-pushed the tenant-settings-watcher branch from 348cdc7 to f6dbccc Compare February 16, 2022 02:08

RaduBerinde commented Feb 16, 2022

View reviewed changes

craig bot merged commit 6d9ceec into cockroachdb:master Feb 16, 2022

RaduBerinde deleted the tenant-settings-watcher branch February 16, 2022 15:31

rafiss mentioned this pull request Feb 16, 2022

sql/pgwire: TestPGTest failed #76655

Closed

RaduBerinde mentioned this pull request Feb 17, 2022

settings: multi-tenant cluster settings tracking issue #73857

Closed

16 tasks

erikgrinaker mentioned this pull request Feb 19, 2022

server: TestUpgradeHappensAfterMigrations failed [unexpected stopped rangefeed processor] #76649

Closed

Conversation

RaduBerinde commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tenantsettingswatcher: in-memory data structure

tenantsettingswatcher: implement watcher

tenantsettingswatcher: integrate into server

logictestccl: add tenant_settings test

catalog: add SystemTableIDResolver

Uh oh!

cockroach-teamcity commented Feb 11, 2022

Uh oh!

RaduBerinde commented Feb 12, 2022

Uh oh!

RaduBerinde commented Feb 13, 2022

Uh oh!

RaduBerinde commented Feb 13, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ajwerner Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Feb 16, 2022

Uh oh!

craig bot commented Feb 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RaduBerinde commented Feb 11, 2022 •

edited

Loading