Skip to content

bugfix: optimize shim lock in runtime v1 avoid dead lock#2743

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
Ace-Tang:shim_lock
Oct 29, 2018
Merged

bugfix: optimize shim lock in runtime v1 avoid dead lock#2743
crosbymichael merged 1 commit intocontainerd:masterfrom
Ace-Tang:shim_lock

Conversation

@Ace-Tang
Copy link
Contributor

@Ace-Tang Ace-Tang commented Oct 24, 2018

apply lock only around process map of shim service, avoid lock affect
other procs operations.

Signed-off-by: Ace-Tang aceapril@126.com

should also fix #2739 .

untitled diagram

@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #2743 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2743   +/-   ##
=======================================
  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 c444666...c206da7. Read the comment docs.

Copy link
Member

@fuweid fuweid Oct 24, 2018

Choose a reason for hiding this comment

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

init is only used by moby I guess.

Copy link
Member

Choose a reason for hiding this comment

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

You could do this as an empty string, however I think it might just make sense to have another function with a different signature, like getInitProcess()

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"init" string is I want to distinguish s.id and r.ID, consider that r.ID also can be empty. I agree
with to make getInitProcess() to do this, since this is also my first considered plan. It not make confused with init.

apply lock only around process map of shim service, avoid lock affect
other procs operations.

Signed-off-by: Ace-Tang <aceapril@126.com>
@crosbymichael crosbymichael added this to the 1.2.1 milestone Oct 26, 2018
@mlaventure
Copy link
Contributor

Just curious, have you run benchmark on this, does it leads to some time improvement?

@Ace-Tang
Copy link
Contributor Author

@mlaventure , when multiple exec runs, it will cause dead lock, the detail I describe at top and in issue #2709. benchmark not done.

@Ace-Tang Ace-Tang changed the title optimize shim lock in runtime v1 bugfix: optimize shim lock in runtime v1 avoid dead lock Oct 29, 2018
@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 6f8edc2 into containerd:master Oct 29, 2018
@Ace-Tang Ace-Tang deleted the shim_lock branch October 30, 2018 02:13
estesp referenced this pull request Nov 6, 2018
[release/1.1] cherry-pick: optimize shim lock in runtime v1
Ace-Tang added a commit to Ace-Tang/containerd that referenced this pull request May 8, 2019
origin issue is containerd#2709, detail dead lock information is in containerd#2743.
This is long time issue, I wonder if the patch this can fix instead
of workaround it

Signed-off-by: Ace-Tang <aceapril@126.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.

ctr shim not support runtime v1 shim

8 participants