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

fix: lock the whole markStoppendAndRelease to be automic #1504

Closed

Conversation

allencloud
Copy link
Collaborator

@allencloud allencloud commented Jun 11, 2018

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

Ⅰ. Describe what this PR did

And we found that in the test case of restart, here we have a scenario that when a have ran a container, we have no idea about when the container exits. If the container exits immediately, then container would trigger the hook in pouchd, and pouchd would execute the function markStoppendAndRelease. However, if the execution time is durationA, if in this duration, pouchd accepts another request to stop or start the container, some unexpected situation would happen.

markStoppendAndRelease would excute after exit of container and release the container IO and release the container endpoint or related network items. And we must guarantee the atomic of markStoppendAndRelease.

Ⅱ. Does this pull request fix one issue?

fix #1470

Ⅲ. Describe how you did it

none

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Jun 11, 2018
@allencloud allencloud force-pushed the fix-1470 branch 3 times, most recently from 79e72ba to 9a53ca8 Compare June 13, 2018 04:06
@codecov-io
Copy link

codecov-io commented Jun 13, 2018

Codecov Report

Merging #1504 into master will increase coverage by 22.57%.
The diff coverage is 56.41%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1504       +/-   ##
===========================================
+ Coverage   16.88%   39.45%   +22.57%     
===========================================
  Files         211      254       +43     
  Lines       14020    17097     +3077     
===========================================
+ Hits         2367     6746     +4379     
+ Misses      11490     9482     -2008     
- Partials      163      869      +706
Impacted Files Coverage Δ
daemon/mgr/container_state.go 77.46% <100%> (+77.46%) ⬆️
daemon/mgr/container.go 37.53% <45.16%> (+37.53%) ⬆️
client/container_logs.go 0% <0%> (-76.93%) ⬇️
pkg/utils/utils.go 57.51% <0%> (-20.85%) ⬇️
cli/logs.go 0% <0%> (ø) ⬆️
cri/stream/portforward/httpstream.go 0% <0%> (ø)
cri/stream/remotecommand/websocket.go 0% <0%> (ø)
cri/stream/remotecommand/httpstream.go 0% <0%> (ø)
cri/stream/httpstream/spdy/upgrade.go 0% <0%> (ø)
registry/auth.go 54.12% <0%> (ø)
... and 117 more

@allencloud allencloud force-pushed the fix-1470 branch 2 times, most recently from 072ac03 to b7b7643 Compare June 13, 2018 14:14
@pouchrobot pouchrobot added size/M and removed size/S labels Jun 13, 2018
@allencloud allencloud force-pushed the fix-1470 branch 6 times, most recently from 2943875 to b13dc79 Compare June 13, 2018 14:57
Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@pouchrobot
Copy link
Collaborator

ping @allencloud
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

@allencloud allencloud closed this Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict/needs-rebase kind/bug This is bug report for project size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] upgrade/restart container failed on AliOS4.9
3 participants