Adding v0.15.0 firecracker docker image #87

xibz · 2019-03-09T06:57:59Z

Adds a docker file that allows for testing of v0.15.0 of Firecracker.

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

fctesting/sandbox/fc-latest

nmeyerhans

In keeping with convention, please name your docker file Dockerfile. Append a suffix for clarity if you want, e.g. Dockerfile.fc-latest

fctesting/sandbox/fc-latest

Makefile

fctesting/sandbox/Dockerfile

sipsma

Just FYI, I think in the fullness of time it might be beneficial for us to merge your efforts here with what I just sent out for PR in the firecracker-containerd repo. There is a fair amount of overlap (i.e. both Dockerfiles build firecracker).

For now it's probably best to get the two Dockerfiles out separately, but we can think about how we could have one unified Dockerfile for both, which should make maintenance and consistency simpler

sipsma · 2019-03-11T22:08:46Z

fctesting/sandbox/Dockerfile

+
+WORKDIR "$FIRECRACKER_GO_SDK_SRC_DIR"
+RUN cp -rf testdata/* "$TMP_BUILD_DIR"
+RUN curl -fsSL -o "$TMP_BUILD_DIR"/vmlinux https://s3.amazonaws.com/spec.ccfc.min/img/hello/kernel/hello-vmlinux.bin


You can use ADD https://s3.amazonaws.com/spec.ccfc.min/img/hello/kernel/hello-vmlinux.bin <destination inside container>, which will result in the build checking to see if the remote file changed and re-download it if so.

ADD has some surprising behavior around auto-extraction of tarballs and always creating a separate layer, so I don't really have a problem avoiding it. See this best-practices document for some details.

Makefile

fctesting/sandbox/Dockerfile

Makefile

fctesting/sandbox/Dockerfile

samuelkarp · 2019-03-13T23:09:58Z

fctesting/sandbox/Dockerfile

+        musl-tools
+
+# Add musl to rust std
+RUN mkdir "$TMP_BUILD_DIR" \


Why do you need to use "$TMP_BUILD_DIR"? Did you want to make this variable instead of picking a constant path?

Naw, it could be a constant path, just thought ENV were specifically for environment variables and didn't know what is considered to be a constant other than ENV. The TMP_BUILD_DIR is used as the test folder where it searches for firecracker, jailer, kernel image, and root files.

I would probably just write the literal /build instead of using an environment variable.

I went with using an ENV just due to it being easier to change if we ever do decide to move it from /build.

fctesting/sandbox/Dockerfile

samuelkarp · 2019-03-13T23:13:48Z

fctesting/sandbox/Dockerfile

+
+WORKDIR "$FIRECRACKER_GO_SDK_SRC_DIR"
+RUN cp -rf testdata/* "$TMP_BUILD_DIR"
+RUN curl -fsSL -o "$TMP_BUILD_DIR"/vmlinux https://s3.amazonaws.com/spec.ccfc.min/img/hello/kernel/hello-vmlinux.bin


ADD has some surprising behavior around auto-extraction of tarballs and always creating a separate layer, so I don't really have a problem avoiding it. See this best-practices document for some details.

fctesting/sandbox/Dockerfile

Signed-off-by: xibz <[email protected]>

samuelkarp · 2019-03-14T19:47:44Z

fctesting/sandbox/Dockerfile

+ARG FIRECRACKER_SRC_DIR="/firecracker"
+ARG FIRECRACKER_BUILD_DIR="$FIRECRACKER_SRC_DIR/build"
+ARG CARGO_REGISTRY_DIR="$FIRECRACKER_BUILD_DIR/cargo_registry"
+ARG FIRECRACKER_VERSION="0.15.0"


While I think you want to get rid of most of the ARG statements, this one makes a lot of sense to keep.

samuelkarp · 2019-03-14T19:48:18Z

fctesting/sandbox/Dockerfile

+        && rustup target add x86_64-unknown-linux-musl
+
+# Install Firecracker and set binaries to TMP_BUILD_DIR
+RUN git clone https://github.com/firecracker-microvm/firecracker.git


Is there any particular reason you have these as separate RUN statements instead of chaining them together? In general, each directive (like RUN, ENV, etc) adds a new layer to the resulting image. When you're building images, you generally want to reduce the total number of layers except when you want to share layers between disparate images (think a base OS or language image). This firecracker-build one is internal only and you never really push it as an artifact, but introducing separate layers still has a performance impact during the build as you cause Docker's storage driver (layer storage component) to create new snapshots for each one.

I'd rewrite like this:

RUN git clone https://github.com/firecracker-microvm/firecracker.git \ cd firecracker \ git checkout tags/v"$FIRECRACKER_VERSION" \ cargo build --release --features vsock --target-dir /build \ ...

Had no idea that having multiple RUN commands caused such overhead was the main reason. I've went ahead and updated it based on your feedback.

Signed-off-by: xibz <[email protected]>

fctesting/sandbox/Dockerfile

Signed-off-by: xibz <[email protected]>

fctesting/sandbox/Dockerfile

Makefile

Signed-off-by: xibz <[email protected]>

nmeyerhans

This works. One last thing: we'll need to update the clean rule to remove the stamp file and anything else created on the filesystem that isn't already cleaned up with go clean. I doubt we'll want to also remove the generated image from Docker, but that is up to you.

…image Signed-off-by: xibz <[email protected]>

samuelkarp · 2019-05-07T22:08:39Z

Makefile

@@ -27,4 +27,27 @@ all-tests:
 generate build clean:


You should remove "clean" from this line.

samuelkarp · 2019-05-07T22:09:01Z

Makefile

@@ -27,4 +27,27 @@ all-tests:
 generate build clean:
 	go $@ $(EXTRAGOARGS)

-.PHONY: all generate clean build test unit-tests all-tests
+clean:
+	go $@ $(EXTRAGOARGS)


Since you don't have a variable argument, you can just write go clean $(EXTRAGOARGS).

samuelkarp · 2019-05-07T22:10:30Z

Makefile

+clean:
+	go $@ $(EXTRAGOARGS)
+	# --force to prevent erroring on stamp file not existing
+	rm --force sandbox-test-fc-build-stamp


You can also write rm sandbox-test-fc-build-stamp ||: prevent an error. || is a logical or and : is a builtin for true.

xibz · 2019-08-07T00:11:08Z

Closing due to inactivity and age.

Issue firecracker-microvm#87 was previously addressed by having our shim process exit when all tasks it was managing exited. However, it turns out that is not the expected behavior for a containerd shim. The expected behavior is that a shim continue running until all tasks have been *deleted* by the client (via the containerd Delete API call). When any task has been deleted (or create of a task fails) containerd will send the shim a Shutdown API call, at which time the shim is expected to exit if it *all* tasks it was managing have been deleted. This commit fixes brings our shim in line with that expected behavior. It, however, also retains the existing FCControl parameter "ExitAfterAllTasksGone" (though now renamed to "ExitAfterAllTasksDeleted") to give users the ability to specify whether the default containerd shim behavior should be ignored and instead allow the Shim+VM to keep running even after all Tasks have been deleted. The majority of the changes here actually end up just being a refactor of the TaskManager interface to safely handle Create/Delete task APIs alongside checking whether the shim has any tasks left being managed (which is uses to decide if it should shutdown). The refactorization also ensures that all IO is flushed before a Delete call returns (which is a better solution to handling I/O races than the temporary fix applied in 1e36219). Signed-off-by: Erik Sipsma <[email protected]>

xibz requested review from nmeyerhans, samuelkarp, mxpv and sipsma March 9, 2019 06:57

mxpv reviewed Mar 11, 2019

View reviewed changes

fctesting/sandbox/fc-latest Outdated Show resolved Hide resolved

nmeyerhans suggested changes Mar 11, 2019

View reviewed changes

fctesting/sandbox/fc-latest Outdated Show resolved Hide resolved

nmeyerhans reviewed Mar 11, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

xibz force-pushed the docker branch from 5b30d67 to ef93bcf Compare March 11, 2019 21:30

mxpv reviewed Mar 11, 2019

View reviewed changes

fctesting/sandbox/Dockerfile Outdated Show resolved Hide resolved

mxpv reviewed Mar 11, 2019

View reviewed changes

fctesting/sandbox/Dockerfile Outdated Show resolved Hide resolved

xibz force-pushed the docker branch from ef93bcf to e6c8f2a Compare March 11, 2019 21:43

mxpv reviewed Mar 11, 2019

View reviewed changes

fctesting/sandbox/Dockerfile Outdated Show resolved Hide resolved

mxpv reviewed Mar 11, 2019

View reviewed changes

fctesting/sandbox/Dockerfile Show resolved Hide resolved

xibz force-pushed the docker branch from e6c8f2a to f35ebc9 Compare March 11, 2019 22:08

sipsma reviewed Mar 11, 2019

View reviewed changes

Adding v0.15.0 firecracker docker image

e41e9fb

xibz force-pushed the docker branch from f35ebc9 to e41e9fb Compare March 11, 2019 22:25

samuelkarp suggested changes Mar 13, 2019

View reviewed changes

This was referenced Mar 13, 2019

Adding release make tag firecracker-microvm/firectl#25

Closed

Adding CRI Dockerfile firecracker-microvm/firecracker-containerd#132

Closed

fixup! Adding v0.15.0 firecracker docker image

8184b76

Signed-off-by: xibz <[email protected]>

samuelkarp reviewed Mar 14, 2019

View reviewed changes

fixup! fixup! Adding v0.15.0 firecracker docker image

d6ecb0e

Signed-off-by: xibz <[email protected]>

samuelkarp reviewed Mar 14, 2019

View reviewed changes

fctesting/sandbox/Dockerfile Outdated Show resolved Hide resolved

fixup! fixup! fixup! Adding v0.15.0 firecracker docker image

7709598

Signed-off-by: xibz <[email protected]>

xibz requested review from samuelkarp and nmeyerhans March 14, 2019 22:08

nmeyerhans reviewed Mar 18, 2019

View reviewed changes

fctesting/sandbox/Dockerfile Outdated Show resolved Hide resolved

fctesting/sandbox/Dockerfile Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

fixup! fixup! fixup! fixup! Adding v0.15.0 firecracker docker image

4c1a6bd

Signed-off-by: xibz <[email protected]>

xibz force-pushed the docker branch from a6d7f81 to 4c1a6bd Compare March 18, 2019 17:49

nmeyerhans suggested changes Mar 18, 2019

View reviewed changes

fixup! fixup! fixup! fixup! fixup! Adding v0.15.0 firecracker docker …

ebce062

…image Signed-off-by: xibz <[email protected]>

xibz requested a review from nmeyerhans March 18, 2019 18:23

samuelkarp reviewed May 7, 2019

View reviewed changes

xibz closed this Aug 7, 2019

Adding v0.15.0 firecracker docker image #87

Adding v0.15.0 firecracker docker image #87

Uh oh!

Conversation

xibz commented Mar 9, 2019

Uh oh!

Uh oh!

nmeyerhans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

xibz commented Aug 7, 2019

Uh oh!

Uh oh!