Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Explicitly set namespace when publishing an event #689

Merged
merged 1 commit into from
Sep 5, 2019

Conversation

kevpar
Copy link
Member

@kevpar kevpar commented Sep 5, 2019

This fixes a regression introduced when we switched to TTRPC for
publishing events to containerd. Previously we explicitly passed the
namespace for each event as a command line parameter to containerd.exe,
which was invoked to publish the event. Now that TTRPC is used, the
context passed to Publish is expected to include the namespace as a
stored value.

Signed-off-by: Kevin Parsons kevpar@microsoft.com

This fixes a regression introduced when we switched to TTRPC for
publishing events to containerd. Previously we explicitly passed the
namespace for each event as a command line parameter to containerd.exe,
which was invoked to publish the event. Now that TTRPC is used, the
context passed to Publish is expected to include the namespace as a
stored value.

Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
@kevpar kevpar requested a review from a team September 5, 2019 04:57
Copy link
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM. Good catch. Any idea why we didnt see this?

@katiewasnothere
Copy link
Contributor

LGTM. @jterry75 I must have missed that the namespaces weren't what they were intended to be when I tested the update to TTRPC.

@kevpar
Copy link
Member Author

kevpar commented Sep 5, 2019

I noticed the issue when looking at a trace for a different issue, and saw failed publishEvent spans. Note that a bunch of the events appeared to already have namespace in the context due to using a context passed through from containerd, but any publishing from a background goroutine tended to use context.Background(). We should probably add a test that does container operations and ensures it sees the expected events delivered.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants