Skip to content

Add events method to docker clients #9932

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

Merged
merged 4 commits into from
Jan 5, 2024
Merged

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Dec 22, 2023

Motivation

For an upcoming change to the way containers are handled, we will want to be able to capture the docker events in order to properly handle notifications of container termination or shutdown.

Changes

This PR adds an events method, which returns a generator of the events from the docker daemon.

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Dec 22, 2023
@simonrw simonrw self-assigned this Dec 22, 2023
@coveralls
Copy link

coveralls commented Dec 22, 2023

Coverage Status

coverage: 83.901% (-0.03%) from 83.926%
when pulling 2af4b6e on docker/add-events-method
into 124a4e2 on master.

Copy link

github-actions bot commented Dec 22, 2023

LocalStack Community integration with Pro

    2 files      2 suites   1h 18m 21s ⏱️
2 450 tests 2 224 ✅ 226 💤 0 ❌
2 451 runs  2 224 ✅ 227 💤 0 ❌

Results for commit 2af4b6e.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review December 22, 2023 19:21
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

I think this can help us significantly, but we need to make the streams cancellable, to avoid resource leaks.

Comment on lines 439 to 454
for msg in CancellableProcessStream(process):
try:
yield json.loads(msg)
except json.JSONDecodeError as e:
LOG.warning("Error decoding docker event %s: %s", msg, e)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this. The stream is not cancellable with this construct. I think for these method we should always use a CancellableStream, so we can cancel it if necessary.

@simonrw simonrw force-pushed the docker/add-events-method branch from c44f20e to 1f3ad85 Compare January 5, 2024 16:54
@simonrw simonrw requested a review from dfangl January 5, 2024 16:55
@simonrw simonrw force-pushed the docker/add-events-method branch from 1f3ad85 to 2af4b6e Compare January 5, 2024 17:20
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

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

Thanks for making it cancellable!

@simonrw simonrw merged commit 94f9ddf into master Jan 5, 2024
@simonrw simonrw deleted the docker/add-events-method branch January 5, 2024 19:11
dfangl added a commit that referenced this pull request Jan 9, 2024
dfangl added a commit that referenced this pull request Jan 9, 2024
dfangl added a commit that referenced this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants