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

Remove intermediate containers and name detect_os images #133

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Remove intermediate containers and name detect_os images #133

merged 1 commit into from
Mar 30, 2021

Conversation

stonier
Copy link
Contributor

@stonier stonier commented Mar 26, 2021 •
edited
Loading

detect_os creates a flurry of images and containers and dumps them on your system, with little introspectability as to what they are.

CONTAINER ID        IMAGE               COMMAND                  CREATED             STATUS                      PORTS               NAMES
748d5a7159d8        64201bbf98b6        "/bin/sh -c '. /tmp/…"   21 minutes ago      Exited (0) 21 minutes ago                       boring_volhard
580a68e8061e        88db64389dbb        "/bin/sh -c 'echo 'i…"   21 minutes ago      Exited (0) 21 minutes ago                       exciting_raman
0b9286fd1589        61dd783afb2b        "/bin/sh -c 'apt-get…"   21 minutes ago      Exited (0) 21 minutes ago                       compassionate_kilby
439a8199141e        98a0ac288d5f        "/bin/sh -c '. /tmp/…"   21 minutes ago      Exited (0) 21 minutes ago                       eager_ride
01e2786bee74        e06fe2bcb9e4        "/bin/sh -c 'python3…"   22 minutes ago      Exited (0) 21 minutes ago                       brave_colden
d8124fb028b6        b9d77e48a75c        "/bin/sh -c 'mkdir -…"   22 minutes ago      Exited (0) 22 minutes ago                       hardcore_ritchie

This does a partial cleanup:

  • Removes intermediate containers spun up by the dockerfile
  • Names the final image (though not the preliminary build image) for the os detector

@stonier stonier requested a review from tfoote as a code owner March 26, 2021 17:10
@stonier stonier force-pushed the stonier/os_detector_containers branch from 23e80b2 to dea58ac Compare March 26, 2021 20:32
tfoote
tfoote requested changes Mar 30, 2021
View reviewed changes
Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Cleaning intermediate containers sounds good as well as tagging the final one.

src/rocker/os_detector.py
@@ -53,6 +53,13 @@ def detect_os(image_name, output_callback=None, nocache=False):

iof = StringIO((DETECTION_TEMPLATE % locals()).encode())
image_id = docker_build(fileobj = iof, output_callback=output_callback, nocache=nocache)
image_id = docker_build(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The double invocation of docker_build looks like an oversight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh indeed. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@stonier stonier force-pushed the stonier/os_detector_containers branch from dea58ac to a224076 Compare March 30, 2021 13:18
@tfoote tfoote merged commit 80602eb into osrf:master Mar 30, 2021
@stonier stonier deleted the stonier/os_detector_containers branch March 31, 2021 12:56
130s pushed a commit to plusone-robotics/rocker that referenced this pull request Apr 17, 2021
@stonier @130s
Remove intermediate containers and name os_detect images (osrf#133)
81170e8
@130s 130s mentioned this pull request Apr 19, 2021
Closed
@meyerj
Copy link
Contributor

meyerj commented Oct 12, 2021

Since the temporary images are tagged they would not be removed automatically by docker image prune or docker system prune without the --all/-a option anymore. Instead, they slowly fill up the disk and require manual cleaning from time to time. Adding --all is not an option either, because then all tagged images would be removed and need to be rebuilt or pulled from a remote registry again.

Actually the intention of #60 was more or less the opposite, i.e. to avoid having tags assigned to temporary, cached images, such that they are considered as "dangling" by docker's builtin cleanup tools.

@meyerj meyerj mentioned this pull request Oct 12, 2021
Closed
meyerj added a commit to Intermodalics/rocker that referenced this pull request Nov 15, 2021
@meyerj
Do not tag os_detect images
b9637f7 
This commit partially reverts 80602eb (osrf#133). See osrf#133 (comment).
@meyerj meyerj mentioned this pull request Nov 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tfoote tfoote tfoote approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stonier @meyerj @tfoote