runtime/opts: move WithNamespaceCgroupDeletion from containerd to its own package by tiborvass · Pull Request #3564 · containerd/containerd

tiborvass · 2019-08-20T22:10:09Z

The cgroup dependency brings in quite a lot only for WithNamespaceCgroupDeletion, which is a namespaces.DeleteOpt.

Signed-off-by: Tibor Vass tibor@docker.com

tiborvass · 2019-08-20T22:12:22Z

FYI this came up when vendoring containerd in buildkit: moby/buildkit#1107 (comment)

namespaces/opts_linux.go

tiborvass · 2019-08-20T22:50:06Z

I'm not sure why TestImagesCreateUpdateDelete is failing on https://ci.appveyor.com/project/mlaventure/containerd-3g73f/builds/26840706 cc @mlaventure

theopenlab-ci · 2019-08-20T23:05:25Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 7m 52s (non-voting)

codecov-io · 2019-08-21T04:01:09Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
- Coverage    42.3%   37.26%   -5.04%     
==========================================
  Files         126       84      -42     
  Lines       13869    11554    -2315     
==========================================
- Hits         5867     4306    -1561     
+ Misses       7116     6648     -468     
+ Partials      886      600     -286

Flag	Coverage Δ
#linux	`?`
#windows	`37.26% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
archive/tar_opts.go	`11.76% <0%> (-47.06%)`	⬇️
cio/io.go	`1.4% <0%> (-45.08%)`	⬇️
snapshots/native/native.go	`1.79% <0%> (-41.26%)`	⬇️
archive/tar.go	`19.18% <0%> (-28.78%)`	⬇️
metadata/snapshot.go	`23.86% <0%> (-24.05%)`	⬇️
content/local/writer.go	`57.69% <0%> (-0.97%)`	⬇️
gc/scheduler/scheduler.go	`66.34% <0%> (-0.97%)`	⬇️
oci/spec_opts.go	`28.96% <0%> (-0.24%)`	⬇️
mount/temp_unix.go
sys/reaper_linux.go
... and 40 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 640860a...527eb99. Read the comment docs.

codecov-io · 2019-08-21T04:01:09Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
- Coverage    42.3%   37.26%   -5.04%     
==========================================
  Files         126       84      -42     
  Lines       13869    11554    -2315     
==========================================
- Hits         5867     4306    -1561     
+ Misses       7116     6648     -468     
+ Partials      886      600     -286

Flag	Coverage Δ
#linux	`?`
#windows	`37.26% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
archive/tar_opts.go	`11.76% <0%> (-47.06%)`	⬇️
cio/io.go	`1.4% <0%> (-45.08%)`	⬇️
snapshots/native/native.go	`1.79% <0%> (-41.26%)`	⬇️
archive/tar.go	`19.18% <0%> (-28.78%)`	⬇️
metadata/snapshot.go	`23.86% <0%> (-24.05%)`	⬇️
content/local/writer.go	`57.69% <0%> (-0.97%)`	⬇️
gc/scheduler/scheduler.go	`66.34% <0%> (-0.97%)`	⬇️
oci/spec_opts.go	`28.96% <0%> (-0.24%)`	⬇️
mount/temp_unix.go
sys/reaper_linux.go
... and 40 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 640860a...527eb99. Read the comment docs.

codecov-io · 2019-08-21T04:01:09Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
- Coverage    42.3%   37.26%   -5.04%     
==========================================
  Files         126       84      -42     
  Lines       13869    11554    -2315     
==========================================
- Hits         5867     4306    -1561     
+ Misses       7116     6648     -468     
+ Partials      886      600     -286

Flag	Coverage Δ
#linux	`?`
#windows	`37.26% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
archive/tar_opts.go	`11.76% <0%> (-47.06%)`	⬇️
cio/io.go	`1.4% <0%> (-45.08%)`	⬇️
snapshots/native/native.go	`1.79% <0%> (-41.26%)`	⬇️
archive/tar.go	`19.18% <0%> (-28.78%)`	⬇️
metadata/snapshot.go	`23.86% <0%> (-24.05%)`	⬇️
content/local/writer.go	`57.69% <0%> (-0.97%)`	⬇️
gc/scheduler/scheduler.go	`66.34% <0%> (-0.97%)`	⬇️
oci/spec_opts.go	`28.96% <0%> (-0.24%)`	⬇️
mount/temp_unix.go
sys/reaper_linux.go
... and 40 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 640860a...527eb99. Read the comment docs.

codecov-io · 2019-08-21T04:01:09Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
- Coverage    42.3%   37.26%   -5.04%     
==========================================
  Files         126       84      -42     
  Lines       13869    11554    -2315     
==========================================
- Hits         5867     4306    -1561     
+ Misses       7116     6648     -468     
+ Partials      886      600     -286

Flag	Coverage Δ
#linux	`?`
#windows	`37.26% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
archive/tar_opts.go	`11.76% <0%> (-47.06%)`	⬇️
cio/io.go	`1.4% <0%> (-45.08%)`	⬇️
snapshots/native/native.go	`1.79% <0%> (-41.26%)`	⬇️
archive/tar.go	`19.18% <0%> (-28.78%)`	⬇️
metadata/snapshot.go	`23.86% <0%> (-24.05%)`	⬇️
content/local/writer.go	`57.69% <0%> (-0.97%)`	⬇️
gc/scheduler/scheduler.go	`66.34% <0%> (-0.97%)`	⬇️
oci/spec_opts.go	`28.96% <0%> (-0.24%)`	⬇️
mount/temp_unix.go
sys/reaper_linux.go
... and 40 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 640860a...527eb99. Read the comment docs.

codecov-io · 2019-08-21T04:01:09Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #3564   +/-   ##
=======================================
  Coverage   42.26%   42.26%           
=======================================
  Files         126      126           
  Lines       13881    13881           
=======================================
  Hits         5867     5867           
  Misses       7128     7128           
  Partials      886      886

Flag	Coverage Δ
#linux	`45.75% <ø> (ø)`	⬆️
#windows	`37.23% <ø> (ø)`	⬆️

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 3096178...6624a70. Read the comment docs.

codecov-io · 2019-08-21T04:01:09Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3564      +/-   ##
==========================================
- Coverage    42.3%   37.26%   -5.04%     
==========================================
  Files         126       84      -42     
  Lines       13869    11554    -2315     
==========================================
- Hits         5867     4306    -1561     
+ Misses       7116     6648     -468     
+ Partials      886      600     -286

Flag	Coverage Δ
#linux	`?`
#windows	`37.26% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
archive/tar_opts.go	`11.76% <0%> (-47.06%)`	⬇️
cio/io.go	`1.4% <0%> (-45.08%)`	⬇️
snapshots/native/native.go	`1.79% <0%> (-41.26%)`	⬇️
archive/tar.go	`19.18% <0%> (-28.78%)`	⬇️
metadata/snapshot.go	`23.86% <0%> (-24.05%)`	⬇️
content/local/writer.go	`57.69% <0%> (-0.97%)`	⬇️
gc/scheduler/scheduler.go	`66.34% <0%> (-0.97%)`	⬇️
oci/spec_opts.go	`28.96% <0%> (-0.24%)`	⬇️
mount/temp_unix.go
sys/reaper_linux.go
... and 40 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 640860a...527eb99. Read the comment docs.

estesp

LGTM

namespaces/opts_linux.go

dmcgowan · 2019-08-21T19:02:01Z

The cgroup dependency brings in quite a lot only for WithNamespaceCgroupDeletion, which is a namespaces.DeleteOpt.

Are you sure this solves the dependency issue? It is moving it to an interface defining package still.

tiborvass · 2019-08-27T01:38:34Z

@dmcgowan

Are you sure this solves the dependency issue?

When I tested it it removed 17047 lines of vendoring.

It is moving it to an interface defining package still

I agree that's not great. So what do you suggest? namespaces/nscgroups ? namespaces/nsutil ?

tiborvass · 2019-08-27T01:40:59Z

@dmcgowan updated PTAL

theopenlab-ci · 2019-08-27T01:53:57Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 47s (non-voting)

crosbymichael · 2019-08-27T14:01:54Z

@tiborvass I think this should probably go in runtime/opts

tiborvass · 2019-08-27T18:41:14Z

Thanks @crosbymichael I updated accordingly.

runtime/opts/opts_linux.go

… own package The cgroup dependency brings in quite a lot only for WithNamespaceCgroupDeletion, which is a namespaces.DeleteOpt. Signed-off-by: Tibor Vass <tibor@docker.com>

crosbymichael · 2019-08-27T19:38:16Z

LGTM

theopenlab-ci · 2019-08-27T20:10:10Z

Build succeeded.

containerd-build-arm64 : NODE_FAILURE in 0s (non-voting)

dmcgowan

LGTM

tiborvass force-pushed the move-cgroups-dep-to-namespaces-pkg branch from e22c227 to 527eb99 Compare August 20, 2019 22:11

tiborvass mentioned this pull request Aug 20, 2019

vendor: update containerd to 1.3.0-beta moby/buildkit#1107

Merged

thaJeztah reviewed Aug 20, 2019

View reviewed changes

namespaces/opts_linux.go Outdated Show resolved Hide resolved

estesp approved these changes Aug 21, 2019

View reviewed changes

dmcgowan requested changes Aug 21, 2019

View reviewed changes

namespaces/opts_linux.go Outdated Show resolved Hide resolved

tiborvass force-pushed the move-cgroups-dep-to-namespaces-pkg branch from 527eb99 to faadd59 Compare August 27, 2019 01:40

tiborvass force-pushed the move-cgroups-dep-to-namespaces-pkg branch from faadd59 to 949c572 Compare August 27, 2019 18:40

tiborvass changed the title ~~namespaces: move WithNamespaceCgroupDeletion from containerd to namespaces package~~ runtime/opts: move WithNamespaceCgroupDeletion from containerd to its own package Aug 27, 2019

crosbymichael reviewed Aug 27, 2019

View reviewed changes

runtime/opts/opts_linux.go Outdated Show resolved Hide resolved

runtime/opts: move WithNamespaceCgroupDeletion from containerd to its…

6624a70

… own package The cgroup dependency brings in quite a lot only for WithNamespaceCgroupDeletion, which is a namespaces.DeleteOpt. Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the move-cgroups-dep-to-namespaces-pkg branch from 949c572 to 6624a70 Compare August 27, 2019 19:03

dmcgowan approved these changes Sep 3, 2019

View reviewed changes

dmcgowan merged commit b039c39 into containerd:master Sep 3, 2019

Conversation

tiborvass commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiborvass commented Aug 20, 2019

Uh oh!

Uh oh!

tiborvass commented Aug 20, 2019

Uh oh!

theopenlab-ci bot commented Aug 20, 2019

Uh oh!

codecov-io commented Aug 21, 2019

Codecov Report

Uh oh!

codecov-io commented Aug 21, 2019

Codecov Report

Uh oh!

codecov-io commented Aug 21, 2019

Codecov Report

Uh oh!

codecov-io commented Aug 21, 2019

Codecov Report

Uh oh!

codecov-io commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-io commented Aug 21, 2019

Codecov Report

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dmcgowan commented Aug 21, 2019

Uh oh!

tiborvass commented Aug 27, 2019

Uh oh!

tiborvass commented Aug 27, 2019

Uh oh!

theopenlab-ci bot commented Aug 27, 2019

Uh oh!

crosbymichael commented Aug 27, 2019

Uh oh!

tiborvass commented Aug 27, 2019

Uh oh!

Uh oh!

crosbymichael commented Aug 27, 2019

Uh oh!

theopenlab-ci bot commented Aug 27, 2019

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tiborvass commented Aug 20, 2019 •

edited

Loading

codecov-io commented Aug 21, 2019 •

edited

Loading