Cherry-picks for 2.10.23-RC.5 #6171
Merged
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
Co-authored-by: Reuben Ninan <reuben@nats.io> Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
…te request A candidate could incorrectly revert to an older term without resetting if an old AE arrived with a term that is at least newer than the pterm but not necessarily newer than the term. Also a node that is isolated for a period of time with a high term number should cause the rest of the cluster to move forward with that term number, rather than going backwards to the leader term. Co-authored-by: Reuben Ninan <reuben@nats.io> Signed-off-by: Neil Twigg <neil@nats.io>
The stepdown channel interleaves with other channels such as the apply queue, leader change notifications etc in the `runAs` goroutines in an unpredictable order, so processing a stepdown request might be delayed behind other work. Doing this inline should be safer with stronger guarantees. Signed-off-by: Neil Twigg <neil@nats.io>
Beforehand when we were trying to run a catchup, we were reverting the `term` back to `pterm`. We can't ever move the term backwards safely and the catchup itself does not rely on this behaviour in order to work (as the catchup entries are matched only on `pindex`), so don't revert it. Signed-off-by: Neil Twigg <neil@nats.io>
Log consistency should only be enforced when handling append entries, not in other types of RPC. In this case, the higher term could cause us to blow away our entire log if a node with a higher term but a more out-of-date log comes along as leader. Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Otherwise we may end up in a situation where we are dropping append entries from a leader that is obviously behind, but we cannot help to bring the cluster forward to make progress. Signed-off-by: Neil Twigg <neil@nats.io>
Candidates should not be stepping down based on their pterm but rather their actual term. Signed-off-by: reubenninan <reuben@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Derek Collison <derek@nats.io>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Antithesis testing has found that late or out-of-order delivery of these snapshots, likely due to latency or thread pauses, can cause stream assignments to be reverted which results in assets being deleted and recreated. There may also be a race condition where the metalayer comes up before network connectivity to all other nodes is fully established so we may end up generating snapshots that don't include assets we don't know about yet. We will want to audit all uses of `SendSnapshot` as it somewhat breaks the consistency model, especially now that we have fixed a significant number of Raft bugs that `SendSnapshot` usage may have been papering over. Further Antithesis runs without this code run fine and have eliminated a number of unexpected calls to `processStreamRemoval`. We've also added a new unit test `TestJetStreamClusterHardKillAfterStreamAdd` for a long-known issue, as well as a couple tweaks to the ghost consumer tests to make them reliable. Signed-off-by: Neil Twigg <neil@nats.io> --------- Signed-off-by: Neil Twigg <neil@nats.io> Signed-off-by: Maurice van Veen <github@mauricevanveen.com> Co-authored-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Neil Twigg <neil@nats.io> Co-authored-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Neil Twigg <neil@nats.io>
… at `seq=ae.pindex+1` (#5987) This PR makes three complementary fixes to the way how catchup and truncating is handled. Specifically: - when doing `n.loadEntry(index)` we need to pass where the AppendEntry is in terms of stream sequence, this is equal to `ae.pindex+1` since the `ae.pindex` is the value before it's stored in the stream. - start catchup from `n.commit`, we could have messages past our commit that have been invalidated and need to be truncated since there was a switch between leaders - because we catchup from `n.commit`, we check if our local AppendEntry matches terms with the incoming AppendEntry, we only need to truncate if the terms don't match Signed-off-by: Maurice van Veen <github@mauricevanveen.com> --------- Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
…mmitted Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
If we haven't recovered the `pterm`/`pindex` from a snapshot, and the WAL is empty, then we shouldn't tell other nodes that we have a log that we don't have (i.e. by vote request). Instead leave both `pterm` and `pindex` as zero to correctly signal that we don't know anything about the state of the log. Signed-off-by: Neil Twigg <neil@nats.io>
…g shutdown Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
…sequent catchup messages Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
This should fix some logical races where multiple sets of goroutines can fight over the same store directory, i.e. when shutting down and recreating a group rapidly. Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
…g shutdown Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
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.
Includes the following:
termtoptermon AE mismatch #5684TestNRGSwitchStateClearsQueues#5691TestNRGSwitchStateClearsQueues#5707ptermto beginning of log when installing snapshots #5792TestNRGTermDoesntRollBackToPtermOnCatchup#5991n.commit& fix AppendEntry is stored atseq=ae.pindex+1#5987pterm#6027pterm/pindexif no snapshots and/or log entries #6060TestNRGCandidateDontStepdownDueToLeaderOfPreviousTerm#6107Signed-off-by: Neil Twigg neil@nats.io