Skip to content

Fix CI container name collision on podman runners - #2881

Open
anvacaru wants to merge 1 commit into
masterfrom
anvacaru/ci-container-fix
Open

anvacaru wants to merge 1 commit into
masterfrom
anvacaru/ci-container-fix

Conversation

@anvacaru

Copy link
Copy Markdown
Contributor

CI has been failing intermittently with creating container storage: the container name "kevm-ci-haskell-<suite>-<sha>" is already in use, most recently in the DSS job.

The container name only embeds the merge SHA. On the podman-based self-hosted runners, when a job is canceled (e.g., the DSS suite hitting its 45-minute timeout), the teardown's docker stop reports success, but removing a --rm container is not part of stop itself.

Per podman-container-cleanup(1), the cleanup "is automatically executed when containers are run in daemon mode by the conmon process when the container exits", and it is that cleanup's --rm option that, "after cleanup, remove[s] the container entirely".

The runner's end-of-job orphan-process sweep kills conmon before that happens, so the container stays registered as running and is never removed. Every re-run of the same commit on that runner then collides with it, and docker stop on the ghost fails with refusing to clean up: container state improper.

Changes:

  • .github/actions/with-docker/action.yml: force-remove any existing container with the target name before docker run, making container setup idempotent across retries regardless of why the previous container survived. Per podman-rm(1), --force "also removes containers from container storage even if the container is not known to Podman" and "can be used to remove unusable containers", which covers the ghost state left behind.

  • .github/workflows/test-pr.yml: tear down with docker rm -f instead of docker stop --timeout=0, so removal happens synchronously inside the step rather than depending on conmon's asynchronous --rm cleanup surviving the end of the job.

@palinatolmach palinatolmach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I left a small comment, but the only real concern I have about this change is that two matrix entries (Rules (booster) and Rules (booster-dev) ) produce the same container name:

kevm-ci-haskell-test-prove-rules-${{ github.sha }}

With the new setup step, if those two jobs ever run on runners that share the same Podman storage, the second job to start will rm -f the first job's live container and the first job would fail on one of its docker exec calls; and teardown could have a similar race condition.

I'm not sure whether our self-hosted runners are guaranteed to have isolated Podman storage (I don't think they are, but @ehildenb would definitely know), so maybe it won't happen with the current runner setup. A more permanent solution would be making the container name unique per matrix entry, e.g., by adding some dedicated id to matrix entries, but it can also be done as a follow up.

- name: 'Tear down Docker'
if: always()
run: |
# Best effort: the container does not exist if 'Set up Docker' failed, and a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this comment (and a similar one below) still apply, as it documents the reasoning behind || true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants