cli,server: static configuration profiles#98466
Conversation
fc2444e to
edf31c4
Compare
f119880 to
ce2a284
Compare
edf31c4 to
2dd1f66
Compare
ce2a284 to
59bfefe
Compare
2dd1f66 to
dd0e914
Compare
59bfefe to
867deef
Compare
dd0e914 to
db90e42
Compare
867deef to
310cb7a
Compare
d39d7fd to
00af0fb
Compare
Done |
|
todo:
|
b36f0f2 to
2e5d038
Compare
2e1f390 to
1c83567
Compare
98899: feat: allow starting docker container via env variable r=rickystewart a=btkostner Fixes #87043 by allowing you to specify args via the `COCKROACH_ARGS` env value instead of a command. This is required to be able to use the official cockroach image with GitHub actions via a service. More details in the issue. Note, do to weirdness with merging commands and env values, I decided that setting the `COCKROACH_ARGS` would ignore any command given. This should reduce issues with people trying to use both ways of specifying args and instead force them to pick one. Release note (general change): Allow setting docker command args via the `COCKROACH_ARGS` environment variable. 99607: sql: block DROP TENANT based on a session var r=stevendanna a=knz Fixes #97972. Epic: CRDB-23559 In clusters where we will promote tenant management operations, we would like to ensure there is one extra step needed for administrators to drop a tenant (and thus irremedially lose data). Given that `sql_safe_updates` is not set automatically when users open their SQL session using their own client, we need another mechanism. This change introduces the new (hidden) session var, `disable_drop_tenant`. When set, tenant deletion fails with the following error message: ``` demo@127.0.0.1:26257/movr> drop tenant foo; ERROR: rejected (via sql_safe_updates or disable_drop_tenant): DROP TENANT causes irreversible data loss SQLSTATE: 01000 ``` (The session var `sql_safe_updates` is _also_ included as a blocker in the mechanism so that folk using `cockroach sql` get double protection). The default value of this session var is `false` in single-tenant clusters, for compatibility with CC Serverless. It will be set to `true` via a config profile (#98466) when suitable. Release note: None 99690: ui: drop index with space r=maryliag a=maryliag Previously, if the index had a space on its name, it would fail to drop. This commit adds quotes so it can be executed. Fixes #97988 Schema Insights https://www.loom.com/share/04363b7f83484b5da19c760eb8d0de21 Table Details page https://www.loom.com/share/1519b897a14440ddb066fb2ab03feb2d Release note (bug fix): Index recommendation to DROP an index that have a space on its name can now be properly executed. 99750: sql: remove no longer used channel in createStatsNode r=yuzefovich a=yuzefovich This hasn't been used as of fe6377c. Also mark `create_stats.go` as owned by SQL Queries. Epic: None Release note: None 99962: ui: add checks for values r=maryliag a=maryliag Fixes #99655 Fixes #99538 Fixes #99539 Add checks to usages that could cause `Cannot read properties of undefined`. Release note: None 99963: roachtest: use local SSDs for disk-stall failover tests r=andrewbaptist a=nicktrav The disk-stalled roachtests were updated in #99747 to use local SSDs. This change broke the `failover/*/disk-stall` tests, which look for `/dev/sdb` on GCE (the used for GCE Persistent Disks), but the tests still create clusters with local SSDs (the roachtest default). Fix #99902. Fix #99926. Fix #99930. Touches #97968. Release note: None. Co-authored-by: Blake Kostner <git@btkostner.io> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net> Co-authored-by: maryliag <marylia@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Nick Travers <travers@cockroachlabs.com>
pkg/configprofiles/profiles.go
Outdated
| // "ALTER TENANT template SET CLUSTER SETTING spanconfig.tenant_split.enabled = false", | ||
| // Disable RU accounting. | ||
| // TODO(knz): Move this to in-tenant config task. | ||
| "SELECT crdb_internal.update_tenant_resource_limits('template', 10000000000, 0, 10000000000, now(), 0)", |
There was a problem hiding this comment.
I don't think we need this anymore since we won't have the RU limiter for shared-process tenants by default.
|
RFAL - I have also added tests to confirm the config is applied when a profile is selected. |
stevendanna
left a comment
There was a problem hiding this comment.
Overall, this looks reasonable to me although I'm not 100% up to speed on the provider interface. I left one possible addition to the configuration profiles.
pkg/configprofiles/profiles.go
Outdated
| "ALTER TENANT template GRANT CAPABILITY can_admin_relocate_range, can_admin_unsplit, can_view_node_info, can_view_tsdb_metrics, exempt_from_rate_limiting", | ||
| // Disable span config limits and splitter. | ||
| // TODO(knz): Move this to in-tenant config task. | ||
| "ALTER TENANT template SET CLUSTER SETTING spanconfig.tenant_limit = 1000000", |
There was a problem hiding this comment.
I don't think we need this anymore since we don't install the spanconfig limiter for shared-process tenants by default.
| } | ||
|
|
||
| var multitenantClusterInitTasks = []autoconfigpb.Task{ | ||
| makeTask("initial cluster config", |
There was a problem hiding this comment.
We could also enable rangefeed's here so that it can be used as a replication source more easily.
| // Cluster initialization. We only do this for a regular start command; | ||
| // SQL-only servers get their initialization payload from their tenant | ||
| // configuration. | ||
| cliflagcfg.VarFlag(f, configprofiles.NewProfileSetter(&serverCfg.AutoConfigProvider), cliflags.ConfigProfile) |
There was a problem hiding this comment.
If I'm reading the code correctly, the server config is given a NoTaskProvider by default and then the NewProfileSetter will install a new provider if the configuration value is set. 👍
| var _ acprovider.Provider = (*profileTaskProvider)(nil) | ||
|
|
||
| // EnvUpdate is part of the acprovider.Provider interface. | ||
| func (p *profileTaskProvider) EnvUpdate() <-chan struct{} { |
There was a problem hiding this comment.
Might be worth noting the obvious: We return a channel that won't see updates because static profiles can't be updated.
This change introduces a mechanism through which an operator can select a "configuration profile" via the command-line flag `--config-profile` or env var `COCKROACH_CONFIG_PROFILE`. The SQL initialization defined by the profile is applied during server start-up. The profiles are (currently) hardcoded inside CockroachDB. The following profiles are predefined: - `default`: no configuration. - `multitenant+noapp`: no pre-defined `application` tenant, but with a predefined application tenant template that is used whenever a new tenant is defined. This config profile is meant for use for C2C replication target clusters. - `multitenant+app+sharedservice`: shared-process multitenancy with pre-defined `application` tenant, based off the same configuration as `multitenant+noapp`. - `replication-source`: alias for `multitenant+app+sharedservice` - `replication-target`: alias for `multitenant+noapp` Release note: None
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @stevendanna)
pkg/cli/flags.go line 400 at r6 (raw file):
Previously, stevendanna (Steven Danna) wrote…
If I'm reading the code correctly, the server config is given a NoTaskProvider by default and then the NewProfileSetter will install a new provider if the configuration value is set. 👍
Correct.
pkg/configprofiles/profiles.go line 77 at r3 (raw file):
Previously, stevendanna (Steven Danna) wrote…
It's unclear if we'll need this now that this rate limiter is attached to the capability system.
I removed this already.
pkg/configprofiles/profiles.go line 124 at r5 (raw file):
Previously, stevendanna (Steven Danna) wrote…
I don't think we need this anymore since we don't install the spanconfig limiter for shared-process tenants by default.
I think we'll do good to keep it for when/if we ever lift the deployment to separate-process.
pkg/configprofiles/profiles.go line 92 at r6 (raw file):
Previously, stevendanna (Steven Danna) wrote…
We could also enable rangefeed's here so that it can be used as a replication source more easily.
Rangefeeds are enabled by default already.
pkg/configprofiles/provider.go line 35 at r6 (raw file):
Previously, stevendanna (Steven Danna) wrote…
Might be worth noting the obvious: We return a channel that won't see updates because static profiles can't be updated.
Done.
|
bors r=stevendanna |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers, but backport branch blathers/backport-release-23.1-98466 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/101957/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.1.x failed. See errors above. error setting reviewers, but backport branch blathers/backport-release-23.1.0-98466 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/101958/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.1.0 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Epic: CRDB-23559
Informs #98431.
Fixes #94856.
(Based off #98459)
Supersedes #98380.
This change introduces a mechanism through which an operator can
select a "configuration profile" via the command-line flag
--config-profileor env varCOCKROACH_CONFIG_PROFILE. The SQLinitialization defined by the profile is applied during server
start-up.
The profiles are (currently) hardcoded inside CockroachDB.
The following profiles are predefined:
default: no configuration.multitenant+noapp: no pre-definedapplicationtenant, but with apredefined application tenant template that is used whenever a new
tenant is defined. This config profile is meant for use for C2C
replication target clusters.
multitenant+app+sharedservice: shared-process multitenancy withpre-defined
applicationtenant, based off the same configuration asmultitenant+noapp.Release note: None