release-21.1: migration,keys: fix incorrect range descriptor iteration #67366
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
changed the title
tbg
approved these changes
Jul 8, 2021
irfansharif
force-pushed
the
backport21.1-67315
branch
2 times, most recently
from
5dcb2aa
to
ce5f69b
Compare
When executing a split, it's surprisingly tricky to learn what the resulting left-hand and right-hand side is. This is because when you retrieve it "after the fact", other operations may have changed the split already (for example, the split could have been merged, or additional splits added) and while you would get descriptors back, they wouldn't be meaningfully connected to the split any more in all cases. Really one would want to return the descriptors from the split txn itself, but AdminSplit is a no-op when the split already exists and so we would need special logic that performs a range lookup on the left neighbor. It could all be done, but does not seem worth it. There's still a nice simplification here that lets us remove the ad-hoc code, and we'll just accept that when there are concurrent splits the return values may not exactly line up with the split. This came out of cockroachdb#64060 (comment). Release note: None
The long-running migrations infrastructure exposes an `IterateRangeDescriptors` interface to paginate through all the ranges in the system. It was previously doing this (erroneously) by scanning through the meta2 range. It was thus possible to entirely miss descriptors for the meta ranges themselves. This could only happen for large enough clusters with splits in the meta range. This issue was caught during the careful review of cockroachdb#66445. We remedy the situation by scanning over the entire `[MetaMin, MetaMax)` span instead. One thing to take care of is the possibility of finding the same range descriptor in both the `meta1` and `meta2` range. This is possible when the first range in the system includes part of the user keyspace (typically `[/Min, /System/NodeLiveness)`). We de-dup these descriptors away by maintaining an in-memory map of the range IDs seen in meta1. --- What does this mean for existing long running migrations? There was only one that made use of this descriptor iteration API: the truncated state migration. Fortunately that migration was a below-raft one, so as part of it we purged all extant ranges that hadn't seen the corresponding `Migrate` command (see `postTruncatedStateMigration` and `PurgeOutdatedReplicas`; the purge attempt reaches out to every store and ensures every replica has been migrated, and if not, has been GC-ed). If we had found unmigrated ranges (possible for the meta ranges as described above), the migration would be blocked. We haven't seen this happen with users running 21.1, which makes sense since this bug is only possible in clusters with enough ranges to necessitate a split in the meta range (very unlikely with our large default range size of 512 MB). Still, we'll backport this PR to 21.1. Should users run into this stalled migration, we'll recommend upgrading to whatever patch release this will be part of. Release note: None Release justification: bug fixes and low-risk updates to new functionality
irfansharif
force-pushed
the
backport21.1-67315
branch
from
ce5f69b
to
8885e3c
Compare
|
Huh, CI in the release-21.1 branch fails with what looks like buggy testserver code: |
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.
The long-running migrations infrastructure exposes an
IterateRangeDescriptorsinterface to paginate through all the rangesin the system. It was previously doing this (erroneously) by scanning
through the meta2 range. It was thus possible to entirely miss
descriptors for the meta ranges themselves. This could only happen for
large enough clusters with splits in the meta range. This issue was
caught during the careful review of #66445.
We remedy the situation by scanning over the entire
[MetaMin, MetaMax)span instead. One thing to take care of is the possibility of finding
the same range descriptor in both the
meta1andmeta2range. This ispossible when the first range in the system includes part of the user
keyspace (typically
[/Min, /System/NodeLiveness)). We de-dup thesedescriptors away by maintaining an in-memory map of the range IDs seen
in meta1.
What does this mean for existing long running migrations? There was only
one that made use of this descriptor iteration API: the truncated state
migration. Fortunately that migration was a below-raft one, so as part
of it we purged all extant ranges that hadn't seen the corresponding
Migratecommand (seepostTruncatedStateMigrationandPurgeOutdatedReplicas; the purge attempt reaches out to every storeand ensures every replica has been migrated, and if not, has been GC-ed).
If we had found unmigrated ranges (possible for the meta ranges as
described above), the migration would be blocked. We haven't seen this
happen with users running 21.1, which makes sense since this bug is only
possible in clusters with enough ranges to necessitate a split in the
meta range (very unlikely with our large default range size of 512 MB).
Still, we'll backport this PR to 21.1. Should users run into this
stalled migration, we'll recommend upgrading to whatever patch release
this will be part of.
Release note: None
Release justification: bug fixes and low-risk updates to new functionality