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.1] cherry-pick: modify lock location of exec delete avoid exec hang #2757

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Nov 2, 2018

Backport #2624 to release/1.1.

For issues like #2438.

Exec deletion can take forever if the exec leaks processes in the container. This will cause:

  1. Process leakage in the system;
  2. Goroutine leakage in containerd-shim and cri;
  3. containerd-shim hangs forever.

This doesn't fix 1) and 2), but it does fix 3).

In any case, containerd-shim shouldn't hang, so that users can at least stop the bad container.

@lbernail @Ace-Tang

func (e *execProcess) delete(ctx context.Context) error {
    e.wg.Wait()
...
}
delete exec process will wait for io copy finish, if wait here,
other process can not get lock of shim service.

1. apply lock around s.transition() calls in the Delete methods.
2. put lock after wait io copy in exec Delete.

Signed-off-by: Ace-Tang <aceapril@126.com>
@codecov-io
Copy link

Codecov Report

Merging #2757 into release/1.1 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/1.1    #2757   +/-   ##
============================================
  Coverage        48.99%   48.99%           
============================================
  Files               85       85           
  Lines             7603     7603           
============================================
  Hits              3725     3725           
  Misses            3203     3203           
  Partials           675      675
Flag Coverage Δ
#linux 48.99% <ø> (ø) ⬆️

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 c018c6e...deeaac9. 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

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 68a2cbc into containerd:release/1.1 Nov 2, 2018
@Random-Liu Random-Liu deleted the cherry-pick-#2624 branch November 2, 2018 17:45
@Random-Liu
Copy link
Member Author

For #2709

@Random-Liu Random-Liu changed the title [release/1.1] Backport: modify lock location of exec delete avoid exec hang [release/1.1] cherry-pick: modify lock location of exec delete avoid exec hang Nov 9, 2018
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.

None yet

5 participants