implement direct containerd Executor. #1965
Conversation
| @@ -0,0 +1,107 @@ | |||
| package containerd | |||
There was a problem hiding this comment.
With this we now have github.com/docker/swarmkit/agent/exec/container and github.com/docker/swarmkit/agent/exec/containerd which differ in a single character suffix. Perhaps we should consider renaming the former to github.com/docker/swarmkit/agent/exec/dockerengine (or .../dockerapi or whatever suites)?
WRT my comment in the PR text "perhaps be considering a new spec type specifically for containerd" maybe it is also worth considering renaming the ContainerSpec gRPC type too?
There was a problem hiding this comment.
Perhaps we should consider renaming the former to
github.com/docker/swarmkit/agent/exec/dockerengine(or.../dockerapior whatever suites)?
I agree. I find the current naming confusing. I think either dockerengine or dockerapi is better. I guess I have a slight preference for dockerapi.
There was a problem hiding this comment.
Let's call the swarmkit/agent/exec/container one dockerapi.
Would you mind doing that in a separate PR?
There was a problem hiding this comment.
I wouldn't mind at all...
We should consider whether ContainerSpec should become something more dockerapiish too.
|
My thinking on the priority order of the missing bits is (decreasing order or priority):
Not strictly a missing bit but I'd insert:
|
Codecov Report
@@ Coverage Diff @@
## master #1965 +/- ##
==========================================
- Coverage 60.4% 60.38% -0.03%
==========================================
Files 124 124
Lines 20149 20149
==========================================
- Hits 12171 12166 -5
- Misses 6620 6624 +4
- Partials 1358 1359 +1 |
agent/exec/containerd/adapter.go
Outdated
| return err | ||
| } | ||
|
|
||
| // No idea if this is correct... |
There was a problem hiding this comment.
Looks right. Seems kind of unfortunate for swarmkit to know anything about manifests though - this seems like something that belongs at a different layer of the stack. Is the plan to move this into containerd once the relevant APIs exist?
There was a problem hiding this comment.
This should all go away. I'm working on this as we speak.
There was a problem hiding this comment.
Yep, I hope to ditch all this code in favour of something @stevvooe produces soon!
agent/exec/containerd/executor.go
Outdated
| }, | ||
| )) | ||
|
|
||
| conn, err := grpc.Dial(fmt.Sprintf("unix://%s", bindSocket), dialOpts...) |
There was a problem hiding this comment.
nit: "unix://" + bindSocket? No need for Sprintf.
I am working on this right now. The content store will move over to GRPC and be contained in containerd. I am still toying with the right operations and level granularity required, but I want it to be straightforward. |
cmd/dist-pull/dist-pull
Outdated
| ;; | ||
| esac | ||
|
|
||
| exit 0 |
There was a problem hiding this comment.
You might be able to hack up a rootfs by running each layer through dist apply.
vendor.conf
Outdated
| @@ -23,6 +23,13 @@ github.com/docker/libnetwork 3ab699ea36573d98f481d233c30c742ade737565 | |||
| github.com/opencontainers/runc 8e8d01d38d7b4fb0a35bf89b72bc3e18c98882d7 | |||
| github.com/opencontainers/go-digest a6d0ee40d4207ea02364bd3b9e8e77b9159ba1eb | |||
|
|
|||
| github.com/docker/containerd 1dc5d652ac1adf8f0a92ee8eff7af07b129d4e21 | |||
| github.com/docker/libtrust 9cbd2a1374f46905c68a4eb3694a130610adc62a | |||
| github.com/gorilla/context v1.1 | |||
There was a problem hiding this comment.
They seem to be brought in as a side effect of importing distribution packages to parse manifests. I'd prefer if we could move manifest-related functionality out of swarmkit.
There was a problem hiding this comment.
github.com/docker/containerd was needed for the gRPC stuff. I think @aaronlehmann is correct that most of the rest came in due to the need for manifest stuff from distribution. @stevvooe is that something which the content store functionality you are adding will address or shall I look for alternatives?
As an aside, figuring out the container config for a v1 schema (not the manifest itself, but the bit with Cwd, Entrypoint, Cmd etc in it) was basically guess work and reverse engineering, if anyone has an actual reference (to docs, or even better usable library code) I'd be glad to have them...
agent/exec/containerd/executor.go
Outdated
|
|
||
| var _ exec.Executor = &executor{} | ||
|
|
||
| // Lifted from github.com/docker/containerd/cmd/ctr/utils.go |
There was a problem hiding this comment.
I wrote this last night, as well. Maybe, we need to provide some easy client making tools there.
There was a problem hiding this comment.
Yes, that would be great IMHO.
| } | ||
| } | ||
|
|
||
| if err := r.adapter.create(ctx); err != nil { |
There was a problem hiding this comment.
This adapter pattern is proving useful. I wonder if we can pull out an abstraction for implementing executors that support ContainerSpec.
There was a problem hiding this comment.
IMHO it'd be interesting/useful to try, assuming we want to continue down the route of mashing ContainerSpec in to containerd as opposed to adding ContainerdSpec (gah, naming).
agent/exec/containerd/adapter.go
Outdated
|
|
||
| bundle := filepath.Join(bundleDir, task.ID) | ||
|
|
||
| log.G(ctx).Debugf("newContainerAdapter: ID=%s SVC=%s", task.ID, task.ServiceID) |
There was a problem hiding this comment.
You can use log.G(ctx).WithFields to achieve these keyvalue pairs.
There was a problem hiding this comment.
I think I can just kill this debug and do the WithFields thing elsewhere when there is actual logging to do.
agent/exec/containerd/adapter.go
Outdated
| return err | ||
| } | ||
|
|
||
| applyCmd := osexec.CommandContext(ctx, "dist", "apply", rootfs) |
|
👍 |
|
Force push:
|
|
I'm wondering if this should end up out-of-tree in some kind of @stevvooe @aluzzardi WDYT? |
|
@aaronlehmann yeah, it's something I've been wondering too. Do you mean to move the whole of |
I want to understand the architecture better. For example, how should networking be implemented? |
|
@dongluochen the three things you quote (networking, secrets, volumes) are for the most part open questions right now and I don't have a concrete answer for any of them. Any thoughts/advice/feedback anyone has would be useful. For networking I've looked a bit at CNI (via https://github.com/ehazlett/circuit/) and CNM (via https://github.com/docker/libnetwork/), but no real conclusion. For volumes I haven't really looked yet (I had a quick play with creating local volumes in the For secrets I haven't looked in any detail and have no answer at all. |
Thanks @ijc25! @mavenugo any idea how networking should be supported with direct containerd integration? |
|
Force push:
|
|
Force push:
|
|
@ijc Check out containerd/containerd#904 to see if that will help here. The intent of that PR is to make things less hacky. Should we merge this and have another PR that uses the new client package? |
|
@stevvooe Yes, I expect containerd/containerd#904 to help a great deal. I'm OK to merge this and port over to that in a future PR if the maintainers are. The two outstanding review comments here are:
We've already deferred #1965 (review) (healthcheck) with a TODO. |
|
Force pushed an update:
I think I need to spend some time tidying up the usage of the snapshotters etc and the new metadata service, it's a bit organic rather than using as they are designed I think. IMHO these needn't block merging. @stevvooe said he had a nearly complete event filtering package, which I'll hopefully be able to use to address #1965 (review). Maybe that could also be done in a followup. Lastly, "wire up generic resources" was just wiring the supplied |
agent/exec/containerd/controller.go
Outdated
| return err | ||
| } | ||
|
|
||
| log.G(ctx).Infof("Wait") |
There was a problem hiding this comment.
Might be a bit overly verbose?
There was a problem hiding this comment.
Yes, it should either be more informative (i.e. say what it is waiting for) or be removed. I think I'll err on the side of removing it.
There was a problem hiding this comment.
FTR it was more informative, since ctx carries various things including the task id and includes them in the log messages. I've removed it anyway though.
agent/exec/containerd/controller.go
Outdated
| } | ||
| case <-closed: | ||
| // restart! | ||
| eventq, closed, err = r.adapter.events(ctx) |
There was a problem hiding this comment.
Is this the only function that calls r.adapter.events?
Since it unconditionally reopens the Events stream here, I wonder if the retry logic in r.adapter.events is actually necessary or even desirable.
I must have missed this before.
There was a problem hiding this comment.
Well spotted, I'd forgotten all about this code when I added the retry to the r.adapter.events code.
What's odd is that when I simulated an event stream failure (by restarting containerd) the error did bubble up to failing the task rather than restarting here. I'll need to investigate why before I kill the retry code.
There was a problem hiding this comment.
My theory is that the first call to c.taskClient.Events in r.adapter.events didn't have the grpc.FailFast(false), so it would go through this outer retry and them immediately fail because containerd hadn't actually restarted yet.
I think adding the FailFast unconditionally would be ok and allow me to remove the retry loop in r.adapter.events. I'll give that a shot.
There was a problem hiding this comment.
It seemed to work. I took the opportunity to add a check that we hadn't missed an exit event while the stream was away too.
agent/exec/containerd/adapter.go
Outdated
| log.G(ctx).Debug("Mounts are:") | ||
| for i, m := range spec.Mounts { | ||
| log.G(ctx).Debugf(" %2d: %+v", i, m) | ||
| } |
There was a problem hiding this comment.
Non-blocking: I prefer to emit this as a single log line. Otherwise it might be difficult to parse if it's interleaved with other log lines from concurrent threads.
There was a problem hiding this comment.
This set of messages is pretty verbose (with very long lines) and not all that useful now that I have it working, so I think I'll just nuke it.
|
LGTM If this is merged in its current state, please open an issue about event filtering. I'd like to make sure that gets addressed. |
Calls down to containerd via gRPC to create and start containers. Signed-off-by: Ian Campbell <ian.campbell@docker.com>
|
I rebased and squashed the fixups again. I have filed #2219 for the event filtering issue. I don't seem to be able to assign, please someone who can feel free to assign it to me. |
and update some dependent packages. We would like to keep moby/moby and swarmkit somewhat in sync here and moby/swarmkit#2229 proposes a similar bump to swarmkit, needed due to moby/swarmkit#1965 which pulls in containerd which uses some newer features of the grpc package. Signed-off-by: Ian Campbell <ian.campbell@docker.com>
|
How does integration tests work with swarm? Is there a way to test the tests with this integration outside of moby? |
|
We have some swarmkit-level integration tests in the |
|
I was just thinking about automated testing too (OK, I'm a bit late on that one, I confess!) I'll create a new issue. |
and update some dependent packages. We would like to keep moby/moby and swarmkit somewhat in sync here and moby/swarmkit#2229 proposes a similar bump to swarmkit, needed due to moby/swarmkit#1965 which pulls in containerd which uses some newer features of the grpc package. Signed-off-by: Ian Campbell <ian.campbell@docker.com> Upstream-commit: 379557a Component: engine
and update some dependent packages. We would like to keep moby/moby and swarmkit somewhat in sync here and moby/swarmkit#2229 proposes a similar bump to swarmkit, needed due to moby/swarmkit#1965 which pulls in containerd which uses some newer features of the grpc package. Signed-off-by: Ian Campbell <ian.campbell@docker.com> Upstream-commit: 379557a Component: engine
This is a very early implementation of an executor which allows standalone
swarmdto talk directly to containerd via gRPC. Posting this now to get feedback on the direction as well as the priorities wrt the missing bits (see below)It is activated by passing
--containerd-addr=/run/containerd/containerd.socktoswarmd. The contents of bothswarmkit/binandcontainerd/binmust be in$PATH.Note that currently various things expect paths under the swarmkit state dir to be absolute paths, so
--state-dir=$(pwd)/swarmkitstateor similar is a good idea (since the default is the non-absolute./swarmkitstate).Example:
then:
This uses the existing
ContainerSpecand chunks of this new code are pretty similar to the existing dockerapi executor (I have also left in some commented out code which I think will eventually be similar once the underlying scaffolding is implemented). There is likely to be opportunities for refactoring or code sharing. Alternatively (or also) we should perhaps be considering a new spec type specifically for containerd (vs docker engine).There are many and varied holes and missing pieces of implementation (some of which I have papered over in order to get a basic executor working in the expectation of eventually rewriting to something proper, others I have punted on completely). Including (but not limited to and in no particular order):
images are mostly unimplemented, current implementation is a quick hack which shells out to @stevvooe's containerdNow uses the containerd provided content store interfaces to build rootfs. Pull is still via a script for now.disttool for pull and rootfs constructions (the former is laundered via adist-pullhelper script). AIUI it will eventually (soon?) be possible to ask containerd to take care of this via gRPC. For now the content store is placed in«swarmkitstate»/content-storeand image manifests are cached in amanifestssubdirectory of that.Lifecycle tasks other thanDoneCreate()+Start()(e.g.Shutdown(),Terminate()&Remove()) are unimplemented (containerd API doesn't have the same granularity?).Wait()has a simple implementation but is hampered by lack of events (I don't seem to get any fromctr events, haven't actually tried the gRPC API) in containerd at the moment.Lastly, this code cribs bits of functionality from a variety of places (e.g.
"github.com/docker/containerd/cmd/ctr/utils".prepareStdio()&getGRPCConnection,"github.com/docker/docker/oci".DefaultSpec(), types fromgithub.com/docker/docker/imageetc) which mostly ought to be refactored rather than just cut and pasted.This has been written/tested against docker/containerd#1dc5d652ac1adf8f0a92ee8eff7af07b129d4e21. Bundles are placed as subdirectories underNow based on«swarmkitstate»/bundles. I'm aware of @crosbymichael's work in docker/containerd#526 and will look into porting over next.docker/containerd#61400c57ae1d599626b9a5f28b98ac9074b8f1a9docker/containerd#a7ef3e5313e9731274f60e14d396dab4729b562f