fix: modify streaming io url and add docs of sandboxer and io_type #10190

abel-von · 2024-05-08T07:23:20Z

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 a streaming suffix makes the logic complecated.

k8s-ci-robot · 2024-05-08T07:23:30Z

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 /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.

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.

abel-von · 2024-05-08T07:24:45Z

/cc @fuweid @mxpv @dmcgowan

Burning1020 · 2024-05-08T08:42:09Z

/ok-to-test

abel-von · 2024-05-08T08:42:59Z

/retest

abel-von · 2024-05-08T16:38:24Z

/test pull-containerd-k8s-e2e-ec2

fuweid · 2024-05-11T14:23:40Z

internal/cri/io/exec_io.go

@@ -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>


I think we don't need to list all the combinations in here. Suggest to use protocol://address?streaming_id=xyz.

abel-von · 2024-05-13T03:29:20Z

Kubernetes e2e suite: [It] [sig-network] DNS HostNetwork spec.Hostname field is silently ignored and the node hostname is used when hostNetwork is set to true for a Pod expand_less | 3s
-- | --
{ failed [FAILED] expected hostname: ip-172-31-91-82.ec2.internal, got: ip-172-31-91-82 In [It] at: k8s.io/kubernetes/test/e2e/network/dns.go:690 @ 05/13/24 02:17:47.278 }

pull-containerd-k8s-e2e-ec2 seems failed for all new PRs, It seems that some error from execution environment make this error. @fuweid

fuweid

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

fuweid · 2024-05-13T08:15:31Z

internal/cri/io/container_io.go

@@ -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


Suggested change

// 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.

fuweid · 2024-05-13T08:16:07Z

internal/cri/io/exec_io.go

+// 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


Please check last comment on WithStream

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 <fshb1988@gmail.com>

Signed-off-by: Abel Feng <fshb1988@gmail.com>

abel-von · 2024-05-13T09:45:02Z

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.

I think streaming server should support reconnection of the same ID, and add this in the comment, please check. @fuweid

abel-von · 2024-05-13T09:50:20Z

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.

abel-von · 2024-05-16T07:11:44Z

/retest

fuweid

LGTM

k8s-ci-robot added size/M needs-ok-to-test labels May 8, 2024

k8s-ci-robot requested review from dmcgowan, fuweid and mxpv May 8, 2024 07:24

abel-von changed the title ~~fix: modify streaming io url form~~ fix: modify streaming io url form and add docs of sandboxer and io_type May 8, 2024

abel-von changed the title ~~fix: modify streaming io url form and add docs of sandboxer and io_type~~ fix: modify streaming io url and add docs of sandboxer and io_type May 8, 2024

abel-von force-pushed the fix-streaming-io-path branch from e086d21 to 0b55bf6 Compare May 8, 2024 08:01

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 8, 2024

fuweid reviewed May 11, 2024

View reviewed changes

abel-von force-pushed the fix-streaming-io-path branch from 0b55bf6 to f56d5b9 Compare May 13, 2024 03:19

fuweid reviewed May 13, 2024

View reviewed changes

abel-von force-pushed the fix-streaming-io-path branch from f56d5b9 to 52c85a8 Compare May 13, 2024 09:40

abel-von added 3 commits May 13, 2024 17:42

cri: restart created container with correct io type

7cead88

Signed-off-by: Abel Feng <fshb1988@gmail.com>

doc: add the description of sandboxer and io_type

0b113d7

Signed-off-by: Abel Feng <fshb1988@gmail.com>

abel-von force-pushed the fix-streaming-io-path branch from 52c85a8 to 0b113d7 Compare May 13, 2024 09:43

fuweid approved these changes May 16, 2024

View reviewed changes

mxpv approved these changes May 16, 2024

View reviewed changes

mxpv added this pull request to the merge queue May 16, 2024

Merged via the queue into containerd:main with commit 90a8667 May 16, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: modify streaming io url and add docs of sandboxer and io_type #10190

fix: modify streaming io url and add docs of sandboxer and io_type #10190

abel-von commented May 8, 2024

k8s-ci-robot commented May 8, 2024

abel-von commented May 8, 2024

Burning1020 commented May 8, 2024

abel-von commented May 8, 2024

abel-von commented May 8, 2024

fuweid May 11, 2024

abel-von May 13, 2024

abel-von commented May 13, 2024 •

edited

fuweid left a comment

fuweid May 13, 2024

abel-von May 13, 2024

fuweid May 13, 2024

abel-von May 13, 2024

abel-von commented May 13, 2024

abel-von commented May 13, 2024

abel-von commented May 16, 2024

fuweid left a comment

-// 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.

fix: modify streaming io url and add docs of sandboxer and io_type #10190

fix: modify streaming io url and add docs of sandboxer and io_type #10190

Conversation

abel-von commented May 8, 2024

k8s-ci-robot commented May 8, 2024

abel-von commented May 8, 2024

Burning1020 commented May 8, 2024

abel-von commented May 8, 2024

abel-von commented May 8, 2024

fuweid May 11, 2024

Choose a reason for hiding this comment

abel-von May 13, 2024

Choose a reason for hiding this comment

abel-von commented May 13, 2024 • edited

fuweid left a comment

Choose a reason for hiding this comment

fuweid May 13, 2024

Choose a reason for hiding this comment

abel-von May 13, 2024

Choose a reason for hiding this comment

fuweid May 13, 2024

Choose a reason for hiding this comment

abel-von May 13, 2024

Choose a reason for hiding this comment

abel-von commented May 13, 2024

abel-von commented May 13, 2024

abel-von commented May 16, 2024

fuweid left a comment

Choose a reason for hiding this comment

abel-von commented May 13, 2024 •

edited