Add container attach #128

Random-Liu · 2017-08-12T00:00:24Z

This PR added container attach support in cri-containerd, and also enabled the test.

The attach code is really error prone, so please pay extra attention to see whether there is goroutine leakage or deadlock.

@kubernetes-incubator/maintainers-cri-containerd

miaoyq · 2017-08-14T06:09:51Z

pkg/store/container/container.go

+}
+
+// WithContainerIO adds IO into the container.
+func WithContainerIO(io *cio.ContainerIO) func(*Container) {


It 's better if func(*Container) could be defined as a type.

miaoyq · 2017-08-14T07:08:04Z

pkg/server/io/io.go

+}
+
+// WithStdin enables stdin of the container io.
+func WithStdin(stdin bool) func(*ContainerIO) error {


Define func(*ContainerIO) error as a type

miaoyq · 2017-08-14T11:03:27Z

pkg/server/io/io.go

+
+	var stdinCloser io.Closer
+	if c.stdinPath != "" && stdin != nil {
+		f, err := fifo.OpenFifo(c.closer.ctx, c.stdinPath, syscall.O_WRONLY|syscall.O_NONBLOCK, 0700)


Make sure c.closer != nil

Good catch! Will do.

Random-Liu · 2017-08-15T21:34:04Z

pkg/server/container_start.go

+		stdoutWC = cio.NewDiscardLogger()
+		stderrWC = cio.NewDiscardLogger()
+	}
+


Close if there is error.

miaoyq · 2017-08-16T08:16:27Z

@Random-Liu About io, I think that we can reuse the implementation in containerd instead of implementing it again here, Just a suggestion : )

miaoyq · 2017-08-16T08:21:30Z

pkg/server/container_attach.go

+	return c.streamServer.GetAttach(r)
+}
+
+func (c *criContainerdService) attach(ctx context.Context, id string, stdin io.Reader, stdout, stderr io.WriteCloser,


Rename attach to attachContainer , like execInContainer, to avoid confusion with io.attach

Random-Liu · 2017-08-16T18:29:41Z

@Random-Liu About io, I think that we can reuse the implementation in containerd instead of implementing it again here, Just a suggestion : )

The problem is that containerd io doesn't meet our requirement, e.g. tee support. I've made containerd IO an interface containerd/containerd#1343, I'll make our io implement that interface after #113 is merged.

miaoyq · 2017-08-17T00:39:02Z

@Random-Liu Got it, Thank you for your patience to explain.

Random-Liu · 2017-08-17T00:53:05Z

Will rebase this PR soon.

Random-Liu · 2017-08-18T04:07:00Z

@abhinandanpb @mikebrow Rebased, please take a look.

Random-Liu · 2017-08-21T21:11:19Z

@mikebrow I skipped the golang tip test, because it keeps failing even though golang 1.8.x works. And it didn't fail before, seems like a bug in golang HEAD.

Since #145 will remove golang tip test, I'll remove it in this PR.

mikebrow

halfway through review will get to the rest in the morn..

mikebrow · 2017-08-21T21:47:52Z

pkg/ioutil/write_closer_test.go

+	assert.Equal(t, len(data), n)
+	assert.Equal(t, data, original.buf.String())
+	assert.NoError(t, err)
+


maybe use another thread here to test the wait function?

Why another thread is required? One thread seems more deterministic here.

mikebrow · 2017-08-21T21:48:36Z

pkg/ioutil/write_closer_test.go

+	default:
+		assert.Fail(t, "write closer not closed")
+	}
+}


no-op test for the no-op write closer :-)

We mainly want to test WriteCloseInformer will close the inform channel when it is closed. And the test proves that it does. :)

abhi · 2017-08-22T22:58:29Z

pkg/server/io/io.go

+		}
+		// Also increase wait group here, so that `closer.Wait` will
+		// also wait for this fifo to be closed.
+		c.closer.wg.Add(1)


is there a corresponding Wait for this waitgroup ? Or is the closer implicitly waiting on this wg ?

Yeah, ContainerIO.Close will wait for this. This makes sure that ContainerIO.Close won't remove the fifo directory when Attach is still using the stdin pipe.

abhi · 2017-08-22T23:13:21Z

pkg/server/container_attach.go

+
+	// Close stdin after first attach if StdinOnce is specified.
+	if cntr.Config.StdinOnce {
+		task.CloseIO(ctx, containerd.WithStdinCloser)


If StdinOnce is not specified when is the IO closed ? May be worth adding a comment as well.

The IO will never close until the container dies, will add comment.

Random-Liu · 2017-08-23T22:32:43Z

@mikebrow @abhinandanpb Replied or addressed comments.

mikebrow

/LGTM

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu · 2017-08-23T23:57:02Z

Apply LGTM based on #128 (review).

Will merge after test passes.

Add container attach

k8s-ci-robot added the cncf-cla: yes label Aug 12, 2017

Random-Liu assigned abhi and mikebrow Aug 12, 2017

Random-Liu force-pushed the add-container-attach branch 2 times, most recently from 208ba89 to 7a0fefd Compare August 12, 2017 00:16

miaoyq reviewed Aug 14, 2017

View reviewed changes

Random-Liu force-pushed the add-container-attach branch 3 times, most recently from 94a8607 to 8b6e9d4 Compare August 15, 2017 00:36

Random-Liu commented Aug 15, 2017

View reviewed changes

miaoyq reviewed Aug 16, 2017

View reviewed changes

Random-Liu force-pushed the add-container-attach branch from 8b6e9d4 to 4c0d367 Compare August 18, 2017 04:02

Random-Liu force-pushed the add-container-attach branch 5 times, most recently from 54ba1de to be449d4 Compare August 21, 2017 18:31

mikebrow reviewed Aug 21, 2017

View reviewed changes

Random-Liu force-pushed the add-container-attach branch from c095f81 to 8755d8e Compare August 22, 2017 05:37

abhi reviewed Aug 22, 2017

View reviewed changes

Random-Liu force-pushed the add-container-attach branch 2 times, most recently from ed9f2a1 to cfba56e Compare August 23, 2017 22:31

mikebrow approved these changes Aug 23, 2017

View reviewed changes

Random-Liu mentioned this pull request Aug 23, 2017

Add instrumented service. #151

Merged

Random-Liu added 2 commits August 23, 2017 23:46

Move generateID to util.

77b703f

Signed-off-by: Lantao Liu <[email protected]>

Add container attach support.

45ee2e5

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu force-pushed the add-container-attach branch from cfba56e to 45ee2e5 Compare August 23, 2017 23:50

Random-Liu added the lgtm label Aug 23, 2017

Random-Liu merged commit 60c7f51 into containerd:master Aug 24, 2017

Random-Liu deleted the add-container-attach branch August 24, 2017 00:20

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#128 from Random-Liu/add-container-attach

1baefc9

Add container attach

Add container attach #128

Add container attach #128

Uh oh!

Conversation

Random-Liu commented Aug 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miaoyq commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miaoyq commented Aug 17, 2017

Uh oh!

Random-Liu commented Aug 17, 2017

Uh oh!

Random-Liu commented Aug 18, 2017

Uh oh!

Random-Liu commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Aug 23, 2017

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Random-Liu Aug 14, 2017 •

edited

Loading

Random-Liu commented Aug 16, 2017 •

edited

Loading

Random-Liu commented Aug 21, 2017 •

edited

Loading

Random-Liu Aug 23, 2017 •

edited

Loading

Random-Liu commented Aug 23, 2017 •

edited

Loading