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

Run a barrier before config changes when required #402

Closed
wants to merge 3 commits into from

Conversation

cole-miller
Copy link
Contributor

@cole-miller cole-miller commented Sep 15, 2022

Like the other barriers PR, this is prompted by looking into the consequences of canonical/raft#302. When we're attempting a config change, we check that no other config change is already in progress (raft->configuration_uncommitted_index != 0) -- but in order for that check to work as intended, a leader node has to first make sure its commit index is up to date, and that means running a barrier if no entry from the current term has been appended yet. Concretely, this becomes relevant if a leader "A" replicates and commits a config change, but crashes before propagating the new commit index to any other nodes. The next leader "B" will have configuration_uncommitted_index != 0, so it can't accept any further config changes until some other entry from its term is committed. To prevent this from inconveniencing dqlite clients, this PR adds a new mode to leader__barrier, where the needsBarrier check is replaced by the condition

raft_configuration_uncommitted_index(l->raft) != 0 &&
raft_last_log_term(l->raft) < raft_current_term(l->raft)

(See canonical/raft#311 for the implementation of these getters -- this PR won't build unless/until that merges into raft.)

Then we run this version of leader__barrier before attempting any config change.

This is distinctly WIP -- I'm putting it up now to get feedback on the basic approach, in particular whether we want to handle this in dqlite (as opposed to raft). It will need to be rebased once #395 merges, since I picked up the cursor change from that branch here. If you're reviewing, just ignore the first two commits.

Signed-off-by: Cole Miller <cole.miller@canonical.com>
Signed-off-by: Cole Miller <cole.miller@canonical.com>
Signed-off-by: Cole Miller <cole.miller@canonical.com>
@cole-miller
Copy link
Contributor Author

cole-miller commented Sep 26, 2022

Rebased.

@cole-miller
Copy link
Contributor Author

Closing as no longer needed, see canonical/raft#302 (comment)

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

Successfully merging this pull request may close these issues.

1 participant