Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: migrate RaftTombstoneKey to the unreplicated key space #12154

Closed
petermattis opened this issue Dec 7, 2016 · 4 comments
Closed

storage: migrate RaftTombstoneKey to the unreplicated key space #12154

petermattis opened this issue Dec 7, 2016 · 4 comments
Assignees
Milestone

Comments

@petermattis
Copy link
Collaborator

RaftTombstoneKey currently resides in the replicated range-local key space. This means that it will be included in snapshots and was being included (incorrectly) in consistency checks. #12131 added a hack to not include RaftTombstoneKey in consistency checks, but the real fix is to make it an unreplicated key.

Note that RaftTombstoneKey is special in that it is only supposed to exist if there is no corresponding replica data.

A migration would have to perform a one time loop over all of the tombstone keys for a store similar to what is done in Store.IterateRangeDescriptors.

@tbg
Copy link
Member

tbg commented Dec 29, 2017

Nominating for 2.1. This is a big wart I've been running into for the last couple of days. cc @awoods187.

@tbg
Copy link
Member

tbg commented Dec 29, 2017

For example, any invocation of ComputeStatsForRange is getting this wrong:

// ComputeStatsForRange computes the stats for a given range by
// iterating over all key ranges for the given range that should
// be accounted for in its stats.
func ComputeStatsForRange(
	d *roachpb.RangeDescriptor, e engine.Reader, nowNanos int64,
) (enginepb.MVCCStats, error) {
	iter := e.NewIterator(false)
	defer iter.Close()

	ms := enginepb.MVCCStats{}
	for _, r := range makeReplicatedKeyRanges(d) {
		msDelta, err := iter.ComputeStats(r.start, r.end, nowNanos)
		if err != nil {
			return enginepb.MVCCStats{}, err
		}
		ms.Add(msDelta)
	}
	return ms, nil
}

This isn't just some oddity, it's a full-on problem.

@tbg
Copy link
Member

tbg commented Dec 29, 2017

(This is called during splits, for example).

@tbg tbg self-assigned this Dec 29, 2017
@tbg
Copy link
Member

tbg commented Dec 29, 2017

I'm doing this now.

@tbg tbg modified the milestones: 2.1, 2.0 Dec 29, 2017
tbg added a commit to tbg/cockroach that referenced this issue Dec 29, 2017
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
tbg added a commit to tbg/cockroach that referenced this issue Dec 29, 2017
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
tbg added a commit to tbg/cockroach that referenced this issue Dec 29, 2017
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
tbg added a commit to tbg/cockroach that referenced this issue Dec 30, 2017
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
tbg added a commit to tbg/cockroach that referenced this issue Jan 3, 2018
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
tbg added a commit to tbg/cockroach that referenced this issue Jan 8, 2018
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
tbg added a commit to tbg/cockroach that referenced this issue Jan 9, 2018
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
@tbg tbg closed this as completed in #21120 Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants