shimv2: support attachable binary I/O #10020

fahedouch · 2024-03-29T15:50:21Z

nerdctl uses binary I/O to support detached mode, but this is not sufficient because it does not allow reattachment. This PR introduces a new I/O attachablebinary that allows detaching and reattaching of I/O managed by the containerd-shim

AkihiroSuda · 2024-04-01T10:32:27Z

cmd/containerd-shim-runc-v2/process/io.go

@@ -107,6 +110,16 @@ func createIO(ctx context.Context, id string, ioUID, ioGID int, stdio stdio.Stdi
 		pio.io, err = runc.NewPipeIO(ioUID, ioGID, withConditionalIO(stdio))
 	case "binary":
 		pio.io, err = NewBinaryIO(ctx, id, u)
+	case "attachablebinary":


Needs godoc and test

test added.
I am not really sure where to add godoc cc @AkihiroSuda

godoc added

maybe we can merge attachablebinary with existing one, binary, by using reserve param key like _attach=true.

cc @AkihiroSuda @dmcgowan

I intended to merge them but I think it will introduce a non-negligible complexity and more responsibilities to NewBinaryIO() func so I prefer to make a seperate function

maybe we can merge attachablebinary with existing one, binary, by using reserve param key like _attach=true.

That was my thinking too. "attachablebinary" is extended case of "binary", I like the idea of introducing attach flag. The code is indeed complex and needs some refactoring.

cmd/containerd-shim-runc-v2/process/io.go

cmd/containerd-shim-runc-v2/process/io_test.go

cmd/containerd-shim-runc-v2/process/io.go

fuweid · 2024-04-23T10:33:53Z

cmd/containerd-shim-runc-v2/process/io.go

@@ -107,6 +110,16 @@ func createIO(ctx context.Context, id string, ioUID, ioGID int, stdio stdio.Stdi
 		pio.io, err = runc.NewPipeIO(ioUID, ioGID, withConditionalIO(stdio))
 	case "binary":
 		pio.io, err = NewBinaryIO(ctx, id, u)
+	case "attachablebinary":


maybe we can merge attachablebinary with existing one, binary, by using reserve param key like _attach=true.

cc @AkihiroSuda @dmcgowan

fuweid · 2024-04-23T10:35:03Z

cmd/containerd-shim-runc-v2/process/io.go

+			return f, fmt.Errorf("failed to open stdout fifo: %w", retErr)
+		}
+		defer func() {
+			if retErr != nil && f.Stdout != nil {


I think we don't need to check f.Stdout != nil here.

yep, I also cleared some useless defer funcs

fuweid · 2024-04-23T10:36:41Z

cmd/containerd-shim-runc-v2/process/io.go

@@ -243,6 +256,16 @@ func (c *countingWriteCloser) Close() error {
 	return c.WriteCloser.Close()
 }

+type PipesReader struct {


is it required to be exported type?

nope, fixed

fuweid · 2024-04-23T10:39:17Z

cmd/containerd-shim-runc-v2/process/io.go

+		if err == nil {
+			return
+		}
+		result := []error{err}


please use named var defined in return parameters.

it is better now ?

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>

fahedouch · 2024-04-25T08:53:43Z

/retest

henry118 · 2024-05-01T05:39:02Z

cmd/containerd-shim-runc-v2/process/io.go

@@ -307,6 +329,180 @@ func NewBinaryIO(ctx context.Context, id string, uri *url.URL) (_ runc.IO, err e
 	}, nil
 }

+// NewAttachableBinary runs a custom binary process and opens attachable fifos for pluggable shim logging
+func NewAttachableBinary(ctx context.Context, id string, uri *url.URL) (_ *cio.FIFOSet, err error) {
+	binaryFifos, err := cio.NewFIFOSetInDir(defaults.DefaultFIFODir, id, true)


Why terminal=true here?

henry118 · 2024-05-01T05:45:48Z

cmd/containerd-shim-runc-v2/process/io.go

+		err = errors.Join(result...)
+	}()
+
+	pipesAttachableFifosPipes, err := openAttachableFifos(ctx, attachableFifos)


Will it be problematic if there are no consumers of these FIFOs? Pipe has limited capacity and when it's full the writing side may block or fail.

by default fifo would block when it's full, but with O_NONBLOCK flag coming data will be lost and new data will be queued to the writer(fifo), so attaching tails logs and this is the intended behavior

cpuguy83 · 2024-05-01T16:13:31Z

Really not a fan of these runc specific options that really have nothing really to do with runc.
That said this seems fine to unblock an existing workflow.

It would also be extremely helpful to include an explanation of what this does and how its expected to be used in code and the commit.

k8s-ci-robot added the size/L label Mar 29, 2024

fahedouch marked this pull request as draft March 29, 2024 15:50

k8s-ci-robot added the do-not-merge/work-in-progress label Mar 29, 2024

fahedouch force-pushed the binary-io-writes-to-fifos branch from 76fc465 to 8659051 Compare April 1, 2024 09:32

fahedouch marked this pull request as ready for review April 1, 2024 09:33

k8s-ci-robot removed the do-not-merge/work-in-progress label Apr 1, 2024

fahedouch force-pushed the binary-io-writes-to-fifos branch 2 times, most recently from c84d44b to c3ab348 Compare April 1, 2024 09:45

AkihiroSuda reviewed Apr 1, 2024

View reviewed changes

fahedouch force-pushed the binary-io-writes-to-fifos branch from 2627516 to 7fcea19 Compare April 2, 2024 15:14

fahedouch requested a review from AkihiroSuda April 2, 2024 19:44

fahedouch force-pushed the binary-io-writes-to-fifos branch 2 times, most recently from a3da5f4 to b913e4c Compare April 7, 2024 21:26

djdongjin reviewed Apr 9, 2024

View reviewed changes

cmd/containerd-shim-runc-v2/process/io.go Outdated Show resolved Hide resolved

fuweid reviewed Apr 19, 2024

View reviewed changes

cmd/containerd-shim-runc-v2/process/io_test.go Outdated Show resolved Hide resolved

fuweid changed the title ~~support attachable binary I/O~~ shimv2: support attachable binary I/O Apr 19, 2024

fahedouch force-pushed the binary-io-writes-to-fifos branch from 75509a2 to 8837e4f Compare April 21, 2024 22:03

fahedouch requested a review from fuweid April 21, 2024 22:05

fuweid reviewed Apr 23, 2024

View reviewed changes

fahedouch force-pushed the binary-io-writes-to-fifos branch from 90883dc to cd48cde Compare April 24, 2024 15:54

fahedouch requested a review from fuweid April 24, 2024 15:54

support attachable binary I/O

5f19f04

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>

fahedouch force-pushed the binary-io-writes-to-fifos branch from cd48cde to 5f19f04 Compare April 24, 2024 22:09

henry118 reviewed May 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shimv2: support attachable binary I/O #10020

shimv2: support attachable binary I/O #10020

fahedouch commented Mar 29, 2024

AkihiroSuda Apr 1, 2024

fahedouch Apr 2, 2024

fahedouch Apr 4, 2024

fuweid Apr 23, 2024

fahedouch Apr 24, 2024

mxpv Apr 27, 2024 •

edited

fuweid Apr 23, 2024

fuweid Apr 23, 2024

fahedouch Apr 24, 2024

fuweid Apr 23, 2024

fahedouch Apr 24, 2024

fuweid Apr 23, 2024

fahedouch Apr 24, 2024

fahedouch commented Apr 25, 2024

henry118 May 1, 2024

henry118 May 1, 2024

fahedouch May 5, 2024 •

edited

cpuguy83 commented May 1, 2024

shimv2: support attachable binary I/O #10020

Are you sure you want to change the base?

shimv2: support attachable binary I/O #10020

Conversation

fahedouch commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxpv Apr 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch May 5, 2024 • edited

Choose a reason for hiding this comment

cpuguy83 commented May 1, 2024

mxpv Apr 27, 2024 •

edited

fahedouch May 5, 2024 •

edited