pkg/server: initialize tenant version setting in settings watcher #104859

healthy-pod · 2023-06-14T07:05:09Z

This code change removes the early version setting initialization
because it is not needed.

Initialization now happens in the settings watcher during the initial
scan when the tenant logical version is read from the settings table.

The setting is only initialized if it's empty / not pre-initialized
because some tests pre-initialize it.

Release note: None
Epic: CRDB-26691

cockroach-teamcity · 2023-06-14T07:05:18Z

This change is

knz

This change is sound overall but note that this is currently unlikely to work well because we do not yet wait for all setting values to be received when a tenant server is initialized.

Look at (c *connector) Start() in kv/kvclient/kvtenant/connector.go. It starts the setting receiver (c.runTenantSettingsSubscription()) but it does not wait for the initial settings to have been received. So some code after the call to .Start() completes can still see the wrong/outdated values.

The proper fix in combination to your PR is to add a synchronization step there.

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson, @smg260, and @srosenberg)

knz · 2023-06-15T15:28:25Z

The issue I just described is this #96512

knz · 2023-06-26T18:56:35Z

I think this PR will help, which you are welcome to review. #105566

105566: kvtenant,settingswatcher: ensure overrides are applied during start r=yuzefovich a=knz See individual commits for details. Fixes #96512. Needed to unblock #104859. Epic: CRDB-26691 Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

knz

#105566 has merged you can rebase this on top of it.

Reviewed 1 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod, @jeffswenson, @smg260, and @srosenberg)

pkg/server/settingswatcher/settings_watcher.go line 333 at r1 (raw file):

				// Cluster version setting not initialized.
				if err := clusterversion.Initialize(ctx, newVersion.Version, &s.settings.SV); err != nil {
					log.Warningf(ctx, "failed to initialize cluster version setting: %s", err.Error())

May I suggest logcrash.ReportOrPanic here so it fails more loudly in tests thanks.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod, @jeffswenson, @smg260, and @srosenberg)

pkg/server/settingswatcher/settings_watcher.go line 333 at r1 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

May I suggest logcrash.ReportOrPanic here so it fails more loudly in tests thanks.

This might even need a log.Fatal. I don't think it would be safe to let the server start without a valid version setting.

This code change removes the early version setting initialization because it is not needed. Initialization now happens in the settings watcher during the initial scan when the tenant logical version is read from the settings table. The setting is only initialized if it's empty / not pre-initialized because some tests pre-initialize it. Release note: None Epic: none

healthy-pod · 2023-07-17T23:45:53Z

Ready for another look

knz

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson, @smg260, and @srosenberg)

healthy-pod · 2023-08-21T17:27:07Z

bors r=knz

craig · 2023-08-21T17:53:41Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-08-21T19:46:20Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-08-21T23:37:32Z

Build succeeded:

Bazel Essential CI (Cockroach)

Previously, shared-process tenants would initialize their view of the cluster version early and set it to the `MinSupportedVersion` as a "safe" option. However, that setting can likely be wrong, especially now that we plan to support skip-version upgrades. In this commit, we remove that code and rely on the initialization that happens from storage in `preStart`. This means that we now expose a non-negligible amount of code to a state where we don't have an initialized cluster version. However, having that error out is better than have code assume a potentially wrong version. A similar discussion and approach happened for separate process tenants a while ago: see cockroachdb#100684 and cockroachdb#104859. Fixes: cockroachdb#126754 Release note: None

127170: server: do not initialize cluster version to minimum supported r=stevendanna a=renatolabs Previously, shared-process tenants would initialize their view of the cluster version early and set it to the `MinSupportedVersion` as a "safe" option. However, that setting can likely be wrong, especially now that we plan to support skip-version upgrades. In this commit, we remove that code and rely on the initialization that happens from storage in `preStart`. This means that we now expose a non-negligible amount of code to a state where we don't have an initialized cluster version. However, having that error out is better than have code assume a potentially wrong version. A similar discussion and approach happened for separate process tenants a while ago: see #100684 and #104859. Fixes: #126754 Release note: None Co-authored-by: Renato Costa <renato@cockroachlabs.com>

Previously, shared-process tenants would initialize their view of the cluster version early and set it to the `MinSupportedVersion` as a "safe" option. However, that setting can likely be wrong, especially now that we plan to support skip-version upgrades. In this commit, we remove that code and rely on the initialization that happens from storage in `preStart`. This means that we now expose a non-negligible amount of code to a state where we don't have an initialized cluster version. However, having that error out is better than have code assume a potentially wrong version. A similar discussion and approach happened for separate process tenants a while ago: see #100684 and #104859. Fixes: #126754 Release note: None

healthy-pod force-pushed the move-version-init branch 2 times, most recently from 1bf758d to 7ddbcda Compare June 14, 2023 21:46

healthy-pod marked this pull request as ready for review June 14, 2023 21:48

healthy-pod requested review from a team as code owners June 14, 2023 21:48

healthy-pod requested review from jeffswenson, knz, smg260 and srosenberg and removed request for a team June 14, 2023 21:48

healthy-pod mentioned this pull request Jun 14, 2023

server,settings: assert tenant cluster version is initialized #100684

Merged

healthy-pod force-pushed the move-version-init branch from 7ddbcda to e1140bb Compare June 15, 2023 04:58

knz reviewed Jun 15, 2023

View reviewed changes

knz mentioned this pull request Jun 26, 2023

kvtenant,settingswatcher: ensure overrides are applied during start #105566

Merged

knz added the backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only label Jun 26, 2023

blathers-crl bot mentioned this pull request Jun 27, 2023

release-23.1: kvtenant,settingswatcher: ensure overrides are applied during start #105602

Merged

knz reviewed Jun 27, 2023

View reviewed changes

knz added the A-multitenancy Related to multi-tenancy label Jun 27, 2023

healthy-pod force-pushed the move-version-init branch from e1140bb to 3b4e8f6 Compare July 17, 2023 23:07

healthy-pod requested a review from knz July 17, 2023 23:45

knz approved these changes Aug 2, 2023

View reviewed changes

craig bot merged commit 8004dcd into cockroachdb:master Aug 21, 2023

blathers-crl bot mentioned this pull request Aug 21, 2023

release-23.1: pkg/server: initialize tenant version setting in settings watcher #109196

Closed

knz mentioned this pull request Sep 20, 2023

tenant startup hang #108209

Closed

renatolabs mentioned this pull request Jul 11, 2024

multitenant: shared-process tenant auto upgrades even if preserve_downgrade_option was set prior to restart #126754

Closed

renatolabs mentioned this pull request Jul 15, 2024

server: do not initialize cluster version to minimum supported #127170

Merged

blathers-crl bot mentioned this pull request Jul 30, 2024

release-24.2: server: do not initialize cluster version to minimum supported #127931

Merged

pkg/server: initialize tenant version setting in settings watcher #104859

pkg/server: initialize tenant version setting in settings watcher #104859

Uh oh!

Conversation

healthy-pod commented Jun 14, 2023 • edited by knz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 14, 2023

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Jun 15, 2023

Uh oh!

knz commented Jun 26, 2023

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

healthy-pod commented Jul 17, 2023

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

healthy-pod commented Aug 21, 2023

Uh oh!

craig bot commented Aug 21, 2023

Uh oh!

craig bot commented Aug 21, 2023

Uh oh!

craig bot commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

healthy-pod commented Jun 14, 2023 •

edited by knz

Loading