Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Containerd 1.0 client #34895

Merged
merged 3 commits into from Oct 23, 2017
Merged

Containerd 1.0 client #34895

merged 3 commits into from Oct 23, 2017

Conversation

@mlaventure
Copy link
Contributor

mlaventure commented Sep 19, 2017

- 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

@mlaventure mlaventure force-pushed the mlaventure:containerd-1.0-client branch from 667a411 to cd4a2d0 Sep 19, 2017
@mlaventure mlaventure force-pushed the mlaventure:containerd-1.0-client branch from cd4a2d0 to ff82e26 Sep 19, 2017
libcontainerd.WithOOMScore(cli.Config.OOMScoreAdjust),
libcontainerd.WithPlugin("linux", &linux.Config{
Shim: fmt.Sprintf("%s", daemon.DefaultShimBinary),

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 19, 2017 Member

Maybe planned clean-up, but extra Sprintf


// DefaultRuntimeName is the default runtime to be used by
// containerd if none is specified
DefaultRuntimeName = "docker-runc"

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 19, 2017 Member

How differs from DefaultRuntimeBinary?

This comment has been minimized.

@mlaventure

mlaventure Oct 3, 2017 Author Contributor

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

@@ -56,6 +51,21 @@ func (daemon *Daemon) FillPlatformInfo(v *types.Info, sysInfo *sysinfo.SysInfo)
v.RuncCommit.ID = "N/A"
}

v.ContainerdCommit.Expected = dockerversion.ContainerdCommitID
if rv, err := exec.Command("docker-containerd", "--version").Output(); err == nil {

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 19, 2017 Member

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

This comment has been minimized.

@mlaventure

mlaventure Sep 20, 2017 Author Contributor

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

This comment has been minimized.

@@ -1448,7 +1449,7 @@ func (s *DockerDaemonSuite) TestCleanupMountsAfterDaemonAndContainerKill(c *chec
c.Assert(strings.Contains(string(mountOut), id), check.Equals, true, comment)

// kill the container
icmd.RunCommand(ctrBinary, "--address", "unix:///var/run/docker/libcontainerd/docker-containerd.sock", "containers", "kill", id).Assert(c, icmd.Success)
icmd.RunCommand(ctrBinary, "--address", "/var/run/docker/containerd/docker-containerd.sock", "--namespace", "docker", "tasks", "kill", id).Assert(c, icmd.Success)

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 19, 2017 Member

namespace: docker -> moby? moby-core?

This comment has been minimized.

@tiborvass

tiborvass Sep 19, 2017 Collaborator

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

This comment has been minimized.

@mlaventure

mlaventure Sep 20, 2017 Author Contributor

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

defer func() {
if err != nil {
err = errors.Wrap(err, "Failed to connect to containerd. "+
"Please make sure containerd is installed in your PATH or that you have specified the correct address.")

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Sep 19, 2017 Member

containerd -> value of binaryName?

This comment has been minimized.

@mlaventure

mlaventure Sep 20, 2017 Author Contributor

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

@mlaventure mlaventure force-pushed the mlaventure:containerd-1.0-client branch from ff82e26 to 177235a Sep 19, 2017
@@ -58,6 +59,9 @@ func Trap(cleanup func(), logger interface {
logger.Info("Forcing docker daemon shutdown without cleanup; 3 interrupts received")
}
case syscall.SIGQUIT:
logger.Info("Kill USR1 docker-containerd")
exec.Command("pkill", "-USR1", "docker-c").Run()
<-time.After(500 * time.Millisecond)

This comment has been minimized.

@dnephin

dnephin Sep 20, 2017 Member

Leftover debug code?

This comment has been minimized.

@mlaventure

mlaventure Sep 20, 2017 Author Contributor

Not really, in general we want both together

This comment has been minimized.

@dnephin

dnephin Sep 20, 2017 Member

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.

This comment has been minimized.

@mlaventure

mlaventure Sep 20, 2017 Author Contributor

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

@mlaventure
Copy link
Contributor Author

mlaventure commented Sep 20, 2017

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)

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

yongtang commented Oct 20, 2017

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

@cpuguy83
Copy link
Contributor

cpuguy83 commented Oct 20, 2017

@allencloud
Copy link
Contributor

allencloud commented Oct 21, 2017

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
Copy link
Contributor Author

mlaventure commented Oct 21, 2017

@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.

@cpuguy83 cpuguy83 merged commit 4025407 into moby:master Oct 23, 2017
6 checks passed
6 checks passed
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37459 has succeeded
Details
janky Jenkins build Docker-PRs 46150 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6542 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17731 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6344 has succeeded
Details
@crosbymichael
Copy link
Member

crosbymichael commented Oct 23, 2017

LGTM

@thaJeztah
Copy link
Member

thaJeztah commented Oct 24, 2017

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
Copy link
Contributor

tophj-ibm commented Oct 24, 2017

@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
Copy link
Member

dnephin commented Oct 24, 2017

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
Copy link
Member

thaJeztah commented Oct 24, 2017

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented on integration-cli/docker_cli_events_test.go in ddae20c Dec 20, 2017

This change actually masks that the test is defunct; 17 container * 7 events = 119, so the limit of 256 events is never reached.

This comment has been minimized.

Copy link
Contributor Author

mlaventure replied Dec 20, 2017

I see that the string is incorrect, not sure what you mean by "defunct"?

This comment has been minimized.

Copy link
Member

thaJeztah replied Dec 20, 2017

@mlaventure The test tests that the number of events is limited to 256 events, but we only check if 119 events are fired, so basically this is now only testing that we receive all the events that were triggered.

This test was quite problematic anyway, and turns out it was already tested elsewhere, so opened a PR to remove it (see #35844 for a fun walk through history)

This comment has been minimized.

Copy link
Contributor Author

mlaventure replied Dec 20, 2017

The test tests that the number of events is equal to numContainers*eventPerContainer I had to fix the test because the number of events changed with 1.0, I just forgot to update the error message :). Is the test still flaky? I never got it to fail after I updated it.

This comment has been minimized.

Copy link
Member

thaJeztah replied Dec 20, 2017

No, the test should be testing that events are limited to 256; https://github.com/moby/moby/pull/6918/files

This comment has been minimized.

Copy link
Member

thaJeztah replied Dec 20, 2017

(that's the original PR, at which time it was 64)

This comment has been minimized.

Copy link
Contributor Author

mlaventure replied Dec 20, 2017

Well, in that case it should have been checking for LessOrEqual. Now it tests that we have exactly the right number of events, but since it is apparently tested already somewhere, RIP :)

This comment has been minimized.

Copy link
Member

thaJeztah replied Dec 20, 2017

Yes, that test was a big PITA for a long time; I looked into that test a while back to look what it was actually doing, then found the history (but never got round to actually removing it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can鈥檛 perform that action at this time.