Skip to content

enhance: update v1/v2 runtime#2769

Merged
estesp merged 1 commit intocontainerd:masterfrom
fuweid:shim_related_change
Nov 7, 2018
Merged

enhance: update v1/v2 runtime#2769
estesp merged 1 commit intocontainerd:masterfrom
fuweid:shim_related_change

Conversation

@fuweid
Copy link
Copy Markdown
Member

@fuweid fuweid commented Nov 6, 2018

  1. avoid dead lock during kill, fetch allProcesses before handle events
  2. use argu's ctx instead of context.Backgroud() in openlog

Signed-off-by: Wei Fu [email protected]

@fuweid
enhance: update v1/v2 runtime
38d7d59 
1. avoid dead lock during kill, fetch allProcesses before handle events
2. use argu's ctx instead of context.Backgroud() in openlog

Signed-off-by: Wei Fu <[email protected]>
@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 6, 2018

Codecov Report

Merging #2769 into master will decrease coverage by 6.49%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2769     +/-   ##
=========================================
- Coverage   47.39%   40.89%   -6.5%     
=========================================
  Files          91       69     -22     
  Lines        8415     9388    +973     
=========================================
- Hits         3988     3839    -149     
- Misses       3704     4992   +1288     
+ Partials      723      557    -166
Flag Coverage Δ
#linux ?
#windows 40.89% <ø> (?)
Impacted Files Coverage Δ
snapshots/native/native.go 1.78% <0%> (-51.52%) ⬇️
metadata/snapshot.go 21.53% <0%> (-33.23%) ⬇️
archive/tar.go 16.99% <0%> (-33.01%) ⬇️
content/local/writer.go 56.86% <0%> (-7.34%) ⬇️
metadata/containers.go 47.97% <0%> (-6.62%) ⬇️
content/local/store.go 48.51% <0%> (-5.03%) ⬇️
remotes/docker/resolver.go 58.36% <0%> (-4.99%) ⬇️
archive/tar_opts.go 28.57% <0%> (-4.77%) ⬇️
archive/compression/compression.go 58.69% <0%> (-4.7%) ⬇️
metadata/images.go 58.46% <0%> (-4.7%) ⬇️
... and 86 more

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 8dcabd6...38d7d59. Read the comment docs.

Ace-Tang
Ace-Tang reviewed Nov 7, 2018
View reviewed changes
Comment thread runtime/v1/shim/service.go
}

for _, p := range s.processes {
for _, p := range s.allProcesses() {
Copy link
Copy Markdown
Contributor

@Ace-Tang Ace-Tang Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is the different between use s.allProcesses() and s.process

Copy link
Copy Markdown
Member Author

@fuweid fuweid Nov 7, 2018

Choose a reason for hiding this comment

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

we don't want to be hang in locking state so we retrieve processes before handling the runc exit event.

@Ace-Tang
Copy link
Copy Markdown
Contributor

Ace-Tang commented Nov 7, 2018 •
edited
Loading

By the way, @crosbymichael , does runtime v1 will be abandoned , and use runtime v2 instead in project roadmap ? since I see

 // NoPivot disables pivot-root (linux only), required when running a container in a RamDisk with runc
    // This only works for runtime type "io.containerd.runtime.v1.linux".
    // DEPRECATED: use Runtime.Options instead. Remove when shim v1 is deprecated.
    NoPivot bool `toml:"no_pivot" json:"noPivot"`

in cri/pkg/config/config.go

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Nov 7, 2018

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Nov 7, 2018

@Ace-Tang we are going to give it a couple of releases for users to switch over to v2 runtime before removing v1.

estesp
estesp approved these changes Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
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

@estesp estesp merged commit 130d07e into containerd:master Nov 7, 2018
@Random-Liu Random-Liu mentioned this pull request Nov 9, 2018
Merged
crosbymichael referenced this pull request Nov 12, 2018
@crosbymichael
Merge pull request #2774 from Random-Liu/cherrypick-#2769-release-1.1
900f0d4 
[release/1.1] cherry-pick: enhance: update v1/v2 runtime
@estesp estesp mentioned this pull request Nov 15, 2018
Merged
@fuweid fuweid deleted the shim_related_change branch February 20, 2019 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

+1 more reviewer

@Ace-Tang Ace-Tang Ace-Tang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fuweid @codecov-io @Ace-Tang @crosbymichael @estesp @Random-Liu