multitenant: add SQL server startup guardrails #94973

healthy-pod · 2023-01-10T03:52:13Z

This code change prevents a SQL server from starting if its
binary version is less than the tenant's active version.

It makes RowDecoder.DecodeRow thread safe by making it
accept a *tree.DatumAlloc and use a new alloc if its nil.

The check runs after we run permanent upgrades because
that's when the version setting is written to the settings table
of the system tenant (and we want to apply this check to both system
and secondary tenants). To guard against future changes to where we
set the version, if GetClusterVersionFromStorage doesn't find a value for
the version setting it will return an error because this means the
version is not set yet.

Release note: None
Epic: CRDB-10829

cockroach-teamcity · 2023-01-10T03:52:21Z

This change is

knz · 2023-01-10T07:16:03Z

pkg/server/server_sql.go

+
+	// Prevent a secondary tenant's server from starting if its binary version is too low
+	// for the current tenant cluster version.
+	if !s.execCfg.Codec.ForSystemTenant() {


It would be better to structure this code to not need this codec check here.
The check should be the same for the system tenant.

Actually based on how you structured the code, i'd say just drop the check and the remainder of the code is correct.

ajstorm

The code change looks good, but it could use a test. The easiest place to put the test would be in multitenant-upgrade, but you may want to wait to add that until I get #94998 merged as I've reworked that test a bit.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod and @knz)

healthy-pod · 2023-01-10T17:45:29Z

The code change looks good, but it could use a test. The easiest place to put the test would be in multitenant-upgrade, but you may want to wait to add that until I get #94998 merged as I've reworked that test a bit.

I agree and I had a question about this part: only the most recent binary has this code change and makes this check so how would we go around testing this with some binary with an older version?

knz

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod)

ajstorm

@healthy-pod, as mentioned yesterday, you should be able to fake the binary version with a test like we're doing here: https://github.com/cockroachdb/cockroach/pull/94998/files#diff-6e04200b0921d3dce849044b96b198a595d76a9cfda91dcaa4b35076fd392998R128. Let me know when that's done and I'll have another look. Thanks!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod)

healthy-pod · 2023-01-18T23:44:38Z

ready for another pass :)

ajstorm

Modulo my questions, this looks good to me. You'll probably want @ajwerner to have a look as well though, as he's a real upgrades expert. I just play one on TV.

Reviewed 1 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod and @knz)

pkg/server/server_sql.go line 1494 at r3 (raw file):

	if s.execCfg.Settings.Version.BinaryVersion().Less(tenantActiveVersion) {
		return errors.Newf("preventing SQL server from starting because its binary version "+
			"is too low for the tenant active version: server binary version = %v, tenant active version = %v",

nit: We might want to consider wrapping this error message with a hint (errors.WithHint()) to explain that the solution to this problem is to move to a higher tenant binary. Should be obvious to most, but maybe not all.

pkg/server/settingswatcher/settings_watcher.go line 459 at r3 (raw file):

	row, err := db.Get(context.Background(), key)
	if err != nil {
		log.Fatalf(ctx, "failed to get tenant cluster version row: %v", err)

Is there something more user-friendly we can return here? If this happens, the tenant is corrupted, right? Is there any self-service option that the user can perform? I'd imagine not, so perhaps the action they'll need to take is to open a support ticket.

pkg/ccl/serverccl/server_startup_guardrails_test.go line 62 at r3 (raw file):

			test.storageBinaryVersion,
			test.storageBinaryMinSupportedVersion,
			false, /* initializeVersion */

Hmm, I'm surprised that we're not initializing the version here. If we don't initialize the version, what does the active cluster version get set to?

healthy-pod

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @knz)

pkg/ccl/serverccl/server_startup_guardrails_test.go line 62 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Hmm, I'm surprised that we're not initializing the version here. If we don't initialize the version, what does the active cluster version get set to?

It gets set to the binary version and if we attempt to initialize it we get: cannot initialize version to 21.1 because already set to: 21.2. Note that binary version is 21.2 and min supported is 21.1. I am still trying to trace how it gets to 21.2 but I am guessing serverutils.StartServer somehow ends initializing the version regardless.

healthy-pod · 2023-01-19T22:57:47Z

So the ActiveVersion in storageSettings doesn't get initialized when we call MakeTestingClusterSettingsWithVersions (because we set initializeVersion to false) but it gets initialized to 20.2/minBinary here, then get re-initialized to 21.2/binaryVersion here. This is how it ends at binaryVersion when StartServer returns

healthy-pod

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @knz)

pkg/server/server_sql.go line 1494 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

nit: We might want to consider wrapping this error message with a hint (errors.WithHint()) to explain that the solution to this problem is to move to a higher tenant binary. Should be obvious to most, but maybe not all.

Done

pkg/server/settingswatcher/settings_watcher.go line 459 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Is there something more user-friendly we can return here? If this happens, the tenant is corrupted, right? Is there any self-service option that the user can perform? I'd imagine not, so perhaps the action they'll need to take is to open a support ticket.

Since this is a fatal error, the logs will have something like the following right?

F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !This node experienced a fatal error (printed above), and as a result the
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !process is terminating.
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !Fatal errors can occur due to faulty hardware (disks, memory, clocks) or a
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !problem in CockroachDB. With your help, the support team at Cockroach Labs
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !will try to determine the root cause, recommend next steps, and we can
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !improve CockroachDB based on your report.
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !Please submit a crash report by following the instructions here:
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !    https://github.com/cockroachdb/cockroach/issues/new/choose
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !If you would rather not post publicly, please contact us directly at:
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !
F230119 22:39:04.501381 15 clusterversion/setting.go:124  [T1] 3 !    [email protected]

healthy-pod

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @knz)

pkg/ccl/serverccl/server_startup_guardrails_test.go line 62 at r3 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Thanks for looking into this. Your explanation resolves my question. I'm wondering though, if we want to sprinkle a few comments down below to explain how this test works. For example, it might not be immediately obvious to future readers that the tenant below gets created with TLV = SBV (I'm assuming, because it's created and assigned the storage cluster's active version at the time). The more context you add down there, the less time future engineers will need to spend understanding this code before they can modify it.

All that being said, I'm fine with this PR now, but I do think that we still need @ajwerner to have a look before you can merge.

I added more context, let me know if I need to add something else.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @healthy-pod, and @knz)

pkg/server/settings_cache.go line 147 at r6 (raw file):

) error {
	dec := settingswatcher.MakeRowDecoder(codec)
	var alloc *tree.DatumAlloc

nit: this should allocate an alloc or explicitly pass nil. I find declaring this variable to be confusing.

pkg/server/settingswatcher/settings_watcher.go line 48 at r6 (raw file):

	dec              RowDecoder
	storage          Storage
	allocForHandleKV tree.DatumAlloc

nit: I don't think this has to be on the struct -- it can just be captured by the closure if you wanted

ajstorm

I'm good too, mod nits.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

pkg/ccl/serverccl/server_startup_guardrails_test.go line 68 at r6 (raw file):

		// true with an assertion below. This is needed because in some test cases we want to ensure
		// the active version of this server is greater than the binary version of the tenant. By knowing
		// that the SBV is higher than TBV and SLV is equal to SBV we can be sure that SLV is higher than

Nit: Nice comment. My only concern is that SBV, TBV, SLV and TLV are not widely used terms. You might want to spell them out here instead of using acronyms.

healthy-pod · 2023-01-23T23:30:38Z

Addressed the nits.
TFTRs!

bors r+

craig · 2023-01-24T00:09:18Z

Build failed:

Bazel Essential CI (Cockroach)

healthy-pod · 2023-01-30T04:39:28Z

The test I added here failed when #94774 was merged. I made some changes to the test and the tenant creation code to fix the test and make sure there is as few moving parts as possible.

PTAL

ajstorm

All of my comments were addressed, so I'm good now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

pkg/ccl/serverccl/server_startup_guardrails_test.go line 68 at r6 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Nice comment. My only concern is that SBV, TBV, SLV and TLV are not widely used terms. You might want to spell them out here instead of using acronyms.

LGTM.

pkg/ccl/serverccl/server_startup_guardrails_test.go line 96 at r9 (raw file):

		)

		// The tenant will be created with an active version equal to the version corresponding to TenantLogicalVersionKey.

Extremely minor nit: try to keep comment lines to 80 characters or less.

ajwerner

Just nits, otherwise LGTM

ajwerner · 2023-01-31T19:43:16Z

pkg/server/settingswatcher/settings_watcher.go

+) (clusterversion.ClusterVersion, error) {
+	indexPrefix := s.codec.IndexPrefix(keys.SettingsTableID, uint32(1))
+	key := encoding.EncodeUvarintAscending(encoding.EncodeStringAscending(indexPrefix, "version"), uint64(0))
+	log.Infof(ctx, "getting tenant cluster version using key: '%v'", roachpb.Key(key))


nit: probably want to remove this or put it behind some vmodule before merging.

ajwerner · 2023-01-31T19:44:00Z

pkg/server/settings_cache.go

@@ -144,7 +144,7 @@ func initializeCachedSettings(
 ) error {
 	dec := settingswatcher.MakeRowDecoder(codec)
 	for _, kv := range kvs {
-		settings, val, _, err := dec.DecodeRow(kv)
+		settings, val, _, err := dec.DecodeRow(kv, nil)


nit: name the nil

ajwerner · 2023-01-31T19:45:10Z

pkg/clusterversion/cockroach_versions.go

@@ -777,6 +779,10 @@ var versionsSingleton = func() keyedVersions {
 // simply need to check is that the cluster has upgraded to 23.1.
 var V23_1 = versionsSingleton[len(versionsSingleton)-1].Key

+const (
+	BinaryMinSupportedVersionKey = V22_1


@dt do you see any reason to not expose this like this?

cockroach/pkg/clusterversion/clusterversion.go

Lines 241 to 244 in b258d20

// BinaryMinSupportedVersion implements the Handle interface.

func (v *handleImpl) BinaryMinSupportedVersion() roachpb.Version {

return v.binaryMinSupportedVersion

}

is the way to do it I do believe

fwiw I did not end using BinaryMinSupportedVersionKey because it's different from the tenant creation min version but I decided to keep this, assuming we will unify them? I can remove it. We need the key specifically rather than the version because we map the key to the initial values we want from https://github.com/cockroachdb/cockroach/blob/af0beec136262540f2e777c1b52a93ad21ac7481/pkg/sql/catalog/bootstrap/previous_releases.go#L26

ah, I see. I guess what we could do is just write a test that asserts that the key here maps to the min binary version or something like that.

and in a similar test (or in the same one) ensure that when BinaryVersionKey is passed to ByKey it returns the same version as binaryVersion?

ajwerner · 2023-01-31T21:31:15Z

pkg/server/settingswatcher/settings_watcher.go

@@ -236,7 +238,7 @@ func (s *SettingsWatcher) handleKV(
 	name, val, tombstone, err := s.dec.DecodeRow(roachpb.KeyValue{
 		Key:   kv.Key,
 		Value: kv.Value,
-	})


What I was saying here is that you could create a long-lived alloc because this is single-threaded. It doesn't really matter I guess.

ajwerner · 2023-01-31T21:32:07Z

pkg/server/settingswatcher/settings_watcher.go

+
+// GetFn is a function to retrieve data from the kv store. It is implemented
+// by both (*kv.DB).Get and (*kv.Txn).Get.
+type GetFn = func(context.Context, interface{}) (kv.KeyValue, error)


Given what we now know, maybe we just change this to take a *kv.Txn or some interface called Txn which just has this Get method if you don't want to depend on the concrete impl.

This code change prevents a SQL server from starting if its binary version is less than the tenant's active version. It makes `RowDecoder.DecodeRow` thread safe by making it accept a `*tree.DatumAlloc` and use a new alloc if its nil. The check runs after we run permanent upgrades because that's when the version setting is written to the settings table of the system tenant (and we want to apply this check to both system and secondary tenants). To guard against future changes to where we set the version, if GetClusterVersionFromStorage doesn't find a value for the version setting it will return an error because this means the version is not set yet. Release note: None Epic: CRDB-10829

healthy-pod · 2023-02-01T01:16:17Z

TFTsecondRs!
bors r=[ajstorm,ajwerner]

craig · 2023-02-01T01:52:02Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-02-01T04:22:32Z

Build succeeded:

Bazel Essential CI (Cockroach)

healthy-pod requested review from knz and ajstorm January 10, 2023 03:52

healthy-pod requested review from a team as code owners January 10, 2023 03:52

knz reviewed Jan 10, 2023

View reviewed changes

ajstorm requested a review from knz January 10, 2023 14:56

ajstorm reviewed Jan 10, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch from 81aae3f to 709565a Compare January 10, 2023 17:40

healthy-pod changed the title ~~multitenant: secondary tenant SQL server startup guardrails~~ multitenant: add SQL server startup guardrails Jan 10, 2023

knz reviewed Jan 11, 2023

View reviewed changes

ajstorm reviewed Jan 13, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch 2 times, most recently from b8afe42 to 1a44dc0 Compare January 16, 2023 16:28

healthy-pod requested review from ajstorm and knz January 16, 2023 16:30

healthy-pod force-pushed the startup-gr branch 4 times, most recently from e4d1db8 to fc07b22 Compare January 16, 2023 18:47

ajstorm requested changes Jan 19, 2023

View reviewed changes

healthy-pod requested a review from ajwerner January 19, 2023 16:10

healthy-pod commented Jan 19, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch from fc07b22 to d8fdf12 Compare January 19, 2023 23:40

healthy-pod commented Jan 19, 2023

View reviewed changes

healthy-pod requested a review from ajstorm January 19, 2023 23:43

healthy-pod commented Jan 23, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch from 11f3fdc to a9cefae Compare January 23, 2023 19:44

ajwerner reviewed Jan 23, 2023

View reviewed changes

ajstorm requested a review from ajwerner January 23, 2023 21:45

ajstorm approved these changes Jan 23, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch from a9cefae to c282dfe Compare January 23, 2023 22:29

healthy-pod force-pushed the startup-gr branch from c282dfe to d7c6f0c Compare January 30, 2023 03:52

healthy-pod requested a review from a team January 30, 2023 03:52

healthy-pod force-pushed the startup-gr branch from d7c6f0c to d418a4c Compare January 30, 2023 03:57

healthy-pod force-pushed the startup-gr branch 2 times, most recently from 7ea194d to 00eb938 Compare January 31, 2023 01:52

ajstorm approved these changes Jan 31, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch 2 times, most recently from d8170d9 to 5e29d40 Compare January 31, 2023 20:51

ajwerner approved these changes Jan 31, 2023

View reviewed changes

healthy-pod force-pushed the startup-gr branch 2 times, most recently from af0beec to 42b30ea Compare January 31, 2023 22:44

healthy-pod force-pushed the startup-gr branch from 42b30ea to 6093dd3 Compare January 31, 2023 23:01

craig bot merged commit b59fa09 into cockroachdb:master Feb 1, 2023

healthy-pod mentioned this pull request Mar 7, 2023

server,migrations: ensure that no old version can serve traffic #66606

Closed

knz mentioned this pull request Apr 5, 2023

multi-tenant: Set tenant's cluster version correctly on tenant creation #92080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multitenant: add SQL server startup guardrails #94973

multitenant: add SQL server startup guardrails #94973

healthy-pod commented Jan 10, 2023 •

edited

Loading

cockroach-teamcity commented Jan 10, 2023

knz Jan 10, 2023

knz Jan 10, 2023

healthy-pod Jan 10, 2023

ajstorm left a comment

healthy-pod commented Jan 10, 2023

knz left a comment

ajstorm left a comment

healthy-pod commented Jan 18, 2023

ajstorm left a comment

healthy-pod left a comment

healthy-pod commented Jan 19, 2023 •

edited

Loading

healthy-pod left a comment

healthy-pod left a comment

ajwerner left a comment

ajstorm left a comment

healthy-pod commented Jan 23, 2023

craig bot commented Jan 24, 2023

healthy-pod commented Jan 30, 2023

ajstorm left a comment •

edited

Loading

ajwerner left a comment

ajwerner Jan 31, 2023

ajwerner Jan 31, 2023

ajwerner Jan 31, 2023

ajwerner Jan 31, 2023

healthy-pod Jan 31, 2023

ajwerner Jan 31, 2023

healthy-pod Jan 31, 2023

ajwerner Jan 31, 2023

ajwerner Jan 31, 2023

healthy-pod commented Feb 1, 2023

craig bot commented Feb 1, 2023

craig bot commented Feb 1, 2023

	// BinaryMinSupportedVersion implements the Handle interface.
	func (v *handleImpl) BinaryMinSupportedVersion() roachpb.Version {
	return v.binaryMinSupportedVersion
	}

multitenant: add SQL server startup guardrails #94973

multitenant: add SQL server startup guardrails #94973

Conversation

healthy-pod commented Jan 10, 2023 • edited Loading

cockroach-teamcity commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

healthy-pod commented Jan 10, 2023

knz left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

healthy-pod commented Jan 18, 2023

ajstorm left a comment

Choose a reason for hiding this comment

healthy-pod left a comment

Choose a reason for hiding this comment

healthy-pod commented Jan 19, 2023 • edited Loading

healthy-pod left a comment

Choose a reason for hiding this comment

healthy-pod left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

ajstorm left a comment

Choose a reason for hiding this comment

healthy-pod commented Jan 23, 2023

craig bot commented Jan 24, 2023

healthy-pod commented Jan 30, 2023

ajstorm left a comment • edited Loading

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

healthy-pod commented Feb 1, 2023

craig bot commented Feb 1, 2023

craig bot commented Feb 1, 2023

healthy-pod commented Jan 10, 2023 •

edited

Loading

healthy-pod commented Jan 19, 2023 •

edited

Loading

ajstorm left a comment •

edited

Loading