Remove the accessory image a reboot supersedes - #1950
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The successful superseded-image deletion path lacks integration coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds cleanup of superseded accessory images after successful reboots while safely retaining referenced images.
Changes:
- Records the current image ID before replacing an accessory.
- Removes the old image without forcing deletion.
- Adds command and same-image integration coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
lib/kamal/cli/accessory.rb |
Integrates image cleanup into reboot. |
lib/kamal/commands/accessory.rb |
Adds image inspection and removal commands. |
test/commands/accessory_test.rb |
Tests generated Docker commands. |
test/integration/accessory_test.rb |
Verifies referenced images are retained. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Accessory images are pulled, not built, so they carry no `service` label and the label filters in prune never match them. Nothing reclaims them. `kamal accessory reboot` reads the container's image before replacing it, and removes that image once the new one is up. The removal is unforced, so docker refuses while anything still references the image, and a refusal is reported rather than raised. A failed boot leaves both images: the old one is what you would go back to, the new one is what you would inspect. Images orphaned by a repository rename, or on a host dropped from the accessory's hosts, are out of reach — remove those with `docker rmi`.
99b81b2 to
c1fcffa
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The integration test has incomplete per-host coverage and no longer reliably verifies final accessory removal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test/integration/accessory_test.rb:75
- After the config is switched to
busybox:1.37.0, the finalassert_accessory_not_runningstill rejects only1.36.0. That assertion would therefore pass even ifkamal accessory removeleft the new container running. Make the not-running check version-agnostic (or pass the active version) so this test continues to cover removal.
def assert_accessory_running(name, version: "1.36.0")
assert_match /busybox:#{version} "sh -c 'echo \\"Start/, accessory_details(name)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| kamal :accessory, :reboot, :busybox | ||
| assert_accessory_running :busybox, version: "1.37.0" | ||
| assert_not_equal superseded, accessory_image_id | ||
| assert_not_includes vm1_image_ids, superseded |
There was a problem hiding this comment.
Fixed — the superseded image id is now collected and asserted per host, and assert_accessory_not_running no longer names 1.36.0.
The accessory runs on vm1 and vm2, so a regression in the per-host image map, or in the cleanup loop, could leave the image behind on vm2 while a vm1-only assertion still passed. The removal check no longer names 1.36.0 either. The accessory is on 1.37.0 by then, so the old assertion passed even when a container stayed up.
Accessory images are pulled, not built, so they carry no
servicelabel andthe label filters in prune never match them. Nothing reclaims them.
kamal accessory rebootreads the container's image before replacing it, andremoves that image once the new one is up. The removal is unforced, so docker
refuses while anything still references the image, and a refusal is reported
rather than raised.
A failed boot leaves both images: the old one is what you would go back to,
the new one is what you would inspect. Images orphaned by a repository rename,
or on a host dropped from the accessory's hosts, are out of reach — remove
those with
docker rmi.