server: fix sync on setting overrides for secondary tenants #110947
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous patch in this area was merely restarting the rangefeed but did not actually wait for the initial update event to be received. This patch fixes it. Release note: None
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
yuzefovich
reviewed
Sep 20, 2023
There was a problem hiding this comment.
Seems good to me, but should we also apply the same change to the other place (tenant caps watcher) where this code was mostly copied from?
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)
|
The cap watcher already had proper wait code, which is used in the TestServer code just underneath the call to TestRestart. |
|
I see the wait code in one ( |
yuzefovich
approved these changes
Sep 20, 2023
There was a problem hiding this comment.
Discussed offline - we do have waiting in place in both places.
Reviewable status:
|
Filed followup issue to clean this up: #110969 |
|
bors r=yuzefovich |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Sep 20, 2023
110448: roachprod-microbench: change sheet order r=renatolabs a=herkolategan Previously metric units in the summary sheet, and on sheet tabs, were sorted alphabetically. Since `sec/op` is the most important metric it should be first. This change reverses the order to ensure it is. Epic: None Release note: None 110676: settings: assert that SystemOnly settings are not accessed in virtual clusters r=yuzefovich a=knz Epic: CRDB-6671 First commit from #110947 TLDR: tests will now fail (with a panic) if code running in virtual clusters mistakenly attempts to access a SystemOnly cluster setting. ---- Prior to this patch, it was possible for code (e.g. in SQL) running inside a virtual cluster to access a `SystemOnly` setting. This resulted in silently incorrect behavior: the default value of the setting would be observed always, without any relationship to values explicitly set in the storage layer. When the setting mechanisms for virtual clusters had been implemented orignally, this concern was known. We even had implemented a mechanism on `settings.Values` (`SetNonSystemTenant()`) by which we could cause further `.Get()` calls to fail with an error in tests if accessing a `SystemOnly` setting from a virtual cluster. Sadly, this mechanism was never hooked up anywhere. This patch closes that loop by annotating the `settings.Values` properly during server initialization. Since the property is stored inside the `settings.Values`, this means it is not any more possible to share a single `settings.Values` instance (nor `cluster.Settings`) between the system tenant and a secondary tenant. ---- The following bugs were fixed as a result of this change: - certain settings didn't have the right class, including: - kv.bulk_io_write.concurrent_export_requests - kv.closed_timestamp.follower_reads.enabled - kv.range_split.by_load_merge_delay - kv.rangefeed.enabled - sql.hash_sharded_range_pre_split.max - storage.max_sync_duration.fatal.enabled - storage.value_blocks.enabled - ui.display_timezone - kv.bulk_sst.target_size - the API endpoint that lists cluster settings was incorrectly trying to retrieve SystemOnly settings when accessed from a virtual cluster. Co-authored-by: Herko Lategan <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
This was referenced Sep 23, 2023
craig bot
pushed a commit
that referenced
this pull request
Sep 26, 2023
111150: settings: make `.Override` set the value origin r=yuzefovich,stevendanna a=knz Previous in sequence: - [x] #110789 and #110947 - [x] #110676 - [x] #111008 - [x] #111145 - [x] #111147 - [x] #111149 Needed for #110758 Epic: CRDB-6671 Prior to this patch, setting overrides in tests did not have their value origin set properly. This patch fixes it. Co-authored-by: Raphael 'kena' Poss <[email protected]>
craig bot
pushed a commit
that referenced
this pull request
Sep 26, 2023
111153: settings: simple refactors r=yuzefovich,stevendanna a=knz See the last 7 commits for details. Previous in sequence: - [x] #110789 and #110947 - [x] #110676 - [x] #111008 - [x] #111145 - [x] #111147 - [x] #111149 - [x] #111150 Needed for #110758 Epic: CRDB-6671 Co-authored-by: Raphael 'kena' Poss <[email protected]>
craig bot
pushed a commit
that referenced
this pull request
Sep 29, 2023
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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improves/completes #110789
Prerequisite for tests in #110758.
Fixes #110560.
Epic: CRDB-6671
The previous patch in this area was merely restarting the rangefeed but did not actually wait for the initial update event to be received.
This patch fixes it.
Release note: None