Conversation
|
Skipping CI for Draft Pull Request. |
mxpv
force-pushed
the
event_streaming
branch
2 times, most recently
from
f4c2f2e to
479c052
Compare
mxpv
force-pushed
the
event_streaming
branch
5 times, most recently
from
5ffab05 to
2f37cb6
Compare
|
/test all |
|
| Event: evt, | ||
| Timestamp: timestamppb.Now(), | ||
| }); err != nil { | ||
| log.G(ctx).WithError(err).Error("post event") |
There was a problem hiding this comment.
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.
Yeah, that's fair. We should exit stream if failed to send event.
Added 1bb75f5
There was a problem hiding this comment.
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.
mxpv
force-pushed
the
event_streaming
branch
from
5758658 to
1bb75f5
Compare
|
Rebased against |
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
This reverts commit 2f37cb6. Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
|
PR needs rebase. DetailsInstructions 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. |
5 participants
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.