Skip to content
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

thaJeztah
Copy link
Member

@thaJeztah 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

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 <[email protected]>
@stevvooe
Copy link
Contributor

LGTM

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM 🏆

Copy link
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@yongtang
Copy link
Member

yongtang commented Jan 1, 2018

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

@coolljt0725
Copy link
Contributor

LGTM

@coolljt0725 coolljt0725 merged commit 431d3d6 into moby:master Jan 2, 2018
@thaJeztah thaJeztah deleted the fix-namespace-filtering branch January 2, 2018 07:11
@dnephin
Copy link
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.

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

Successfully merging this pull request may close these issues.

After upgrading to 17.12.0-ce containers are reported as "unknown" on start
7 participants