Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tenantsettingswatcher: extend override store with alternate defaults #111212

Merged
merged 5 commits into from
Sep 29, 2023
Merged

tenantsettingswatcher: extend override store with alternate defaults #111212

merged 5 commits into from
Sep 29, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Sep 25, 2023 •
edited
Loading

See the last commit.

Previous in sequence:

As an end-goal, we want that when there is no override in
system.tenant_settings, i.e. the rangefeed over this table has
nothing to stay, we pick up the current value of TenantReadOnly
settings from system.settings instead to feed to virtual clusters.

To achieve this, we need to merge the current TenantReadOnly values
"underneath" the tenant overrides, i.e. use the current
TenantReadOnly as default override value when the rangefeed over
tenant_settings has nothing to say.

This commit updates the overrides store accordingly. It does so by
maintaining an alternateDefaults override slice, updated from
changes to TenantReadOnly settings from system.settings.

Then, updates to the overrides map (in tenantOverrides) for the
special tenant ID allTenantOverridesID are "merged" from
alternateDefaults with the following rule:

  • when a change to alternateDefaults is made, the current
    allTenantOverridesID override slice is updated for each
    setting not currently set from the rangefeed (those missing from the
    explicitKeys map).

  • when an addition or modification is observed from the
    rangefeed (for tenant ID allTenantOverridesID), the overrides slice
    is updated with the rangefeed value, and the rangefeed setting key is
    added to explicitKeys.

  • when a deletion is observed from the rangefeed, the key is removed
    from explicitKeys and any override from alternateDefaults is
    inserted instead.

Note that entries are never deleted from alternateDefaults, even
when a TenantReadOnly customization is removed from
system.settings (i.e. the callback setAlternateDefaults is given a
smaller slice than the current alternateDefaults). This is on
purpose: to properly support mixed-version deployments, we want
that SQL servers observe the storage layer's idea of what the default
value of a TenantReadOnly setting should be. To achieve this, it's
important that the storage server always reports an override for every
TenantReadOnly setting.

Needed for #110758
Epic: CRDB-6671

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz mentioned this pull request Sep 25, 2023
Merged
11 tasks
@knz knz force-pushed the 20230925-settings-overrides-store branch 7 times, most recently from 2ebbd16 to 611eead Compare September 29, 2023 08:17
@knz
settingswatcher: move an inline function to a method
ad326c1 
(This function/method will also be deleted in a later commit.)

Release note: None
@knz knz force-pushed the 20230925-settings-overrides-store branch 2 times, most recently from 62a8bec to 1ea5acc Compare September 29, 2023 14:56
knz and others added 4 commits September 29, 2023 19:06
@knz @stevendanna
server,settingswatcher: properly evict entries from the local persist…
81ebdaf 
…ed cache

(For context, on each node there is a local persisted cache of cluster
setting customizations. This exists to ensure that configured values
can be used even before a node has fully started up and can start
reading customizations from `system.settings`.)

Prior to this patch, entries were never evicted from the local
persisted cache: when a cluster setting was reset, any previously
saved entry in the cache would remain there.

This is a very old bug, which was long hidden and was recently
revealed when commit 2f5d717 was
merged. In a nutshell, before this recent commit the code responsible
to load the entries from the cache didn't fully work and so the stale
entries were never restored from the cache. That commit fixed the
loader code, and so the stale entries became active, which made the
old bug visible.

To fix the old bug, this present commit modifies the settings watcher
to preserve KV deletion events, and propagates them to the persisted
cache.

(There is no release note because there is no user-facing release where
the bug was visible.)

Release note: None

Co-authored-by: Steven Danna <[email protected]>
@knz
settingswatcher: write-through to the persisted cache
c5af624 
Prior to this patch, the rangefeed watcher over `system.settings` was
updating the in-RAM value store before it propagated the updates to
the persisted local cache.

In fact, the update to the persisted local cache was lagging quite a
bit behind, because the rangefeed watcher would buffer updates and
only flush them after a while.

As a result, the following sequence was possible:

1. client updates a cluster setting.
2. server is immediately shut down. The persisted cache
   has not been updated yet.
3. server is restarted. For a short while (until the settings watcher
   has caught up), the old version of the setting remains active.

This recall of ghost values of a setting was simply a bug. This patch
fixes that, by ensuring that the persisted cache is written through
before the in-RAM value store.

By doing this, we give up on batching updates to the persisted local
store. This is deemed acceptable because cluster settings are not
updated frequently.

Release note: None
@knz
settingswatcher: call a function upon updates to TenantReadOnly settings
d632d44 
In a later commit, we will want to feed changes to TenantReadOnly
settings in the system tenant's `system.settings` table into the
connector, for use by the virtual cluster servers.

This commit sets up the first building block: an extra callback
function in the settings watcher that gets called when the watcher
detects a change to TenantReadOnly settings.

Release note: None
@knz
tenantsettingswatcher: extend override store with alternate defaults
7a163cd 
As an end-goal, we want that when there is no override in
`system.tenant_settings`, i.e. the rangefeed over this table has
nothing to stay, we pick up the current value of `TenantReadOnly`
settings from `system.settings` instead to feed to virtual clusters.

To achieve this, we need to merge the current `TenantReadOnly` values
"underneath" the tenant overrides, i.e. use the current
`TenantReadOnly` as default override value when the rangefeed over
`tenant_settings` has nothing to say.

This commit updates the overrides store accordingly. It does so by
maintaining an `alternateDefaults` override slice, updated from
changes to `TenantReadOnly` settings from `system.settings`.

Then, updates to the overrides map (in `tenantOverrides`) for the
special tenant ID `allTenantOverridesID` are "merged" from
`alternateDefaults` with the following rule:

- when a change to `alternateDefaults` is made, the current
  `allTenantOverridesID` override slice is updated for each
  setting *not* currently set from the rangefeed (those missing from the
  `explicitKeys` map).

- when an *addition* or *modification* is observed from the
  rangefeed (for tenant ID `allTenantOverridesID`), the overrides slice
  is updated with the rangefeed value, and the rangefeed setting key is
  added to `explicitKeys`.

- when a *deletion* is observed from the rangefeed, the key is removed
  from `explicitKeys` and any override from `alternateDefaults` is
  inserted instead.

Note that entries are never deleted from `alternateDefaults`, even
when a TenantReadOnly customization is removed from
`system.settings` (i.e. the callback `setAlternateDefaults` is given a
smaller slice than the current `alternateDefaults`). This is on
purpose: to properly support mixed-version deployments, we want
that SQL servers observe the storage layer's idea of what the default
value of a TenantReadOnly setting should be. To achieve this, it's
important that the storage server always reports an override for every
TenantReadOnly setting.

Release note: None
@knz knz force-pushed the 20230925-settings-overrides-store branch from 1ea5acc to 7a163cd Compare September 29, 2023 17:38
craig bot pushed a commit that referenced this pull request Sep 29, 2023
@knz @arulajmani
Merge #110758 #111383 #111440 #111512
1787e21 
110758: server,settings: properly cascade defaults for TenantReadOnly r=stevendanna,yuzefovich a=knz

Previous in sequence:
- [x] #110789 and #110947 
- [x] #110676
- [x] #111008
- [x] #111145
- [x] #111147
- [x]  #111149
- [x] #111150
- [x] #111153
- [x] #111475
- [x] #111210
- [x] #111212

Fixes #108677.
Fixes #85729.
Fixes #91825.
Completes the work described in the settings RFC.
Epic: CRDB-6671

TLDR: this patch ensures that virtual cluster servers observe changes
made to TenantReadOnly settings via SET CLUSTER SETTING in the system
interface, even when there is no override set via ALTER VIRTUAL
CLUSTER SET CLUSTER SETTING.

For example, after `SET CLUSTER SETTING
kv.closed_timestamp.target_duration = '10s'` in the system interface,
this value will show up via `SHOW CLUSTER SETTING` in a virtual
cluster SQL session.

This changes the way that settings are picked up in virtual cluster,
as follows:

1. if there is an override specifically for this tenant's ID
   (in `tenant_settings`), use that.
2. otherwise, if there is an override for the pseudo-ID 0
   (in `tenant_settings` still, set via `ALTER TENANT ALL SET CLUSTER
   SETTING`), then use that.
3. **NEW**  otherwise, if the class is TenantReadOnly and there
   is a custom value in `system.settings`, set via a regular
   `SET CLUSTER SETTING` in the system tenant, then use that.
4. otherwise, use the global default set via the setting's
   `Register()` call.

----

Prior to this patch, TenantReadOnly settings as observed from virtual
clusters were defined as the following priority order:

1. if there is an override specifically for this tenant's ID
   (in `tenant_settings`), use that.
2. otherwise, if there is an override for the pseudo-ID 0
   (in `tenant_settings` still, set via `ALTER TENANT ALL SET CLUSTER
   SETTING`), then use that.
3. otherwise, use the global default set via the setting's
   `Register()` call.

Remarkably, this did not pick up any changes made via a plain `SET
CLUSTER SETTING` statement via the system interface, which only
modifies this setting's value in `system.settings` (thus not
`tenant_settings`).

This situation was problematic in two ways.

To start, settings like `kv.closed_timestamp.target_duration` cannot
be set solely in `system.tenant_settings`; they are also used in the
storage layer and so must also be picked up from changes in
`system.settings`.

For these settings, it is common for operators to just issue the plain
`SET CLUSTER SETTING` statement (to update `system.settings`) and
simply forget to _also_ run `ALTER TENANT ALL SET CLUSTER SETTING`.

This mistake is nearly unavoidable and would result in incoherent
behavior, where the storage layer would use the customized value
and virtual clusters would use the registered global default.

The second problem is in mixed-version configurations, where
the storage layer runs version N+1 and the SQL service runs version N
of the executable. If the registered global default changes from
version N to N+1, the SQL service would not properly pick up
the new default defined in version N+1 of the storage layer.

This patch fixes both problems as follows:

- it integrates changes to TenantReadOnly settings observed
  in `system.settings`, to the watcher that tracks changes
  to `system.tenant_settings`. When a TenantReadOnly setting
  is present in the former but not the latter, a synthetic
  override is added.

- it also initializes synthetic overrides for all the
  TenantReadOnly settings upon server initialization,
  from the registered global default, so that virtual
  cluster servers always pick up the storage layer's
  default as override.


111383: *: simplify tests r=yuzefovich a=knz

All commits except the last are from #110758.
Epic: CRDB-6671

Now that "tenant-ro" settings take their default from the system tenant's value, we do not need `ALTER TENANT ALL` for them any more.

This patch simplifies test code accordingly.

Rcommended by `@yuzefovich` in the review for #110758. 

111440: cluster-ui: fix db page stories r=THardy98 a=THardy98

Epic: none

This change fixes the stories for the database pages.

Release note: None

111512: kv: correctly handle shared lock replays in KVNemesis r=nvanbenschoten a=arulajmani

Previously, we'd return an AssertionFailed error if a SKIP LOCKED request discovered another request from its own transaction waiting in a lock's wait queue. In SQL's use of KV, this can only happen if the SKIP LOCKED request is being replayed -- so returning an error here is fine. However, this tripped KVNemesis up.

This patch marks such errors, for the benefit of KVNemesis, and doesn't call them assertion failed errors.

Fixes #111426
Fixes #111506
Fixes #111513

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Thomas Hardy <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
@craig craig bot merged commit 7a163cd into cockroachdb:master Sep 29, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@knz @cockroach-teamcity