Skip to content
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

Handle blocked I/O of exec'd processes, and remove flaky TestExecWindowsOpenHandles #39383

Merged
merged 1 commit into from Jul 5, 2019

Conversation

crosbymichael
Copy link
Contributor

@crosbymichael crosbymichael commented Jun 20, 2019

fixes #39326

This is the second part to
containerd/containerd#3361 and will help process
delete not block forever when the process exists but the I/O was
inherited by a subprocess that lives on.

Signed-off-by: Michael Crosby crosbymichael@gmail.com

@thaJeztah
Copy link
Member

addresses containerd/containerd#3286 docker exec hang if earlier docker exec left a zombie process

@crosbymichael does this also relate to;

Should we have a test-case for this? (not sure if it's easy to test)

@thaJeztah
Copy link
Member

Haven't seen this one before; https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/25656/console

21:06:29 FAIL: docker_cli_exec_test.go:538: DockerSuite.TestExecWindowsOpenHandles
21:06:29
21:06:29 assertion failed: 2 (int) != 1 (int)

I'll kick CI for RS1 to see if it was just flaky

@crosbymichael
Copy link
Contributor Author

I read through that test and its depending on the "bad" functionality, forking off the sleeps into the background and the actually cmd.exe is exiting. I think it's safe for this test to be removed with this change as well as the test is horribly racy and depending on how many seconds the sleeps wait until they exit, its bad!

This is the second part to
containerd/containerd#3361 and will help process
delete not block forever when the process exists but the I/O was
inherited by a subprocess that lives on.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@codecov
Copy link

codecov bot commented Jun 21, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@52c1667). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #39383   +/-   ##
=========================================
  Coverage          ?   37.34%           
=========================================
  Files             ?      609           
  Lines             ?    45269           
  Branches          ?        0           
=========================================
  Hits              ?    16905           
  Misses            ?    26074           
  Partials          ?     2290

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@kolyshkin @dmcgowan PTAL

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 5, 2019

I/O waiters seems like a good candidate for metrics as well.

@cpuguy83 cpuguy83 merged commit 089757d into moby:master Jul 5, 2019
@thaJeztah thaJeztah added this to backlog in maintainers-session Jul 18, 2019
@thaJeztah thaJeztah changed the title Handle blocked I/O of exec'd processes Handle blocked I/O of exec'd processes, and remove flaky TestExecWindowsOpenHandles Sep 3, 2019
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Zombie reaping not occurring as expected
4 participants