raftengine: complete etcd phase 5 rollout#481
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for the etcd/raft backend, introducing peer metadata persistence, engine-specific data directory markers to prevent unsafe engine switches, and an offline migration tool. The etcd engine is refactored to handle configuration changes and snapshots more robustly, including limits on pending configurations and a new snapshotKind classification. Feedback highlights an opportunity to optimize the Raft loop by caching the persisted configuration index to avoid redundant disk I/O and suggests updating the snapshotInFlight logic to properly handle the new snapshot types and prevent concurrency issues.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the etcd/raft backend as an opt-in engine for Elastickv, providing an alternative to the legacy HashiCorp Raft implementation. The changes include a safety mechanism using a 'raft-engine' marker file to prevent data directory corruption from engine mismatches, logic for persisting peer metadata to ensure consistency across restarts, and updates to the Jepsen test suite and documentation. Feedback identifies a critical concurrency issue in the persistConfigState method, where disk I/O and snapshot serialization are performed without holding the required lock, potentially leading to race conditions with concurrent snapshot restores.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a safety mechanism to prevent switching Raft engines within an existing data directory by using a marker file. It also introduces durable peer metadata persistence for the etcd/raft backend, ensuring cluster configurations are correctly restored after a restart. The changes include refactoring the engine's startup and configuration application logic to synchronize peer metadata with Raft snapshots. Feedback was provided regarding the handling of learner nodes in the peer metadata update logic to ensure future compatibility.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft backend from HashiCorp to etcd/raft. It introduces a migration runbook, an offline migration tool, and safety mechanisms to prevent accidental engine switches in existing data directories via a marker file. Core changes include refactoring the etcd engine to persist peer metadata durably and updating Jepsen and E2E tests to validate the new default. Feedback was provided regarding the peer metadata persistence logic, specifically that the NodeID from the decoded context should be verified against the nodeID being processed to avoid persisting incorrect metadata during batch configuration changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft engine from HashiCorp to etcd/raft, introducing a comprehensive migration runbook, offline migration tooling, and safety markers to prevent engine mismatches in data directories. Key technical changes include the implementation of synchronous configuration snapshotting and persistent peer metadata management within the etcd engine. Jepsen and E2E test suites have been updated to support and default to the new backend. Feedback suggests removing unreachable background plumbing for configuration snapshots and adopting bounded memory allocation during string deserialization to mitigate potential OOM vulnerabilities.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft engine from HashiCorp to etcd/raft, introduces a comprehensive migration runbook, and implements a new peer metadata persistence layer (etcd-raft-peers.bin). It also adds safety mechanisms to detect and prevent unsafe engine switches within existing data directories. The etcd backend implementation was refactored to ensure durable publication of configuration changes and better synchronization with snapshot restores. Feedback identifies a logic issue where peer metadata updates could overwrite known addresses with incomplete information and suggests enhancing engine detection by including the new peer metadata file in the artifact check.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft engine from HashiCorp to etcd/raft, introducing a comprehensive migration runbook and safety mechanisms to prevent engine mismatches in data directories. Key technical changes include the implementation of a persistent peer metadata store to ensure cluster configuration consistency across restarts and a refactored startup sequence that validates local state. The review feedback identifies critical issues in the updated peer management logic, specifically noting that the removal of dummy peer creation for address-less nodes and the silent skipping of normalization errors could lead to inconsistent internal state and cluster mismatch failures during startup.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft backend to etcd/raft, introducing a persistent peer metadata store and a comprehensive migration runbook. Key enhancements include directory-level engine markers to prevent unsafe in-place switches and improved durability for configuration changes by persisting peer state alongside snapshots. The etcd/raft implementation was refactored to support these persistence requirements, and Jepsen/E2E tests were updated accordingly. Feedback was provided to log potential errors when closing the peer metadata file to enhance observability.
Address outstanding review feedback: - Log file.Close() errors in readPersistedPeersFile instead of silently discarding them. Add scripts/engine-migrate.sh for stop-the-world hashicorp→etcd engine migration. The script stops all cluster nodes, runs etcd-raft-migrate on each to bootstrap etcd WAL/snap state from the existing FSM store, archives hashicorp raft artifacts, updates the engine marker, then restarts all nodes via rolling-update.sh with --raftEngine etcd.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft engine from HashiCorp to etcd/raft and introduces a robust offline migration path. Key changes include the implementation of durable peer metadata persistence, the addition of an engine marker file to prevent unsafe in-place switches, and the introduction of a new migration runbook and orchestration script. Feedback suggests refining the snapshot request channel buffering to align with the existing concurrency model and utilizing mktemp for remote migration workspaces to avoid potential path conflicts.
- Reduce snapshotReqCh buffer from 8 to 1 to match the snapshotInFlight guard that already limits concurrency to a single in-flight snapshot. - Use mktemp on remote hosts in engine-migrate.sh instead of hardcoded /tmp paths to avoid conflicts between concurrent runs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Raft engine from HashiCorp to etcd/raft, introducing a mandatory offline migration path, a dedicated migration tool, and a runbook for existing clusters. Key changes include the implementation of persistent peer metadata to ensure consistent cluster membership across restarts, the addition of a raft-engine marker to prevent unsafe in-place engine switches, and updates to the Jepsen and E2E test suites to support the new default. Feedback suggests adding a non-interactive flag to the migration script to support automated environments.
| read -r -p "Continue? [y/N] " confirm | ||
| if [[ "$confirm" != [yY] ]]; then | ||
| echo "aborted" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The migration script is interactive and requires manual confirmation. This will cause the script to hang or fail in automated environments (e.g., CI/CD pipelines or automated recovery scripts). Consider adding a flag (e.g., -y or --yes) to bypass the interactive prompt for non-interactive usage.
1 participant
No description provided.