Skip to content

Do not KillAll on task delete by default#2597

Merged
estesp merged 1 commit intocontainerd:masterfrom
masters-of-cats:kill-all-on-host-ns
Aug 30, 2018
Merged

Do not KillAll on task delete by default#2597
estesp merged 1 commit intocontainerd:masterfrom
masters-of-cats:kill-all-on-host-ns

Conversation

@ostenbom
Copy link
Copy Markdown
Contributor

@ostenbom ostenbom commented Aug 30, 2018 •
edited by crosbymichael
Loading

  • Still KillAll if the task uses the hosts pid namespace
  • Tests for both host pid namespace and normal cases

See 2258 for more context.

Closes #2558

Co-authored-by: Oliver Stenbom [email protected]
Co-authored-by: Georgi Sabev [email protected]

@ostenbom ostenbom force-pushed the kill-all-on-host-ns branch from af03cda to d38e9fa Compare August 30, 2018 14:30
@ostenbom @georgethebeatle
Do not kill all on task delete by default
b5ccc66 
 - Still KillAll if the task uses the hosts pid namespace
 - Test for both host pid namespace and normal cases

Co-authored-by: Oliver Stenbom <[email protected]>
Co-authored-by: Georgi Sabev <[email protected]>
Signed-off-by: Oliver Stenbom <[email protected]>
@ostenbom ostenbom force-pushed the kill-all-on-host-ns branch from d38e9fa to b5ccc66 Compare August 30, 2018 15:03
@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 30, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2597      +/-   ##
==========================================
- Coverage   44.05%   40.97%   -3.09%     
==========================================
  Files          98       69      -29     
  Lines       10130     8350    -1780     
==========================================
- Hits         4463     3421    -1042     
+ Misses       4945     4385     -560     
+ Partials      722      544     -178
Flag Coverage Δ
#linux ?
#windows 40.97% <ø> (ø) ⬆️
Impacted Files Coverage Δ
snapshots/native/native.go 1.78% <0%> (-41.52%) ⬇️
archive/tar.go 16.99% <0%> (-26.15%) ⬇️
metadata/snapshot.go 21.53% <0%> (-24.28%) ⬇️
oci/spec_opts.go 60.24% <0%> (-4.82%) ⬇️
content/local/writer.go 51.57% <0%> (-1.06%) ⬇️
gc/scheduler/scheduler.go 66.34% <0%> (-0.97%) ⬇️
mount/temp_unix.go
sys/reaper_linux.go
services/server/server_linux.go
sys/env.go
... and 25 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 0649e38...b5ccc66. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Aug 30, 2018

LGTM

estesp
estesp approved these changes Aug 30, 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 7e66292 into containerd:master Aug 30, 2018
@estesp
Copy link
Copy Markdown
Member

estesp commented Aug 30, 2018 •
edited
Loading

@crosbymichael do we want to cherry pick this to 1.1.x (at least the v1 shim implementation)?

@ostenbom ostenbom mentioned this pull request Sep 14, 2018
Closed
@thaJeztah thaJeztah mentioned this pull request Sep 19, 2018
Merged
@cpuguy83 cpuguy83 mentioned this pull request Nov 1, 2018
Closed
@jodh-intel jodh-intel mentioned this pull request Nov 22, 2018
Closed
This was referenced Dec 3, 2018
Closed
Merged
@danail-branekov danail-branekov mentioned this pull request May 7, 2019
Merged
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ostenbom @codecov-io @crosbymichael @estesp