raft: forbid direct voter removal without demotion #131445
Conversation
|
This change is |
There was a problem hiding this comment.
A few questions, mostly due to me needing some help to page this back in. Will leave unapproved due to this and your open TODO in the last commit.
Is the argument for why adding an assertion when switching configs is safe that "we haven't done voter removals in 5 years and so nobody will have a direct voter removal sitting in their raft log and getting applied when the new binary boots up"?
Also, I'm doubting that understand the DisableConfChangeValidation comment entirely. This part:
These checks are generally sensible (cannot leave a joint
// config unless in a joint config, et cetera) but they have false positives
// because the active configuration may not be the most recent
// configuration.
Yet, we call ValidateProp on the leader and the first thing it does is make sure we've applied all previous config changes. Which means the only way the visible config isn't the right one to compare with is that the instance isn't actually the leader any more and the caller has seen a newer config already. In this case, we basically need to make sure the config never makes it into the committed log - or at least need to make sure it applies as a no-op (i.e. apply-time validation vs propose-time). I feel like there was more to this than I can reconstruct now. I seem to remember a case in which conf changes were discarded erroneously, and this somehow impacted availability.
Assuming you have a good grasp on this flag, could you touch up the comment here to that effect? If you're similarly unsure I can dig up some dirt. @pav-kv this also seems up your alley.
I also can't help but think how silly it is that we don't have a sequence number in the config and do first a validation of consecutive sequence number and then can authoritatively run sanity checks on the transition. This might be worth filing an issue for? I doubt we want to tackle it directly now.
nvanbenschoten
force-pushed
the
nvanbenschoten/dontAllowVoterRemoval
branch
from
f0c9ee9
to
9b7a345
Compare
This TODO is now addressed. PTAL!
Essentially, yes. It seems exceedingly unlikely that such an unapplied config change would survive across 5 years of biannual upgrades where every upgrade requires a rolling restart. We have also flushed the raft log on each range at least once during this period with the lock table migration. I would be ok with changing it to an
Yes, that's correct, see the discussion on d8d274b, which talks about what was going wrong. Note that this PR is moving some of this code, but it isn't actually changing any of it. I'm happy if we start pulling on some of this again, but it would be better if that happens outside of the critical path of leader leases. |
nvanbenschoten
force-pushed
the
nvanbenschoten/dontAllowVoterRemoval
branch
from
9b7a345
to
b7f63f1
Compare
This commit extracts a confchange.ValidateProp function, which is called from `stepLeader` when handling a ConfChange MsgProp. Extracting will let us more easily test the validation logic. Epic: None Release note: None
Informs cockroachdb#129796. Informs cockroachdb#125355. This commit is a natural continuation of 7f37eaa (from 5 years ago). The commit made it so that the replicateQueue would never issue a ChangeReplicas request that removed a voter replica. Instead, the replicate queue would always demote the voter replica to a learner replica first. This commit makes it so that raft will no longer allow direct voter removal in a proposed configuration change. Instead, it requires the voter (who may be the leader) to first be demoted to a learner. This prevents abuse and strengthens the guarantee that we will never perform such unsafe configuration changes. Release note: None
Informs cockroachdb#129796. Informs cockroachdb#125355. Perform the same assertion as we have in pkg/raft, but for cases where DisableConfChangeValidation is set to true. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/dontAllowVoterRemoval
branch
from
b7f63f1
to
686d59d
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 7 of 7 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
|
TFTR! bors r+ |
Informs #129796.
Informs #125355.
This commit is a natural continuation of 7f37eaa (from 5 years ago). The commit made it so that the replicateQueue would never issue a ChangeReplicas request that removed a voter replica. Instead, the replicate queue would always demote the voter replica to a learner replica first.
This commit makes it so that raft will no longer allow direct voter removal in a proposed configuration change. Instead, it requires the voter (who may be the leader) to first be demoted to a learner. This prevents abuse and strengthens the guarantee that we will never perform such unsafe configuration changes.
Release note: None