-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Don't exit on container destroy events #8859
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Looking at the code below, it looks like only 2 event-types are currently handled (
die
andstart
); is there a reason to performContainerInspect
for every event?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.
Also note that the event stream itself already has some attributes/metadata that is being obtained from the
inspect
. For example, the followingdocker run
;Returns these details in the events stream (so already includes container name, ID, and labels);
Perhaps the events filter should also be tweaked to only include
die
andstart
(as those are the only ones we're interested in?I think all of those should be supported by the API (see https://docs.docker.com/engine/api/v1.41/#operation/SystemEvents) which means that the client has a lot less to deal with, and there'd be no need to perform the filtering client side;
compose/pkg/compose/events.go
Lines 39 to 52 in 1ae9b3c
Running the above again, this time with some filters, shows that only the
start
anddie
events for theservice2
andservice3
containers are returned;docker events \ --format='{{ json .}}' \ --filter type=container \ --filter event=die \ --filter event=start \ --filter label=com.docker.compose.project=bla \ --filter container=service2 \ --filter container=service3
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 did wonder if this approach might be possible/preferable, subscribing only to the events of interest.
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.
Big disclaimer that while I'm a maintainer for the docker engine, I'm not very familiar with the compose code in this repository, so mainly keeping 'half an eye' out of interest 😅
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.
we indeed could filter by events we manage, to avoid unnecessary computation.
IIRC
ContainerInspect
is only required fordie
event to getretryCount
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.
That approach works. Here's a quick proof-of-concept implementation: sdt/compose@72e4519...sdt:8747-add-event-filters
It adds an array of event names to the EventOptions struct, and creates an event filter for each name. The watchContainers function passes in the events it's interested in: currently just die and start.
I don't have any tests for this as yet - please let me know if you'd like me to proceed down either of these paths.