kvserver: fix false positive in 'outstanding reproposal' assertion #97347
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
|
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 |
In cockroachdb#94633, I introduced[^1] an assertion that attempted to catch cases in which we might otherwise accidentally end up applying a proposal twice. This assertion had a false positive, see the updated comment within. I was able to reproduce the failure within ~minutes via `./experiment.sh` in cockroachdb#97173 as of 33dcdef. Better testing of these cases would be desirable. Unfortunately, while there is an abstraction over command application (`apply.Task`), most of the logic worth testing lives in `(*replicaAppBatch)` which is essentially a `*Replica` with more moving parts attached. This does not lend itself well to unit testing. I had a run[^1][^2][^3] earlier this year to make log application standalone, but then didn't have enough time to follow through. It would be desirable to do so at a later date, perhaps with the explicit goals of having interactions like the one discussion in this PR unit become testable. No release note because unreleased (except perhaps in an alpha). [3]: cockroachdb#93309 [2]: cockroachdb#93266 [1]: cockroachdb#93239 Closes cockroachdb#94633. [^1]: https://github.com/cockroachdb/cockroach/pull/94633/files#diff-50e458584d176deae52b20a7c04461b3e4110795c8c9a307cf7ee6696ba6bc60R238 Epic: none Release note: None
tbg
force-pushed
the
fix-assertion-1
branch
2 times, most recently
from
9c8132f to
66b5a13
Compare
pav-kv
approved these changes
Feb 22, 2023
| @@ -150,6 +151,26 @@ func (sm *replicaStateMachine) NewBatch() apply.Batch { | |||
| return b | |||
| } | |||
|
|
|||
| func formatReplicatedCmd(cmd *replicatedCmd) redact.RedactableString { | |||
There was a problem hiding this comment.
Do you want to do a String method, or whatever is the redaction-safe way in CRDB to format things?
OTOH I see that this is not an "official" stringer, just a way to format it for the error message?
There was a problem hiding this comment.
Yeah, this is sort of intermediate - I didn't want to get sidetracked writing a nice stringer and I didn't care about printing "everything". The standard way to implement fmt.Stringer is to implement redact.SafeFormatter and use that to implement the String method. But replicatedCmd is a pretty large nested struct so it'd be a lot of busy work. Generally a good idea to do it - just shied away from it for now.
There was a problem hiding this comment.
SGTM. Fine with a TODO if you end up merging this.
| // [lease seq is 1] | ||
| // idx 99: unrelated cmd at LAI 10000, lease seq = 1 | ||
| // idx 100: cmd X at LAI 10000, lease seq = 1 | ||
| // idx 100: cmd X at LAI 10000, lease seq = 1 |
There was a problem hiding this comment.
Is idx the Raft log index? Should it be 101 here?
Or what does this duplicate mean? Remove it?
Comment on lines
+252
to
+256
| // If this entry was rejected but only on account of the lease applied | ||
| // index, then the call to tryReproposeWithNewLeaseIndex[^1] must have | ||
| // returned an error (or the proposal would not be IsLocal() now; we | ||
| // unbind on success). But that call, by design, does not an error for a | ||
| // proposal that is already superseded. |
There was a problem hiding this comment.
This paragraph focuses a bit on the "symptoms": what other methods return or not return. It's a bit hard to follow such logic because of the indirection. Is there a way to frame this in a stateless manner, maybe more abstractly - i.e. directly answer the question why a rejected MaxLeaseIndex can't be found superseded? And then, if needed, employ the additional argument on what the other methods return (which is more like a "proof").
Comment on lines
+276
to
+277
| // idx 100: cmd X at LAI 10000, lease seq = 1 | ||
| // idx 101: new lease seq=2 |
There was a problem hiding this comment.
Where has the duplicate idx 100 gone?
Comment on lines
+278
to
+279
| // idx 102: cmd X at LAI 10000, lease seq = 1 | ||
| // idx 103: cmd X at LAI 20000, lease seq = 1 |
There was a problem hiding this comment.
Why 2 reproposals here? Is this because there was one before and one after the lease change? So the duplicate idx 100 above is an in-flight idx <TBD> reproposal?
| // | ||
| // [lease seq is 1] | ||
| // idx 99: unrelated cmd at LAI 10000, lease seq = 1 | ||
| // idx 100: cmd X at LAI 10000, lease seq = 1 |
There was a problem hiding this comment.
Maybe add MaxLeaseIndex (MLI?) to the entries, to make it more obvious why things get rejected.
| // idx 102: cmd X at LAI 10000, lease seq = 1 | ||
| // idx 103: cmd X at LAI 20000, lease seq = 1 | ||
| // | ||
| // When we apply index 102, we will see a permanent rejection but the |
There was a problem hiding this comment.
Why is this rejection permanent? Is it something we assume, or it should be obvious from this example?
What is an example of a permanent (non-MaxLeaseIndex) rejection?
| // unbind on success). But that call, by design, does not an error for a | ||
| // proposal that is already superseded. | ||
| // | ||
| // If the command was rejected permanently, a superseding entry *can* |
There was a problem hiding this comment.
Suggested change
| // If the command was rejected permanently, a superseding entry *can* | |
| // If the command was rejected permanently, a superseding proposal *can* |
pav-kv
reviewed
Feb 22, 2023
| // initially. | ||
| // [lease seq is 1] | ||
| // idx 99: unrelated cmd at LAI 10000, lease seq = 1 | ||
| // idx 100: cmd X at LAI 10000, lease seq = 1 |
There was a problem hiding this comment.
Should LAI be > 10000 here?
There was a problem hiding this comment.
Nvm, this comment wasn't for this line. I intended to put this in line 278.
Co-authored-by: Pavel Kalinnikov <[email protected]>
|
Flushed our pair programming edits. |
|
Pavel and I pair-reviewed this PR and I think the assertion can still fire (erroneously). I'll update the PR with that scenario as well, and disable the assertion altogether. Will ping when done. |
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Feb 23, 2023
In cockroachdb#94633, I introduced[^1] an assertion that attempted to catch cases in which we might otherwise accidentally end up applying a proposal twice. This assertion had a false positive. I was able to reproduce the failure within ~minutes via `./experiment.sh` in cockroachdb#97173 as of 33dcdef. Better testing of these cases would be desirable. Unfortunately, while there is an abstraction over command application (`apply.Task`), most of the logic worth testing lives in `(*replicaAppBatch)` which is essentially a `*Replica` with more moving parts attached. This does not lend itself well to unit testing. I had a run[^2][^3][^4] earlier this year to make log application standalone, but then didn't have enough time to follow through. It would be desirable to do so at a later date, perhaps with the explicit goals of having interactions like the one discussion in this PR unit become testable. [^4]: cockroachdb#93309 [^3]: cockroachdb#93266 [^2]: cockroachdb#93239 [^1]: https://github.com/cockroachdb/cockroach/pull/94633/files#diff-50e458584d176deae52b20a7c04461b3e4110795c8c9a307cf7ee6696ba6bc60R238 This assertion was previously trying to assert too much at a distance and was not only incorrect, but additionally inscrutable. It was mixing up two assertions, the first one of which is sensible: If an entry is accepted, it must not be superseded by inflight proposal. If this were violated, this superseded proposal could also apply, resulting in a failure of replay protection. This assertion is now still around as a stand-alone assertion. The other half of the assertion was more confused: if an entry is rejected, it was claiming that it couldn't also be superseded. The thinking was that if a superseding log entry exists, maybe it could apply, and that would be bad since we just told the waiting client that their proposal got rejected. This reasoning is incorrect, as the following example shows. Consider the following initial situation: [lease seq is 1] log idx 99: unrelated cmd at LAI 10000, lease seq = 1 log idx 100: cmd X at LAI 10000, lease seq = 1 And next: - a new lease enters the log at idx 101 (lease seq = 2) - an identical copy of idx 100 enters the log at idx 102 - we apply idx 100, leading to superseding reproposal at idx 103 resulting in the log: [lease seq is 1] log idx 99: unrelated cmd at LAI 10000, lease seq = 1 log idx 100: cmd X at LAI 10000, lease seq = 1 log idx 101: lease seq = 2 log idx 102: (same as idx 100) log idx 103: cmd X at LAI = 20000, lease seq = 1 During application of idx 102, we get a *permanent* rejection and yet the entry is superseded (by the proposal at idx 103). This would erroneously trigger the assertion, even though this is a legal sequence of events with no detrimental outcomes: the superseding proposal will always have the same lease sequence as its superseded copies, so it will also fail. I initially tried only soften the assertion a *little bit*. Observing that the example above led to a *permanent* rejection, should we only require that a proposal (which in this assertion is always local) is not superseded if it got rejected due to its lease index (which implies that it passed the lease check)? It turns out that this is primarily an assertion on when superseded proposals are counted as "local" at this point in the code: if there were multiple copies of this rejected proposal in the current `appTask` (i.e. the current `CommittedEntries` slice handed to us for application by raft), then all copies are initially local; and a copy that successfully spawns a superseding proposal would be made non-local from that point on. On the face of it, All other copies in the same `appTask` would now hit the assertion (erroneously): they are local, they are rejected, so why don't they enter the branch? The magic ingredient is that if an entry is superseded when we handle the lease index rejection, we also unlink the proposal from it. So these never enter this path since it's not local at this point. For example, if these are the log entries to apply (all at valid lease seq): log idx 99: unrelated cmd at LAI 10000 log idx 100: cmd X at LAI 10000 log idx 101: (identical copy of idx 100) and idxs 99-101 are applied in one batch, then idx 100 would spawn a reproposal at a new lease applied index: log idx 99: unrelated cmd at LAI 10000 log idx 100: cmd X at LAI 10000 <- applied log idx 101: (identical copy of idx 100) log idx 100: cmd X at LAI 20000 <- not in current batch When we apply 101, we observe an illegal lease index, but the proposal supersedes the entry, so we mark it as non-local and don't enter the branch that contains the assertion. The above reasoning is very difficult to understand, and it happens too far removed from where the interesting state changes happen. Also, for testing purposes it is interesting to introduce "errors" in the lease applied index assignment to artificially exercise these reproposal mechanisms. In doing so, these assertions can trip because the lease applied index assigned to a reproposal might accidentally (or intentionally!) match the existing lease applied index, in which case copies of the command in the same batch now *don't* consider themselves superseded. The value of this testing outweighs the very limited benefit of this branch of the assertion. An argument could even be made that this assertion alone as negative benefit due to its complexity. We are removing it in this commit and will instead work towards simplifying the mechanisms that played a role in explaining the asssertion. Closes cockroachdb#94633. Closes cockroachdb#97347. No release note because unreleased (except perhaps in an alpha). Epic: none Release note: None
I'm no longer sure what I was thinking here, I think the assertion is now mostly ok, except when introducing incorrect LAIs artificially. Either way, I'm removing it, rationale in #97564 which supersedes this PR. |
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Feb 24, 2023
In cockroachdb#94633, I introduced[^1] an assertion that attempted to catch cases in which we might otherwise accidentally end up applying a proposal twice. This assertion had a false positive. I was able to reproduce the failure within ~minutes via `./experiment.sh` in cockroachdb#97173 as of 33dcdef. Better testing of these cases would be desirable. Unfortunately, while there is an abstraction over command application (`apply.Task`), most of the logic worth testing lives in `(*replicaAppBatch)` which is essentially a `*Replica` with more moving parts attached. This does not lend itself well to unit testing. I had a run[^2][^3][^4] earlier this year to make log application standalone, but then didn't have enough time to follow through. It would be desirable to do so at a later date, perhaps with the explicit goals of having interactions like the one discussion in this PR unit become testable. [^4]: cockroachdb#93309 [^3]: cockroachdb#93266 [^2]: cockroachdb#93239 [^1]: https://github.com/cockroachdb/cockroach/pull/94633/files#diff-50e458584d176deae52b20a7c04461b3e4110795c8c9a307cf7ee6696ba6bc60R238 This assertion was previously trying to assert too much at a distance and was not only incorrect, but additionally inscrutable. It was mixing up two assertions, the first one of which is sensible: If an entry is accepted, it must not be superseded by inflight proposal. If this were violated, this superseded proposal could also apply, resulting in a failure of replay protection. This assertion is now still around as a stand-alone assertion. The other half of the assertion was more confused: if an entry is rejected, it was claiming that it couldn't also be superseded. The thinking was that if a superseding log entry exists, maybe it could apply, and that would be bad since we just told the waiting client that their proposal got rejected. This reasoning is incorrect, as the following example shows. Consider the following initial situation: [lease seq is 1] log idx 99: unrelated cmd at LAI 10000, lease seq = 1 log idx 100: cmd X at LAI 10000, lease seq = 1 And next: - a new lease enters the log at idx 101 (lease seq = 2) - an identical copy of idx 100 enters the log at idx 102 - we apply idx 100, leading to superseding reproposal at idx 103 resulting in the log: [lease seq is 1] log idx 99: unrelated cmd at LAI 10000, lease seq = 1 log idx 100: cmd X at LAI 10000, lease seq = 1 log idx 101: lease seq = 2 log idx 102: (same as idx 100) log idx 103: cmd X at LAI = 20000, lease seq = 1 During application of idx 102, we get a *permanent* rejection and yet the entry is superseded (by the proposal at idx 103). This would erroneously trigger the assertion, even though this is a legal sequence of events with no detrimental outcomes: the superseding proposal will always have the same lease sequence as its superseded copies, so it will also fail. I initially tried only soften the assertion a *little bit*. Observing that the example above led to a *permanent* rejection, should we only require that a proposal (which in this assertion is always local) is not superseded if it got rejected due to its lease index (which implies that it passed the lease check)? It turns out that this is primarily an assertion on when superseded proposals are counted as "local" at this point in the code: if there were multiple copies of this rejected proposal in the current `appTask` (i.e. the current `CommittedEntries` slice handed to us for application by raft), then all copies are initially local; and a copy that successfully spawns a superseding proposal would be made non-local from that point on. On the face of it, All other copies in the same `appTask` would now hit the assertion (erroneously): they are local, they are rejected, so why don't they enter the branch? The magic ingredient is that if an entry is superseded when we handle the lease index rejection, we also unlink the proposal from it. So these never enter this path since it's not local at this point. For example, if these are the log entries to apply (all at valid lease seq): log idx 99: unrelated cmd at LAI 10000 log idx 100: cmd X at LAI 10000 log idx 101: (identical copy of idx 100) and idxs 99-101 are applied in one batch, then idx 100 would spawn a reproposal at a new lease applied index: log idx 99: unrelated cmd at LAI 10000 log idx 100: cmd X at LAI 10000 <- applied log idx 101: (identical copy of idx 100) log idx 100: cmd X at LAI 20000 <- not in current batch When we apply 101, we observe an illegal lease index, but the proposal supersedes the entry, so we mark it as non-local and don't enter the branch that contains the assertion. The above reasoning is very difficult to understand, and it happens too far removed from where the interesting state changes happen. Also, for testing purposes it is interesting to introduce "errors" in the lease applied index assignment to artificially exercise these reproposal mechanisms. In doing so, these assertions can trip because the lease applied index assigned to a reproposal might accidentally (or intentionally!) match the existing lease applied index, in which case copies of the command in the same batch now *don't* consider themselves superseded. The value of this testing outweighs the very limited benefit of this branch of the assertion. An argument could even be made that this assertion alone as negative benefit due to its complexity. We are removing it in this commit and will instead work towards simplifying the mechanisms that played a role in explaining the asssertion. Closes cockroachdb#94633. Closes cockroachdb#97347. No release note because unreleased (except perhaps in an alpha). Epic: none Release note: None
craig bot
pushed a commit
that referenced
this pull request
Feb 26, 2023
97564: kvserver: narrow down 'finishing a proposal with outstanding reproposal' r=pavelkalinnikov a=tbg In #94633, I introduced[^1] an assertion that attempted to catch cases in which we might otherwise accidentally end up applying a proposal twice. This assertion had a false positive. I was able to reproduce the failure within ~minutes via `./experiment.sh` in #97173 as of 33dcdef. Better testing of these cases would be desirable. Unfortunately, while there is an abstraction over command application (`apply.Task`), most of the logic worth testing lives in `(*replicaAppBatch)` which is essentially a `*Replica` with more moving parts attached. This does not lend itself well to unit testing. I had a run[^2][^3][^4] earlier this year to make log application standalone, but then didn't have enough time to follow through. It would be desirable to do so at a later date, perhaps with the explicit goals of having interactions like the one discussion in this PR unit become testable. [^4]: #93309 [^3]: #93266 [^2]: #93239 [^1]: https://github.com/cockroachdb/cockroach/pull/94633/files#diff-50e458584d176deae52b20a7c04461b3e4110795c8c9a307cf7ee6696ba6bc60R238 This assertion was previously trying to assert too much at a distance and was not only incorrect, but additionally inscrutable. It was mixing up two assertions, the first one of which is sensible: If an entry is accepted, it must not be superseded by inflight proposal. If this were violated, this superseded proposal could also apply, resulting in a failure of replay protection. This assertion is now still around as a stand-alone assertion. The other half of the assertion was more confused: if an entry is rejected, it was claiming that it couldn't also be superseded. The thinking was that if a superseding log entry exists, maybe it could apply, and that would be bad since we just told the waiting client that their proposal got rejected. This reasoning is incorrect, as the following example shows. Consider the following initial situation: [lease seq is 1] log idx 99: unrelated cmd at LAI 10000, lease seq = 1 log idx 100: cmd X at LAI 10000, lease seq = 1 And next: - a new lease enters the log at idx 101 (lease seq = 2) - an identical copy of idx 100 enters the log at idx 102 - we apply idx 100, leading to superseding reproposal at idx 103 resulting in the log: [lease seq is 1] log idx 99: unrelated cmd at LAI 10000, lease seq = 1 log idx 100: cmd X at LAI 10000, lease seq = 1 log idx 101: lease seq = 2 log idx 102: (same as idx 100) log idx 103: cmd X at LAI = 20000, lease seq = 1 During application of idx 102, we get a *permanent* rejection and yet the entry is superseded (by the proposal at idx 103). This would erroneously trigger the assertion, even though this is a legal sequence of events with no detrimental outcomes: the superseding proposal will always have the same lease sequence as its superseded copies, so it will also fail. I initially tried only soften the assertion a *little bit*. Observing that the example above led to a *permanent* rejection, should we only require that a proposal (which in this assertion is always local) is not superseded if it got rejected due to its lease index (which implies that it passed the lease check)? It turns out that this is primarily an assertion on when superseded proposals are counted as "local" at this point in the code: if there were multiple copies of this rejected proposal in the current `appTask` (i.e. the current `CommittedEntries` slice handed to us for application by raft), then all copies are initially local; and a copy that successfully spawns a superseding proposal would be made non-local from that point on. On the face of it, All other copies in the same `appTask` would now hit the assertion (erroneously): they are local, they are rejected, so why don't they enter the branch? The magic ingredient is that if an entry is superseded when we handle the lease index rejection, we also unlink the proposal from it. So these never enter this path since it's not local at this point. For example, if these are the log entries to apply (all at valid lease seq): log idx 99: unrelated cmd at LAI 10000 log idx 100: cmd X at LAI 10000 log idx 101: (identical copy of idx 100) and idxs 99-101 are applied in one batch, then idx 100 would spawn a reproposal at a new lease applied index: log idx 99: unrelated cmd at LAI 10000 log idx 100: cmd X at LAI 10000 <- applied log idx 101: (identical copy of idx 100) log idx 100: cmd X at LAI 20000 <- not in current batch When we apply 101, we observe an illegal lease index, but the proposal supersedes the entry, so we mark it as non-local and don't enter the branch that contains the assertion. The above reasoning is very difficult to understand, and it happens too far removed from where the interesting state changes happen. Also, for testing purposes it is interesting to introduce "errors" in the lease applied index assignment to artificially exercise these reproposal mechanisms. In doing so, these assertions can trip because the lease applied index assigned to a reproposal might accidentally (or intentionally!) match the existing lease applied index, in which case copies of the command in the same batch now *don't* consider themselves superseded. The value of this testing outweighs the very limited benefit of this branch of the assertion. An argument could even be made that this assertion alone as negative benefit due to its complexity. We are removing it in this commit and will instead work towards simplifying the mechanisms that played a role in explaining the asssertion. Closes #97102. Closes #97347. Closes #97447. Closes #97612. No release note because unreleased (except perhaps in an alpha). Epic: none Release note: None Co-authored-by: Tobias Grieger <[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.
In #94633, I introduced an assertion that attempted to catch cases
in which we might otherwise accidentally end up applying a proposal
twice.
This assertion had a false positive, see the updated comment within.
I was able to reproduce the failure within ~minutes via
./experiment.shin #97173 as of 33dcdef.Better testing of these cases would be desirable. Unfortunately, while
there is an abstraction over command application (
apply.Task), mostof the logic worth testing lives in
(*replicaAppBatch)which isessentially a
*Replicawith more moving parts attached. This doesnot lend itself well to unit testing.
I had a run123 earlier this year to make log application
standalone4, but then didn't have enough time to follow through.
It would be desirable to do so at a later date, perhaps with
the explicit goals of having interactions like the one discussion
in this PR unit become testable.
No release note because unreleased (except perhaps in an alpha).
Closes #94633.
Epic: none
Release note: None
Footnotes
kvserver: decouple cmd checks in replicaAppBatch #93239 ↩
kvserver: refactor replicaAppBatch for standalone log application #93266 ↩
kvserver: handle AddSST for standalone log application #93309 ↩
https://github.com/cockroachdb/cockroach/issues/75729 ↩