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

[release/1.2] Fix process locking and state management #2803

Merged
merged 1 commit into from
Nov 20, 2018

Conversation

crosbymichael
Copy link
Member

There were races with the way process states. This displayed in ways,
especially around pausing the container for atomic operations. Users
would get errors like, cannnot delete container in paused state and
such.

This can be eaisly reproduced with docker and the following command:

> (for i in `seq 1 25`; do id=$(docker create  alpine usleep 50000);docker start $id;docker commit $id;docker wait $id;docker rm $id; done)

This two issues that this fixes are:

  • locks must be held by the owning process, not the state operations.
  • If a container ends up being paused but before the operation
    completes, the process exists, make sure we resume the container before
    setting the the process as exited.

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

There were races with the way process states.  This displayed in ways,
especially around pausing the container for atomic operations.  Users
would get errors like, cannnot delete container in paused state and
such.

This can be eaisly reproduced with `docker` and the following command:

```bash
> (for i in `seq 1 25`; do id=$(docker create  alpine usleep 50000);docker start $id;docker commit $id;docker wait $id;docker rm $id; done)
```

This two issues that this fixes are:

* locks must be held by the owning process, not the state operations.
* If a container ends up being paused but before the operation
completes, the process exists, make sure we resume the container before
setting the the process as exited.

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

Codecov Report

Merging #2803 into release/1.2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/1.2    #2803   +/-   ##
============================================
  Coverage        43.74%   43.74%           
============================================
  Files              100      100           
  Lines            10728    10728           
============================================
  Hits              4693     4693           
  Misses            5305     5305           
  Partials           730      730
Flag Coverage Δ
#linux 47.41% <ø> (ø) ⬆️
#windows 40.9% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 040e73f...4c72bef. Read the comment docs.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

backport of #2773

@dmcgowan
Copy link
Member

LGTM

@dmcgowan dmcgowan merged commit 9be591e into containerd:release/1.2 Nov 20, 2018
Random-Liu added a commit to Random-Liu/gvisor-containerd-shim that referenced this pull request Jan 28, 2019
Signed-off-by: Lantao Liu <lantaol@google.com>
Random-Liu added a commit to Random-Liu/gvisor-containerd-shim that referenced this pull request Jan 30, 2019
Signed-off-by: Lantao Liu <lantaol@google.com>
Random-Liu added a commit to google/gvisor-containerd-shim that referenced this pull request Jan 30, 2019
* Update containerd to 1.2.2

Signed-off-by: Lantao Liu <lantaol@google.com>

* Port containerd/containerd#2803.

Signed-off-by: Lantao Liu <lantaol@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants