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.

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

Fix docker clean #656

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Fix docker clean #656

merged 5 commits into from
Jan 17, 2025

Conversation

aittalam
Copy link
Member

@aittalam aittalam commented Jan 16, 2025

What's changing

make clean-docker-all breaks when no containers are running because the first command clean-docker-containers returns an error if the list of containers is empty.

This PR fixes that by using xargs to pass the list (when not empty) to docker rm and docker rmi.

Closes #660

How to test it

run make clean-all or make clean-docker-all. This should remove all containers + images if present, without breaking if there is none

Additional notes for reviewers

I am not sure why we'd need both make clean-all and make clean-docker-all. The semantics I had in mind was "clean-all should call clean-docker-all plus something else" but that does not happen (actually this is a subset of clean-docker-all). WDYT? Should we remove clean-all or is there a good reason to keep it (perhaps with a different name)?

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and product documentation under /docs)
  • Checked if a (backend) DB migration step was required and included it if required

@aittalam aittalam requested review from chainlink and agpituk January 16, 2025 19:18
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@aittalam
Copy link
Member Author

aittalam commented Jan 17, 2025

Addressed reviews - with prune everything is neater, thank you!
Just a heads up that, differently from before, after this update we will not remove running containers which are hanging for some reason (or that one purposely left running).

Another warning: we are still removing ALL dangling containers and ALL images!
Many thanks @agpituk for raising this issue. For whoever will pick it up, the filter in the compose file works with containers but not with images, so we'll have to align on a way to do it (I'd be fine pruning only dangling images + lumigator's at this point).

@aittalam
Copy link
Member Author

aittalam commented Jan 17, 2025

I added a commit with a possible solution for image removing:

  • we run docker images "mzdotai/*" -q | xargs -n1 docker rmi -f to only remove our own images
  • we run docker image prune to remove dangling images (useful for us because we generate tons of them)

WDYT @agpituk ?
Also please lmk if you have an answer to the question I raised in "Additional notes for reviewers" above

@agpituk
Copy link
Contributor

agpituk commented Jan 17, 2025

I added a commit with a possible solution for image removing:

* we run `docker images "mzdotai/*" -q | xargs -n1 docker rmi -f` to only remove our own images

* we run `docker image prune` to remove dangling images (useful for us because we generate tons of them)

WDYT @agpituk ? Also please lmk if you have an answer to the question I raised in "Additional notes for reviewers" above

That works very well for me, thanks @aittalam !!

About the question, I agree, clean-all should call clean-docker-all plus additional stuff. Happy for this to be merged as it is or for you to also review that one.

@aittalam aittalam merged commit 78e39cd into main Jan 17, 2025
13 checks passed
@aittalam aittalam deleted the davide/makefile-docker-clean-fix branch January 17, 2025 12:24
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.

Only clean our docker objects
3 participants