Follow-up from #27777 (review).
Labeling the docker images produced by the CI allows us/the user to apply batch operations to all images (including dangling ones) produced by the ci without affecting other, non-bitcoin-ci images. With labeling, we can safely always prune dangling bitcoin-ci-test images without checking for RESTART_CI_DOCKER_BEFORE_RUN, which we enable on our persistent runners.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fanquake |
| Concept ACK | Sjors, hebasto, pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Concept ACK. Haven't tested.
It would also be nice to have an incantation (e.g. in the help) to kill dangling images after CI failure / interruption (if you don't plan on running it again).
Rebased to address merge conflict from #27844.
It would also be nice to have an incantation (e.g. in the help) to kill dangling images after CI failure / interruption (if you don't plan on running it again).
Thanks, added doc: ci: add instructions for pruning dangling images manually
Concept ACK
Rebased to address merge conflict from #28138.
Rebased to address merge conflict from #28161.
25 | @@ -26,6 +26,13 @@ To run the test stage with a specific configuration, 26 | FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh 27 | ``` 28 | 29 | +The ci automatically prunes all dangling docker images every time it runs. If you want to perform 30 | +the cleanup manually (e.g. if you won't be running the ci again), you can run 31 | + 32 | +```sh 33 | +docker image prune --force --filter "label=bitcoin-ci-test"
I think if one wants to not run the CI again, they'll need to use rm instead of prune, no? The goal of prune is to only remove dangling/stale/intermediate image layers?
Yeah, would you prefer it like this?
diff --git a/ci/README.md b/ci/README.md
index ab5e6ce866..16310b7703 100644
--- a/ci/README.md
+++ b/ci/README.md
@@ -26,13 +26,19 @@ To run the test stage with a specific configuration,
FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
```
-The ci automatically prunes all dangling docker images every time it runs. If you want to perform
-the cleanup manually (e.g. if you won't be running the ci again), you can run
+The ci automatically prunes all dangling (i.e. unused) docker images every time it runs. To do this
+manually, run
```sh
docker image prune --force --filter "label=bitcoin-ci-test"
```
+To also remove the used images (e.g. if you're not planning on running the ci again), run
+
+```sh
+docker rmi -f $(docker images -q -a --filter "label=bitcoin-ci-test")
+```
+
### Configurations
The test files (`FILE_ENV`) are constructed to test a wide range of
Yeah, would you prefer it like this?
No preference. Currently it is wrong, so you'll have to remove it, or fix it, or close this pull.
Concept ACK.
Force pushed to update documentation addressing this review comment.
31 | + 32 | +```sh 33 | +docker image prune --force --filter "label=bitcoin-ci-test" 34 | +``` 35 | + 36 | +To also remove the used images (e.g. if you're not planning on running the ci again), run
this isn't enough. Volumes also exist
Thanks Dropped the README.md commit, don't feel like spending more time on this. @Sjors feel free to follow up if you think it's important.
Could rebase for green CI, if still relevant?
Rebased to address merge conflict from #27976.
Concept ACK.
Perhaps description needs to be updated regarding the user instructions (ci/README.md).
Perhaps description needs to be updated regarding the user instructions (
ci/README.md).
Thanks, reverted that description.
Closed, not an essential change and doesn't seem like there's sufficient interest.
27 | @@ -26,9 +28,9 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then 28 | if [ -n "${RESTART_CI_DOCKER_BEFORE_RUN}" ] ; then 29 | echo "Restart docker before run to stop and clear all containers started with --rm" 30 | podman container stop --all # Similar to "systemctl restart docker" 31 | - echo "Prune all dangling images" 32 | - docker image prune --force
Maybe keep this, just to be safe, in case the label stuff doesn't work? (For example from a previous run when labels weren't set) :sweat_smile:
Makes sense, especially the migration aspect which I didn't think about. Added a comment that it can probably be removed in the future, though.
lgtm, after the nit
This allows us or the user to perform batch operations on all
images produced by the ci, e.g. to prune all dangling images,
without affecting non-ci images.
Since all bitcoin-ci-test images are now labeled, we can always
prune all dangling images, regardless of whether we are in
RESTART_CI_DOCKER_BEFORE_RUN.
To be safe, still prune all images if RESTART_CI_DOCKER_BEFORE_RUN
in case the filtering doesn't work, or if images were created on
an earlier version that did not assign labels.
Force-pushed to address merge conflict from #28547 and incorporate suggestion from maflcko to keep the unfiltered pruning for now, as belt and suspenders approach.
utACK e44c574650827f18e12ac0ba378c0a19d23a07b4