Repository navigation
test_reclaim: a drive-override restore makes no mountpoint at the source directory - #1351
Conversation
…rce directory Since #1318 the pinned Firecracker overrides every source path at snapshot load, so a restore no longer recreates the source VM's directory as a mountpoint. a_sigkilled_restore_leaves_its_mountpoint_and_runtime_dir_to_the_next_launch still required that directory while the restore was parked, and failed in every Host-Root and Container lane on main since 0991437 ("the parked restore has not made <source dir> and <clone dir>"). The test, renamed a_sigkilled_restore_leaves_its_runtime_dir_to_the_next_launch, now requires the parked restore to have made its own runtime directory and NOT the source directory, and the next launch to remove the runtime directory. Reclaiming a mountpoint an older binary made stays covered by the unit tests in src/reclaim.rs. Tested: make test-root FILTER="-E 'binary(test_reclaim) & test(/a_sigkilled_restore/)'" (1 passed); the previous form fails on main CI run 40411315700.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Restore runtime directory checks tests/test_reclaim.rs |
The test focuses on the killed restore’s runtime directory. While the restore is parked, it checks that the restore directory exists and the source VM directory remains absent. It continues to check reclamation after the next launch. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Possibly related issues
ejc3/fcvm#1181— The test checks that the restore does not recreate the source VM directory.ejc3/fcvm#1187— The test no longer expects a source mountpoint to be created and checks restore runtime directory reclamation.ejc3/fcvm#1311— The test checks that the restore leaves the source VM directory absent.
Possibly related PRs
- ejc3/fcvm#1304 — This PR removes the integration assertion for a source mountpoint that the current restore path does not create.
- ejc3/fcvm#1314 — Both changes cover restore mount targets and source VM directory behavior.
- ejc3/fcvm#1318 — This PR tests that the restore does not create a source VM directory mountpoint.
Merge Risk: 
2917d
-
No merge-blocking risk is identified in this test-only change.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the test file and the change in behavior it covers: a drive-override restore creates no mountpoint at the source directory. The PR updates tests/test_reclaim.rs to match the snapshot-l… Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the test file and the change in behavior it covers: a drive-override restore creates no mountpoint at the source directory. The PR updates tests/test_reclaim.rs to match the snapshot-load behavior from
#1318, where pinned Firecracker overrides every source path and the restore no longer recreates the source VM's directory as a mountpoint. The title is specific to the main change and matches the objectives. It is slightly long at 82 characters, but it stays on topic.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
ejc3
left a comment
ejc3
left a comment
There was a problem hiding this comment.
NOT-A-DEFECT: CodeRabbit's walkthrough reports no actionable comments and minimal merge risk; it claims nothing to fix.
Main has failed
test_reclaim::a_sigkilled_restore_leaves_its_mountpoint_and_runtime_dir_to_the_next_launchin every Host-Root and Container lane since 0991437 (#1318):#1318 pinned a Firecracker whose snapshot load overrides every source path, so a restore no longer recreates the source VM's directory as a mountpoint. The test still required that directory.
Change: the test, renamed
a_sigkilled_restore_leaves_its_runtime_dir_to_the_next_launch, requires the parked restore to have made its own runtime directory and not the source directory, and the next launch to remove the runtime directory. Reclaim of a mountpoint made by an older binary stays covered by the unit tests insrc/reclaim.rs.Contract: main's reclaim test matches the restore #1318 ships. Test-only change.
Tested:
make test-root FILTER="-E 'binary(test_reclaim) & test(/a_sigkilled_restore/)'": 1 passed.make lintpasses.Summary by CodeRabbit