Repository navigation
feat #369: prepare d-engine node image and CLI for Jepsen integration - #370
Merged
Merged
Conversation
…age-standalone-demo
- Delete docker/jepsen/: moved to d-engine-jepsen repo - Delete docker/config/n*.toml: config now mounted via ../config directly - Fix Dockerfile: workspace-root build context, bookworm base, BuildKit cache mounts - Fix docker-compose.yml: config volume default ./config -> ../config - Update README: remove symlink step, reference d-engine-jepsen for tests
- iproute2: partition nemesis needs `ip` to enumerate network interfaces - wget: nc/nemesis-packages setup! downloads bitflip binary via wget
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds CAS (compare-and-swap) testing capabilities, refactors Docker infrastructure into a multi-stage build, updates configuration and documentation for Docker deployment, and removes deprecated Jepsen-based testing infrastructure. The changes update example applications and testing frameworks while simplifying deployment configurations. ChangesTesting & Example Infrastructure Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
examples/three-nodes-standalone/docker/README.md (1)
41-47: ⚡ Quick winDirectory switching may confuse users.
The workflow requires switching between
d-engine(steps 1-3) andd-engine-jepsen(steps 2.3 and 4) directories multiple times. Consider adding explicitcdcommands or a clearer directory context indicator for each step.🧭 Proposed improvement
### 4. Run Jepsen Tests ```bash -# From d-engine-jepsen directory +# Switch to the Jepsen test repository cd ../d-engine-jepsen/ + +# Run all Jepsen workloads make test # Run a specific workload (register / bank / set) make test WORKLOAD=bankAlternatively, add a navigation section at the beginning summarizing the directory structure: ```markdown ## Directory Structure - `d-engine/` - Main repository (workspace root) - `d-engine-jepsen/` - External Jepsen test suite (sibling directory)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/three-nodes-standalone/docker/README.md` around lines 41 - 47, The README currently switches directories implicitly and may confuse users; update the instructions to show explicit directory context by adding clear cd commands and descriptive comments: prepend a navigation section listing the two repositories (d-engine and d-engine-jepsen), update the code block to include an explicit "Switch to the main repository" or "Switch to the Jepsen test repository" comment and a cd ../d-engine-jepsen/ line before running make test, and keep the examples for running "make test" and "make test WORKLOAD=bank" (reference symbols: directories "d-engine" and "d-engine-jepsen" and commands "make test" and "WORKLOAD=bank") so users know exactly which directory to be in for each step.d-engine-server/tests/cas_operations/bank_transfer_invariant_embedded.rs (1)
77-78: ⚡ Quick winAvoid hardcoded
/tmppaths for the per-node config file.
/tmp/d-engine-bank-node{node_id}.tomllives outsidetemp_dir, so it (a) is not cleaned up after the test, (b) is not portable to environments without/tmp(e.g. Windows CI), and (c) can collide with any other test that uses the same naming pattern (the#[serial]annotation only guards within a binary, not across binaries running in parallel). Writing the config undertemp_dirkeeps the test self-contained.♻️ Proposed fix to keep the config inside the temp dir
let (storage, state_machine) = RocksDBUnifiedEngine::open(&db_path)?; - let config_path = format!("/tmp/d-engine-bank-node{node_id}.toml"); - tokio::fs::write(&config_path, &config_str).await?; + let config_path = temp_dir + .path() + .join(format!("d-engine-bank-node{node_id}.toml")); + tokio::fs::write(&config_path, &config_str).await?; let engine = EmbeddedEngine::start_custom( Arc::new(storage), Arc::new(state_machine), - Some(&config_path), + Some(config_path.to_str().unwrap()), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-server/tests/cas_operations/bank_transfer_invariant_embedded.rs` around lines 77 - 78, The test currently writes the per-node config to a hardcoded "/tmp/d-engine-bank-node{node_id}.toml"; change this to create the config file inside the existing temp_dir instead (use temp_dir.path() combined with format!("d-engine-bank-node{node_id}.toml") to build the file name), assign that joined path to config_path (replacing the hardcoded string), and then call tokio::fs::write on that path so the file is created within temp_dir, ensuring cleanup, portability, and no cross-test collisions; update any uses of config_path accordingly.d-engine-server/tests/cas_operations/mod.rs (1)
6-13: 💤 Low valueOptional: list the new bank-invariant scenario in the module doc.
The module-level doc explicitly enumerates each scenario and each embedded test file (lines 6–13 and 37–40), so adding the new
bank_transfer_invariant_embedded.rsthere keeps the doc in sync and helps future readers find the scenario.📝 Suggested doc additions
//! 5. **Leader Failover + CAS (Embedded)**: CAS atomicity during leader crash //! 6. **Leader Failover + CAS (Standalone)**: gRPC client retry after leader crash +//! 7. **Bank Transfer Invariant (Embedded)**: Concurrent CAS transfers preserve total balance under follower failure//! - ✅ **Embedded Mode**: In-process EmbeddedClient //! - `distributed_lock_embedded.rs` - Multi-client lock competition //! - `snapshot_recovery_embedded.rs` - CAS state persistence //! - `leader_failover_cas_embedded.rs` - CAS atomicity during leader crash +//! - `bank_transfer_invariant_embedded.rs` - Concurrent transfers preserve total balance under follower failureAlso applies to: 37-40, 84-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-server/tests/cas_operations/mod.rs` around lines 6 - 13, The module doc lists the test scenarios but omits the new bank_transfer_invariant_embedded.rs test; update the module-level documentation comment to include a new entry for "Bank Transfer Invariant (Embedded)" and also add "bank_transfer_invariant_embedded.rs" to the enumerated embedded test file lists (the same doc sections currently listing other *_embedded.rs files) so the scenario and filename appear alongside the existing entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-server/tests/cas_operations/bank_transfer_invariant_embedded.rs`:
- Around line 27-30: The file fails rustfmt: reformat the unpack function and
the test function signature to match cargo fmt; change the unpack function
(unpack) so the returned [u64; 3] array is written across multiple lines with
each element on its own line (e.g., open bracket, each element expression on its
own line, close bracket) and adjust the test function signature to a single-line
form (e.g., fn test_name() { ...) so cargo fmt --all -- --check passes; run
cargo fmt to verify.
In `@examples/three-nodes-standalone/docker/Dockerfile`:
- Line 86: The Dockerfile currently starts three services by backgrounding them
in the CMD string (sshd, demo, promtail), which prevents proper signal
propagation and process supervision; instead, add a lightweight init/supervisor
(e.g., tini or supervisord), create an entrypoint script (e.g.,
/app/entrypoint.sh) that starts sshd and promtail in the background and then
uses exec to run the foreground process (demo) so PID 1 is the main process,
update the Dockerfile to install/configure the chosen init (tini) and set
ENTRYPOINT to run it and CMD to invoke the entrypoint script; ensure the
entrypoint uses exec for demo so signals are forwarded and backgrounded services
are children of the init process for correct reaping and shutdown.
In `@examples/three-nodes-standalone/docker/README.md`:
- Around line 32-35: Clarify that the docker-compose commands must be run from
the examples/three-nodes-standalone/docker/ directory (because
docker-compose.yml uses CONFIG_PATH default ../config), update the README to
show changing into that directory (cd examples/three-nodes-standalone/docker/)
before running docker-compose up -d for the main cluster, and instruct using
docker-compose -f monitoring/docker-compose.yml up -d (or docker-compose up -d
from the monitoring subdir) for the monitoring stack so relative volume paths in
docker-compose.yml resolve correctly.
---
Nitpick comments:
In `@d-engine-server/tests/cas_operations/bank_transfer_invariant_embedded.rs`:
- Around line 77-78: The test currently writes the per-node config to a
hardcoded "/tmp/d-engine-bank-node{node_id}.toml"; change this to create the
config file inside the existing temp_dir instead (use temp_dir.path() combined
with format!("d-engine-bank-node{node_id}.toml") to build the file name), assign
that joined path to config_path (replacing the hardcoded string), and then call
tokio::fs::write on that path so the file is created within temp_dir, ensuring
cleanup, portability, and no cross-test collisions; update any uses of
config_path accordingly.
In `@d-engine-server/tests/cas_operations/mod.rs`:
- Around line 6-13: The module doc lists the test scenarios but omits the new
bank_transfer_invariant_embedded.rs test; update the module-level documentation
comment to include a new entry for "Bank Transfer Invariant (Embedded)" and also
add "bank_transfer_invariant_embedded.rs" to the enumerated embedded test file
lists (the same doc sections currently listing other *_embedded.rs files) so the
scenario and filename appear alongside the existing entries.
In `@examples/three-nodes-standalone/docker/README.md`:
- Around line 41-47: The README currently switches directories implicitly and
may confuse users; update the instructions to show explicit directory context by
adding clear cd commands and descriptive comments: prepend a navigation section
listing the two repositories (d-engine and d-engine-jepsen), update the code
block to include an explicit "Switch to the main repository" or "Switch to the
Jepsen test repository" comment and a cd ../d-engine-jepsen/ line before running
make test, and keep the examples for running "make test" and "make test
WORKLOAD=bank" (reference symbols: directories "d-engine" and "d-engine-jepsen"
and commands "make test" and "WORKLOAD=bank") so users know exactly which
directory to be in for each step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f33d27f7-ef3e-4b00-b847-46d09b44c1f3
📒 Files selected for processing (21)
.dockerignored-engine-server/tests/cas_operations/bank_transfer_invariant_embedded.rsd-engine-server/tests/cas_operations/mod.rsexamples/client-usage-standalone/src/main.rsexamples/three-nodes-standalone/docker/Dockerfileexamples/three-nodes-standalone/docker/README.mdexamples/three-nodes-standalone/docker/config/n1.tomlexamples/three-nodes-standalone/docker/config/n2.tomlexamples/three-nodes-standalone/docker/config/n3.tomlexamples/three-nodes-standalone/docker/config/n4.tomlexamples/three-nodes-standalone/docker/config/n5.tomlexamples/three-nodes-standalone/docker/docker-compose.ymlexamples/three-nodes-standalone/docker/jepsen/Dockerfileexamples/three-nodes-standalone/docker/jepsen/Makefileexamples/three-nodes-standalone/docker/jepsen/README.mdexamples/three-nodes-standalone/docker/jepsen/entrypoint.shexamples/three-nodes-standalone/docker/jepsen/project.cljexamples/three-nodes-standalone/docker/jepsen/src/jepsen/d_engine.cljexamples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/Cargo.tomlexamples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/README.mdexamples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/src/main.rs
💤 Files with no reviewable changes (14)
- examples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/README.md
- examples/three-nodes-standalone/docker/jepsen/project.clj
- examples/three-nodes-standalone/docker/jepsen/Dockerfile
- examples/three-nodes-standalone/docker/config/n3.toml
- examples/three-nodes-standalone/docker/config/n5.toml
- examples/three-nodes-standalone/docker/jepsen/src/jepsen/d_engine.clj
- examples/three-nodes-standalone/docker/config/n1.toml
- examples/three-nodes-standalone/docker/config/n4.toml
- examples/three-nodes-standalone/docker/jepsen/README.md
- examples/three-nodes-standalone/docker/jepsen/Makefile
- examples/three-nodes-standalone/docker/config/n2.toml
- examples/three-nodes-standalone/docker/jepsen/entrypoint.sh
- examples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/Cargo.toml
- examples/three-nodes-standalone/docker/jepsen/vendor/dengine_ctl/src/main.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…om git - Exclude .rules and .github/copilot-instructions.md from version control (consistent with existing CLAUDE.md / .claude/ policy) - Fix README ambiguity: docker-compose must run from docker/ so that relative volume paths (../config) resolve correctly
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What Does This PR Do?
Adds
cassubcommand to the standalone demo CLI and extends the node Docker image with tools required by Jepsen nemesis packages (partition, kill, pause). Removes the embedded Jepsen suite that has been superseded by the standaloned-engine-jepsenrepo.Type:
Why Is This Needed?
For features: Link to approved issue #369
Jepsen integration (d-engine-jepsen) requires:
casCLI subcommand — all Jepsen workloads (bank, set, append) depend on atomic compare-and-swapiproute2in the node image — partition nemesis usesipto inject network ruleswgetin the node image — Jepsen nemesis setup downloads tooling at runtimedocker/jepsen/— Jepsen tests have moved to the standaloned-engine-jepsenrepo to avoid coupling test infrastructure with the engineChecklist
Required:
make testpassesbank_transfer_invariant_embedded)If changing APIs:
Testing
How tested:
bank_transfer_invariant_embeddedcovers CAS-based concurrent transfer invariantmake test-all(register/bank/set/append) passes ind-engine-jepsenclient-usage-standalone-demo cas <key> <expected> <new>verified against live 3-node clusterFor bug fixes:
Does This Follow d-engine's Principles?
Reviewer Notes
The
docker/jepsen/subtree deletion is intentional — all Jepsen infrastructure now lives in thed-engine-jepsenrepo. The embedded vendor crate (dengine_ctl) and its Makefile/Dockerfile are no longer needed.Estimated review complexity:
Summary by CodeRabbit
Release Notes
New Features
CasCLI subcommand for atomic compare-and-swap operations in the example client.Documentation
Chores