feat: docker-volume-backup-restore#74
Conversation
refactor: move build_tar to utils::compress refactor: move choose_restore_path to utils::common docs: remove docker-volume README section fix: keep archive path for multi-file non-docker-volume restores docs: trim docker-volume README section to essentials refactor: run docker-volume backup/restore inside spawn_blocking like other providers docs: enable docker socket, add volume example and security notes feat: sweep orphaned ephemeral helper containers on startup feat: docker-volume clean-replace restore via upload_to_container fix: serialize env-var access in docker-volume tests to avoid setenv/getenv UB feat: docker-volume backup via download_from_container feat: docker-volume ping via inspect_volume fix: return extraction dir for multi-file restore archives feat: gzip already-tar inputs directly instead of double-wrapping feat: docker helper (client, self-image, container lifecycle, sweep) feat: docker-volume provider
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a Docker-volume-backed database backend that uses ephemeral helper containers for backup and restore. It introduces ChangesDocker Volume Backend
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant DockerVolumeDatabase
participant BackupRun as backup::run
participant Docker
participant TarFile
DockerVolumeDatabase->>BackupRun: run(cfg, backup_dir, logger)
BackupRun->>Docker: stop_container (optional)
BackupRun->>Docker: create_helper(volume)
BackupRun->>Docker: stream /vol archive
Docker-->>TarFile: write bytes
BackupRun->>Docker: remove_helper
BackupRun->>Docker: start_container (optional)
BackupRun-->>DockerVolumeDatabase: PathBuf
sequenceDiagram
participant DockerVolumeDatabase
participant RestoreRun as restore::run
participant Docker
participant Archive
DockerVolumeDatabase->>RestoreRun: run(cfg, archive, logger)
RestoreRun->>Docker: stop_container (optional)
RestoreRun->>Docker: create_helper + start
RestoreRun->>Docker: wipe /vol
RestoreRun->>Archive: upload_to_container
RestoreRun->>Docker: remove_helper
RestoreRun->>Docker: start_container (optional)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (5)
src/tests/domain/mysql.rs (1)
24-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
Defaultimpl forDatabaseConfigto avoid repetitive test edits.Every new field added to
DatabaseConfig(herevolume_name/container_name) requires touching every test's config literal (mysql.rs, postgres.rs, redis.rs, valkey.rs, firebird.rs, mariadb.rs, mongodb.rs, mssql.rs). ImplementingDefaultforDatabaseConfigand using..Default::default()in these test builders would isolate future field additions to a single place.🤖 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 `@src/tests/domain/mysql.rs` around lines 24 - 38, The test config literals for DatabaseConfig are duplicated across many domain tests, so adding fields forces repeated edits. Add a Default implementation for DatabaseConfig that sets sensible baseline values, then update the test builders (including mysql.rs and the other database tests) to construct the struct with ..Default::default() and only override the fields each test actually needs. This will centralize future field additions and keep the test setup stable.src/utils/common.rs (1)
5-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
_extraction_dirparameter.The parameter is never used in the selection logic; the multi-file branch just falls back to the raw archive path. If this is intentional (mirroring prior inline behavior), consider dropping the parameter for a cleaner API, or wire it in if extraction-dir-based selection was intended for future dump formats (e.g. directory-format multi-file dumps).
🤖 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 `@src/utils/common.rs` around lines 5 - 14, The choose_restore_path function has an unused _extraction_dir parameter and the current multi-file branch just returns the archive path. Either remove the parameter from choose_restore_path and update its call sites if it is intentionally unused, or incorporate _extraction_dir into the path-selection logic if extraction-dir-based resolution is expected for multi-file dump formats. Keep the behavior consistent with the existing single-file and fallback cases.src/tests/domain/docker_volume.rs (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGate the Docker-daemon integration tests so they don't hard-fail CI.
docker_volume_ping_true_for_existing_volume,docker_volume_backup_captures_files,docker_volume_restore_is_clean_replace, andsweep_removes_labeled_helpersall callclient().expect("docker daemon required..."). In any environment without a reachable Docker socket thesepanicand fail the test run rather than being skipped. Consider#[ignore](run explicitly in a docker-enabled job) or a runtime skip when the daemon is unavailable.♻️ Example: mark daemon-dependent tests as ignored
#[tokio::test] +#[ignore = "requires a running Docker daemon"] async fn docker_volume_ping_true_for_existing_volume() {Also applies to: 103-103, 147-147, 228-228
🤖 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 `@src/tests/domain/docker_volume.rs` at line 27, The Docker-daemon integration tests are hard-failing when no socket is available because `docker_volume_ping_true_for_existing_volume`, `docker_volume_backup_captures_files`, `docker_volume_restore_is_clean_replace`, and `sweep_removes_labeled_helpers` all use `client().expect(...)`. Update these tests to be gated instead of panicking, either by marking the affected test functions as ignored for explicit Docker-enabled runs or by adding a runtime skip when `client()` cannot connect; keep the change localized to the test module so the daemon-dependent paths still run where Docker is present.docker-compose.yml (1)
14-14: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocker socket mount grants root-equivalent host access.
Mounting
/var/run/docker.sockgives this container full control of the host Docker daemon. It's required for the helper-container workflow, but treat it as a significant privilege escalation surface: keep it scoped to environments that need backup/restore, and consider a socket proxy with a restricted API allowlist for production.🤖 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 `@docker-compose.yml` at line 14, The service is mounting the host Docker socket, which exposes root-equivalent access to the daemon; keep this mount only for the helper-container workflow that actually needs backup/restore, and gate it behind a dedicated profile or environment-specific override so it is not enabled broadly. If production must use it, replace the direct /var/run/docker.sock bind in docker-compose with a socket proxy or similarly restricted endpoint and limit the allowed Docker API surface.src/domain/docker_volume/backup.rs (1)
16-17: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the blocking wrapper around this async backup flow.
runis already async, and the work inside is justawait-driven bollard/tokio I/O.spawn_blocking+block_ononly burns a blocking thread here; run the body directly unlessclient()or helper setup has a separate blocking step.🤖 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 `@src/domain/docker_volume/backup.rs` around lines 16 - 17, The backup flow in run is unnecessarily wrapped in tokio::task::spawn_blocking and futures::executor::block_on even though it is already async and the bollard/tokio operations can be awaited directly. Remove the blocking wrapper from the backup path in the run function, keep the async body as normal awaits, and only introduce blocking code if a specific helper such as client() truly requires it.
🤖 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 `@docker-compose.yml`:
- Line 22: Remove the hardcoded EDGE_KEY from the docker-compose configuration
and source it from an external secret store or untracked environment file using
environment interpolation instead. Update the compose setup to reference the
variable by name rather than embedding the base64 payload, and ensure the secret
is rotated since the current masterKeyB64 has been exposed.
In `@src/domain/docker_volume/backup.rs`:
- Around line 29-56: The cleanup in the backup flow is only happening after the
happy path in the async block, so the helper container created by create_helper
can leak if File::create, the stream loop, write_all, or flush fails. Refactor
the logic in backup.rs so remove_helper is guaranteed to run whenever
create_helper succeeds, even on early returns from the download/write path; keep
the cleanup tied to the helper.id lifecycle rather than the end of the success
path.
In `@src/domain/docker_volume/database.rs`:
- Around line 32-44: The `backup` and `restore` methods in `Database` currently
let `FileLock::release(...).await?` override the actual result from
`backup::run`/`restore::run`, which can hide the real operation outcome. Update
these methods so the result from `run` is preserved and any release failure is
handled separately, ideally by logging it while still returning the original
backup/restore result. Use the existing `backup`, `restore`, and
`FileLock::release` symbols to locate the change.
In `@src/domain/docker_volume/restore.rs`:
- Around line 30-89: The helper container cleanup in restore should run even
when an earlier step fails, because `remove_helper` in `restore` is only
executed on the success path after `upload_to_container`. Move the cleanup for
the helper created by `create_helper` into a guaranteed teardown path (for
example, a guard/Drop-based cleanup or a `finally`-style pattern around the
async block) so it executes after `start_container`, `create_exec`/`start_exec`,
`File::open`, or `upload_to_container` errors. Keep the existing success
logging, but ensure the helper identified by `helper.id` is always removed once
created.
---
Nitpick comments:
In `@docker-compose.yml`:
- Line 14: The service is mounting the host Docker socket, which exposes
root-equivalent access to the daemon; keep this mount only for the
helper-container workflow that actually needs backup/restore, and gate it behind
a dedicated profile or environment-specific override so it is not enabled
broadly. If production must use it, replace the direct /var/run/docker.sock bind
in docker-compose with a socket proxy or similarly restricted endpoint and limit
the allowed Docker API surface.
In `@src/domain/docker_volume/backup.rs`:
- Around line 16-17: The backup flow in run is unnecessarily wrapped in
tokio::task::spawn_blocking and futures::executor::block_on even though it is
already async and the bollard/tokio operations can be awaited directly. Remove
the blocking wrapper from the backup path in the run function, keep the async
body as normal awaits, and only introduce blocking code if a specific helper
such as client() truly requires it.
In `@src/tests/domain/docker_volume.rs`:
- Line 27: The Docker-daemon integration tests are hard-failing when no socket
is available because `docker_volume_ping_true_for_existing_volume`,
`docker_volume_backup_captures_files`, `docker_volume_restore_is_clean_replace`,
and `sweep_removes_labeled_helpers` all use `client().expect(...)`. Update these
tests to be gated instead of panicking, either by marking the affected test
functions as ignored for explicit Docker-enabled runs or by adding a runtime
skip when `client()` cannot connect; keep the change localized to the test
module so the daemon-dependent paths still run where Docker is present.
In `@src/tests/domain/mysql.rs`:
- Around line 24-38: The test config literals for DatabaseConfig are duplicated
across many domain tests, so adding fields forces repeated edits. Add a Default
implementation for DatabaseConfig that sets sensible baseline values, then
update the test builders (including mysql.rs and the other database tests) to
construct the struct with ..Default::default() and only override the fields each
test actually needs. This will centralize future field additions and keep the
test setup stable.
In `@src/utils/common.rs`:
- Around line 5-14: The choose_restore_path function has an unused
_extraction_dir parameter and the current multi-file branch just returns the
archive path. Either remove the parameter from choose_restore_path and update
its call sites if it is intentionally unused, or incorporate _extraction_dir
into the path-selection logic if extraction-dir-based resolution is expected for
multi-file dump formats. Keep the behavior consistent with the existing
single-file and fallback cases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00d78fdc-f85b-4389-85d5-8700062e5402
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
Cargo.tomldatabases.jsondocker-compose.ymlsrc/domain/docker_volume/backup.rssrc/domain/docker_volume/database.rssrc/domain/docker_volume/docker.rssrc/domain/docker_volume/mod.rssrc/domain/docker_volume/ping.rssrc/domain/docker_volume/restore.rssrc/domain/factory.rssrc/domain/mod.rssrc/main.rssrc/services/config.rssrc/services/restore/archive.rssrc/services/restore/executor.rssrc/tests/domain/cluster/database.rssrc/tests/domain/cluster/mod.rssrc/tests/domain/docker_volume.rssrc/tests/domain/firebird.rssrc/tests/domain/mariadb.rssrc/tests/domain/mod.rssrc/tests/domain/mongodb.rssrc/tests/domain/mssql.rssrc/tests/domain/mysql.rssrc/tests/domain/postgres.rssrc/tests/domain/redis.rssrc/tests/domain/valkey.rssrc/tests/services/config_tests.rssrc/tests/utils/common_tests.rssrc/tests/utils/compress_tests.rssrc/utils/common.rssrc/utils/compress.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes