Fix process locking and state management#2773
Fix process locking and state management#2773crosbymichael merged 1 commit intocontainerd:masterfrom
Conversation
|
This was still an implicit issue in 1.x branches, however, it recovered because of the |
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>
c02f6c4 to
831a41b
Compare
Codecov Report
@@ Coverage Diff @@
## master #2773 +/- ##
==========================================
- Coverage 43.72% 40.89% -2.83%
==========================================
Files 100 69 -31
Lines 10734 9388 -1346
==========================================
- Hits 4693 3839 -854
+ Misses 5311 4992 -319
+ Partials 730 557 -173
Continue to review full report at Codecov.
|
| if err := s.p.delete(ctx); err != nil { | ||
| return err | ||
| } | ||
| s.p.mu.Lock() |
There was a problem hiding this comment.
Just point out that the lock location is changed in this PR. The lock was moved once in #2757.
But I think the move in this PR is fine, because grabbing process lock only potentially blocks operations to the process. As long as it doesn't block the whole shim, it is fine.
Actually the other fix in #2757 is more important.
There was a problem hiding this comment.
ok, let me think about it some
There was a problem hiding this comment.
I think the change made by this PR is fine. Not grabbing shim lock is more important. :)
|
LGTM |
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
dockerand the following command:This two issues that this fixes are:
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