Fixed jailer build process #2125

krishnakumar4a4 · 2020-09-08T12:42:22Z

This fix is to make tools/devtool to build jailer binary
only for musl targets.

Currently, jailer binary gets built successfully for even
non-musl targets but doesn't work with corresponding
firecracker binary.

Reason for This PR

As mentioned in #2102

Currently, jailer binary gets built successfully for even
non-musl targets but doesn't work with corresponding
firecracker binary. This commit ensures jailer is not built for non-musl
targets.

Description of Changes

Modifed Cargo.toml workspace to exclude jailer for default build
Modified devtool script to build jailer binary only for musl targets
Updated changelog with the fix information
This functionality can be added in rust-vmm.

License Acceptance

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

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

alindima

Thank you for the PR.
In addition to modifying devtool, it seems that we need to also modify the get_firecracker_binaries() method (in tests/host_tools/cargo_build.py) that is used for building the binaries for CI tests. This is the reason the tests are failing, because they are unable to find the jailer binary.

I have tried adding the additional jailer build command, but some tests time out building the binaries. The two commands, instead of one, take too long, so we need a mechanism for building the binaries inside get_firecracker_binaries only once per CI test run. Using something like a function attribute maybe?

Cargo.toml

alindima · 2020-09-09T09:17:20Z

CHANGELOG.md

@@ -82,6 +82,8 @@
  accepting negative values.
 - Fixed #1754 - net: traffic blocks when running ingress UDP performance tests
  with very large buffers.
+- Fixed `devtool build` to build jailer binary for `musl` only targets. Building 


This should be in the UNRELEASED section of the changelog

Wondering if this will be moved from "UNRELEASED" to "Fixed" section before release? Anyways I have made the change. Working on the test now.

It will be moved from Unreleased in the future. The v0.22.0 release already exists so these new changes will be included in a future release.

Are we actually fixing something through the PR? Or should this line be expressed as updated or something else?

What do you think?

Hi @gc-plp and @alindima , modified this line to "changed" instead of "fixed" and moved it to Changed sub-section of UnReleased.

krishnakumar4a4 · 2020-09-13T04:09:02Z

Hi @alindima , I have made requested changes and fixed tests as well. Please let me know if anything else needs to be done here?

alindima

The changes look good to me. I added some minor comments that need to be addressed first.

CHANGELOG.md

tests/host_tools/cargo_build.py

This change is to make tools/devtool to build jailer binary only for musl targets. Currently, jailer binary gets built successfully for even non-musl targets but doesn't work with corresponding firecracker binary. Also modified integration build tests to build firecracker and jailer binaries only once to be used repeatitively

krishnakumar4a4 · 2020-09-15T04:12:27Z

The changes look good to me. I added some minor comments that need to be addressed first.

I have made necessary changes. Please have a look

alindima

LGTM

alindima · 2020-09-15T08:37:14Z

@krishnakumar4a4 Thank you for the contribution!

krishnakumar4a4 · 2020-09-15T17:46:48Z

@alindima and @gc-plp, I really appreciate your support and feedback.

Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263) * Fix Benchmark Goroutine (firecracker-microvm#259) * Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255) * Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253) * Fixed error that was not being test against in `TestWait` (firecracker-microvm#251) * Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230) * Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225) * Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218) Signed-off-by: Debian <admin@ip-172-31-27-214.us-east-2.compute.internal>

* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263) * Fix Benchmark Goroutine (firecracker-microvm#259) * Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255) * Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253) * Fixed error that was not being test against in `TestWait` (firecracker-microvm#251) * Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230) * Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225) * Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218) Signed-off-by: Debian <admin@ip-172-31-27-214.us-east-2.compute.internal> Signed-off-by: xibz <impactbchang@gmail.com>

* Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. (firecracker-microvm#263) * Fix Benchmark Goroutine (firecracker-microvm#259) * Jailer configuration API cleanup and improved logging with Debug log level (firecracker-microvm#255) * Firecracker is internally has an instance ID, but the SDK didn't have the way to configure the ID. This change connects Config.VMID to the instance ID. (firecracker-microvm#253) * Fixed error that was not being test against in `TestWait` (firecracker-microvm#251) * Fixes issue where socket path may not be defined since the config file has yet to be loaded (firecracker-microvm#230) * Fixed error that was not being test against in `TestNewPlugin` (firecracker-microvm#225) * Download Firecracker 0.21.1 and its jailer from Makefile (firecracker-microvm#218) Signed-off-by: xibz <impactbchang@gmail.com>

alindima self-assigned this Sep 9, 2020

alindima self-requested a review September 9, 2020 09:04

alindima removed their assignment Sep 9, 2020

alindima suggested changes Sep 9, 2020

View reviewed changes

krishnakumar4a4 force-pushed the fix-jailer-build branch 5 times, most recently from b7531f7 to 8898af9 Compare September 12, 2020 08:30

krishnakumar4a4 marked this pull request as ready for review September 12, 2020 08:57

alindima suggested changes Sep 14, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

tests/host_tools/cargo_build.py Outdated Show resolved Hide resolved

krishnakumar4a4 force-pushed the fix-jailer-build branch 2 times, most recently from 21e7534 to 2b7cc5e Compare September 14, 2020 18:32

krishnakumar4a4 force-pushed the fix-jailer-build branch from 2b7cc5e to 4196369 Compare September 15, 2020 02:53

Merge branch 'master' into fix-jailer-build

04d4915

alindima requested a review from gbionescu September 15, 2020 07:07

alindima approved these changes Sep 15, 2020

View reviewed changes

gbionescu approved these changes Sep 15, 2020

View reviewed changes

alindima merged commit 3bf285c into firecracker-microvm:master Sep 15, 2020

alindima mentioned this pull request Sep 15, 2020

Disable jailer build for Glibc #2102

Closed

kzys added a commit to kzys/firecracker-go-sdk that referenced this pull request Sep 21, 2020

Build firecracker and jailer explictly

f3e0121

Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

kzys mentioned this pull request Sep 21, 2020

Build firecracker and jailer explictly firecracker-microvm/firecracker-go-sdk#263

Merged

kzys added a commit to kzys/firecracker-go-sdk that referenced this pull request Sep 21, 2020

Build firecracker and jailer explictly

b9c5c1e

Since firecracker-microvm/firecracker#2125, `cargo build` doesn't build jailer by default. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

xibz mentioned this pull request Oct 9, 2020

Release v0.22.0 firecracker-microvm/firecracker-go-sdk#266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed jailer build process #2125

Fixed jailer build process #2125

krishnakumar4a4 commented Sep 8, 2020 •

edited

Loading

alindima left a comment

alindima Sep 9, 2020

krishnakumar4a4 Sep 11, 2020

alindima Sep 11, 2020

gbionescu Sep 11, 2020

krishnakumar4a4 Sep 15, 2020

krishnakumar4a4 commented Sep 13, 2020

alindima left a comment

krishnakumar4a4 commented Sep 15, 2020

alindima left a comment

alindima commented Sep 15, 2020

krishnakumar4a4 commented Sep 15, 2020

Fixed jailer build process #2125

Fixed jailer build process #2125

Conversation

krishnakumar4a4 commented Sep 8, 2020 • edited Loading

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

alindima left a comment

Choose a reason for hiding this comment

alindima Sep 9, 2020

Choose a reason for hiding this comment

krishnakumar4a4 Sep 11, 2020

Choose a reason for hiding this comment

alindima Sep 11, 2020

Choose a reason for hiding this comment

gbionescu Sep 11, 2020

Choose a reason for hiding this comment

krishnakumar4a4 Sep 15, 2020

Choose a reason for hiding this comment

krishnakumar4a4 commented Sep 13, 2020

alindima left a comment

Choose a reason for hiding this comment

krishnakumar4a4 commented Sep 15, 2020

alindima left a comment

Choose a reason for hiding this comment

alindima commented Sep 15, 2020

krishnakumar4a4 commented Sep 15, 2020

krishnakumar4a4 commented Sep 8, 2020 •

edited

Loading