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

Fix event filter filtering on "or" #35896

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
7 participants
@thaJeztah
Member

thaJeztah commented Dec 29, 2017

fixes #35891

The event filter used two separate filter-conditions for "namespace" and "topic". As a result, both events matching "topic" and events matching "namespace" were subscribed to, causing events to be handled both by the "plugin" client, and "container" client.

This patch rewrites the filter to match only if both namespace and topic match.

Thanks to Stephen Day for providing the correct filter :)

- Description for the changelog

* Fix containerd events being processed twice
Fix event filter filtering on "or"
The event filter used two separate filter-conditions for
"namespace" and "topic". As a result, both events matching
"topic" and events matching "namespace" were subscribed to,
causing events to be handled both by the "plugin" client, and
"container" client.

This patch rewrites the filter to match only if both namespace
and topic match.

Thanks to Stephen Day for providing the correct filter :)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Dec 29, 2017

Contributor

LGTM

Contributor

stevvooe commented Dec 29, 2017

LGTM

@dnephin

LGTM

@boaz1337

LGTM 🏆

@yongtang

LGTM

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 1, 2018

Member

z timeout after 300 min. Will give it another try. Or maybe timeout should be increased?

Member

yongtang commented Jan 1, 2018

z timeout after 300 min. Will give it another try. Or maybe timeout should be increased?

@coolljt0725

This comment has been minimized.

Show comment
Hide comment
@coolljt0725

coolljt0725 Jan 2, 2018

Contributor

LGTM

Contributor

coolljt0725 commented Jan 2, 2018

LGTM

@coolljt0725 coolljt0725 merged commit 431d3d6 into moby:master Jan 2, 2018

5 of 6 checks passed

z Jenkins build Docker-PRs-s390x 7520 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38541 has succeeded
Details
janky Jenkins build Docker-PRs 47276 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7663 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18784 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:fix-namespace-filtering branch Jan 2, 2018

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 2, 2018

Member

z timeout after 300 min. Or maybe timeout should be increased?

300min is already way too long to wait for tests. We shouldn't need to re-run every test on every platform. I think we need to be looking for tests that can be skipped on z instead of increasing any timeouts.

Member

dnephin commented Jan 2, 2018

z timeout after 300 min. Or maybe timeout should be increased?

300min is already way too long to wait for tests. We shouldn't need to re-run every test on every platform. I think we need to be looking for tests that can be skipped on z instead of increasing any timeouts.

banuchka added a commit to banuchka/docker-ce that referenced this pull request Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment