Codecov Report

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

@@           Coverage Diff           @@
##           master     #103   +/-   ##
=======================================
  Coverage   45.42%   45.42%           
=======================================
  Files          23       23           
  Lines        1638     1638           
=======================================
  Hits          744      744           
  Misses        768      768           
  Partials      126      126

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 5fbad35...27e64bc. Read the comment docs.

reviewing

Looks good. How do you like this approach? Do you think we should move forward with this implementation?

I think this is nice, clean, and simple.

@fuweid PTAL

Looks good. How do you like this approach? Do you think we should move forward with this implementation?

Let's try to move forward this and see if it work.

I have still a concern how we can support systemd, but I think the systemd interface can be revisited later, because CRI and Moby don't seem using it for v1.

For CRI and Moby, it seems we only need cgroupfs-based metrics functions: #104

Will v%d directory name cause some go mod issue? 😕
https://blog.golang.org/using-go-modules

Will v%d directory name cause some go mod issue? 😕
https://blog.golang.org/using-go-modules

I think we can add new go.mod to support v2 version.

We have already several "v1" and "v2" pkgs in the main repo, probably we can keep this as-is?

Do we want to create a v2 branch that we can work out of for this?

As the package is separate out, unlikely to cause regression for the existing v1 code.
Probably no need to create a branch?

addressed comments, but adding go.mod to v2 seems difficult

~/gopath/src/github.com/containerd/cgroups/v2$ go mod tidy                                                                  
go: finding github.com/opencontainers/runtime-spec v1.0.1                                                                                  
go: finding github.com/containerd/cgroups latest                                                                                           
go: downloading github.com/opencontainers/runtime-spec v1.0.1                                                                              
go: downloading github.com/containerd/cgroups v0.0.0-20191011165608-5fbad35c2a7e                                                           
go: extracting github.com/containerd/cgroups v0.0.0-20191011165608-5fbad35c2a7e                                                            
go: extracting github.com/opencontainers/runtime-spec v1.0.1                                                                               
github.com/containerd/cgroups/v2 imports
        github.com/containerd/cgroups/stats/v2: module github.com/containerd/cgroups@latest found (v0.0.0-20191011165608-5fbad35c2a7e), but
 does not contain package github.com/containerd/cgroups/stats/v2

just keep it simple and don't mess with go.mod if we don't have to. getting the functionality in is what is important

can we merge this as-is?

@AkihiroSuda how about moving stats/v2 into v2/stats? Yeah, the projects in whole org doesn't fully support go module. We do some work to prepare to migrate to go module. If we want stats out of cgroup controller functionality, we need 3 go.mod files in the future. One is for root dir of repo for v1, second one is for the stats API module, the last one is for v2 version. It seems not necessary. IMO, it is more clear to put stats/v2 into v2/stats.

And according to go.mod in v1 cgroup, it looks weird that we don't support go.mod for v2.

Moved stats/v2 to v2/stats.

Adding go.mod should be separate issue/PR.

LGTM