Containerd 1.0 client by mlaventure · Pull Request #34895 · moby/moby

mlaventure · 2017-09-19T18:14:15Z

- What I did

Replaced containerd v0.2.x with v1.0

- How I did it

Refactored libcontainerd

- How to verify it

CI must be green

- Description for the changelog

Move to containerd v1.0

- A picture of a cute animal (not mandatory but encouraged)

🐼

--

What's not finished

Checkpoint backward compatibility (there seem to be no test in CI)
Some cleanup is needed. I've kept the code that is supposed to make it work on windows via containerd around for now, would probably be better to remove it and redo it later.

Closes #34662

AkihiroSuda · 2017-09-19T19:00:35Z

cmd/dockerd/daemon_unix.go

Maybe planned clean-up, but extra Sprintf

AkihiroSuda · 2017-09-19T19:02:27Z

daemon/daemon_unix.go

How differs from DefaultRuntimeBinary?

This is the name used for the --runtime option map

AkihiroSuda · 2017-09-19T19:05:23Z

daemon/info_unix.go

Can we let docker-containerd --version print JSON?

That's a request for https://github.com/containerd/containerd ;-)

opened containerd/containerd#1561

AkihiroSuda · 2017-09-19T19:14:24Z

integration-cli/docker_cli_daemon_test.go

namespace: docker -> moby? moby-core?

@mlaventure maybe put "docker" in a variable so we can change it easily.

Will do, will call the namespace moby too while at it.

AkihiroSuda · 2017-09-19T19:18:17Z

libcontainerd/remote_daemon.go

containerd -> value of binaryName?

If using an already running containerd, that value might be wrong

dnephin · 2017-09-20T15:33:33Z

pkg/signal/trap.go

Leftover debug code?

Not really, in general we want both together

This seems like the wrong place to implement this logic. pkg/signal shouldn't really know about the binary names.

I think Trap() needs to be refactored to accept a function to call when it receives SIGQUIT.

Oh, completely forgot it was inside pkg/trap. Will remove it for now.

mlaventure · 2017-09-20T22:19:12Z

unable to remove filesystem for ce4f7a31758fb7bd47c91f6210a2e768c4dfe32876109e4df87d6d7f47622f6d: remove /var/lib/docker/containers/ce4f7a31758fb7bd47c91f6210a2e768c4dfe32876109e4df87d6d7f47622f6d/shm: device or resource busy

Those errors are fixed when upgrading the kernel pass 3.13, could we upgrade CI to something a bit more recent? The lts kernel in trusty is 4.4.0 now (cc @andrewhsu)

mlaventure · 2017-10-19T17:14:02Z

all 💚 😇

cpuguy83

LGTM

I am worried about the upgrade case as the new shim protocol is completely different, and runtime state is also in a completely different location there doesn't appear to be a way to successfully upgrade without killing all running containers, which is problematic for live-restore.

Right now it looks like we basically just end up abandoning the pre-1.0 shims and the container processes end up blocking on I/O once the kernel buffer limit is hit.

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

mlaventure · 2017-10-19T20:36:08Z

@cpuguy83 it's effectively the best solution:

stop all the containers
stop the daemon
update
restart the containers

Maybe the install script could take care of this, it should at least be clearly stated in the release note when it comes to that.

AkihiroSuda · 2017-10-20T09:04:20Z

Should we merge this asap or wait for 1.0 GA?

vdemeester

LGTM 🦁 but same question as @AkihiroSuda

vdemeester · 2017-10-20T13:23:35Z

daemon/daemon.go

vdemeester · 2017-10-20T13:25:57Z

daemon/daemon.go

why this comment ?

While doing the modification so I could remember what was the context 😅. I can remove it.

cpuguy83 · 2017-10-20T13:56:27Z

@AkihiroSuda Would like to get it in ASAP (i.e. today). Since this is only using the runtime portion the risk is low except for the incompatibility with the previous version.

cpuguy83

I'm ok that an upgrade breaks people who have live-restore on the previous version, while this has generally worked in the past it really should never be a guarantee.... people consuming moby will need to make sure to handle this properly.

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

yongtang · 2017-10-20T15:36:11Z

It seems that there are enough LGTMs. Move it to status/4-merge. Let's see if all Jenkins builds pass.

cpuguy83 · 2017-10-20T15:59:57Z

ping @crosbymichael

allencloud · 2017-10-21T00:43:04Z

I think there are several breaking back compatibility.
For example, containerd 2.3.x allows user to choose one runtime when running, such as runc, runv and so on.
But containerd 1.0.0 does not allow this.
Do we need to record this kind of thing is some documentation?

mlaventure · 2017-10-21T01:05:14Z

@allencloud changing the runtime binary is still supported by containerd 1.0

You will notice that I did not remove any test, and setting the runtime binary is part of them. I've replicated all of the previous features, there should be no regressions normally. If there is, it means we're lacking tests for it.

crosbymichael · 2017-10-23T15:01:52Z

LGTM

thaJeztah · 2017-10-24T18:26:40Z

Wait; why did Janky show it passed?

16:31:46 Step 1 : FROM golang:1.8.4-alpine3.6 as builder
16:31:46 Error parsing reference: "golang:1.8.4-alpine3.6 as builder" is not a valid repository/tag
16:31:46 Set build name.
16:31:46 New build name is '#34895'
16:31:46 Variable with name 'BUILD_

tophj-ibm · 2017-10-24T18:39:04Z

@thaJeztah from the jenkins-job config, not sure why this is the case:
docker build --build-arg DOCKER_GITCOMMIT=$GITCOMMIT -t docker-e2e-test:$GITCOMMIT -f Dockerfile.e2e . || true
https://jenkins.dockerproject.org/job/Docker-PRs/configure

dnephin · 2017-10-24T18:46:01Z

The Dockerfile.e2e is an image that is used to test docker-ce/docker-ee in other builds. We'd like to test it here as an artifact of the `moby/moby build.

|| true was added because this image uses multi-stage builds, and not all jenkins node run a Docker version that supports multi-stage builds, which is the error you pasted.

thaJeztah · 2017-10-24T18:46:37Z

Oh, possibly because those tests are only used in the internal e2e suite, yes. I got confused, was looking why the vendor check didn't fail on this PR (https://github.com/moby/moby/pull/35283/files#r146650223)

edit: see @dnephin's comment, our comments just crossed

mlaventure requested review from cpuguy83, dnephin, tianon, tonistiigi and vdemeester as code owners September 19, 2017 18:14

GordonTheTurtle added the status/0-triage label Sep 19, 2017

mlaventure added the rebuild/arm label Sep 19, 2017

GordonTheTurtle removed the rebuild/arm label Sep 19, 2017

mlaventure force-pushed the containerd-1.0-client branch from 667a411 to cd4a2d0 Compare September 19, 2017 18:20

mlaventure added the rebuild/arm label Sep 19, 2017

GordonTheTurtle removed the rebuild/arm label Sep 19, 2017

mlaventure force-pushed the containerd-1.0-client branch from cd4a2d0 to ff82e26 Compare September 19, 2017 18:35

AkihiroSuda reviewed Sep 19, 2017

View reviewed changes

cmd/dockerd/daemon_unix.go Outdated

Copy link
Copy Markdown

Member

AkihiroSuda Sep 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe planned clean-up, but extra Sprintf

AkihiroSuda reviewed Sep 19, 2017

View reviewed changes

mlaventure force-pushed the containerd-1.0-client branch from ff82e26 to 177235a Compare September 19, 2017 22:09

dnephin reviewed Sep 20, 2017

View reviewed changes

mlaventure force-pushed the containerd-1.0-client branch 4 times, most recently from a4b39a9 to 263d181 Compare September 20, 2017 17:30

mlaventure added rebuild/arm labels Sep 20, 2017

GordonTheTurtle removed rebuild/arm labels Sep 20, 2017

cpuguy83 reviewed Oct 19, 2017

View reviewed changes

Vendor containerd 1.0

7acea2a

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

vdemeester reviewed Oct 20, 2017

View reviewed changes

cpuguy83 approved these changes Oct 20, 2017

View reviewed changes

mlaventure added 2 commits October 20, 2017 07:11

Update libcontainerd to use containerd 1.0

ddae20c

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

Fix tests creating zombie processes

617f89b

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>

cpuguy83 mentioned this pull request Oct 20, 2017

Increase container default shutdown timeout on Windows #35184

Merged

This was referenced Oct 26, 2017

dockerd panic when running LCOW container on 1709 #35302

Closed

Fixes LCOW after containerd 1.0 introduced regressions #35320

Merged

thaJeztah mentioned this pull request Oct 31, 2017

containerd binaries no longer static #35349

Closed

allencloud mentioned this pull request Nov 9, 2017

[bug report] /images/json API isn't compatible with Moby AliyunContainerService/pouch#76

Closed

thaJeztah mentioned this pull request Nov 9, 2017

Bump runc to b2567b37d7b75eb4cf325b77297b140ea686ce8f #35439

Closed

lowenna mentioned this pull request Nov 28, 2017

Windows: Fix width/height swap #35631

Merged

thaJeztah mentioned this pull request Dec 20, 2017

Remove TestEventsLimit(), and minor cleanups #35844

Merged

thaJeztah mentioned this pull request Nov 6, 2018

fixes add-runtime error when path or args contain some special chars #37978

Open

thaJeztah mentioned this pull request Apr 2, 2019

Docker stats without CPU precentage calculation #28881

Closed

thaJeztah mentioned this pull request Sep 3, 2025

chore: libcontainerd/remote: remove uses of wrapErr if possible #50882

Closed

Conversation

mlaventure commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure commented Sep 20, 2017

Uh oh!

mlaventure commented Oct 19, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

mlaventure commented Oct 19, 2017

Uh oh!

AkihiroSuda commented Oct 20, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Oct 20, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

yongtang commented Oct 20, 2017

Uh oh!

cpuguy83 commented Oct 20, 2017

Uh oh!

allencloud commented Oct 21, 2017

Uh oh!

mlaventure commented Oct 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crosbymichael commented Oct 23, 2017

Uh oh!

thaJeztah commented Oct 24, 2017

Uh oh!

tophj-ibm commented Oct 24, 2017

Uh oh!

dnephin commented Oct 24, 2017

Uh oh!

thaJeztah commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

mlaventure commented Oct 21, 2017 •

edited

Loading

thaJeztah commented Oct 24, 2017 •

edited

Loading