server,settings: assert tenant cluster version is initialized #100684
Conversation
For the longest time, we had a comment that suggested we weren't sure about it. We are now. Release note: None
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
It is invalid to start a tenant server if the version cluster setting is not initialized. However, it's fairly easy for test code to forget to do this, usually by mistakenly using `cluster.MakeClusterSettings()` without calling `clusterversion.Initialize()`. This change adds an assertion that errors out if the version setting is not set correctly. Release note: None
knz
force-pushed
the
20230405-tenant-version
branch
from
f95d8af to
4aa59bc
Compare
| // | ||
| // TODO(knz): Check if this special initialization can be removed. | ||
| // See: https://github.com/cockroachdb/cockroach/issues/90831 | ||
| // Since we don't know at which binary version the tenant |
There was a problem hiding this comment.
This is a bit pedantic, but I don't agree that assuming an old binary version is the conservative choice. Some versions are one way doors and using an old version after the migration runs is also broken.
The pain point here is we don't have a way to represent "unknown version". The tenant needs an up to date setting before any version gates are checked that make durable decisions.
If anything, picking the oldest version is actually more likely to cause problems than picking the newest version, since most clusters are running with binary version == cluster version. But that is probably a good thing, because it increases the probability we will catch version bugs during initialization.
There was a problem hiding this comment.
I'm going to merge this but we should leave this convo open. I hope that @healthy-pod can weigh in when he's back.
There was a problem hiding this comment.
I see two questions here:
-
Can the current state lead to issues?
Hopefully not, because we won't run any migrations/upgrades or check version gates before starting the settings watcher. When the settings watcher is started inSQLServer.preStart, it will set the right version and we will be good. -
Do we need to initialize the version where it gets initialized now? Instead of that we may be able to initialize it in
preStartwith the right version (tenant logical version).
I don't have an answer yet. Do you have any thoughts on this?
There was a problem hiding this comment.
Do we need to initialize the version where it gets initialized now? Instead of that we may be able to initialize it in preStart with the right version (tenant logical version).
I don't have an answer yet. Do you have any thoughts on this?
I think it would be a good thing to investigate. Mind filing an issue?
knz
added
the
backport-23.1.x
|
bors r=rafiss,JeffSwenson |
|
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-100684 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/100812/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. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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
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
5 participants
Fixes #100609.
It is invalid to start a tenant server if the version cluster setting
is not initialized. However, it's fairly easy for test code to forget to
do this, usually by mistakenly using
cluster.MakeClusterSettings()without calling
clusterversion.Initialize().This change adds an assertion that errors out if the version setting
is not set correctly.
Release note: None
Epic: CRDB-23559