-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: modify streaming io url and add docs of sandboxer and io_type #10190
Conversation
Hi @abel-von. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
e086d21
to
0b55bf6
Compare
/ok-to-test |
/retest |
/test pull-containerd-k8s-e2e-ec2 |
internal/cri/io/exec_io.go
Outdated
@@ -55,6 +55,15 @@ func NewFifoExecIO(id, root string, tty, stdin bool) (*ExecIO, error) { | |||
} | |||
|
|||
// NewStreamExecIO creates exec io with streaming. | |||
// address should be in the form of | |||
// <ttrpc|grpc>+<unix|vsock|hvsock>://<uds-path|vsock-cid:vsock-port|uds-path:hvsock-port> |
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 don't need to list all the combinations in here. Suggest to use protocol://address?streaming_id=xyz
.
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.
done
0b55bf6
to
f56d5b9
Compare
pull-containerd-k8s-e2e-ec2 seems failed for all new PRs, It seems that some error from execution environment make this error. @fuweid |
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.
Left two comments and one question:
When containerd restarts, the streaming_id is the same. Is it possible to establish stream tunnel successfully with same ID? If not, we need to consider to how to recover this and please file issue to track this. Thanks
internal/cri/io/container_io.go
Outdated
@@ -73,6 +73,14 @@ func WithNewFIFOs(root string, tty, stdin bool) ContainerIOOpts { | |||
} | |||
|
|||
// WithStreams creates new streams for the container io. | |||
// address should be in the form of protocol://address |
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.
// address should be in the form of protocol://address | |
// The stream address is in format of `protocol://address`. | |
// It allocates ContainerID-stdin, ContainerID-stdout and ContainerID-stderr as streaming IDs. | |
// For example, that advertiser address of shim is `ttrpc+unix:///run/demo.sock` and container ID is `app`. | |
// There are three streaming IDs if stdin is enable and TTY is disable. | |
// | |
// * Stdin: app-stdin | |
// * Stdout: app-stdout | |
// * stderr: app-stderr | |
// | |
// These streaming IDs will be used as unique key to establish stream tunnel. |
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.
done
internal/cri/io/exec_io.go
Outdated
// the result form is: protocol://address?streaming_id=xyz | ||
// for example ttrpc+hvsock:///run/test.hvsock:1024?streaming_id=xyz, | ||
// grpc+vsock://1111111:1024?streaming_id=xyz, | ||
// or ttrpc+unix:///run/test.sock?streaming_id=xyz |
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.
Please check last comment on WithStream
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.
done
f56d5b9
to
52c85a8
Compare
sandbox address should be in the form of <ttrpc|grpc>+<unix|vsock|hvsock>://<uds-path|vsock-cid:vsock-port|uds-path:hvsock-port> for example: ttrpc+hvsock:///run/test.hvsock:1024 or: grpc+vsock://1111111:1024 and the Stdin/Stdout/Stderr will add a `streaming_id` as a parameter of the url result form is: <ttrpc|grpc>+<unix|vsock|hvsock>://<uds-path|vsock-cid:vsock-port|uds-path:hvsock-port>?streaming_id=<stream-id> for example ttrpc+hvsock:///run/test.hvsock:1024?streaming_id=111111 or grpc+vsock://1111111:1024?streaming_id=222222 Signed-off-by: Abel Feng <[email protected]>
Signed-off-by: Abel Feng <[email protected]>
Signed-off-by: Abel Feng <[email protected]>
52c85a8
to
0b113d7
Compare
I think streaming server should support reconnection of the same ID, and add this in the comment, please check. @fuweid |
and add a fix of restart recovey of container io in restart.go https://github.com/containerd/containerd/pull/10190/files#diff-ad49a10c586e0f81890f9ee72f6dcc26ce03607c37b4c4856786f459e1e43a38 please take a look. |
/retest |
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
sandbox address should be in the form of
<ttrpc|grpc>+<unix|vsock|hvsock>://<uds-path|vsock-cid:vsock-port|uds-path:hvsock-port>
for example: ttrpc+hvsock:///run/test.hvsock:1024
or: grpc+vsock://1111111:1024
and the Stdin/Stdout/Stderr will add a
streaming_id
as a parameter of the url result form is:<ttrpc|grpc>+<unix|vsock|hvsock>://<uds-path|vsock-cid:vsock-port|uds-path:hvsock-port>?streaming_id= for example ttrpc+hvsock:///run/test.hvsock:1024?streaming_id=111111 or grpc+vsock://1111111:1024?streaming_id=222222
Change the current logic of add a
streaming
in the path, because unix domain socket or hybrid vsock alread have the path field and add astreaming
suffix makes the logic complecated.