storage: don't include RaftTombstoneKey in consistency checks #12131

petermattis · 2016-12-07T15:16:20Z

This change is

petermattis · 2016-12-07T16:00:23Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

pkg/storage/store.go, line 3025 at r2 (raw file):

				log.Warningf(ctx, "unable to retrieve tombstone: %s", err)
			} else if ok {
				log.Fatalf(ctx, "tombstone should not exist")

This check fires in TestRaftRemoveRace. Not sure if we should keep it or try to move a similar check into that test.

Comments from Reviewable

Fixes cockroachdb#12130

tamird · 2016-12-07T19:27:20Z

Reviewed 1 of 1 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

pkg/storage/store.go, line 3025 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This check fires in TestRaftRemoveRace. Not sure if we should keep it or try to move a similar check into that test.

What did you conclude here? Why did this check fire here?

Comments from Reviewable

petermattis · 2016-12-07T19:30:30Z

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

pkg/storage/store.go, line 3025 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

What did you conclude here? Why did this check fire here?

The check fires when raft.IsEmptySnap(ready.Snapshot) is true. In that case applySnapshot() is a no-op and doesn't touch the existing replica data leaving the tombstone that was just added in place. Hence the change above to not add the tombstone key and only to bump Replica.mu.minReplicaID.

Comments from Reviewable

tamird · 2016-12-07T19:34:10Z

though the method setTombstoneKey is mildly detritus-y now.

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

spencerkimball · 2016-12-07T19:44:03Z

pkg/storage/replica_command.go

+			// Skip the tombstone key which is marked as replicated even though it
+			// isn't.
+			//
+			// TODO(peter): Figure out a way to migrate this key to the unreplicated


Did you file an issue for this? Seems problematic and worth fixing sooner than a TODO in the code is likely to prompt.

bdarnell · 2016-12-08T08:53:21Z

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

RaftTombstoneKey was accidentally made a replicated key when it was first introduced, a problem we first realized existed when it was [included in snapshots]. At the time, we included workarounds to skip this key in various places (snapshot application, consistency checker) but of course we have failed to insert further hacks of the same kind elsewhere since (the one that prompting this PR being the stats recomputation on splits, which I'm looking into as part of cockroachdb#20181 -- unfortunately this commit doesn't seem to pertain to that problem) It feels sloppy that we didn't follow through back then, but luckily the damage appears to be limited; it is likely that the replicated existence of this key results in MVCCStats SysBytes inconsistencies, but as it happens, these stats are [already] [very] [inconsistent]. This commit does a few things: - renames the old tombstone key to `RaftIncorrectLegacyTombstoneKey` - introduces a (correctly unreplicated) `RaftTombstoneKey` - introduces a migration that rewrites all legacy keys early in the node start sequence; as a result, a node running this commit or later will never see any legacy keys, except when they come in through a snapshot - when applying a snapshot, forcibly delete any legacy tombstone contained within. `RaftIncorrectLegacyTombstoneKey` can be purged from the codebase in binaries post v2.0, as at that point all peers have version at least v2.0 (which has this commit and will never send legacy tombstones in snapshots). Since sending legacy keys in snapshots was never intended (and isn't relied upon), none of this needs a cluster version migration. Fixes cockroachdb#12154. Release note: None [included in snapshots]: cockroachdb#12131 [already]: cockroachdb#20554 [very]: cockroachdb#20996 [inconsistent]: cockroachdb#21070

RaftTombstoneKey was accidentally made a replicated key when it was first introduced, a problem we first realized existed when it was [included in snapshots]. At the time, we included workarounds to skip this key in various places (snapshot application, consistency checker) but of course we have failed to insert further hacks of the same kind elsewhere since (the one that prompting this PR being the stats recomputation on splits, which I'm looking into as part of cockroachdb#20181 -- unfortunately this commit doesn't seem to pertain to that problem) It feels sloppy that we didn't follow through back then, but luckily the damage appears to be limited; it is likely that the replicated existence of this key results in MVCCStats SysBytes inconsistencies, but as it happens, these stats are [already] [very] [inconsistent]. This commit does a few things: - renames the old tombstone key to `RaftIncorrectLegacyTombstoneKey` - introduces a (correctly unreplicated) `RaftTombstoneKey` - introduces a migration that rewrites all legacy keys, to be activated when the cluster version surpasses the newly introduced version. Until then, both tombstones are checked, but only the old one written. - when applying a snapshot, forcibly delete any legacy tombstone contained within (even before the cluster version is bumped). `RaftIncorrectLegacyTombstoneKey` can be purged from the codebase in binaries post v2.1, as at that point all peers have booted with a version that runs the migration (it's morally true in `v2.0` as well, but it's possible that the cluster version update would interleave with a write of a legacy tombstone). In v2.0, there may be both legacy and old-style tombstones, but they'll never proliferate through snapshots. This implies that if a tombstone is left around from before the `v2.0` migration runs, then it is in a replica that since hasn't been recreated (for if it had, the tombstone would have been wiped), and thus it can't trip up the consistency checker. Thus, post v2.0, the replica consistency checker can stop skipping the legacy tombstone key. Fixes cockroachdb#12154. Release note: None [included in snapshots]: cockroachdb#12131 [already]: cockroachdb#20554 [very]: cockroachdb#20996 [inconsistent]: cockroachdb#21070

RaftTombstoneKey was accidentally made a replicated key when it was first introduced, a problem we first realized existed when it was [included in snapshots]. At the time, we included workarounds to skip this key in various places (snapshot application, consistency checker) but of course we have failed to insert further hacks of the same kind elsewhere since (the one that prompting this PR being the stats recomputation on splits, which I'm looking into as part of cockroachdb#20181 -- unfortunately this commit doesn't seem to pertain to that problem) It feels sloppy that we didn't follow through back then, but luckily the damage appears to be limited; it is likely that the replicated existence of this key results in MVCCStats SysBytes inconsistencies, but as it happens, these stats are [already] [very] [inconsistent]. This commit does a few things: - renames the old tombstone key to `RaftIncorrectLegacyTombstoneKey` - introduces a (correctly unreplicated) `RaftTombstoneKey` - introduces a migration. Once activated, only the new tombstone is written, but both tombstones are checked. Additionally, as the node restarts, all legacy tombstones are replaced by correct non-legacy ones. - when applying a snapshot, forcibly delete any legacy tombstone contained within (even before the cluster version is bumped). This prevents new legacy tombstones to trickle on from other nodes. `RaftIncorrectLegacyTombstoneKey` can be purged from the codebase in binaries post v2.1, as at that point all peers have booted with a version that runs the migration. Thus, post v2.1, the replica consistency checker can stop skipping the legacy tombstone key. Fixes cockroachdb#12154. Release note: None [included in snapshots]: cockroachdb#12131 [already]: cockroachdb#20554 [very]: cockroachdb#20996 [inconsistent]: cockroachdb#21070

storage: don't include RaftTombstoneKey in consistency checks

efb8d93

See cockroachdb#12130

petermattis assigned spencerkimball and bdarnell Dec 7, 2016

storage: don't write the tombstone key before applying a snapshot

1fe2feb

Fixes cockroachdb#12130

petermattis force-pushed the pmattis/consistency-raft-tombstone-key branch from feb1921 to 1fe2feb Compare December 7, 2016 16:50

petermattis merged commit 8f253c3 into cockroachdb:master Dec 7, 2016

petermattis deleted the pmattis/consistency-raft-tombstone-key branch December 7, 2016 19:37

spencerkimball reviewed Dec 7, 2016

View reviewed changes

petermattis mentioned this pull request Dec 7, 2016

storage: migrate RaftTombstoneKey to the unreplicated key space #12154

Closed

tbg mentioned this pull request Dec 29, 2017

storage: make RaftTombstoneKey unreplicated #21120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: don't include RaftTombstoneKey in consistency checks #12131

storage: don't include RaftTombstoneKey in consistency checks #12131

petermattis commented Dec 7, 2016 •

edited by rjnn

Loading

petermattis commented Dec 7, 2016

tamird commented Dec 7, 2016

petermattis commented Dec 7, 2016

tamird commented Dec 7, 2016

spencerkimball Dec 7, 2016

bdarnell commented Dec 8, 2016

storage: don't include RaftTombstoneKey in consistency checks #12131

storage: don't include RaftTombstoneKey in consistency checks #12131

Conversation

petermattis commented Dec 7, 2016 • edited by rjnn Loading

petermattis commented Dec 7, 2016

tamird commented Dec 7, 2016

petermattis commented Dec 7, 2016

tamird commented Dec 7, 2016

spencerkimball Dec 7, 2016

Choose a reason for hiding this comment

bdarnell commented Dec 8, 2016

petermattis commented Dec 7, 2016 •

edited by rjnn

Loading