Run tests in containers. #134

sipsma · 2019-03-11T21:59:37Z

The original motivation for this change was to enable tests that verify
our shim exits as expected when a container exits. We decided this was
best enabled via running tests in containers, which allows you to
easily separate shim processes running in parallel from one another
and make assertions about the one created by your test in particular.
The Dockerfile used to build the environment in which tests run should
be useful in general too for enabling devs to quickly rebuild/test
changes.

While Buildkite does offer plugins for docker-compose and docker, they
are not used here for a few reasons:

docker-compose doesn't support docker-buildkit, which we use to
parallelize builds and cache gomod/apt dependencies, which both
speed up the builds (~4 mins vs. ~10 mins) and improve reliability
The plain docker Buildkite plugin doesn't support all the parameters
we need to configure, such as setting the ipc namespace to host for
the snapshotter tests

Signed-off-by: Erik Sipsma sipsma@amazon.com

As of this commit, only the "firecracker-containerd-test-base" image is actually used by our CI system; the "firecracker-containerd-e2etest" and "firecracker-containerd-dev" images are not yet. However, I wanted to keep them here for a few reasons:

They can already provide utility by giving devs a way to quickly test changes to code (this was helpful for me while trying to get firecracker 0.15 working in the docker container)
They will be used in the CI system very soon to implement a test for the new event forwarder

If there are objections to this, I can remove those stages for now and add them later when I add the test for the event forwarder PR, let me know.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tools/docker/Dockerfile

tools/docker/fc-agent.start

sipsma · 2019-03-13T16:11:36Z

The travis CI build failed because of commits layered on top of the first one that I'm planning on squashing into one before merging, so I don't think it's a "real" failure.

.buildkite/pipeline.yml

tools/docker/Dockerfile

tools/docker/entrypoint.sh

tools/docker/Dockerfile

nmeyerhans · 2019-03-14T22:04:51Z

tools/docker/Dockerfile

+		wget \
+		build-essential \
+		libseccomp-dev \
+		btrfs-tools \


Why do we need btrfs-tools?

Without it you get this containerd compilation error:

#19 15.06 bin/containerd #19 16.02 # github.com/containerd/containerd/vendor/github.com/containerd/btrfs #19 16.02 vendor/github.com/containerd/btrfs/btrfs.go:22:25: fatal error: btrfs/ioctl.h: No such file or directory #19 16.02 #include <btrfs/ioctl.h> #19 16.02 ^ #19 16.02 compilation terminated.

However, I found in their docs you can use BUILDTAGS=no_btrfs, which I'll update with given that I can't envision any scenario in which we'd use their (unmodified) btrfs snapshotter (though let me know if that may not be true).

nmeyerhans · 2019-03-14T22:07:09Z

tools/docker/Dockerfile

+ENV RUSTUP_HOME=/home/builder/rustup \
+    CARGO_HOME=/home/builder/cargo \
+    PATH=/home/builder/cargo/bin:$PATH \
+	RUST_VERSION=1.32.0


nit: whitespace

fixed (my IDE was formatting tabs as 4 spaces)

nmeyerhans · 2019-03-14T22:12:37Z

tools/docker/Dockerfile

+
+# TODO using ADD <url> should work instead of having to wget, but buildkit has a bug where --chown is ignored for URLs
+# https://github.com/moby/buildkit/issues/813
+RUN wget "https://static.rust-lang.org/rustup/archive/1.16.0/x86_64-unknown-linux-gnu/rustup-init" \


I'd usually prefer curl over wget, with options such as --max-time, --retry, and --silent (maybe among others, depending on circumstances). You don't need to switch to curl, but it'd be nice make sure equivalent options are applied here (or not, if wget's defaults are reasonable).

Ah good to know, I just updated to curl with those options you suggested

nmeyerhans · 2019-03-14T22:22:35Z

tools/docker/Dockerfile

+ 	--mount=type=cache,id=aptlib,target=/var/lib/apt,sharing=locked \
+	mkdir -p /etc/apt/sources.list.d \
+	&& echo "deb http://ftp.debian.org/debian stretch-backports main" > /etc/apt/sources.list.d/stretch-backports.list \
+	&& apt update \


Similar comments to above about DEBIAN_FRONTEND and apt-get

nmeyerhans · 2019-03-14T22:25:21Z

tools/docker/Dockerfile

+ADD https://s3.amazonaws.com/spec.ccfc.min/img/hello/kernel/hello-vmlinux.bin /var/lib/firecracker-containerd/runtime/hello-vmlinux.bin
+ADD tools/docker/config.toml /etc/containerd/config.toml
+ADD tools/docker/firecracker-runtime.json /etc/containerd/firecracker-runtime.json
+ADD tools/docker/entrypoint.sh /entrypoint.sh


nit: It's often a good idea to drop file extensions on installation. If we ever decide to rewrite this script in something other than shell, it'd be nice not to have to worry about chasing down and updating all references. (Not a big deal here, just something to consider generally.)

Great point, updated

nmeyerhans · 2019-03-14T22:41:56Z

.buildkite/pipeline.yml

+          --progress=plain \
+          -f tools/docker/Dockerfile \
+          --target firecracker-containerd-test-e2e \
+          -t firecracker-containerd-test-e2e .


Am I missing something or are we not actually using this image anywhere? I understand why you might want to keep it defined in the Dockerfile, but why are we actually building it here?

Mentioned this in the PR description towards the bottom, but I changed the approach here either way to make there be a "base" test image that is capable of starting a full firecracker-containerd stack and one image derived from that base that specifically starts up the naive snapshotter.

The idea is that we can add another image deriving from the base in the future that starts up the devmapper-snapshotter (and more images for whatever other permutations of our stack we want to test with).

mxpv · 2019-03-15T23:25:48Z

I'm not super fan of huge Dockerfile with lots of dependencies between stages as it's harder to maintain. I would rather split it on smaller files somehow. But probably this is fine for now.

tools/docker/Dockerfile

sipsma · 2019-03-19T16:10:29Z

I'm not super fan of huge Dockerfile with lots of dependencies between stages as it's harder to maintain. I would rather split it on smaller files somehow. But probably this is fine for now.

I agree this Dockerfile is pretty much at max capacity at this point. Unfortunately, splitting it up into separate files would make it more difficult for buildkit to automatically parallelize different build stages. If we expand it in the future we can consider splitting it up into separate files and then just appending them together into a single file during docker build as a Makefile target or something like that.

sipsma · 2019-03-19T17:21:24Z

Combined all the fixup commits into one; there was a final fix needed which I noticed this morning after seeing a build on a fresh instance related to setting the go mod cache permissions. This is working now and both Travis+Buildkite builds are passing.

tools/docker/Dockerfile

.buildkite/pipeline.yml

tools/docker/firecracker-runtime.json

The original motivation for this change was to enable tests that verify our shim exits as expected when a container exits. We decided this was best enabled via running tests in containers, which allows you to easily separate shim processes running in parallel from one another and make assertions about the one created by your test in particular. The Dockerfile used to build the environment in which tests run should be useful in general too for enabling devs to quickly rebuild/test changes. While Buildkite does offer plugins for docker-compose and docker, they are not used here for a few reasons: 1. docker-compose doesn't support docker-buildkit, which we use to parallelize builds and cache gomod/apt dependencies, which both speed up the builds (~4 mins vs. ~10 mins) and improve reliability 2. The plain docker Buildkite plugin doesn't support all the parameters we need to configure, such as setting the ipc namespace to host for the snapshotter tests Signed-off-by: Erik Sipsma <sipsma@amazon.com>

Addressed feedback, have approval from others

sipsma requested review from nmeyerhans, samuelkarp, mxpv and xibz March 11, 2019 21:59

xibz previously requested changes Mar 12, 2019

View reviewed changes

tools/docker/Dockerfile Outdated Show resolved Hide resolved

tools/docker/Dockerfile Outdated Show resolved Hide resolved

tools/docker/Dockerfile Outdated Show resolved Hide resolved

nmeyerhans suggested changes Mar 12, 2019

View reviewed changes

tools/docker/Dockerfile Outdated Show resolved Hide resolved

tools/docker/Dockerfile Outdated Show resolved Hide resolved

tools/docker/Dockerfile Outdated Show resolved Hide resolved

tools/docker/fc-agent.start Outdated Show resolved Hide resolved

sipsma requested review from nmeyerhans and xibz March 13, 2019 16:12

samuelkarp reviewed Mar 14, 2019

View reviewed changes

nmeyerhans reviewed Mar 14, 2019

View reviewed changes

sipsma mentioned this pull request Mar 15, 2019

Forward agent containerd events to shim and monitor task exits via event subscriptions #118

Merged

sipsma force-pushed the container-ci-pr branch from 79016c8 to 7b8e45e Compare March 15, 2019 21:06

mxpv approved these changes Mar 15, 2019

View reviewed changes

nmeyerhans reviewed Mar 16, 2019

View reviewed changes

tools/docker/Dockerfile Outdated Show resolved Hide resolved

sipsma force-pushed the container-ci-pr branch from 2286d1d to 733650b Compare March 19, 2019 17:11

sipsma force-pushed the container-ci-pr branch from 733650b to f58f484 Compare March 19, 2019 18:55

nmeyerhans approved these changes Mar 19, 2019

View reviewed changes

samuelkarp reviewed Mar 19, 2019

View reviewed changes

tools/docker/Dockerfile Outdated Show resolved Hide resolved

.buildkite/pipeline.yml Show resolved Hide resolved

.buildkite/pipeline.yml Outdated Show resolved Hide resolved

tools/docker/firecracker-runtime.json Outdated Show resolved Hide resolved

sipsma force-pushed the container-ci-pr branch from f58f484 to 8b510fd Compare March 20, 2019 00:54

sipsma force-pushed the container-ci-pr branch from 8b510fd to cc7d9be Compare March 20, 2019 01:11

samuelkarp approved these changes Mar 20, 2019

View reviewed changes

sipsma merged commit 6927857 into firecracker-microvm:master Mar 20, 2019

sipsma deleted the container-ci-pr branch April 11, 2019 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests in containers. #134

Run tests in containers. #134

sipsma commented Mar 11, 2019 •

edited

Loading

sipsma commented Mar 13, 2019

nmeyerhans Mar 14, 2019

sipsma Mar 15, 2019

nmeyerhans Mar 14, 2019

sipsma Mar 15, 2019

nmeyerhans Mar 14, 2019

sipsma Mar 15, 2019

nmeyerhans Mar 14, 2019

sipsma Mar 15, 2019

nmeyerhans Mar 14, 2019

sipsma Mar 15, 2019

nmeyerhans Mar 14, 2019 •

edited

Loading

sipsma Mar 15, 2019

mxpv commented Mar 15, 2019 •

edited

Loading

sipsma commented Mar 19, 2019

sipsma commented Mar 19, 2019

Run tests in containers. #134

Run tests in containers. #134

Conversation

sipsma commented Mar 11, 2019 • edited Loading

sipsma commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmeyerhans Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxpv commented Mar 15, 2019 • edited Loading

sipsma commented Mar 19, 2019

sipsma commented Mar 19, 2019

sipsma commented Mar 11, 2019 •

edited

Loading

nmeyerhans Mar 14, 2019 •

edited

Loading

mxpv commented Mar 15, 2019 •

edited

Loading