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

sql: per-tenant cluster settings #56623

Closed
awoods187 opened this issue Nov 12, 2020 · 3 comments · Fixed by #58362
Closed

sql: per-tenant cluster settings #56623

awoods187 opened this issue Nov 12, 2020 · 3 comments · Fixed by #58362
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@awoods187
Copy link
Contributor

In a multi-tenant model, different tenants may want to deploy different cluster settings. We need to create a concept of a per-tenant cluster setting to provide this distinction.

@awoods187 awoods187 added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Nov 12, 2020
@rafiss
Copy link
Collaborator

rafiss commented Nov 16, 2020

Notes from team discussion:

  • We can't really make per-tenant cluster settings that affect the KV layer, since KV nodes are shared across all tenants.
  • We should make a list of settings that can be per-tenant or not.
  • Some, hopefully most, of the KV cluster settings have the prefix kv in the name.

Next step in order for us to decide if we need this for 21.1 is for the multi-tenancy team to let us know which cluster settings from here are needed on a per-tenant basis: https://www.cockroachlabs.com/docs/stable/cluster-settings.html

@ajwerner
Copy link
Contributor

Using a rangefeed here is not a lot of lift for detecting the actual changes. There are two steps:

  1. Mark the settings which we should allow tenants to read or set just so that expectations aren't violated.
  2. Hook up a rangefeed to watch for setting to change and updated the settings.SV.

It is critical that we get this done for 21.1.

cc @andy-kimball

@dt
Copy link
Member

dt commented Dec 28, 2020

The work I started in #57926 includes putting a bit on each setting indicating if it has only system effects (i.e. those that are used by kvserver) and thus would not make sense for a tenant to set, or if it has effects on SQL behavior that they may need to alter.

The whole reason we add settings is to have these knobs at runtime, particularly to have them as options if we find ourselves in a production incident. Given running as a tenant should the same as in a dedicated cluster wherever possible, we should default to assuming that if a dedicated cluster would need a setting, a tenant will too, unless it is actually only applicable to the system layer.

Writing a value for a system-only setting within a tenant should also be harmless -- nothing would read that value. Allowing a user to do so may cause a some mild confusion, if the user expected it to do something, but beyond that nothing should be affected by it. Rejecting attempts by tenants to ineffectually set a system-only setting could avoid confusion and provide a more helpful error though.

In general though, I think we should opt such system-only settings out, rather than opt tenant-ok settings in because the downside of forgetting to opt out is mild confusion while the downside to accidentally not opting in is that we discover we don't have our escape-hatch in the middle of a production incident.

For now, in the above PR, attempts by tenants to set system-only settings are denied outright, but for the same reasons we want to err on side of making settings settable, we might also want a session var to allow setting one we opt-out anyway, e.g. set allow_setting_system_settings = on, just in case we realize we mistakenly opted one out that we need to set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants