Add Config.DisableConfChangeValidation #81
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 10, 2023
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 11, 2023
Relies on etcd-io/raft#81. We don't want it to, since that causes issues due to false positives. We are taking responsibility for carrying out only valid conf changes, as we always have. See also etcd-io/raft#80. Fixes cockroachdb#105797. Epic: CRDB-25287 Release note (bug fix): under rare circumstances, a replication change could get stuck when proposed near lease/leadership changes (and likely under overload), and the replica circuit breakers could trip. This problem has been addressed. Note to editors: this time it's really addressed (fingers crossed); a previous attempt with an identical release note had to be reverted.
pav-kv
approved these changes
Jul 11, 2023
ahrtr
reviewed
Jul 13, 2023
Signed-off-by: Tobias Grieger <tobias.b.grieger@gmail.com>
DisableConfChangeValidation turns off propose-time verification of configuration changes against the currently active configuration of the raft instance. 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. This is because configurations are activated during log application, and even the leader can trail log application by an unbounded number of entries. Symmetrically, the mechanism has false negatives - because the check may not run against the "actual" config that will be the predecessor of the newly proposed one, the check may pass but the new config may be invalid when it is being applied. In other words, the checks are best-effort. Users should *not* use this option unless they have a reliable mechanism (above raft) that serializes and verifies configuration changes. In CockroachDB, we doubly hit the false positive case. In CockroachDB, each proposal (including configuration changes) has a UUID which the leader tracks while the command is inflight. To detect that a command is no longer inflight, the leader has to see it apply (or needs some proof that it will never apply in the future). When a CockroachDB Replica loses leadership in the middle of a configuration change, a new leader may carry out additional changes that now take effect, while the old Replica is still trying to finalize its old, now incompatible, configuration change. What CockroachDB needs is to see the old proposal in the log so that it can be rejected at apply time, thus terminating the inflight status of the old configuration change. But since raft will forward the proposal to the leader where it is rejected (since it's incompatible with the active, much newer, config), this would never happen, thus leaking an inflight[^1]. The other case became obvious when we tried to work around the above by terminating the config change when we noticed that raft rejected it. Because the rejection is not made against a stable basis (it's against whatever config is active at the given time), legitimate configuration changes would be rejected frequently. In particular, this could occur in a way that would still result in the respective entries becoming applied later! This caused corruption in CockroachDB[^2] by leading to a divergence between what we thought the config was and what it actually was. CockroachDB does have a higher-level protection mechanism (configuration changes are serialized through transactions reading from and writing to the same key). The intention is for CockroachDB to use this setting, however it is likely that other systems are susceptible to similar issues (and perhaps unknowingly so). This setting is thus of general interest. Touches #80. [^1]: cockroachdb/cockroach#105797 (comment) [^2]: cockroachdb/cockroach#106172 (comment) Signed-off-by: Tobias Grieger <tobias.b.grieger@gmail.com>
tbg
force-pushed
the
skip-conf-change-validation
branch
from
ab163ca
to
d8d274b
Compare
ahrtr
approved these changes
Jul 17, 2023
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 17, 2023
Picks up - etcd-io/raft#77 - etcd-io/raft#79 (essentially an off-by-default for 77) - etcd-io/raft#82 - etcd-io/raft#81 (needed for cockroachdb#105797) Epic: none Release note: None
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 17, 2023
Relies on etcd-io/raft#81. We don't want it to, since that causes issues due to false positives. We are taking responsibility for carrying out only valid conf changes, as we always have. See also etcd-io/raft#80. Fixes cockroachdb#105797. Epic: CRDB-25287 Release note (bug fix): under rare circumstances, a replication change could get stuck when proposed near lease/leadership changes (and likely under overload), and the replica circuit breakers could trip. This problem has been addressed. Note to editors: this time it's really addressed (fingers crossed); a previous attempt with an identical release note had to be reverted.
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
Jul 17, 2023
105368: backupccl: add unit tests for FileSSTSink r=rhu713 a=rhu713 Backfill unit tests for the basic functionality of FileSSTSink with additional test cases involving inputs of keys with many entries in its revision history. Epic: CRDB-27758 Release note: None 105624: jobsprofiler: dump trace recording on job completion r=dt a=adityamaru This change teaches the job resumer to fetch and write its trace recording before finishing its tracing span. These traces will be consumed by the job profiler bundle that is being introduced in #105384. These traces will be valuable in understanding a job's execution characteristics during each resumption, even if the job has reached a terminal state. Currently, this behaviour is opt-in and has been enabled for backups, restore, import and physical replication jobs. Informs: #102794 Release note: None 106515: DEPS: bump across etcd-io/raft#81 and disable conf change validation r=erikgrinaker a=tbg We don't want raft to validate conf changes, since that causes issues due to false positives (the check is above raft, but needs to be below raft to always work correctly). We are taking responsibility for carrying out only valid conf changes, as we always have. See also etcd-io/raft#80. Fixes #105797. Epic: CRDB-25287 Release note (bug fix): under rare circumstances, a replication change could get stuck when proposed near lease/leadership changes (and likely under overload), and the replica circuit breakers could trip. This problem has been addressed. Note to editors: this time it's really addressed (fingers crossed); a previous attempt with an identical release note had to be reverted. 106939: changefeedccl: fix flake in TestParquetRows r=miretskiy a=jayshrivastava changefeedccl: fix flake in TestParquetRows Previously, this test would flake when rows were not emitted in the exact order they were inserted/modified. This change makes the test resilient to different ordering. Epic: None Fixes: #106911 Release note: None --- util/parquet: make metadata transparent in tests Previously, users of the library would need to explicitly call `NewWriterWithReaderMetadata()` to configure the parquet writer to add metadata required to use reader utils in `pkg/util/parquet/testutils.go`. This led to a lot of code uncessary code duplication. This moves the logic to decide if metadata should be written to `NewWriter()` so callers do not need to do the extra work. Epic: None Release note: None Co-authored-by: Rui Hu <rui@cockroachlabs.com> Co-authored-by: adityamaru <adityamaru@gmail.com> Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com> Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com>
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 18, 2023
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 18, 2023
Relies on etcd-io/raft#81. We don't want it to, since that causes issues due to false positives. We are taking responsibility for carrying out only valid conf changes, as we always have. See also etcd-io/raft#80. Fixes cockroachdb#105797. Epic: CRDB-25287 Release note (bug fix): under rare circumstances, a replication change could get stuck when proposed near lease/leadership changes (and likely under overload), and the replica circuit breakers could trip. This problem has been addressed. Note to editors: this time it's really addressed (fingers crossed); a previous attempt with an identical release note had to be reverted.
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 18, 2023
Had to leave out the test, but we'll exercise it through the CRDB-level unit test added in the next commit.
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Jul 18, 2023
Relies on etcd-io/raft#81. We don't want it to, since that causes issues due to false positives. We are taking responsibility for carrying out only valid conf changes, as we always have. See also etcd-io/raft#80. Fixes cockroachdb#105797. Epic: CRDB-25287 Release note (bug fix): under rare circumstances, a replication change could get stuck when proposed near lease/leadership changes (and likely under overload), and the replica circuit breakers could trip. This problem has been addressed. Note to editors: this time it's really addressed (fingers crossed); a previous attempt with an identical release note had to be reverted.
tbg
added a commit
to cockroachdb/cockroach
that referenced
this pull request
Jul 24, 2023
release-22.2: DEPS: bump across etcd-io/raft#81 and disable conf change validation
tbg
added a commit
to cockroachdb/cockroach
that referenced
this pull request
Jul 24, 2023
release-23.1: DEPS: bump across etcd-io/raft#81 and disable conf change validation
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
May 29, 2024
124804: raft: re-enable config change safety r=nvanbenschoten a=pav-kv 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. Related to #105797, etcd-io/raft#81, #106515 (#106515 (comment)) Epic: none Release note: none 124806: release: push PRs to main repo r=celiala a=rail Previously, we pushed PRs using the forked repos. For auto-merge to work, we need to push these PRs back to the main repo, so `GITHUB_TOKEN` has enough privileges to merge the PRs. This PR adds an ability to push the PR branches to the origin repo. Note: the push user has to be given `write` permissions to the repos. Epic: none Release note: None Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com> Co-authored-by: Rail Aliiev <rail@iqchoice.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DisableConfChangeValidation turns off propose-time verification of
configuration changes against the currently active configuration of the
raft instance. 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. This is because configurations are activated during log
application, and even the leader can trail log application by an
unbounded number of entries.
Symmetrically, the mechanism has false negatives - because the check may
not run against the "actual" config that will be the predecessor of the
newly proposed one, the check may pass but the new config may be invalid
when it is being applied. In other words, the checks are best-effort.
Users should not use this option unless they have a reliable mechanism
(above raft) that serializes and verifies configuration changes.
In CockroachDB, we doubly hit the false positive12 case and we do have a
higher-level protection mechanism. The intention is for CockroachDB to use this
setting, however it is likely that other systems are susceptible to similar
issues (and perhaps unknowingly so). This setting is thus of general interest.
Footnotes
https://github.com/cockroachdb/cockroach/issues/106172#issuecomment-1623568845 ↩
https://github.com/cockroachdb/cockroach/issues/105797#issuecomment-1613201509 ↩