-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Cleanup docker images #5768
Cleanup docker images #5768
Conversation
13ad91b
to
d00ed5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is lgtm - but we probably should be resilient so we handle out-of-band container removals.
…erroring early due to an attempt to inspect an image that had already been removed
- modified tests to cleanup now that RemoveContainer isn't in StartTask - fix some broken tests by removing docker images/containers before test
45a3408
to
3b82770
Compare
} else { | ||
h.logger.Debug("not removing container due to config") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Container removed was moved from here, in run
, to DestroyTask
:
https://github.com/hashicorp/nomad/pull/5768/files#diff-2b17483fc1838ce536b7f8e6f6f6ea8eR1122
…ners removed out of band
3b68ae6
to
262c863
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - with a minor point about logging level - merge when you are ready.
drivers/docker/driver.go
Outdated
} | ||
switch err.(type) { | ||
case *docker.NoSuchContainer: | ||
h.logger.Error("failed to inspect container state, will proceed with DestroyTask", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit odd to be Error log considering that we handle it quite well. I would downgrade it to info or debug with it indicating "container was deleted out-of-band, will proceed with DestroyTask"`.
4211b39
to
7b6d233
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Resolves #5659
Before 0.9, the docker driver would remove the docker container in
run()
after it had finished. After the refactor as a task driver plugin, this code was left inStartTask()
. However, this was causingDestroyTask()
to error early when it attempted to inspect the (deleted) container. This early exit was preventing image cleanup from happening.This change moves the container remove over to
DestroyTask()
, so that it can be inspected and stopped. Tests are added for the image GC.Update: Also, the process has been hardened to handle cases where the docker image has been removed out-of-band.