Skip to content

Add multi-subscriber support to GetContainerEvents CRI API #8916

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.

Sign up for GitHub

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

Jump to bottom
Closed
surik wants to merge 1 commit into from
Closed

Add multi-subscriber support to GetContainerEvents CRI API #8916

surik wants to merge 1 commit into from

Conversation

@surik
Copy link
Contributor

@surik surik commented Aug 3, 2023 •
edited
Loading

This pull request addresses issue #7318 by introducing events broadcasting to the current implementation.

The integration/container_event_test.go is extended to demonstrate the broadcasting capabilities of two simultaneous connected clients.

@k8s-ci-robot
Copy link

k8s-ci-robot commented Aug 3, 2023

Hi @surik. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@surik surik force-pushed the broadcast-get-container-events-cri branch 3 times, most recently from 3e5c3fd to 32f7ea1 Compare August 4, 2023 08:38
@surik surik marked this pull request as ready for review August 4, 2023 09:35
@surik surik marked this pull request as draft August 4, 2023 11:11
@surik surik force-pushed the broadcast-get-container-events-cri branch from 32f7ea1 to f200dd5 Compare August 4, 2023 11:12
@surik
Copy link
Contributor Author

surik commented Aug 4, 2023

/retest

@k8s-ci-robot
Copy link

k8s-ci-robot commented Aug 4, 2023

@surik: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@surik surik marked this pull request as ready for review August 4, 2023 12:02
dcantah
dcantah reviewed Aug 7, 2023
View reviewed changes
@surik surik force-pushed the broadcast-get-container-events-cri branch from 1619c3e to d75372e Compare August 7, 2023 09:25
@surik surik mentioned this pull request Aug 7, 2023
Closed
1 task
@surik surik requested a review from dcantah August 10, 2023 07:46
@surik surik mentioned this pull request Aug 15, 2023
Closed
@surik surik force-pushed the broadcast-get-container-events-cri branch from dbe3c5b to 4cfa8bd Compare August 16, 2023 08:36
dcantah
dcantah approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

LGTM assuming CI is happy, I'll check in the morning. Thanks!

@surik surik force-pushed the broadcast-get-container-events-cri branch from 4cfa8bd to 227354d Compare August 16, 2023 08:46
@surik surik mentioned this pull request Aug 16, 2023
Merged
ruiwen-zhao
ruiwen-zhao approved these changes Aug 18, 2023
View reviewed changes
Copy link
Member

@ruiwen-zhao ruiwen-zhao left a comment

Choose a reason for hiding this comment

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

Just one nit comment on test, otherwise LGTM!

Thanks for working on this!

@surik surik force-pushed the broadcast-get-container-events-cri branch from 1eb882b to 37ea59a Compare August 19, 2023 09:10
@samuelkarp
Copy link
Member

samuelkarp commented Aug 19, 2023

/ok-to-test

@samuelkarp
Copy link
Member

samuelkarp commented Aug 19, 2023

Would you mind squashing your commits into one? We don't generally need to preserve intermediate commits that are just addressing review comments.

@surik surik force-pushed the broadcast-get-container-events-cri branch from 37ea59a to 0c90d32 Compare August 20, 2023 10:56
@surik
Copy link
Contributor Author

surik commented Aug 20, 2023

/test pull-containerd-node-e2e

@surik
Copy link
Contributor Author

surik commented Aug 20, 2023

All squished. I'm not sure if pull-containerd-node-e2e is related to the changes.

@surik surik force-pushed the broadcast-get-container-events-cri branch from 0c90d32 to 815422e Compare August 22, 2023 16:12
@surik
Copy link
Contributor Author

surik commented Aug 23, 2023

Ok now pull-containerd-node-e2e is good. CI / Windows Integration (windows-2019, sandboxed) (pull_request) fails. I don't have the ability to retry it.

@dcantah
Copy link
Member

dcantah commented Aug 24, 2023

Re-running the failed test suite

@surik
Copy link
Contributor Author

surik commented Aug 25, 2023

Hi @samuelkarp this is my first PR to containerd. Is there anything else I have to do to proceed with it?

@dcantah
Copy link
Member

dcantah commented Aug 25, 2023 •
edited
Loading

Just needs another maintainer to review and approve. We generally view two approvals as good to check in. There's exceptions where we'll wait for more folks to get a chance to take a peek if it's a large change, or if we want someone specific to take a look. I think @mikebrow might be interested in this for example.

henry118
henry118 reviewed Sep 5, 2023
View reviewed changes
@surik surik force-pushed the broadcast-get-container-events-cri branch from 815422e to 244f030 Compare September 6, 2023 14:11
@surik
Copy link
Contributor Author

surik commented Sep 6, 2023

I believe the two failed tests just need retry

henry118
henry118 approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

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

LGTM

@surik
Copy link
Contributor Author

surik commented Oct 4, 2023

Hey everyone, I wonder if there is something that I have to do to proceed with this PR.

cc @dcantah @samuelkarp @mikebrow

cpuguy83
cpuguy83 reviewed Nov 13, 2023
View reviewed changes
pkg/cri/server/container_events.go
}

func (c *criService) broadcastEvents() {
for containerEvent := range c.containerEventsChan {
Copy link
Member

@cpuguy83 cpuguy83 Nov 13, 2023

Choose a reason for hiding this comment

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

With this implementation a single slow client will block other clients from receiving updates.
Also each new client slows down delivery since these are being sent sequentially.

Copy link
Contributor Author

@surik surik Nov 13, 2023

Choose a reason for hiding this comment

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

@cpuguy83 Thank you for highlighting the concerns. I'm considering wrapping the range function in a goroutine to address the blocking issue. Would it be reasonable to limit the maximum routines running concurrently? If so, what do you think would be a good limit here?

Copy link
Member

@cpuguy83 cpuguy83 Nov 14, 2023

Choose a reason for hiding this comment

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

Is there any reason subscribers need to attach to the same event stream over creating new streams for each subscriber?

Copy link
Contributor Author

@surik surik Nov 14, 2023

Choose a reason for hiding this comment

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

The subscribers need to receive the same events. We can consider doing a fan out from a different place. For example here

containerd/pkg/cri/server/helpers.go

Line 446 in 7deb68f

case c.containerEventsChan <- event:

Is this what you have in mind?

Copy link
Contributor Author

@surik surik Nov 30, 2023

Choose a reason for hiding this comment

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

Hey @cpuguy83, following up on our recent discussion about optimizing the event streaming implementation. Would love to get your thoughts on the proposed changes. Thanks!

@surik
Add multi-subscriber support to GetContainerEvents CRI API
a97e987 
This commit addresses issue containerd#7318 by introducing events broadcasting
to the current implementation. The integration/container_event_test.go
is extended to demonstrate the broadcasting capabilities
of two simultaneous connected clients.

Signed-off-by: Yury Gargay <yury.gargay@gmail.com>
@surik surik force-pushed the broadcast-get-container-events-cri branch from 244f030 to a97e987 Compare November 30, 2023 19:04
@dmcgowan dmcgowan mentioned this pull request Jan 20, 2024
Merged
@surik
Copy link
Contributor Author

surik commented Jan 27, 2024

I'm closing this one as #9661 has been merged.

@surik surik closed this Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 left review comments

@henry118 henry118 henry118 approved these changes

@ruiwen-zhao ruiwen-zhao ruiwen-zhao approved these changes

@dcantah dcantah dcantah approved these changes

Assignees

No one assigned

Labels

ok-to-test size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@surik @k8s-ci-robot @samuelkarp @dcantah @cpuguy83 @henry118 @ruiwen-zhao