Skip to content

Keep a newly joined server in the config it appends as leader - #136

Open
groeneai wants to merge 1 commit into
ClickHouse:masterfrom
groeneai:fix-become-leader-uncommitted-config
Open

groeneai wants to merge 1 commit into
ClickHouse:masterfrom
groeneai:fix-become-leader-uncommitted-config

Conversation

@groeneai

Copy link
Copy Markdown

A server that joins takes its config from the join request. That is the leader's committed config, and it does not contain the joiner. The joiner's log is then synced to it (from index 1 for a new server), so it holds config entries older than that config, followed by the entry that adds the joiner.

If the joiner is elected before its state machine applies those entries, become_leader() scans the uncommitted range for the newest config, but it stops at the first entry older than the current config (the check from #37). ClickHouse Keeper's rcfg does transfer_leadership right after adding servers, so it can hit this. The scan never reaches the entry that adds the joiner and re-appends the join config. Once that commits:

  • the followers drop the joiner (server 7 is removed from cluster);
  • the new leader runs a cluster it is not a member of;
  • requests forwarded to it fail (cannot find leader peer).

Seen in ClickHouse CI, test_keeper_4lw_reconfiguration on amd_tsan: 1, 2, 3.

Changes:

  • Skip older config entries instead of stopping at them. A config that force recovery sets at next_slot still wins, because every entry in the scanned range is older than it.
  • Set uncommitted_config_ to the appended config, as the membership and priority changes do. Otherwise a priority change handled before the new leader applies that config is built from its previous config, which on a joiner still lacks the joiner.

Of the other config changes, add and remove also start from uncommitted_config_. flip_learner_flag(), set_user_ctx() and update_srv_config() start from get_config(); ClickHouse Keeper calls none of them (nor sets use_new_joiner_type_), so they are unchanged here.

The new test new_joiner_elected_before_sm_catches_up_keeps_itself_in_config_test pauses the joiner's state machine, elects the joiner with yield_leadership, then forwards a set_priority_v2 to it:

  • without the first change it fails at the appended config;
  • with only the first change it fails once the priority change commits (the joiner is removed again);
  • with both it passes.

A server that joins the cluster takes its config from the join request.
That is the leader's committed config, which does not contain the joiner.
The joiner's log is then synced from the start, so it holds config entries
older than its current config, followed by the config that adds it.

If the joiner is elected before its state machine applies those entries,
become_leader() scans the uncommitted range for the newest config, but the
scan stopped at the first entry older than the current config. It never
reached the config that adds the joiner and re-appended the join config
instead, so the new leader removed itself from the cluster: the followers
dropped it as a peer, and requests they forward to it failed.

Skip older config entries instead of stopping at them. A config that
force recovery sets ahead of the log is still preferred, because every
entry in the scanned range is older than it.

Also set uncommitted_config_ to the appended config, as every other
leader-side config change does. Otherwise a priority change handled before
the new leader applies that config is built from its previous config,
which on a new joiner still lacks the joiner.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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