ephemeral: Add test-basic subcommand - #357
Conversation
Adds `bcvk ephemeral test-basic` which boots an ephemeral VM and verifies that systemd reaches a healthy state via `systemctl is-system-running`. This provides a quick smoke test for bootc container images. The subcommand: - Boots an ephemeral VM from the specified container image - Waits for SSH to be ready - Runs systemctl is-system-running to check system health - Returns success if the system is in "running" or "degraded" state - Automatically cleans up the container on exit Integration tests are included to verify the functionality. Generated-by: AI
bootc-bot
Bot
added
agent/review-working
![bootc-bot[bot]](https://avatars.githubusercontent.com/u/202312630?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
I've reviewed this PR for correctness, quality, safety, and adherence to repository conventions. The implementation is mostly solid and follows the existing patterns well, but there's one code quality issue that needs to be fixed before merging.
Issue: Code Duplication
Location: crates/kit/src/ephemeral.rs:433-446
The ContainerCleanup struct added in this PR (lines 433-446) is a duplicate of the existing struct in crates/kit/src/run_ephemeral_ssh.rs:99-116. According to REVIEW.md:
"It can be OK to duplicate a bit of code in a slightly different form twice, but having it happen in 3 places asks for deduplication."
While both implementations are nearly identical RAII guards, the existing one in run_ephemeral_ssh.rs is slightly better as it includes:
- A
new()constructor method - Debug logging when cleaning up
Required fix: Deduplicate by either:
- Extract
ContainerCleanupto a shared module (e.g.,crates/kit/src/common.rsor similar) and make itpub(crate), or - Move it to the
ephemeralmodule at the top level, make itpub(crate), and haverun_ephemeral_sshimport it
Use the better implementation from run_ephemeral_ssh.rs (with the new() method and debug logging) as the canonical version.
What looks good ✓
- Correct use of existing patterns: The implementation correctly follows the pattern from
run-sshand other ephemeral commands - Proper error handling: All errors are propagated with
?, no silent failures - RAII cleanup: The cleanup guard pattern is correctly implemented (though duplicated)
- SSH integration: Correctly uses
wait_for_ssh_ready()and the bcvk entrypoint SSH mechanism - Appropriate health check logic: Accepting both "running" and "degraded" states is reasonable for a basic smoke test
- Good test coverage: Integration tests verify the command works and check for specific expected output
- Clear documentation: The command has helpful documentation explaining what it does
The implementation is sound once the duplication is resolved.
bootc-bot
Bot
added
agent/fix-working
Extract the ContainerCleanup struct from both run_ephemeral_ssh.rs and ephemeral.rs into a shared pub(crate) implementation in ephemeral.rs. This eliminates code duplication while using the better implementation that includes a new() constructor and debug logging. Both modules now import and use the shared ContainerCleanup via crate::ephemeral::ContainerCleanup. Assisted-by: AI
|
Commit pushed:
|
bootc-bot
Bot
added
agent/review-working
There was a problem hiding this comment.
Review Complete ✅
I've reviewed this PR for correctness, code quality, safety, and adherence to repository conventions. The implementation looks excellent.
What I Checked
Code correctness:
- ✅ Systemd state checking is correct (
runningordegradedare the appropriate healthy states) - ✅ RAII cleanup pattern properly ensures container removal on all exit paths
- ✅ Error handling follows Rust best practices - no swallowed errors, proper propagation with context
- ✅ Integration with existing infrastructure (
run_detached,wait_for_ssh_ready) is correct
Code quality:
- ✅ Excellent refactoring of
ContainerCleanupto eliminate duplication between modules - ✅ Clean separation of concerns following the project's patterns
- ✅ Proper use of
pub(crate)visibility for shared internal types - ✅ Clear, informative user output with progress indication
Testing:
- ✅ Comprehensive integration tests with proper assertions
- ✅ Tests verify actual output content, not just "didn't crash"
- ✅ Both basic and parameterized test variants
- ✅ Follows the critical integration test policy (no warn-and-continue)
Project conventions:
- ✅ Atomic commits that separate refactoring from feature addition
- ✅ Clear commit messages with component prefixes and rationale
- ✅ Proper AI attribution (
Generated-by: AI,Assisted-by: AI) - ✅ Addresses issue #356 as intended
The implementation follows established patterns from run-ssh and other ephemeral commands, uses proper RAII for resource cleanup, and includes thorough test coverage. No issues found.
Ready to merge.
bootc-bot
Bot
removed
the
agent/review-working
| "/var/lib/bcvk/entrypoint", | ||
| "ssh-exec", | ||
| "systemctl", | ||
| "is-system-running", |
There was a problem hiding this comment.
No we can literally just exec bcvk ephemeral run-ssh systemctl is-system-running in general however this fails because we havent' shipped the bootupd fix. Let's just run say systemctl status --no-pager and toss the output to /dev/null by default.
| let bcvk = get_bck_command()?; | ||
|
|
||
| // Run the test-basic command | ||
| let output = cmd!(sh, "{bcvk} ephemeral test-basic {image}").output()?; |
There was a problem hiding this comment.
Let's just run the command with .run() no custom output capturing etc
|
Also missing |
bootc-bot
Bot
added
agent/fix-working
Change the health check from 'systemctl is-system-running' to 'systemctl status --no-pager' with output redirected to /dev/null, as the bootupd fix hasn't shipped yet and is-system-running fails. Also simplify the integration tests to use .run() instead of custom output capturing, as requested in review. Assisted-by: AI
|
Commit pushed:
|
bootc-bot
Bot
added
agent/review-working
There was a problem hiding this comment.
Review Summary
I've reviewed the implementation of the ephemeral test-basic subcommand. The code quality is good and follows the project's patterns well. However, there's a documentation mismatch that needs to be fixed.
Issue: Documentation Mismatch
Location: crates/kit/src/ephemeral.rs:201
The help text says:
/// that systemd reaches a healthy state via `systemctl is-system-running`.
But the implementation at line 433 uses:
"systemctl",
"status",
"--no-pager",The commit message 3ff3ea7 fix: Use systemctl status instead of is-system-running shows this change was intentional, but the documentation comment wasn't updated to match.
Required Fix
Update line 201 in crates/kit/src/ephemeral.rs from:
/// that systemd reaches a healthy state via `systemctl is-system-running`.
To:
/// that systemd reaches a healthy state via `systemctl status`.
What I Checked
✅ Code correctness: Implementation follows existing patterns and properly uses RAII cleanup
✅ Error handling: Appropriate use of Result and ? throughout
✅ Code reuse: Good refactoring of ContainerCleanup to avoid duplication
✅ Testing: Integration tests follow project standards with proper error handling
✅ Module organization: Clean separation of concerns
✅ Consistency: Matches patterns from run_ephemeral_ssh.rs
Once the documentation is updated to match the implementation, this will be ready to merge.
bootc-bot
Bot
added
agent/fix-working
Automated fix loop iteration limit reachedThe automated fix loop has reached its iteration limit (3 commits) and automated fixing has stopped. What this means:
Next steps:
Important: Re-applying the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.anthropic.com"See Network Configuration for more information.
|
bootc-bot
Bot
removed
the
agent/fix-working
|
|
||
| println!("Running basic smoke test on {}", opts.image); | ||
|
|
||
| // Build ephemeral VM options |
There was a problem hiding this comment.
Nah let's just re-exec our own process Command::new(proc/self/exe).args().exec()
1 participant
Summary
Adds
bcvk ephemeral test-basicsubcommand which boots an ephemeral VM and verifies that systemd reaches a healthy state. This addresses issue #356 by rebasing and implementing the changes from PR #244 with the correctagent/branch prefix required by the deployment system.Changes
New subcommand:
bcvk ephemeral test-basic <image>systemctl is-system-runningto verify system healthIntegration tests: Added comprehensive tests in
crates/integration-tests/src/tests/run_ephemeral_test_basic.rsImplementation Details
The implementation follows the established pattern from
run-sshand other ephemeral commands:run_detached()to start an ephemeral VMwait_for_ssh_ready()for boot readiness detectionsystemctl is-system-runningvia the bcvk entrypoint SSH mechanismValidation
The implementation:
run-sshmake validate(cargo fmt check, clippy, tests)Generated-by: AI
The implementation was reviewed and follows the project's existing patterns for ephemeral VM management.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
api.anthropic.comgithub.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.