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

Improve the robustness of image cleanup #313

Merged
merged 3 commits into from
Feb 3, 2025
Merged

Improve the robustness of image cleanup #313

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b953727
improve robustness of image cleanup
tfoote Jan 24, 2025
4a6e191
Don't automatically clean up a named image
tfoote Jan 25, 2025
df00fb4
Remove unused output_callback
tfoote Feb 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/rocker/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ def main():
exit_code = dig.build(**vars(args))
if exit_code != 0:
print("Build failed exiting")
if not args_dict['persist_image']:
if not (args_dict['persist_image'] or args_dict.get('image_name')):
dig.clear_image()
return exit_code
# Convert command into string
args.command = ' '.join(args.command)
result = dig.run(**args_dict)
if not args_dict['persist_image']:
if not (args_dict['persist_image'] or args_dict.get('image_name')):
print(f'Clearing Image: {dig.image_id}s\nTo not clean up use --persist-images')
dig.clear_image()
return result
Expand Down
19 changes: 16 additions & 3 deletions src/rocker/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,24 @@ def docker_build(docker_client = None, output_callback = None, **kwargs):
print("no more output and success not detected")
return None

def docker_remove_image(image_id, docker_client = None, output_callback = None, **kwargs):
def docker_remove_image(
image_id,
docker_client = None,
output_callback = None,
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of drive-by but output_callback appears completely unused. Is it deprecated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this doesn't have the streaming interface so removing it makes sense.

fail_on_error = False,
force = False,
**kwargs):

if not docker_client:
docker_client = get_docker_client()

docker_client.remove_image(image_id)
try:
docker_client.remove_image(image_id, force=force)
except docker.errors.APIError as ex:
## removing the image can fail if there's child images
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little weird not to print or log a message when catching this error. The end-user has no way of knowing whether the image was cleaned up or not, especially with the default behavior of not failing on error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will return a True/False and then it warns the user based on the results:

rocker/src/rocker/core.py

Line 437 in 4a6e191

print(f'Failed to clear image {self.image_id} it likely has child images.')

And the default is False so it's only suppressed if the developer asks for it to be suppressed.

if fail_on_error:
return False
return True

class SIGWINCHPassthrough(object):
def __init__ (self, process):
Expand Down Expand Up @@ -421,7 +433,8 @@ def run(self, command='', **kwargs):

def clear_image(self):
if self.image_id:
docker_remove_image(self.image_id)
if not docker_remove_image(self.image_id):
print(f'Failed to clear image {self.image_id} it likely has child images.')
self.image_id = None
self.built = False

Expand Down