migration,server: plumb in initial version to the migration manager #57118
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
irfansharif
force-pushed
the
201125.mgr-plumb-from
branch
5 times, most recently
from
b81d921 to
c5ee31b
Compare
tbg
approved these changes
Nov 27, 2020
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 2 of 2 files at r2, 8 of 8 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @knz)
pkg/migration/manager.go, line 254 at r3 (raw file):
for _, cv := range cvs { h := &Helper{Manager: m} clusterVersion := cv
What good is this? Let's avoid unnecessary aliasing. Why not just for _, clusterVersion := range cvs?
And for deleting old ones. This stuff is endlessly confusing. Release note: None
Jump to symbol for a given version tag will now group together the intent of the version itself (instead of expecting readers to know to grep for the same version in the block below). Release note: None
It'll let us return early, and makes the manager "more functional" in its behavior. We should also be using the ClusterVersion type when talking about migrations, so we make that change here. Release note: None
Release note: None
in the external facing API for pkg/migration. The migration package concerns itself with migrations between cluster versions, so we should have our API reflect as much. The proto changes are safe, we haven't had a major release with the previous proto definitions. Release note: None
irfansharif
force-pushed
the
201125.mgr-plumb-from
branch
from
c5ee31b to
3783e63
Compare
irfansharif
commented
Nov 27, 2020
There was a problem hiding this comment.
Thanks!
bors r+
Reviewable status:
pkg/migration/manager.go, line 254 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
What good is this? Let's avoid unnecessary aliasing. Why not just
for _, clusterVersion := range cvs?
Done (was just a brain fart).
craig bot
pushed a commit
that referenced
this pull request
Nov 27, 2020
57118: migration,server: plumb in initial version to the migration manager r=irfansharif a=irfansharif It'll let us return early, and makes the manager "more functional" in its behavior. We also promote the clusterversion type to a first-class citizen in the external facing API for pkg/migration. This package concerns itself with migrations between cluster versions and we should have our API reflect as much. The proto changes are safe, we haven't had a major release with the previous proto definitions. Release note: None 57155: clusterversion,*: remove VersionContainsEstimatesCounter r=irfansharif a=irfansharif This, and all surrounding migration code and tests, are now safe to remove. It mostly served as documentation, which we've moved to the field itself. Part of #47447. Fixes #56401. (While here, Let's also tell git that `versionkey_string.go` is a generated file.) Release note: None --- First commit is from #57154. 57156: sql,clusterversion: remove VersionAuthLocalAndTrustRejectMethods r=irfansharif a=irfansharif It's an old cluster version, introduced in the 19.2 release cycle. It's now safe to remove. Part of #47447. Fixes #56398. Release note: None --- See only last commit. First two are from #57154 and #57155 respectively. Co-authored-by: irfan sharif <[email protected]>
|
Build failed (retrying...): |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Nov 28, 2020
57186: migration: introduce fence versions r=irfansharif a=irfansharif
The migrations infrastructure makes use of internal fence versions when
stepping through consecutive versions. We'll need to first bump the
fence version for each intermediate cluster version, before bumping the
"real" one. Doing so allows us to provide the invariant that whenever a
cluster version is active, all nodes in the cluster (including ones
added concurrently during version upgrades) are running binaries that
know about the version.
It's instructive to walk through how we expect a version migration from
v21.1 to v21.2 to take place, and how we behave in the presence of new
v21.1 or v21.2 nodes being added to the cluster.
- All nodes are running v21.1
- All nodes are rolled into v21.2 binaries, but with active cluster
version still as v21.1
- The first version bump will be into v21.2(fence=1), see the
migration manager above for where that happens
Then concurrently:
- A new node is added to the cluster, but running binary v21.1
- We try bumping the cluster gates to v21.2(fence=1)
If the v21.1 nodes manages to sneak in before the version bump, it's
fine as the version bump is a no-op one (all fence versions are). Any
subsequent bumps (including the "actual" one bumping to v21.2) will fail
during the validation step where we'll first check to see that all nodes
are running v21.2 binaries.
If the v21.1 node is only added after v21.2(fence=1) is active, it won't
be able to actually join the cluster (it'll be prevented by the join
RPC).
We rely on the invariant we introduced earlier that requires new
user-defined versions (users being crdb engineers) to always have
even-numbered Internal versions. This reserved the odd numbers to slot
in fence versions for each user-defined one.
Release note: None
---
Only the last two commits here are of interest. Earlier commits are from
#57154 and #57118.
Co-authored-by: irfan sharif <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It'll let us return early, and makes the manager "more functional" in its
behavior.
We also promote the clusterversion type to a first-class citizen in the
external facing API for pkg/migration. This package concerns itself with
migrations between cluster versions and we should have our API reflect as much.
The proto changes are safe, we haven't had a major release with the previous
proto definitions.
Release note: None