Skip to content

fix: remove docker container after exit #1197

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
Merged
merged 1 commit into from
Oct 30, 2025
Merged

fix: remove docker container after exit #1197

merged 1 commit into from
Oct 30, 2025

Conversation

@qwersem
Copy link
Contributor

@qwersem qwersem commented Oct 24, 2025

The build script leaves multiple containers in a host environment. This update adds an --rm option to remove a Docker container after a command is executed.

Steps to reproduce the issue

$ git clone https://github.com/riscv-software-src/riscv-unified-db.git
$ cd riscv-unified-db
$ export DOCKER=1
$ ./do test:smoke
Fetching gem metadata from https://rubygems.org/......
$ docker container ls -a
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
CONTAINER ID  IMAGE                        COMMAND               CREATED         STATUS                     PORTS       NAMES
b49df1bcbc2c  docker.io/riscvintl/udb:0.9  bundle config set...  11 minutes ago  Exited (0) 11 minutes ago              quirky_swirles
28b906b730bf  docker.io/riscvintl/udb:0.9  bundle config set...  11 minutes ago  Exited (0) 11 minutes ago              amazing_goldberg
e189ebdd5e27  docker.io/riscvintl/udb:0.9  bundle config set...  11 minutes ago  Exited (0) 11 minutes ago              optimistic_kowalevski
9c34939c2770  docker.io/riscvintl/udb:0.9  bundle install        11 minutes ago  Exited (0) 10 minutes ago              magical_murdock
c118fdd21182  docker.io/riscvintl/udb:0.9  /usr/bin/python3 ...  10 minutes ago  Exited (0) 10 minutes ago              intelligent_margulis
bad9ac0863cf  docker.io/riscvintl/udb:0.9  /home/q/workspace...  10 minutes ago  Exited (0) 10 minutes ago              blissful_keller
c53716cb242a  docker.io/riscvintl/udb:0.9  npm i                 10 minutes ago  Exited (0) 10 minutes ago              cranky_colden
4eb3b575a6df  docker.io/riscvintl/udb:0.9  bundle exec --gem...  10 minutes ago  Exited (0) 5 minutes ago               nervous_germain

@dhower-qc
Copy link
Collaborator

dhower-qc commented Oct 27, 2025

I'm not very familiar with Docker. Does this have any performance impact on subsequent runs? (I think it probably doesn't, but want to check)

@codecov
Copy link

codecov bot commented Oct 27, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.05%. Comparing base (e3fb9b1) to head (203bccd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1197   +/-   ##
=======================================
  Coverage   46.05%   46.05%           
=======================================
  Files          11       11           
  Lines        4942     4942           
  Branches     1345     1345           
=======================================
  Hits         2276     2276           
  Misses       2666     2666
Flag Coverage Δ
idlc 46.05% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ThinkOpenly
Copy link
Collaborator

ThinkOpenly commented Oct 27, 2025

I'm going to show my ignorance. Are the docker run commands intended to modify an existing container? My reading of the documentation (admittedly, for podman) shows the "run" subcommand running in a new container. I presume that's why all of these containers are being left around.

Should we be using docker exec instead of docker run?

@qwersem
Copy link
Contributor Author

qwersem commented Oct 28, 2025

Are the docker run commands intended to modify an existing container?
docker run is intended to create a new container.

Should we be using docker exec instead of docker run?
Yes, it's preferable way, but then we should run detached container and should handle its removing. Or we can accumulate a queue of commands and run it once with autoremoving option.

It's better to prepare build environment using Github Actions tool, for example to do stage 0 which builds an image with all required dependencies and deploys a built image and stage 1 which uses prepared image and only builds a document.

dhower-qc
dhower-qc approved these changes Oct 29, 2025
View reviewed changes
Copy link
Collaborator

@dhower-qc dhower-qc left a comment

Choose a reason for hiding this comment

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

Approving, though from the discussion it sounds like there could be some follow on work to make this even better.

@qwersem
Copy link
Contributor Author

qwersem commented Oct 29, 2025

@dhower-qc , yes, it's just fix of critical issue.

@qwersem qwersem force-pushed the fix-remove-containers-after-exit branch from fd532c9 to 7d52618 Compare October 29, 2025 18:44
ThinkOpenly
ThinkOpenly approved these changes Oct 29, 2025
View reviewed changes
Copy link
Collaborator

@ThinkOpenly ThinkOpenly left a comment

Choose a reason for hiding this comment

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

@jordancarlin relieved me of my ignorance by pointing out that each of the "run" commands has side-effects in the local directory which is mounted as a volume in the container. Thus, each "run" still achieves its purpose, and the container itself is of no use afterwards, and removing it is appropriate.

@qwersem
fix: remove docker container after exit
203bccd 
The build script leaves multiple containers in a host environment. This update adds an `--rm` option to remove a Docker container after a command is executed.

Signed-off-by: Evgeny Semenov <[email protected]>
@qwersem qwersem force-pushed the fix-remove-containers-after-exit branch from 7d52618 to 203bccd Compare October 30, 2025 11:32
@ThinkOpenly ThinkOpenly enabled auto-merge October 30, 2025 15:33
@ThinkOpenly ThinkOpenly added this pull request to the merge queue Oct 30, 2025
Merged via the queue into riscv-software-src:main with commit fcae1f7 Oct 30, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ThinkOpenly ThinkOpenly ThinkOpenly approved these changes

@dhower-qc dhower-qc dhower-qc approved these changes

+1 more reviewer

@jordancarlin jordancarlin jordancarlin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@qwersem @dhower-qc @ThinkOpenly @jordancarlin