running tests in a container by tophj-ibm · Pull Request #1957 · containerd/containerd

tophj-ibm · 2018-01-03T20:32:44Z

This provides a dockerfile for building an image to run the containerd tests.

Signed-off-by: Christopher Jones [email protected]

few WIP issues:

optimize getting protobuf, which currently takes ~25minutes to build from source, which is done because google only provides amd64 downloads
make integration fails at the end because it tries to unmount the test dir which is a mounted vol
make root-test fails ~~because it runs of out of loop devices, this is~~ because previous tests devices are erroring out and not being freed because of :

failed to make btrfs filesystem (out: "ERROR: '/dev/loop3' is too small to make a usable filesystem\nERROR: minimum size for each btrfs device is 142606336\nbtrfs-progs v4.7.3\nSee http://btrfs.wiki.kernel.org for more information.\n\n")

stevvooe · 2018-01-03T21:56:10Z

Is this necessary? I think we mostly expect a tmpfs here. Were there problems with that?

It's the same issue that /var/lib/containerd-test was having, the overlay tests attempt to mount to /tmp but fail to.

Docker has a dind script that mounts cgroups and /tmp as tmpfs, I think we should run that as the entrypoint.

stevvooe · 2018-01-03T21:57:01Z

Should the entry point be make test or make or go test ./...?

dnephin

Each of the "install x" blocks could be separate build stages. That should help with "optimize getting protobuf".

For the protobuf stage, I would personally make that a COPY script/install-protobuf /tmp/; RUN /tmp/install-protobuf and in that script have logic to use the binary release on amd64, and compile it from source otherwise. At least that way amd64 is faster.

The multi-stage build will help with caching on other platforms.

dnephin · 2018-01-04T23:13:53Z

I generally avoid this "git checkout of the source repo" in a Dockerfile. I think it's safe to assume that this Dockerfile will only be used with a build context of a containerd checkout, so it should use COPY . /go/src/github.com/containerd/containerd or a bind mount to get the source.

make binaries install can be done as part of the ENTRYPOINT or omitted entirely.

dnephin · 2018-01-04T23:18:23Z

I think this should use the pinned version from RUNC.md since that's what we document as being supported.

codecov-io · 2018-01-11T18:39:15Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1957   +/-   ##
=======================================
  Coverage   45.35%   45.35%           
=======================================
  Files          96       96           
  Lines        9453     9453           
=======================================
  Hits         4287     4287           
  Misses       4455     4455           
  Partials      711      711

Flag	Coverage Δ
#linux	`50.24% <ø> (ø)`	⬆️
#windows	`40.29% <ø> (ø)`	⬆️

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 eed3b1c...f65cdc1. Read the comment docs.

crosbymichael · 2018-01-17T15:33:27Z

@tophj-ibm whats the status on this one now that the tests are passing?

dnephin · 2018-01-17T16:11:26Z

I believe the 3 bullet points in the description are still relevant and need to be fixed. The tests aren't running using this image just yet, but we should be able to use it with #634

tophj-ibm · 2018-01-17T16:44:32Z

@crosbymichael bullet points are still relevant, while the non-root tests pass, I'm still having issues with the root-ones.

I have a patch for downloading protobuf on amd64 only, ~~but I haven't timed it yet and it doesn't look like it will be much faster~~ which is significantly faster. I have to change the tests to not unmount the directory probably or move it to a tmpfs for the integration test issue, and I believe the btrfs-progs package is bugged on the version of debian I am working with causing the root-test issue, I'm still investigating.

tophj-ibm · 2018-01-22T14:53:13Z

Fixed the first two. Anyone have any ideas on the last point? No matter what I try or how big a mount I give it, mkfs.btrfs just complains about the size and errors out in a container.

crosbymichael · 2018-01-22T16:10:37Z

Are you enabling btrfs by default?

tophj-ibm · 2018-01-22T16:19:26Z

I'm not doing anything special. Building containerd with or without it doesn't make a difference. Does the host docker mount and graphdriver need to be btrfs?

crosbymichael · 2018-01-22T16:22:18Z

It shouldn't. I guess we need to figureout where in the code that error is coming from.

estesp · 2018-01-22T21:28:42Z

@tophj-ibm the problem with item 3 is discussed here: moby/moby#27886

If I add -v /dev:/dev to your docker run command the tests work. I found the following other changes make things smoother:

Add btrfs-tools and xfstools to your final section of the Dockerfile RUN apt-get install
Correct your docker run comment at the top of the Dockerfile to add bash as the command; otherwise you just get an error and exit instead of a shell. And of course add the -v /dev:/dev to handle the loop hotplug issue.
Not required, but adding the vim-tiny or some simple vi editor would make it easier to do simple activities inside the container shell :)

tophj-ibm · 2018-01-22T21:39:19Z

Thanks for testing @estesp! I'll update those things

I found the underlying issue ended up being ppc64le has a higher threshold for a minimum btrfs size on mkfs, so the error message was accurate, and my attempts to test it with a larger volume were... incorrect. Upping the size of the file worked.

I think I'm close to being done with this, running into a small issue now with running out of space because the parallel tests all create filesystems that aren't being cleaned up immediately but I have a few things I can do.

dnephin · 2018-01-22T21:56:10Z

Maybe just ignore the remove error instead of skipping remove?

If someone uses a bind mount for this directory, or runs a shell and does multiple test runs in the same container, they'll still want the files inside the directory cleaned up (even if the directory itself can't be removed).

tophj-ibm · 2018-01-23T20:36:27Z

Got all the tests to pass, feel free to review!

tophj-ibm · 2018-01-23T20:38:55Z

I can leave a code comment here if you want, but this was the minimum size I could get btrfs to work in a container on ppc64le. I tested with s390x and it worked with an even smaller value. I don't have the right hardware to test this with arm*

Why the required size significantly differs across architectures?
Is there ppc-specific bug in our code or in kernel?

I couldn't find any information / bugs reported on this, but it's definitely a kernel issue and not a containerd one because I can replicate this anywhere.

Could you open a github issue?

Please fix this for systems that work under this size. This is way too big to be allocating on small hosts.

dnephin

We could use some of the new install scripts in CI, but I guess that could be a follow up as well.

dnephin · 2018-01-23T20:45:16Z

There's a line in .travis.yml which installs runc, it should probably be replaced with ./script/setup/install-runc

dnephin · 2018-01-23T20:46:30Z

Remove -x by default? could be enabled with $DEBUG, but I'm not sure if it's necessary.

dnephin · 2018-01-23T20:46:35Z

same about -x

dnephin · 2018-01-23T20:47:23Z

There are a few lines in .travis.yml that could be replaced by ./script/setup/install-protobuf

AkihiroSuda · 2018-01-25T03:16:30Z

Let's create the root within the volume and remove this code

I think we can just remove this code anyway, we don't need to change the path.

There's no reason to os.Exit(1) if the RemoveAll fails, just ignore the error in all cases. We already do that in dozens of places in the test suite.

AkihiroSuda · 2018-01-25T03:20:35Z

s/Dockerfile/Dockerfile.test/ , as this Dockerfile is not suitable for other purposes

In a follow up we could add another stage which runs script/setup/install-dev-tools. That would make this Dockerfile usable for just about every project task. Then do you think we could keep it Dockerfile or Dockerfile.dev ?

I would rather expect (the last stage of) Dockerfile to have minimum dependencies.

Probably Dockerfile should be as follows

FROM golang:x.y.z AS proto3 ... FROM golang:x.y.z AS runc ... # `docker build` with `--target test` for testing FROM golang:x.y.z AS test ... # should be added in follow-up PR FROM test AS test-extra COPY script/setup/install-dev-tools . RUN install-dev-tools ... # the last stage has minimum dependency. # this stage needs to be the last so that Docker Hub can build automatically. FROM golang:x.y.z.-alpine RUN apt --no-cache add some-minimum-deps # no `COPY .` here ...

AkihiroSuda · 2018-01-25T04:50:40Z

Off-topic: I think we should register https://hub.docker.com/u/containerd before somebody else takes over, (even if we don't use Docker Hub).

tophj-ibm · 2018-01-25T15:55:41Z

@AkihiroSuda what do you think about this?

Generally vendor.conf != RUNC.md, but I think it is ok to call it a day

AkihiroSuda · 2018-01-25T16:51:55Z

s/RUNC.md/vendor.conf

stevvooe · 2018-01-26T00:40:00Z

What's the status of this PR? It seems like it creates a bunch of dependencies on docker, which seems pretty backwards for the goals of this project. I really don't want to bring the testing experience of moby/moby to this project.

If we want to self-host test inside of a container for containerd, that is great, but we need to have a plan that doesn't create a dependency on docker.

estesp · 2018-01-26T00:51:54Z

I don't think this was proposed to be a replacement for any of the development processes we are following today in containerd. We've said multiple times (in now closed issues) if someone wants to contribute a Dockerfile then that's fine, as long as it is maintained over time. If we think we need to move it to contrib/ to make that clearer that's fine with me.

To me this is simply a convenience offering for those who want to leave their system unmodified and yet do development and test related activities with containerd in an isolated container. That was kind of one of the promises of that Docker thing. :)

tophj-ibm · 2018-01-26T15:52:17Z

@stevvooe I see your concerns and I think @estesp had a good suggestion, what do you think about contrib/? It decreases visibility, but I think it addresses your issues regarding docker dependencies in this project.

I created this PR to make it easy to run containerd, and more specifically for me, all the tests, in a container. The dockerfile and subsequent scripts solves issues that I ran into while building and testing, including making sure specific directories such as /var/lib/containerd-test and /tmp were mounted, as well as providing an easy way to install the dependencies on other platforms where it isn't as simple as "download protobuf".

As for the state of this PR: all the tests pass, feel free to try it yourself.

I am tracking down a probable bug with btrfs-progs where ppc64le needs a significantly larger minimum storage size (~650MB vs ~100MB) for mkfs.btrs, but I don't think that's a blocker here.

dnephin · 2018-01-26T16:34:59Z

it creates a bunch of dependencies on docker

There is no dependency on docker. The Dockerfile can be added as a convenience for contributors, but there is no requirement to use it. I will replace my adhoc Dockerfile with this one if it merges. I sure there are other contributors who will benefit.

I really don't want to bring the testing experience of moby/moby to this project.

No one does.

There are thousands of projects that use Docker as a development tool, many of which have much better dev/testing experiences than the moby/moby project.

self-host test inside of a container for containerd, that is great

This sounds like a lot more work to develop and maintain. We already have a tool for this.

stevvooe · 2018-01-26T21:14:44Z

Fair enough. Place these in contrib/ and let’s keep them out of the Makefile.

This process needs to be updated in the building.md file so people know how to use it.

stevvooe · 2018-01-26T21:15:16Z

This changes the behavior of testmain.

We had some discussion about this previously in this PR. I suggested that there shouldn't be any reason to exit(1) here. Am I mistaken? What's the reason for an Exit(1) if the remove fails?

Ok, can we fix in another PR? This is an area where we previously exited but do not now. I have no clue why this is the way it is, so let's see if we can figure that out.

I've removed it for the time being. It does cause the integration tests to fail at the end, but it's pretty obvious it fails because of a umount. Still, should investigate further

stevvooe · 2018-01-26T21:16:43Z

This is confusing to have all these mentions of docker but actually this is containerd in docker. Is this script still necessary? You might be able to get it running with just privileged and a few bind mounts.

stevvooe · 2018-01-26T21:17:48Z

Looks like arm and windows have binary releases, as well.

stevvooe · 2018-01-26T21:18:54Z

Does this need to be built with seccomp and apparmor enabled?

tophj-ibm · 2018-01-29T22:12:35Z

I'm not 100% sure on this one, does 64-bit windows run win32 binaries? EDIT: looks like it

stevvooe · 2018-01-29T22:25:26Z

Does this match what is in RUNC.md?

Generally no. I think we should replace RUNC.md with TOML/YAML that is readable for both machine and human. (Can be done in a separate PR)

Well, it does match as long as PR changes are changing both locations. So far that is the case, but if we are worried about manual sync, then @AkihiroSuda might have a good idea to make a runc commit hash definition parseable. Of course, I assume we hope this goes away at some point as stable released runc will be viable for (hopefully) all releases of containerd.

This provides a dockerfile for building a container to run the containerd tests Signed-off-by: Christopher Jones <[email protected]>

Cleans up loop devices if part of the test or mount process fails. Also increases btrfs default file size to 650MB to accommodate minimum btrfs size on ppc64le and s390x Signed-off-by: Christopher Jones <[email protected]>

Signed-off-by: Christopher Jones <[email protected]>

estesp

LGTM; I think this is ready; @stevvooe PTAL

crosbymichael · 2018-02-01T16:39:35Z

LGTM

stevvooe · 2018-02-07T00:46:32Z

This process needs to be updated in the building.md file so people know how to use it.

Why was this merged without an update to building.md?

stevvooe reviewed Jan 3, 2018

View reviewed changes

Comment thread Dockerfile Outdated

Copy link
Copy Markdown

Member

stevvooe Jan 3, 2018

Choose a reason for hiding this comment

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

Should the entry point be make test or make or go test ./...?

dnephin reviewed Jan 4, 2018

View reviewed changes

tophj-ibm force-pushed the check-for-mounts-tests branch 3 times, most recently from cb15839 to 018ad60 Compare January 11, 2018 18:30

tophj-ibm force-pushed the check-for-mounts-tests branch 2 times, most recently from f41c71d to 695b143 Compare January 22, 2018 14:34

dnephin reviewed Jan 22, 2018

View reviewed changes

tophj-ibm force-pushed the check-for-mounts-tests branch 3 times, most recently from ecf6bd7 to 5f40539 Compare January 23, 2018 20:35

tophj-ibm changed the title ~~[WIP] running tests in a container~~ running tests in a container Jan 23, 2018

tophj-ibm commented Jan 23, 2018

View reviewed changes

dnephin reviewed Jan 23, 2018

View reviewed changes

tophj-ibm force-pushed the check-for-mounts-tests branch 4 times, most recently from a7b3998 to 8088eb1 Compare January 24, 2018 22:06

AkihiroSuda reviewed Jan 25, 2018

View reviewed changes

tophj-ibm force-pushed the check-for-mounts-tests branch 2 times, most recently from 8696987 to 3a9f45b Compare January 25, 2018 15:52

tophj-ibm commented Jan 25, 2018

View reviewed changes

AkihiroSuda reviewed Jan 25, 2018

View reviewed changes

Comment thread script/setup/install-runc Outdated

Copy link
Copy Markdown

Member

AkihiroSuda Jan 25, 2018

Choose a reason for hiding this comment

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

s/RUNC.md/vendor.conf

tophj-ibm force-pushed the check-for-mounts-tests branch from 3a9f45b to fbb36f8 Compare January 25, 2018 18:54

stevvooe requested changes Jan 26, 2018

View reviewed changes

tophj-ibm force-pushed the check-for-mounts-tests branch 3 times, most recently from 3418316 to 6eb50fd Compare January 29, 2018 22:10

tophj-ibm commented Jan 29, 2018

View reviewed changes

stevvooe reviewed Jan 29, 2018

View reviewed changes

tophj-ibm force-pushed the check-for-mounts-tests branch from 6eb50fd to b9ae074 Compare January 30, 2018 19:44

tophj-ibm added 3 commits January 30, 2018 14:44

running tests in a container

051ac5d

This provides a dockerfile for building a container to run the containerd tests Signed-off-by: Christopher Jones <[email protected]>

Cleanup loop devices after test failure

a8c5ff5

Cleans up loop devices if part of the test or mount process fails. Also increases btrfs default file size to 650MB to accommodate minimum btrfs size on ppc64le and s390x Signed-off-by: Christopher Jones <[email protected]>

Use protobuf and runc setup scripts in .travis.yml

f65cdc1

Signed-off-by: Christopher Jones <[email protected]>

tophj-ibm force-pushed the check-for-mounts-tests branch from b9ae074 to f65cdc1 Compare January 30, 2018 19:44

estesp approved these changes Jan 31, 2018

View reviewed changes

crosbymichael merged commit 7e1d8aa into containerd:master Feb 1, 2018

tophj-ibm deleted the check-for-mounts-tests branch February 1, 2018 16:40

AkihiroSuda mentioned this pull request May 11, 2018

fix RUNC.md vs vendor.conf mismatch #2337

Merged

Conversation

tophj-ibm commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dnephin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

crosbymichael commented Jan 17, 2018

Uh oh!

dnephin commented Jan 17, 2018

Uh oh!

tophj-ibm commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tophj-ibm commented Jan 22, 2018

Uh oh!

crosbymichael commented Jan 22, 2018

Uh oh!

tophj-ibm commented Jan 22, 2018

Uh oh!

crosbymichael commented Jan 22, 2018

Uh oh!

estesp commented Jan 22, 2018

Uh oh!

tophj-ibm commented Jan 22, 2018

Uh oh!

dnephin Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tophj-ibm commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tophj-ibm Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dnephin 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!

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!

tophj-ibm commented Jan 3, 2018 •

edited

Loading

dnephin left a comment •

edited

Loading

dnephin Jan 4, 2018 •

edited

Loading

codecov-io commented Jan 11, 2018 •

edited

Loading

tophj-ibm commented Jan 17, 2018 •

edited

Loading

dnephin Jan 22, 2018 •

edited

Loading

tophj-ibm Jan 25, 2018 •

edited

Loading

AkihiroSuda Jan 25, 2018 •

edited

Loading