Move stdio attach from libcontainerd backend to callback #27467

tonistiigi · 2016-10-17T22:42:21Z

Move stdio attaching from backend to callback to fix to keep the lock during startup. Still not very nice because we need to expose public methods because the split between daemon/container/streamconfig is quite arbitrary.

@mlaventure

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

mlaventure · 2016-10-18T15:18:31Z

👍 for the moving of the logging routine into the container object.

LK4D4 · 2016-10-18T15:32:41Z

runconfig/streams.go

+	copyFunc := func(w io.Writer, r io.Reader) {
+		streamConfig.Add(1)
+		go func() {
+			if _, err := io.Copy(w, r); err != nil {


I wonder if we're here it makes sense to use pools.Copy, because each copy allocates 32Kb regardless of real copy size.

LK4D4 · 2016-10-18T15:34:27Z

libcontainerd/client_linux.go

-
-	if err := clnt.backend.AttachStreams(processFriendlyName, *iopipe); err != nil {
-		clnt.lock(containerID)
+	if err := attachStdio(*iopipe); err != nil {


Sort of unrelated question, but why don't just use IOPipe values everywhere to avoid *.

Could change that. It's so because it is easier to return nil instead empty struct in openFifos and I didn't want to pass pointers through pkg boundary when the other side is not meant to change the struct.

thanks for an explanation, definitely not this PR material

tonistiigi · 2016-10-18T16:43:34Z

@ncdc Can you check if this fixes #27289 for you?

ncdc · 2016-10-18T17:06:51Z

@tonistiigi I'll be happy to give it a spin. Although, given the current lack of a deterministic reproducer, it's hard to say with certainty that any potential fix resolves the issue. Do you have any suggestions for how to make this race happen all the time?

tonistiigi · 2016-10-18T18:12:00Z

@ncdc In old code the problem can be reproduced by adding a sleep to https://github.com/docker/docker/blob/668189e7f007dcf276f94453504ee1d7a187ba7f/daemon/monitor.go#L159 . With this patch that part of the code is removed.

LK4D4 · 2016-10-18T19:50:24Z

runconfig/streams.go

+	if stdin := streamConfig.Stdin(); stdin != nil {
+		if iop.Stdin != nil {
+			go func() {
+				io.Copy(iop.Stdin, stdin)


should be pools too :)

LK4D4 · 2016-10-18T23:56:12Z

LGTM

ncdc · 2016-10-19T14:04:18Z

cc @runcom @mrunalp @jwhonce

runcom · 2016-10-21T11:08:29Z

@ncdc is this working for you? (at least to your reproducer)

ncdc · 2016-10-21T12:37:09Z

So far so good. I had my endless loop running for a couple of days and it didn't fail.

runcom · 2016-10-23T15:05:13Z

@tonistiigi needs a rebase, ping @mlaventure

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

runcom · 2016-10-25T14:22:59Z

LGTM

ncdc · 2016-10-25T18:14:05Z

I've got a test running and will take a look once I'm at my desk
On Fri, Oct 21, 2016 at 7:10 AM Antonio Murdaca notifications@github.com
wrote:

@ncdc https://github.com/ncdc is this working for you? (at least to
your reproducer)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27467 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAABYqUXY4IckrER785qSZ-IWH56ZOmbks5q2J2MgaJpZM4KZMJG
.

Problem: if a process that's been started for 'docker exec' exits fast enough, the daemon can receive a 'process exited' update from containerd before it starts passing stdio data back and forth, losing its output. Why it happens: the 'process exited' state change message is only processed after acquiring a lock for the container, and while most of the exec() setup is done while holding that lock, the lock is freed when libcontainerd calls back to set up the passing of stdio data. Proposed change: call getExecConfig() before AddProcess(), and modify AddProcess() to take a callback that the daemon can hand it that remembers the exec.Config value that was retrieved. The effect should be the same as that of the changes in moby#27467, but without as much refactoring. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

tonistiigi · 2016-11-07T19:59:32Z

@tonylambiris That should be a dupe of #27760

tonylambiris · 2016-11-08T15:49:47Z

Thanks for the update. 👍

This is cherry-picked from docker upstream PR: moby#27467 Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Deng Guangxing <dengguangxing@huawei.com>

- Bugfix: Increase udev wait timeout to 185s (mr 365) - Bugfix: Add restarting check before attach a container (mr 362) - Bugfix: daemon: reorder mounts before setting them (mr 350) - Feature: modify centos rpm spec according to that of EulerOS (mr 361) - Backport: fix TestDaemonRestartWithInvalidBasesize (mr 358) - Backport: Ignore "failed to close stdin" if container or process not found (mr 364) - Backport: Reset RemovalInProgress flag on daemon restart (mr 360) - Backport: backport some prs after bumping containerd to v0.2.x branch (mr 342) - Fix restoring behavior when live-restore is not set (moby#24984) - Fix a race in libcontainerd/pausemonitor_linux.go (moby#26695) - Fix libcontainerd: attach streams before create (moby#26744) - add lock in libcontainerd client AddProcess (moby#27094) - Fix issues with fifos blocking on open (moby#27405) - inherit the daemon log options when creating containers (moby#21153) - Fix panic while merging log configs to nil map (moby#24548) - Move stdio attach from libcontainerd backend to callback (moby#27467) - Fix race on sending stdin close event (moby#28682) - Handle paused container when restoring without live-restore set (moby#31704) - Fix daemon panic on restoring containers (moby#25111) Signed-off-by: Lei Jitang <leijitang@huawei.com>

bump to v1.11.2.19 - Bugfix: fix TestDaemonRestartWithInvalidBasesize (mr 358) - Bugfix: Increase udev wait timeout to 185s (mr 365) - Bugfix: Ignore "failed to close stdin" if container or process not found (mr 364) - Bugfix: Add restarting check before attach a container (mr 362) - Feature: modify centos rpm spec according to that of EulerOS (mr 361) - Backport: daemon: reorder mounts before setting them (mr 350) - Backport: Reset RemovalInProgress flag on daemon restart (mr 360) - Backport: backport some prs after bumping containerd to v0.2.x branch (mr 342) - Fix restoring behavior when live-restore is not set (moby#24984) - Fix a race in libcontainerd/pausemonitor_linux.go (moby#26695) - Fix libcontainerd: attach streams before create (moby#26744) - add lock in libcontainerd client AddProcess (moby#27094) - Fix issues with fifos blocking on open (moby#27405) - inherit the daemon log options when creating containers (moby#21153) - Fix panic while merging log configs to nil map (moby#24548) - Move stdio attach from libcontainerd backend to callback (moby#27467) - Fix race on sending stdin close event (moby#28682) - Handle paused container when restoring without live-restore set (moby#31704) - Fix daemon panic on restoring containers (moby#25111) Signed-off-by: Lei Jitang <leijitang@huawei.com> See merge request docker/docker!366

GordonTheTurtle added the status/0-triage label Oct 17, 2016

tonistiigi force-pushed the attach-cb branch 2 times, most recently from 889ee58 to ea345e5 Compare October 17, 2016 23:06

LK4D4 reviewed Oct 18, 2016

View reviewed changes

tonistiigi changed the title ~~wip: Move stdio attach from libcontainerd backend to callback~~ Move stdio attach from libcontainerd backend to callback Oct 18, 2016

tonistiigi force-pushed the attach-cb branch from ea345e5 to 0a0369d Compare October 18, 2016 16:40

tonistiigi added status/2-code-review and removed status/0-triage labels Oct 18, 2016

LK4D4 reviewed Oct 18, 2016

View reviewed changes

tonistiigi force-pushed the attach-cb branch from 0a0369d to 0dc36ab Compare October 18, 2016 20:23

tonistiigi force-pushed the attach-cb branch from 0dc36ab to 21bea12 Compare October 21, 2016 00:18

Move stdio attach from libcontainerd backend to callback

37a3be2

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the attach-cb branch from 21bea12 to 37a3be2 Compare October 24, 2016 07:20

runcom merged commit 8ed3108 into moby:master Oct 25, 2016

thaJeztah added this to the 1.13.0 milestone Oct 25, 2016

nalind mentioned this pull request Oct 28, 2016

Attempt to mimic changes from upstream PR#27467 projectatomic/docker#212

Merged

thaJeztah mentioned this pull request Nov 8, 2016

panic: sync: WaitGroup is reused before previous Wait has returned #28161

Closed

mlaventure mentioned this pull request Nov 29, 2016

Docker ps hang #28889

Closed

tonistiigi mentioned this pull request Dec 3, 2016

backported IO fixes for v1.12 #29095

Merged

kolyshkin mentioned this pull request Apr 29, 2020

WIP io fixes backports projectatomic/docker#374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move stdio attach from libcontainerd backend to callback #27467

Move stdio attach from libcontainerd backend to callback #27467

tonistiigi commented Oct 17, 2016

mlaventure commented Oct 18, 2016

LK4D4 Oct 18, 2016

tonistiigi Oct 18, 2016

LK4D4 Oct 18, 2016

tonistiigi Oct 18, 2016

LK4D4 Oct 18, 2016

tonistiigi commented Oct 18, 2016

ncdc commented Oct 18, 2016

tonistiigi commented Oct 18, 2016

LK4D4 Oct 18, 2016

tonistiigi Oct 18, 2016

LK4D4 commented Oct 18, 2016

ncdc commented Oct 19, 2016

runcom commented Oct 21, 2016

ncdc commented Oct 21, 2016

runcom commented Oct 23, 2016

runcom commented Oct 25, 2016

ncdc commented Oct 25, 2016

tonistiigi commented Nov 7, 2016

tonylambiris commented Nov 8, 2016

Move stdio attach from libcontainerd backend to callback #27467

Move stdio attach from libcontainerd backend to callback #27467

Conversation

tonistiigi commented Oct 17, 2016

mlaventure commented Oct 18, 2016

LK4D4 Oct 18, 2016

Choose a reason for hiding this comment

tonistiigi Oct 18, 2016

Choose a reason for hiding this comment

LK4D4 Oct 18, 2016

Choose a reason for hiding this comment

tonistiigi Oct 18, 2016

Choose a reason for hiding this comment

LK4D4 Oct 18, 2016

Choose a reason for hiding this comment

tonistiigi commented Oct 18, 2016

ncdc commented Oct 18, 2016

tonistiigi commented Oct 18, 2016

LK4D4 Oct 18, 2016

Choose a reason for hiding this comment

tonistiigi Oct 18, 2016

Choose a reason for hiding this comment

LK4D4 commented Oct 18, 2016

ncdc commented Oct 19, 2016

runcom commented Oct 21, 2016

ncdc commented Oct 21, 2016

runcom commented Oct 23, 2016

runcom commented Oct 25, 2016

ncdc commented Oct 25, 2016

tonistiigi commented Nov 7, 2016

tonylambiris commented Nov 8, 2016