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.

Sign up for GitHub

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

Jump to bottom

Reinit consensus #5265

Merged
merged 5 commits into from
Oct 29, 2024
Merged

Reinit consensus #5265

merged 5 commits into from
Oct 29, 2024

Conversation

generall
Copy link
Member

@generall generall commented Oct 20, 2024 •
edited by timvisee
Loading

This PR adds an ability to re-initialize consensus for existing cluster, while keeping the data intact.
This might be useful in cases, when we need to make a copy of the existing cluster, but this new cluster should have all URLs different. It is impossible to do with existing consensus, as the URL change is consensus operation, which requires quorum to be applied.

How to use reinit

This PR adds a new --reinit flag to the CLI arguments, which changes the logic on consensus loading.
Correct usage of the --reinit flag is the following:

  • Make a copy of each peer's storage of the existing cluster
  • Run the first peer with --reinit --uri <NEW-URI>.
  • Run other peers with --reinit --bootstrap <FIRST-PEER-NEW-URL> --uri <NEW-URI>

Use --reinit only once per peer, don't use it for further launches.

How this change works

There are 2 logic paths for reinit, one for the first peer, and the second for the bootstrapped peer.

For the first peer:

  • We want to make the first peer the sole voter in the consensus, therefore we modify the state to remove other peers from voters. This results in peer electing itself as a leader, which makes it capable of applying consensus operations.
  • We force the compaction of WAL, this is needed to force snapshot-sending to other, newly attaching peers. If we don't do this, incoming peers might start re-applying consensus WAL with operations, which were in fact already applied. This might result in data loss (e.g. if we create and delete same collection multiple times)

For bootstrapped peers:

  • We remove consensus state completely, with an exception for peer_id. Peer ID is needed to preserve shards topology.
    • We remove ConsensusState
    • We remove Consensus WAL, to force snapshot consensus recover

Test scenario used:

  • Create cluster of 2 peers (locally, with ports 63{3,4}5)
  • delete and create same collection multiple times (with some data in 2 shards)
  • Stop both peers
  • Run first peer with new port 6435 and --reinit param
  • Run the second peer with new port 6445 and --reinit param and bootstrap param pointed to 6435
  • Make sure that cluster functions on a new port and data is not lost
  • Attach another empty peer to the cluster, asses that new peer receives config correctly and is able to vote

@generall generall requested review from timvisee and qdrntsf October 20, 2024 19:20
@generall generall marked this pull request as ready for review October 20, 2024 19:26
generall
generall commented Oct 20, 2024
View reviewed changes
lib/storage/src/content_manager/consensus_manager.rs
Comment on lines +796 to +797
// ToDo: it seems like a mistake, need to check if it's correct
// debug_assert!(first_unapplied_index <= first_entry.index);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need extra review

Copy link
Member

@timvisee timvisee Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I think this is only valid if we always compact all entries.

Doesn't it need to be like this?

Suggested change
// ToDo: it seems like a mistake, need to check if it's correct
// debug_assert!(first_unapplied_index <= first_entry.index);
debug_assert!(first_unapplied_index >= first_entry.index);

@generall generall merged commit 34887c8 into dev Oct 29, 2024
17 checks passed
@generall generall deleted the reinit-consensus branch October 29, 2024 10:48
timvisee pushed a commit that referenced this pull request Nov 8, 2024
@generall @tellet-q @timvisee
Reinit consensus (#5265)
c71b008 
* option to clear consensus state while preserving peer id

* compact or clear consensus WAL on re-init

* Add test on reinit

* minor changes

* more points check suggestion

---------

Co-authored-by: tellet-q <elena.dubrovina@qdrant.com>
@timvisee timvisee mentioned this pull request Nov 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvisee timvisee timvisee approved these changes

@qdrntsf qdrntsf Awaiting requested review from qdrntsf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@generall @timvisee @tellet-q