optimize shim lock in exec to avoid exec hang by lifubang · Pull Request #2820 · containerd/containerd

lifubang · 2018-11-21T03:28:08Z

Exec shim lock optimize before may cause two exec operations can enter to process.Exec area with the same exec id. (With small probability)
Please see:

containerd/runtime/v1/shim/service.go

Lines 247 to 276 in c206da7

    
           func (s *Service) Exec(ctx context.Context, r *shimapi.ExecProcessRequest) (*ptypes.Empty, error) { 
        
           	s.mu.Lock() 
        
           	if p := s.processes[r.ID]; p != nil { 
        
           		s.mu.Unlock() 
        
           		return nil, errdefs.ToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ID) 
        
           	} 
        
           	p := s.processes[s.id] 
        
           	s.mu.Unlock() 
        
           	if p == nil { 
        
           		return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created") 
        
           	} 
        
           	process, err := p.(*proc.Init).Exec(ctx, s.config.Path, &proc.ExecConfig{ 
        
           		ID:       r.ID, 
        
           		Terminal: r.Terminal, 
        
           		Stdin:    r.Stdin, 
        
           		Stdout:   r.Stdout, 
        
           		Stderr:   r.Stderr, 
        
           		Spec:     r.Spec, 
        
           	}) 
        
           	if err != nil { 
        
           		return nil, errdefs.ToGRPC(err) 
        
           	} 
        
           	s.mu.Lock() 
        
           	s.processes[r.ID] = process 
        
           	s.mu.Unlock() 
        
           	return empty, nil 
        
           }

They can both enter here:

containerd/runtime/v1/shim/service.go

Line 261 in c206da7

process, err := p.(*proc.Init).Exec(ctx, s.config.Path, &proc.ExecConfig{

Because the exec id has not been added to s.processes:

containerd/runtime/v1/shim/service.go

Line 273 in c206da7

s.processes[r.ID] = process

This will cause one of exec hang when exit:
This one can exit succ:

root@dockerdemo:/opt/runctest/redis# ctr --address /run/docker/containerd/containerd.sock t exec --exec-id test -t redis1 sh
/data # ls /
bin    dev    home   media  proc   run    srv    tmp    var
data   etc    lib    mnt    root   sbin   sys    usr
/data # root@dockerdemo:/opt/runctest/redis#

This one can't exit:

root@dockerdemo:~# ctr --address /run/docker/containerd/containerd.sock t exec --exec-id test -t redis1 sh
/data # ls /
bin    dev    home   media  proc   run    srv    tmp    var
data   etc    lib    mnt    root   sbin   sys    usr
/data # 
^C

So, I think this lock should be for the whole func, and just one exec can run succ, the other one should return an error.

But this is not important for docker, because each exec id in docker are different.
If maintainers feel it's not really important because of some other factors, this can be ignored. Because it's really a very small probability.

codecov-io · 2018-11-21T03:44:55Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #2820   +/-   ##
=======================================
  Coverage   43.79%   43.79%           
=======================================
  Files         100      100           
  Lines       10741    10741           
=======================================
  Hits         4704     4704           
  Misses       5307     5307           
  Partials      730      730

Flag	Coverage Δ
#linux	`47.45% <ø> (ø)`	⬆️
#windows	`40.96% <ø> (ø)`	⬆️

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 32aa0cd...2fe5c8d. Read the comment docs.

lifubang · 2018-11-21T04:11:57Z

~~Or just add a pre value to s.processes[r.ID] after exec id check before s.mu.Unlock() in Line 256?~~

containerd/runtime/v1/shim/service.go

Lines 248 to 256 in c206da7

    
           s.mu.Lock() 
        
           if p := s.processes[r.ID]; p != nil { 
        
           	s.mu.Unlock() 
        
           	return nil, errdefs.ToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ID) 
        
           } 
        
           p := s.processes[s.id] 
        
           s.mu.Unlock()

For example:
s.processes[r.ID]=&execProcess{}
And delete it if Exec error.

lifubang · 2018-11-21T04:21:15Z

Or: for avoid global shim lock, I think we may just add a new lock(such as: s.execmu) just for Exec func, because it's different from other func.

Random-Liu · 2018-11-21T18:32:24Z

If we are confident that Exec is reliable, and will never hang, we can lock the whole function;
If not, I prefer we fix it in another way, e.g. an id reservation table.

Ace-Tang · 2018-11-22T09:35:17Z

Does same exec id should be consider in the upper level, like moby? I am not sure

lifubang · 2018-11-22T09:44:20Z

Does same exec id should be consider in the upper level, like moby? I am not sure

Yes, moby have consider it, each exec id is different from each other.
But if the user just use lib containerd, it will cause errors.

ehazlett · 2019-01-29T18:15:34Z

@lifubang this needs a rebase please. thanks!

Signed-off-by: Lifubang <[email protected]>

lifubang · 2019-02-13T07:09:34Z

@lifubang this needs a rebase please. thanks!

have done, thanks.

crosbymichael · 2019-06-21T18:42:18Z

I've been looking through this code recently, I don't think we want to hold the lock during execution but we should maybe "reserve" the id at the entrypoint of the Exec. I'm going to try updating this for this functionality.

ref containerd#2820 Signed-off-by: Michael Crosby <[email protected]>

lifubang added 2 commits February 13, 2019 14:45

optimize exec shim lock for runtime v1

d075329

Signed-off-by: Lifubang <[email protected]>

optimize exec shim lock for runtime v2

0511bdd

lifubang force-pushed the execshimlog branch from 2fe5c8d to 0511bdd Compare February 13, 2019 06:48

crosbymichael closed this Jun 21, 2019

crosbymichael added a commit to crosbymichael/containerd that referenced this pull request Jun 21, 2019

Reserve exec id to prevent race

1a8df3f

ref containerd#2820 Signed-off-by: Michael Crosby <[email protected]>

crosbymichael mentioned this pull request Jun 21, 2019

Reserve exec id to prevent race #3365

Merged

tussennet pushed a commit to tussennet/containerd that referenced this pull request Sep 11, 2020

Reserve exec id to prevent race

be386c1

ref containerd#2820 Signed-off-by: Michael Crosby <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize shim lock in exec to avoid exec hang#2820

optimize shim lock in exec to avoid exec hang#2820
lifubang wants to merge 2 commits intocontainerd:masterfrom
lifubang:execshimlog

lifubang commented Nov 21, 2018 •

edited

Loading

Uh oh!

codecov-io commented Nov 21, 2018

Uh oh!

lifubang commented Nov 21, 2018 •

edited

Loading

Uh oh!

lifubang commented Nov 21, 2018 •

edited

Loading

Uh oh!

Random-Liu commented Nov 21, 2018 •

edited

Loading

Uh oh!

Ace-Tang commented Nov 22, 2018 •

edited

Loading

Uh oh!

lifubang commented Nov 22, 2018 •

edited

Loading

Uh oh!

ehazlett commented Jan 29, 2019

Uh oh!

lifubang commented Feb 13, 2019

Uh oh!

crosbymichael commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	func (s Service) Exec(ctx context.Context, r shimapi.ExecProcessRequest) (*ptypes.Empty, error) {
	s.mu.Lock()

	if p := s.processes[r.ID]; p != nil {
	s.mu.Unlock()
	return nil, errdefs.ToGRPCf(errdefs.ErrAlreadyExists, "id %s", r.ID)
	}

	p := s.processes[s.id]
	s.mu.Unlock()
	if p == nil {
	return nil, errdefs.ToGRPCf(errdefs.ErrFailedPrecondition, "container must be created")
	}

	process, err := p.(*proc.Init).Exec(ctx, s.config.Path, &proc.ExecConfig{
	ID: r.ID,
	Terminal: r.Terminal,
	Stdin: r.Stdin,
	Stdout: r.Stdout,
	Stderr: r.Stderr,
	Spec: r.Spec,
	})
	if err != nil {
	return nil, errdefs.ToGRPC(err)
	}
	s.mu.Lock()
	s.processes[r.ID] = process
	s.mu.Unlock()
	return empty, nil
	}

Conversation

lifubang commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 21, 2018

Codecov Report

Uh oh!

lifubang commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Random-Liu commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ace-Tang commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifubang commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehazlett commented Jan 29, 2019

Uh oh!

lifubang commented Feb 13, 2019

Uh oh!

crosbymichael commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lifubang commented Nov 21, 2018 •

edited

Loading

lifubang commented Nov 21, 2018 •

edited

Loading

lifubang commented Nov 21, 2018 •

edited

Loading

Random-Liu commented Nov 21, 2018 •

edited

Loading

Ace-Tang commented Nov 22, 2018 •

edited

Loading

lifubang commented Nov 22, 2018 •

edited

Loading