Skip to content

Build firecracker from a submodule. #260

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 16, 2019

Conversation

sipsma
Copy link
Contributor

@sipsma sipsma commented Sep 16, 2019

This is intended to ease development with Firecracker and version upgrades.

Signed-off-by: Erik Sipsma [email protected]

Note this does not upgrade Firecracker to a new version ( ref #259 ), it keeps the version at 0.17 for now, but it should make moving to 0.18 easier.

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

# express or implied. See the License for the specific language governing
# permissions and limitations under the License.

FROM rust:1.32-stretch
Copy link
Contributor

@samuelkarp samuelkarp Sep 16, 2019

Choose a reason for hiding this comment

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

  1. Any particular reason you're using -stretch instead of the default or instead of -buster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not see a buster image for rust 1.32 on dockerhub (only for newer rust versions), so I don't think they ever released one.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We probably should target same version of Rust as Firecracker:
    https://github.com/firecracker-microvm/firecracker/blob/master/tools/devctr/Dockerfile.x86_64#L7
    (or even we may consider using the Dockerfile from their repo as base to keep up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That Dockerfile is for Firecracker v0.18 (which updates to rust 1.35). We are still building Firecracker v0.17 for now, which uses an older rust toolchain.

I considered using their Dockerfile, but it's doing a lot more than we need here and not necessarily compatible with the way we structure our CI system, so I felt it would be simpler overall for us to just have this tiny Dockerfile of our own. We can reconsider in the future if maintaining this Dockerfile becomes meaningful overhead.

Copy link
Contributor

@nmeyerhans nmeyerhans left a comment

Choose a reason for hiding this comment

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

No major concerns, so feel free to merge. We can revisit my questions at a later time if you don't want to address them here. (please at least add comments and/or issues so they don't get lost)

Makefile Outdated

FIRECRACKER_DIR=$(SUBMODULES)/firecracker
FIRECRACKER_BIN=$(FIRECRACKER_DIR)/target/x86_64-unknown-linux-musl/release/firecracker
JAILER_BIN=$(FIRECRACKER_DIR)/target/x86_64-unknown-linux-musl/release/jailer
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make these paths architecture independent without too much work? At some point we'll need arm64, at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for now I just updated to use an explicit FIRECRACKER_TARGET var that is used in the Makefile and also passed to the Dockerfile as a build-arg.

docker rmi localhost/runc-builder:latest
docker rmi localhost/$(RUNC_BUILDER_NAME):$(DOCKER_IMAGE_TAG)
docker rmi localhost/$(FIRECRACKER_BUILDER_NAME):$(DOCKER_IMAGE_TAG)
docker volume rm -f $(CARGO_CACHE_VOLUME_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

these docker commands should be prefixed with '-' so they don't count as a failure if a given image doesn't exist.

Raise the bar by providing a make function that checks to see if an image exists, exits true if not, tries to delete it if so (at that point you can fail meaningfully if it doesn't exist)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I learned some more Make and added a macro for conditionally deleting the images. For the docker volume, the "-f" gets us that effect already (there is also a -f flag for rmi but it does much more that we don't want).

@@ -179,11 +152,12 @@ RUN mkdir -p /var/lib/firecracker-containerd/runtime \
&& echo "882fa465c43ab7d92e31bd4167da3ad6a82cb9230f9b0016176df597c6014cef default-vmlinux.bin" | sha256sum -c - \
&& mv default-vmlinux.bin /var/lib/firecracker-containerd/runtime/default-vmlinux.bin

COPY _submodules/firecracker/target/x86_64-unknown-linux-musl/release/firecracker /usr/local/bin/
COPY _submodules/firecracker/target/x86_64-unknown-linux-musl/release/jailer /usr/local/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above regarding architecture independent paths.

This is intended to ease development with Firecracker and version upgrades.

Signed-off-by: Erik Sipsma <[email protected]>
@sipsma sipsma merged commit 498dbf0 into firecracker-microvm:master Sep 16, 2019
@sipsma sipsma deleted the fcsubmod branch October 8, 2019 18:38
fangn2 pushed a commit to fangn2/firecracker-containerd that referenced this pull request Mar 23, 2023
…ependabot/go_modules/github.com/go-openapi/errors-0.19.7

Bump github.com/go-openapi/errors from 0.19.6 to 0.19.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants