-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Prune from JVM hook if Ryuk is disabled #4960
Conversation
a.k.a. Poor Man's Ryuk :D
.exec(); | ||
|
||
containers.parallelStream().forEach(container -> { | ||
stopContainer(container.getId(), container.getImage()); |
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.
I wonder if the stopContainer
method name has become misleading and should be renamed. When I read this, my first thought was that this overall switch construct would stop containers but not explicitly remove them.
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.
stop
behaves like this for a long time (since forever?) of course (and differentiates from the semantics of Docker's container stop
). So while I was always in favor of aligning our domain language to the Docker one, by now this would a such a breaking change of semantics in our API, I am unsure how to easily deliver it?
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.
stopContainer
specifically is a private method, and IIRC is only called by this and a stopAndRemove
(public) method.
So maybe more do-able?
(+100 for aliging to docker's domain language)
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.
Happy to add it to the scope - do you have naming recommendations? reapContainer
? :D
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.
@rnorth Ok you are right, I was mixing it up with our public stop()
method, which will call stopAndRemoveContainer
(which I consider confusing but hard to change).
@bsideup WDTY about just removeContainer
, since this would be aligned with the methods for other Docker resources?
public synchronized void performCleanup() {
registeredContainers.forEach(this::stopContainer);
registeredNetworks.forEach(this::removeNetwork);
registeredImages.forEach(this::removeImage);
}
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.
sounds good to me 👍
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.
Only left some superficial comments - LGTM overall.
Co-authored-by: Richard North <rich.north@gmail.com>
a.k.a. Poor Man's Ryuk :D