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
Shim events streaming #9704
Shim events streaming #9704
Conversation
Skipping CI for Draft Pull Request. |
f4c2f2e
to
479c052
Compare
5ffab05
to
2f37cb6
Compare
/test all |
|
Namespace: ns, | ||
Event: evt, | ||
Timestamp: timestamppb.Now(), | ||
}); err != nil { | ||
log.G(ctx).WithError(err).Error("post event") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If containerd is restarting, original connection has been closed. It seems that the server side is still running. After containerd restarted, there are two connections watching on one channel. If old connection wins, it seems containerd won't get the event. Is it correct?
Maybe we can fail-fast if connection is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fair. We should exit stream if failed to send event.
Added 1bb75f5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need Context()
in streamer. When ttrpc connection is closed, we can get notification from context and stop receiving events from channel. Or introduce multiple event receiver, like #9661. It can be done in the follow-up.
5758658
to
1bb75f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Rebased against |
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
This reverts commit 2f37cb6. Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
PR needs rebase. 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. |
I don't think this one should be considered for 2.0. We can accomplish this without changing the task interface and I think we need more time to test the push vs pull events model. This approach is better but we need to consider how the shim can know an event was handled after it is placed on the stream. |
yes, fair enough. I'll reiterate this some time in future. |
In current implementation, for each shim instance, we typically maintain two TTRPC/GRPC connections. One from containerd to the shim to manage tasks, and the other one from shim back to containerd to post shim events. Since both TTRPC and GRPC now support streaming, we can utilize one bi-directional connection.
This PR needs a couple of cleanups, I plan to follow up to keep the diff reasonable.Also 2f37cb6 adds a workaround for containerd/ttrpc#126 that needs to be reverted once the issue is addressed.