Codecov Report

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

@@           Coverage Diff           @@
##           master    #1615   +/-   ##
=======================================
  Coverage   45.29%   45.29%           
=======================================
  Files          95       95           
  Lines        9461     9461           
=======================================
  Hits         4285     4285           
  Misses       4464     4464           
  Partials      712      712

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 424c0fb...40fffad. Read the comment docs.

I think this is fine to do. I'm not sure about the app package name, maybe cli? Anyone else have ideas how to handle this?

I feel that cli would make people think of a "client", maybe server ?

We already have server

daemon ?

I'd like to propose that this is a post-1.0 change. This makes a lot of changes for a use case we'd like to support but need to consider whether this is the right avenue of support.

Basically, I am worried that merging this will make it hard to do something else in the future.

I'm fine with postponing too, these changes are not really urgent and doesn't break our contract post 1.0 as it only covers the GRPC side.

Lets close this for now and revisit after 1.0

reopening for 1.1

related: #1934

rebased and addressed comments

🇩 check

gometalinter --config .gometalinter.json ./...

cmd/containerd/daemon/builtins.go:5:2:warning: a blank import should be only in a main or test package, or have a comment justifying it (golint)

cmd/containerd/daemon/builtins_btrfs_linux.go:5:8:warning: a blank import should be only in a main or test package, or have a comment justifying it (golint)

cmd/containerd/daemon/builtins_linux.go:4:2:warning: a blank import should be only in a main or test package, or have a comment justifying it (golint)

make: *** [check] Error 1

What should we do with this?

I think we need to solve this problem, of compiling a release with the added extra packages and repos. But I don't think this refactoring is the solution. We need to think about how we want to handle this, maybe build some tooling around this.

Lets think about what we want to accomplish first before changing code.

1. We want packages both in tree and out of tree
2. We want builds with out of tree packages built in
3. The person building shouldn't have to write Go code to include the out of tree repos
4. ???

Any other requirements?

What should we do with [the golint errors]?

Either add a comment explaining the blank import above each one, or add // nolint: golint above the import block.

@crosbymichael 👍

I also can't express myself fully in how much I don't want to have a package called "daemon". It is probably the most incorrect name in the world. :D

For this process to work, we need to be iterative in the following questions:

1. Can we include everything?
2. If answer to 1 is no, what do we exclude and how do we exclude it?

If we can keep the answer to 1 "yes", then we have a very small problem. An inclusive containerd will be a healthy one. With this approach, we can solve the problem of "how do I exclude package x?" which will effect a smaller subset of users.

Then, how do allow arbitrary plugin x? We should consider that on a case by case basis. If the plugin functionality can't be done with vanilla containerd, then we need to reconsider our design to support it. Most users should never have to deal with containerd plugin support. It is not friendly for users and allows vendors to split communities ("we don't ship x"). Making it easier to include or exclude plugins may have an unintended effect on the ecosystem.

The underlying problem that caused this PR is that we didn't ship zfs and aufs. I think shipping these is a better solution than refactoring into a daemon package (although, I'm open to an app package) or coming up with a half-broken plugin system.

1. Can we include everything?

I think we are all in agreement that we should just include these by default. We might not be in agreement in how we want to achieve this. In my opinion, having a separate app package is a good idea, I agree daemon is not an appropriate package name (neither is app 😄 ). I also like having the compile flags to remove the non-default drivers like we do for btrfs.

Can we include everything?
If answer to 1 is no, what do we exclude and how do we exclude it?

AUFS, ZFS (and CRI) can be included, but 3rd party plugins can't be included

Then, how do allow arbitrary plugin x? We should consider that on a case by case basis. If the plugin functionality can't be done with vanilla containerd, then we need to reconsider our design to support it. Most users should never have to deal with containerd plugin support. It is not friendly for users and allows vendors to split communities ("we don't ship x"). Making it easier to include or exclude plugins may have an unintended effect on the ecosystem.

What do you mean by containerd plugin support ? If you are just referring to the interface, I agree. For adding plugin to containerd itself, at one point users (which I see more as sysadmin/devops in containerd case) will eventually have to deal with plugins. We can't have all the plugin in the world in the main tree (especially if noone engage itself to maintain it/them), meaning having tooling to make it easier for third party to easily build their plugin against vanilla containerd (i.e. without having to touch at its source) will be beneficial. I don't think we can't prevent the split communities as there will always be some people/scenario where users decide to disable x or y. What we can do is provide a solid core and allow for users to add their own extensions easily. Will just have to be swift about closing issue that are triggered by 3rd party plugins 😇

Any thought to move this forward?

Another solution I can come up with is to create a tool like this, but I wonder it would be too much:

$ containerd-rebuild -o "./containerd-custom" --add "github.com/3rd/party/plugin" --remove "github.com/containerd/containerd/snapshots/overlay"
INFO: Executing `containerd --version`... 
             RepoURL="https://github.com/containerd/containerd", 
             Commit="deadbeefdeadbeefdeadbeef", 
             Go="go1.10", 
             BuiltinPlugins=["github.com/containerd/containerd/snapshots/overlay",         
                             "github.com/containerd/cri", ...]
INFO: Pulling image "docker.io/library/golang:1.10"
INFO: Checking out https://github.com/containerd/containerd@deadbeefdeadbeefdeadbeef 
INFO: Injecting plugin configuration to package "github.com/containerd/containerd/cmd/containerd"
INFO: Creating container "containerd-rebuild-tmp424242"
INFO: Building package "github.com/containerd/containerd/cmd/containerd" in container "containerd-rebuild-tmp424242"
INFO: Copying the build artifact from container "containerd-rebuild-tmp424242" to "./containerd-custom"
INFO: Successful. Please replace your containerd binary with "./containerd-custom".

@Random-Liu
Do you have a suggestion?

Also, it should be worth considering again to promote go build -buildmode=plugin, with some helper scripts.
It should be better for RPM/DPKG distribution, as no need to replace /usr/bin/containerd binary.

LGTM. This will make test in cri-containerd for plugin mode much simpler I think.

Any other requirements?

@crosbymichael Another requirement we have is for testing. After we make cri-containerd a containerd plugin, we can't test it without building with containerd. There are several possible solutions, but this PR is definitely a simple one.

Other possible solutions:

1. Keep standalone cri-containerd mode for testing. Maintaining standalone cri-containerd is an overhead, I prefer deprecating it in next release if we think CRI plugin is the way to go.
2. Automatically update containerd vendor code. This is what we are doing in cri-containerd for plugin mode testing today. It is hacky and ugly. :( (See Improve hack/update-vendor.sh and make install.deps. cri#561, Add initial version of plugin mode cri#527)
3. Merge CRI plugin in-tree. This may affect velocity, and add much noise in containerd repo. And I don't think we gonna merge all plugins in tree, right? We still have to fix this anyway.

Given above, I feel like this PR seems to be the best solution, unless we come up with another.

Also, it should be worth considering again to promote go build -buildmode=plugin, with some helper scripts.

@AkihiroSuda I tried the plugin mode. I don't think we can rely on it at all.

For example, if the plugin uses same packages with containerd, e.g. golang.org/x/net, all the init functions in those packages will run once when containerd starts, and another time when the plugin is loaded. Many init functions do have side effect, and shouldn't run more than once.

I think we are all in agreement that we should just include these by default. We might not be in agreement in how we want to achieve this. In my opinion, having a separate app package is a good idea, I agree daemon is not an appropriate package name (neither is app smile ). I also like having the compile flags to remove the non-default drivers like we do for btrfs.

@dmcgowan @stevvooe I agree. We may be able to vendor many different plugins into containerd, but I don't think we can merge all repos into one. That will be very hard to maintain.

As long as plugins are in separate repos, we need a way to test them.

It is possible to test them case by case, e.g. write specific integration test for ZFS plugin, test cri-containerd in standalone mode. However, I believe there will still be surprise when we update the vendors in containerd, because they are not actually tested with containerd.

An example is that: In Kubernetes, kubelet vendors cadvisor. Every release, we need to update cadvisor version a week before release. Although we do have cadvisor integration test in cadvisor repo. There is always surprise.

Vendoring aufs/zfs/cri SGTM, but note that it cannot be the replacement of this PR, as we should do our best to facilitate installation of 3rd party plugins. e.g. continuity/FILEgrain snapshotter/differ/content plugin, and maybe some kind of IPFS-based content plugin

I'll rebase this PR soon, but what should be the name of github.com/containerd/containerd/cmd/containerd/daemon pkg?

Candidates suggested so far:

• app (my initial PR)
• daemon (current PR)
• cli
• server

@AkihiroSuda I tried the plugin mode. I don't think we can rely on it at all.

For example, if the plugin uses same packages with containerd, e.g. golang.org/x/net, all the init functions in those packages will run once when containerd starts, and another time when the plugin is loaded. Many init functions do have side effect, and shouldn't run more than once.

Agreed, do you know whether the Go maintainers are working on this issue?

if the plugin uses same packages with containerd, e.g. golang.org/x/net, all the init functions in those packages will run once when containerd starts, and another time when the plugin is loaded.

https://golang.org/pkg/plugin/ says

When a plugin is first opened, the init functions of all packages not already part of the program are called.

I tried it out and from what I can see init() is only called once.

Are you sure init() can run twice?

what should be the name of github.com/containerd/containerd/cmd/containerd/daemon pkg?

based on what is being exported, I think cli, or command would be the most appropriate.

With this package structure, we can build fully vendorable and go-gettable custom daemon binary

Why do we want to support building from vendor? That seems very strange to me. Vendor is for "library dependencies" to "satisfy imports" (ref: https://blog.golang.org/go1.7). It is not designed as a source to build binaries.

If we only support building from a git checkout the current solution isn't so bad. We could make a few small changes to make it easier to build.

• a build tag to remove some or all default plugins (omit_default_plugins ?)
• add go:generate to run a script which reads a config file or some environment variable and generates a file that is in .gitignore. This would let someone easily add new plugins without writing any go code. So building a containerd with custom plugins would be just this:

cat > plugins.txt
go generate ./cmd/containerd
go build ./cmd/containerd

based on what is being exported, I think cli, or command would be the most appropriate.

ok, updated PR to use command

Why do we want to support building from vendor?

I meant we should allow 3rd parties to vendor containerd/cmd/command "library dependencies" and build its own binary.

It is not designed as a source to build binaries.

I didn't meant this, sorry for confusion.

Are you sure init() can run twice?

@dnephin Really? Last time when I tried that, one init function in golang.org/x/ tries to register an http handler, and reports error saying that the handler has been registered.

I remember it is this line https://github.com/golang/net/blob/master/trace/trace.go#L115-L116.

Yes, really 😄 It says so in the docs! and also my test confirmed the documented behaviour.

Maybe there was a bug in earlier versions of go? Or maybe there is some case I'm not ware of, but at least the expected case is to only run once.

tries to register an http handler, and reports error saying that the handler has been registered.

That doesn't necessarily mean the init() ran twice. It could mean that two different init() functions tried to register the same handler.

@dnephin I tried it.If the plugin project has its own vendor directory, the bug will be triggered. If all projects build with packages from GOPATH, it is fine.

I think golang treats packages in different vendor directories as different packages, which is really not good. :P

You can try (please vndr init in all projects):

$ cat main/main.go 
package main

import "plugin"
import _ "golang.org/x/net/trace"

func main() {
	if _, err := plugin.Open("plugin1.so"); err != nil {
		panic(err)
	}
	if _, err := plugin.Open("plugin2.so"); err != nil {
		panic(err)
	}
}

$ cat plugin1/plugin1.go 
package main

import "fmt"
import _ "golang.org/x/net/trace"

func init() {
	fmt.Println("plugin1")
}

$ cat plugin2/plugin2.go 
package main

import "fmt"
import _ "golang.org/x/net/trace"

func init() {
	fmt.Println("plugin2")
}

$ ./main 
panic: http: multiple registrations for /debug/requests

goroutine 1 [running]:
net/http.(*ServeMux).Handle(0xc746c0, 0x7fb0bd3f06dc, 0xf, 0xc460a0, 0x7fb0bd729c90)
	/usr/local/go/src/net/http/server.go:2270 +0x631
net/http.(*ServeMux).HandleFunc(0xc746c0, 0x7fb0bd3f06dc, 0xf, 0x7fb0bd729c90)
	/usr/local/go/src/net/http/server.go:2302 +0x57
net/http.HandleFunc(0x7fb0bd3f06dc, 0xf, 0x7fb0bd729c90)
	/usr/local/go/src/net/http/server.go:2314 +0x50
github.com/random-liu/plugin1/vendor/golang.org/x/net/trace.init.0()
	/home/lantaol/workspace/src/github.com/random-liu/plugin1/vendor/golang.org/x/net/trace/trace.go:115 +0x44
github.com/random-liu/plugin1/vendor/golang.org/x/net/trace.init()
	<autogenerated>:1 +0x1fa
github.com/random-liu/plugin1.init()
	<autogenerated>:1 +0x53
plugin.open(0x8ad96b, 0xa, 0x0, 0x0, 0x0)
	/usr/local/go/src/plugin/plugin_dlopen.go:136 +0x91a
plugin.Open(0x8ad96b, 0xa, 0x0, 0xc420059f70, 0x800fc9)
	/usr/local/go/src/plugin/plugin.go:31 +0x35
main.main()
	/home/lantaol/workspace/src/github.com/random-liu/main/main.go:7 +0x3a

I think golang treats packages in different vendor directories as different packages

Yes, that's exactly it! I believe all dependency management tools (vndr, dep, glide) should be removing vendor/ from dependencies in vendor/, so hopefully that isn't a real problem in most projects.

Let's see a proposed package dependency graph.

@stevvooe Also ref #2096 (comment). Yeah I think if plugin mode works, that might be the best solution for testing containerd plugins.

However, I'm not sure how much effort is needed to make plugin mode work. Let's think about it. It may work out. ;)

I think we need to split this PR up into separate steps. There is too much going on and its trying to solve too many things from the discussion but not delivering in the code.

The two things we want to solve are:

1. Better plugin registration/imports
2. Plugins adding CLI functionality

From the code I see in this PR, I think it is safe to solve (2) first. We can export the App and provide a way for plugins to register cli.Command that are later assembled in the main().

Then once that is done, we can start on the default registration/imports. These are two problems but we are not really solving any of the current state of discussion and code of this PR.

ok, opened #2131 for (2)

opened issue #2134 for (1)