Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN #27777

Merged
merged 2 commits into from May 31, 2023
Merged

ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN #27777

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa9c65a
ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN
May 30, 2023
fa12307
ci: Use podman for persistent workers
May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion ci/test/04_install.sh
View file
Open in desktop
Expand Up @@ -42,7 +42,9 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then

if [ -n "${RESTART_CI_DOCKER_BEFORE_RUN}" ] ; then
echo "Restart docker before run to stop and clear all containers started with --rm"
systemctl restart docker
podman container kill --all # Similar to "systemctl restart docker"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this works? Even though the CLIs have a compatible syntax, I think they operate on their own set of resources. I'm pretty sure podman ps right before this line won't show anything, so I think no containers are killed after this line? At least, that's what I get when I try locally spinning up a container with docker run and then try to show/kill it with podman ps or podman container kill --all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already switched all machines to podman-docker, because it is not possible to install docker.io and podman-docker at the same time. So docker run is actually just an alias for podman run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can't comment on that. Since there is no mention of podman anywhere else in the codebase/documentation, I think this may make maintaining the CI more opaque, but perhaps that's an okay tradeoff.

Copy link
Member Author

@maflcko maflcko May 30, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't expect anyone to use RESTART_CI_DOCKER_BEFORE_RUN beside the persistent workers, so it is an undocumented setting. Anyone using it is expected to infer the documentation from the two echo calls and two lines of code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this doesn't work some of the time:

++ echo 'Restart docker before run to stop and clear all containers started with --rm'
Restart docker before run to stop and clear all containers started with --rm
++ podman container kill --all
++ echo 'Prune all dangling images'
Prune all dangling images
++ docker image prune --force
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
+++ docker run --rm --interactive --detach --tty --mount type=bind,src=/tmp/cirrus-build,dst=/ro_base,readonly --mount type=volume,src=ci_native_qt5_ccache,dst=/tmp/ccache_dir --mount type=volume,src=ci_native_qt5_depends,dst=/tmp/cirrus-build/depends --mount type=volume,src=ci_native_qt5_previous_releases,dst=/tmp/cirrus-build/releases/x86_64-pc-linux-gnu -w /tmp/cirrus-build --env-file /tmp/env --name ci_native_qt5 ci_native_qt5
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
time="2023-06-05T06:45:57+02:00" level=warning msg="The input device is not a TTY. The --tty and --interactive flags might not work properly"
Error: error creating container storage: the container name "ci_native_qt5" is already in use by "1b176e38eff6bdc928f2e213260cb9c8abb9efaa5cd6b0cccd09033a1b707601". You have to remove that container to be able to reuse that name.: that name is already in use
++ CI_CONTAINER_ID=
Exit status: 125

echo "Prune all dangling images"
docker image prune --force
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be prudent to use the --filter option here to only prune images created by the CI? This may be especially helpful to people running the CI locally/unsandboxed. We'd just need to add a --label "bitcoin-ci" argument to docker build.

Suggested change
docker image prune --force
docker image prune --force --filter label=bitcoin-ci

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do dangling image (layer)s have a label? Anyway, this isn't for local runs, only for the persistent runners, guarded via RESTART_CI_DOCKER_BEFORE_RUN. Happy to rename RESTART_CI_DOCKER_BEFORE_RUN to DANGER_RESTART_CI_DOCKER_BEFORE_RUN?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do dangling image (layer)s have a label?

Yeah they do, just tested it to confirm.

guarded via RESTART_CI_DOCKER_BEFORE_RUN

Fair enough, looks like this is unlikely to be hit locally. I don't think renaming to DANGER_RESTART_CI_DOCKER_BEFORE_RUN is an improvement, so happy to keep as is. Just thought adding a label and only applying batch operations to our own images is a good practice, e.g. if in the future we remove the RESTART_CI_DOCKER_BEFORE_RUN condition, given that it's almost no overhead. But up to you, it's no big deal and I don't have a strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, could make sense in a follow-up to apply the label to both places and then unguard the image prune from DANGER_RESTART_CI_DOCKER_BEFORE_RUN?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #27793

fi

# shellcheck disable=SC2086
Expand Down