raft: re-enable config change safety #124804
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
pav-kv
force-pushed
the
fix-confchange-safety
branch
from
711def6
to
7de8650
Compare
pav-kv
added
backport-24.1.x
Config changes in this raft implementation require a safety constraint: the leader must not append a config change if it hasn't applied all config changes in its log. The DisableConfChangeValidation flag disables this check under the assumption that the state machine layer provides the equivalent guarantee. However, it is hard to argue that this is true in split leaseholder/leader scenarios. This commit re-enables this check, to bring the safety back. The other two state-machine-level checks concerned with entering and leaving joint configs can still be disabled. Epic: none Release note: none
pav-kv
force-pushed
the
fix-confchange-safety
branch
from
7de8650
to
182fabf
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @pav-kv)
pkg/raft/testdata/confchange_disable_validation.txt line 59 at r1 (raw file):
1->3 MsgApp Term:1 Log:1/4 Commit:5 Entries:[1/5 EntryConfChangeV2 l2 l3] # Propose new config change. Note how it isn't rejected,
Do we want to adjust this test to demonstrate the new behavior where even with DisableConfChangeValidation=true, a new config change is rejected if the previous has not been applied yet?
There was a problem hiding this comment.
Reviewable status:
pkg/raft/testdata/confchange_disable_validation.txt line 59 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Do we want to adjust this test to demonstrate the new behavior where even with
DisableConfChangeValidation=true, a new config change is rejected if the previous has not been applied yet?
Done
This commit adds a test which ensures a config change is not proposed if the leader has another yet unapplied config change. Epic: none Release note: none
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
pav-kv
force-pushed
the
fix-confchange-safety
branch
from
44e4e55
to
8587815
Compare
|
bors r=nvanbenschoten |
Config changes in this raft implementation require a safety constraint: the leader must not append a config change if it hasn't applied all config changes in its log. The DisableConfChangeValidation flag disables this check under the assumption that the state machine layer provides the equivalent guarantee. However, it is hard to argue that this is true in split leaseholder/leader scenarios. This commit re-enables this check, to bring the safety back. The other two state-machine-level checks concerned with entering and leaving joint configs can still be disabled. Ported from cockroachdb/cockroach#124804
This commit adds a test which ensures a config change is not proposed if the leader has another yet unapplied config change. Ported from cockroachdb/cockroach#124804
Config changes in this
raftimplementation require a safety constraint: the leader must not append a config change if it hasn't applied all config changes in its log.The
DisableConfChangeValidationflag disables this check under the assumption that the state machine layer provides the equivalent guarantee. However, it is hard to argue that this is true in split leaseholder/leader scenarios.This commit re-enables this check, to bring the safety back. The other two state-machine-level checks concerned with entering and leaving joint configs can still be disabled.
Related to #105797, etcd-io/raft#81, #106515 (#106515 (comment))
Epic: CRDB-37617
Release note: none