Skip to content

cli,server: static configuration profiles#98466

Merged
craig[bot] merged 1 commit intomasterfrom
20230312-knz-auto-config-profiles
Apr 20, 2023
Merged

cli,server: static configuration profiles#98466
craig[bot] merged 1 commit intomasterfrom
20230312-knz-auto-config-profiles

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Mar 12, 2023

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-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.

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@knz knz force-pushed the 20230312-knz-auto-config-profiles branch from fc2444e to edf31c4 Compare March 12, 2023 23:33
@knz knz force-pushed the 20230312-knz-auto-config-run branch 2 times, most recently from f119880 to ce2a284 Compare March 13, 2023 00:11
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch from edf31c4 to 2dd1f66 Compare March 13, 2023 00:12
@knz knz requested review from ajwerner and stevendanna March 13, 2023 00:12
@knz knz marked this pull request as ready for review March 13, 2023 00:13
@knz knz requested review from a team as code owners March 13, 2023 00:13
@knz knz requested review from a team, herkolategan and srosenberg and removed request for a team March 13, 2023 00:13
@knz knz added the A-multitenancy Related to multi-tenancy label Mar 13, 2023
@knz knz force-pushed the 20230312-knz-auto-config-run branch from ce2a284 to 59bfefe Compare March 13, 2023 22:10
@knz knz requested review from a team and msirek March 13, 2023 22:10
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch from 2dd1f66 to dd0e914 Compare March 13, 2023 22:10
@knz knz force-pushed the 20230312-knz-auto-config-run branch from 59bfefe to 867deef Compare March 13, 2023 22:33
@knz knz requested a review from a team as a March 13, 2023 22:33
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch from dd0e914 to db90e42 Compare March 13, 2023 22:33
@knz knz force-pushed the 20230312-knz-auto-config-run branch from 867deef to 310cb7a Compare March 13, 2023 23:31
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch from d39d7fd to 00af0fb Compare March 21, 2023 15:19
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 21, 2023

if, for 23.1 it makes sense to alias multitenant+app+sharedservice to replication-source and alias multitenant+noapp to replication-target?

Done

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Mar 26, 2023

todo:

  • EITHER disable range coalescing OR disable deprecatedd show ranges compatibiity done

@knz knz force-pushed the 20230312-knz-auto-config-run branch from b36f0f2 to 2e5d038 Compare March 26, 2023 21:46
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch 2 times, most recently from 2e1f390 to 1c83567 Compare March 26, 2023 21:55
craig bot pushed a commit that referenced this pull request Mar 30, 2023
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>
// "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)",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this anymore since we won't have the RU limiter for shared-process tenants by default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Apr 14, 2023

RFAL - I have also added tests to confirm the config is applied when a profile is selected.

Copy link
Copy Markdown
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"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",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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{} {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: 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.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Apr 20, 2023

bors r=stevendanna

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 20, 2023

Build succeeded:

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Apr 20, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

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.

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 backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

server: separate config defaults for serverless vs dedicated/SH secondary tenants

5 participants