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.

Sign up for GitHub

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

Jump to bottom

cli,server: static configuration profiles #98466

Merged
merged 1 commit into from
Apr 20, 2023
Merged

cli,server: static configuration profiles #98466

merged 1 commit into from
Apr 20, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Mar 12, 2023 •
edited
Loading

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
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 mentioned this pull request Mar 12, 2023
Closed
@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 stevendanna and ajwerner 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 35d6141 to 62781e4 Compare April 2, 2023 19:35
@knz knz force-pushed the 20230312-knz-auto-config-run branch 2 times, most recently from 20883fd to c79194c Compare April 3, 2023 19:25
Base automatically changed from 20230312-knz-auto-config-run to master April 3, 2023 20:56
@knz knz mentioned this pull request Apr 6, 2023
Closed
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch 5 times, most recently from 958eb8a to 3dfa064 Compare April 10, 2023 18:15
@knz knz mentioned this pull request Apr 10, 2023
Open
stevendanna
stevendanna reviewed Apr 14, 2023
View reviewed changes
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)",
Copy link
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
Contributor Author

Choose a reason for hiding this comment

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

Simplified.

@knz knz mentioned this pull request Apr 14, 2023
Closed
@knz
Copy link
Contributor Author

knz commented Apr 14, 2023

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

@knz knz requested a review from stevendanna April 14, 2023 14:37
@knz knz force-pushed the 20230312-knz-auto-config-profiles branch 2 times, most recently from 13fd15a to 6e33f98 Compare April 14, 2023 15:13
stevendanna
stevendanna approved these changes Apr 20, 2023
View reviewed changes
Copy link
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.

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

pkg/configprofiles/profiles.go
}

var multitenantClusterInitTasks = []autoconfigpb.Task{
makeTask("initial cluster config",
Copy link
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.

pkg/cli/flags.go
// 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
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. 👍

pkg/configprofiles/provider.go
var _ acprovider.Provider = (*profileTaskProvider)(nil)

// EnvUpdate is part of the acprovider.Provider interface.
func (p *profileTaskProvider) EnvUpdate() <-chan struct{} {
Copy link
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.

@knz
cli,server: static configuration profiles
f3547ec 
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
knz commented Apr 20, 2023
View reviewed changes
Copy link
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 knz force-pushed the 20230312-knz-auto-config-profiles branch from 6e33f98 to f3547ec Compare April 20, 2023 21:10
@knz knz added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.1.0 labels Apr 20, 2023
@knz
Copy link
Contributor Author

knz commented Apr 20, 2023

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Apr 20, 2023

Build succeeded:

@craig craig bot merged commit 17331ef into master Apr 20, 2023
@craig craig bot deleted the 20230312-knz-auto-config-profiles branch April 20, 2023 21:56
This was referenced Apr 20, 2023
Merged
Merged
@blathers-crl
Copy link

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
Reviewers

@ajstorm ajstorm ajstorm left review comments

@stevendanna stevendanna stevendanna approved these changes

@ajwerner ajwerner Awaiting requested review from ajwerner

Assignees
No one assigned
Labels
A-multitenancy Related to multi-tenancy backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

server: separate config defaults for serverless vs dedicated/SH secondary tenants
5 participants
@knz @cockroach-teamcity @stevendanna @ajwerner @ajstorm