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.

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

*: reads of SystemOnly settings from tenant process #91825

Closed
stevendanna opened this issue Nov 14, 2022 · 1 comment · Fixed by #110676 or #110758
Closed

*: reads of SystemOnly settings from tenant process #91825

stevendanna opened this issue Nov 14, 2022 · 1 comment · Fixed by #110676 or #110758
Assignees
Labels
A-multitenancy Related to multi-tenancy C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@stevendanna
Copy link
Collaborator

stevendanna commented Nov 14, 2022

Describe the problem

SystemOnly settings are not intended to be read from a tenant process. However, nothing actually disallows a tenant process from reading the in-memory default of a SystemOnly setting and taking actions on it.

The settings implementation includes a safety check for this was intended to be enabled in test builds, but that safety check requires (*Values).SetNonSystemTenant is called at some point during tenant startup. Currently we do not call that method so no accesses to SystemOnly settings from tenants are disallowed.

This recently tripped us up here #91824 and a quick test run with SetNonSystemTenant immediately hit our safety check on scheduler_latency.sample_period. It is likely that there are a number of other SystemOnly settings being accessed from tenant processes.

We should enable SetNonSystemTenant in our tests and work through the problems.

Jira issue: CRDB-21443
Epic: CRDB-6671

@stevendanna stevendanna added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Nov 14, 2022
@knz knz added the A-multitenancy Related to multi-tenancy label Jan 11, 2023
@knz
Copy link
Contributor

knz commented Sep 27, 2023

fixed by #110676.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
2 participants