Update Rust toolchain to 1.43.1

[timvisee]

Reason for This PR

The latest cargo-audit used in the Docker images requires Rust >=1.40. The current image still uses Rust 1.39.

Blocked by #1913

Fixes #1904

Description of Changes

The Dockerfiles have been changed to use Rust 1.43.1, the latest stable Rust version.

• 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

[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.

[lauralt]

Hi, @timvisee! In order to use Rust 1.43 in the container, the change that you've done is unfortunately not enough, we will still use the old container image this way. For fixing the issue, we need to also publish a new container version (fcuvm/dev:v15), if you take a look in /tools/devtool you can see that the current version is fcuvm/dev:v14. Publishing a new container version requires: building the image and pushing it to Docker Hub for both x86_64 and aarch64 (you can see a nice guide example here at Publishing a New Version section). The pushing part must be done by somebody from our team.

Also, after publishing the container image, there will be some clippy fixes in our codebase that will need to be done because of the new rust version (and probably some other fixes), so this issue will probably require some time and effort (also, sorry for not being clearer in the issue description, will update it).

Do you still want to help on this (we can help you with it, but there is absolutely no problem if you don't want as it is not an easy item at all) or on other issue? Generally, the issues that are a good starting point for first-time contributions are marked with: Contribute: Good First Issue.

[timvisee]

Thanks for the explanation. I definitely thought the actual upgrade would be much simpler.

I'd like to give this a try later today. I only have limited time available for this though. If I can't get it to work I'll leave it open for someone else.

[timvisee]

@lauralt I've resolved all clippy suggestions and reformatted for Rust 1.43.1 in 234a8f8.

I think that's all I can do for now. Someone on the Firecracker team needs to publish an updated image before I can continue. I'll update the devtool script afterwards.

Please let me know if there's anything else I can do.

[lauralt]

Thanks, @timvisee! We'll be working today to publish a new container image and then come back here with an update.

I would like to also remove musl-tools from the Dockerfiles, that was needed before for the backtrace crate, but we've decided to not use it anymore (see #1841). We'll try to merge #1841 as soon as possible to unblock this PR.

[gbionescu]

Hi @timvisee. I've experimented a bit with the container images that use the 1.43.1 and 1.39.0 rust toolchain versions in order to outline a scenario where we avoid possible CI failures on other PRs that don't have anything to do with updating the rust toolchain.

After running the test suite on both this PR and #1841, it seems that we need to go through a few iterations before merging, mainly due to a few test failures - we should see them once the v15 container image is used in tests.

I pushed a new container tag to Docker Hub, that uses rust toolchain 1.43.1.
You should find it tagged as fcuvm/dev:v15, meaning that in order to run have the CI run the test suite with the updated container image, you only need to update:

diff --git a/tools/devtool b/tools/devtool
index 7a53a267..6297337d 100755
--- a/tools/devtool
+++ b/tools/devtool
@@ -73,7 +73,7 @@
 # Development container image (name:tag)
 # This should be updated whenever we upgrade the development container.
 # (Yet another step on our way to reproducible builds.)
-DEVCTR_IMAGE="fcuvm/dev:v14"
+DEVCTR_IMAGE="fcuvm/dev:v15"
 
 # Naming things is hard
 MY_NAME="Firecracker $(basename "$0")"

[gbionescu]

The binary size test failure should be fixed by merging and rebasing #1841.

To address the python styling failure, I created issue #1913

[timvisee]

The binary size test failure should be fixed by merging and rebasing #1841.

I've increased the binary size limit for now, based on the reasoning here #1841 (comment).

This PR is now blocked by #1913

[sandreim]

The binary size test failure should be fixed by merging and rebasing #1841.

To address the python styling failure, I created issue #1913

Where is the increase in size coming from ? Is it just the upgrade of the toolchain ? I am little bit worried about this since we already inflated the binary with versioned serialization support.

[timvisee]

It seems there's one issue left to be fixed, judging by the CI status.

It shows 'Device/resource busy' on x86_64. I'm not sure what is causing this, and don't believe this showed up when I started working on this PR.

---- device_manager::persist::tests::test_device_manager_persistence stdout ----
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Net(CreateNet(TapOpen(CreateTap(Os { code: 16, kind: Other, message: "Device or resource busy" }))))', src/vmm/src/device_manager/persist.rs:450:13

Anyway, I'm not sure how to go about this right now. Does anybody have an idea?

[gbionescu]

Hi @timvisee.
It looks like one of the unit tests is failing. I'm not sure if it's related to the 1.43.1 upgrade or not, but we will look into this as soon as possible.

[timvisee]

Also, the buildkite CI build for this seems to be stuck. It looks like it's still running. Maybe someone should cancel it to prevent high cost for nothing, I can't do this myself.

[ioanachirca]

Hello @timvisee! While we are looking into this problem, could you please squash the two clippy commits together? It would make for a cleaner git log 😄

[iulianbarbu]

Just one more nit. Please sign-off every commit: git commit --amend -s.
All seems good. I'll approve once you integrate this feedback.

[ioanachirca]

We should still check where the increase in binary size comes from and if we can lower it.

[acatangiu]

Please also update coverage on AMD.

[timvisee]

I've lowered it by the same amount, see: fcb38bd#diff-49bb55812e565341f0e92d70ff51b302R20-R27

[iulianbarbu]

We should still check where the increase in binary size comes from and if we can lower it.

Oh, right. I think it would help if we would compare the dependencies of the project now, with the previous rust version. To do this, we can use cargo-bloat.

[timvisee]

We should still check where the increase in binary size comes from and if we can lower it.

@ioanachirca Is this something that can be done in a separate issue/PR? This PR didn't introduce significant code changes.

[ioanachirca]

We should still check where the increase in binary size comes from and if we can lower it.

@ioanachirca Is this something that can be done in a separate issue/PR? This PR didn't introduce significant code changes.

I would rather reach a conclusion about this now (it is related to the Rust update after all), rather than introduce a (maybe) avoidable change and have to fix it later (or postpone it until it's forgotten).

[ioanachirca]

Running cargo bloat --release -n 15 on current master branch:

 File  .text      Size       Crate Name
 2.3%   5.2%   67.1KiB  api_server api_server::parsed_request::ParsedRequest::try_from_request
 1.2%   2.7%   34.8KiB         vmm vmm::builder::build_microvm
 0.9%   2.1%   26.5KiB firecracker firecracker::main
 0.8%   1.8%   22.8KiB  micro_http micro_http::server::HttpServer::requests
 0.8%   1.7%   22.1KiB         vmm vmm::persist::create_snapshot
 0.7%   1.5%   19.0KiB         vmm <vmm::vstate::VcpuState as versionize::Versionize>::serialize
 0.6%   1.5%   18.8KiB         vmm vmm::resources::VmResources::from_json
 0.6%   1.3%   17.2KiB  api_server api_server::ApiServer::handle_request
 0.6%   1.3%   16.8KiB         vmm <vmm::persist::MicrovmState as versionize::Versionize>::serialize
 0.5%   1.1%   13.8KiB         vmm vmm::builder::configure_system_for_boot
35.0%  78.9% 1013.2KiB             And 1690 smaller methods. Use -n N to show more.
44.3% 100.0%    1.3MiB             .text section size, the file size is 2.8MiB

Running cargo bloat --release -n 15 on this branch:

 File  .text    Size        Crate Name
 2.1%   4.7% 64.6KiB   api_server api_server::parsed_request::ParsedRequest::try_from_request
 1.4%   3.0% 41.6KiB          vmm <vmm::vstate::VcpuState as versionize::Versionize>::serialize
 1.1%   2.5% 34.9KiB          vmm vmm::builder::build_microvm
 0.8%   1.8% 24.8KiB  firecracker firecracker::main
 0.8%   1.7% 23.8KiB          vmm vmm::persist::create_snapshot
 0.6%   1.4% 19.1KiB          vmm vmm::resources::VmResources::from_json
 0.6%   1.4% 18.8KiB    [Unknown] _ZN9libunwind10CFI_ParserINS_17LocalAddressSpaceEE17parseInstructionsERS1_mmRKNS2_8CIE_InfoEmRPNS2_20PrologInf...
 0.6%   1.3% 18.4KiB   micro_http micro_http::server::HttpServer::requests
 0.5%   1.1% 15.2KiB      devices devices::virtio::net::device::Net::process_tx
 0.5%   1.0% 14.4KiB          vmm vmm::builder::configure_system_for_boot
 0.5%   1.0% 14.0KiB          vmm <vmm::device_manager::persist::DeviceStates as versionize::Versionize>::serialize
 0.5%   1.0% 13.8KiB firecracker? <firecracker::api_server_adapter::ApiServerAdapter as polly::event_manager::Subscriber>::process
 0.4%   0.9% 13.0KiB      devices <devices::virtio::vsock::unix::muxer::VsockMuxer as devices::virtio::vsock::VsockEpollListener>::notify
 0.4%   0.9% 12.0KiB   api_server api_server::ApiServer::handle_request
 0.4%   0.9% 11.8KiB         mmds mmds::parse_request
33.7%  74.5%  1.0MiB              And 1670 smaller methods. Use -n N to show more.
45.3% 100.0%  1.3MiB              .text section size, the file size is 3.0MiB

We can see that the binary file size increased from 2.8MiB to 3.0MiB, so setting the limit to 3.5MiB is too high.
A rather big difference comes from vmm <vmm::vstate::VcpuState as versionize::Versionize>::serialize which doubled from 19KiB to 41.6KiB.
Also, _ZN9libunwind10CFI_ParserINS_17LocalAddressSpaceEE17parseInstructionsERS1_mmRKNS2_8CIE_InfoEmRPNS2_20PrologIn... (libunwind) increased from 8.4KiB to 18.8KiB

[acatangiu]

We can see that the binary file size increased from 2.8MiB to 3.0MiB, so setting the limit to 3.5MiB is too high.

I think the target should be the current size and the limit should be +10% or +15%.

A rather big difference comes from vmm <vmm::vstate::VcpuState as versionize::Versionize>::serialize which doubled from 19KiB to 41.6KiB.

We should open an issue for this and investigate (but not block this PR).

_ZN9libunwind10CFI_ParserINS_17LocalAddressSpaceEE17parseInstructionsERS1_mmRKNS2_8CIE_InfoEmRPNS2_20PrologIn... (libunwind) increased from 8.4KiB to 18.8KiB

I believe this will go away altogether with #1841

[ioanachirca]

We can see that the binary file size increased from 2.8MiB to 3.0MiB, so setting the limit to 3.5MiB is too high.

I think the target should be the current size and the limit should be +10% or +15%.

A rather big difference comes from vmm <vmm::vstate::VcpuState as versionize::Versionize>::serialize which doubled from 19KiB to 41.6KiB.

We should open an issue for this and investigate (but not block this PR).

_ZN9libunwind10CFI_ParserINS_17LocalAddressSpaceEE17parseInstructionsERS1_mmRKNS2_8CIE_InfoEmRPNS2_20PrologIn... (libunwind) increased from 8.4KiB to 18.8KiB

I believe this will go away altogether with #1841

SGTM. I'll open an issue. @timvisee , could you update the binary limit?

[timvisee]

@ioanachirca Sure, what limit(s) should I choose though?

This is what I came up with based on the last CI results:

FC_BINARY_SIZE_TARGET = 3124000 if MACHINE == "x86_64" else 3349336
FC_BINARY_SIZE_LIMIT = 3300000 if MACHINE == "x86_64" else 3500000

[ioanachirca]

@ioanachirca Sure, what limit(s) should I choose though?

This is what I came up with based on the last CI results:

FC_BINARY_SIZE_TARGET = 3124000 if MACHINE == "x86_64" else 3349336
FC_BINARY_SIZE_LIMIT = 3300000 if MACHINE == "x86_64" else 3500000

Seems fine.

[timvisee]

@ioanachirca I've just pushed these new limits. This should be all then.

[timvisee]

It looks like this AMD branch isn't working:

if "AMD" in platform.processor():
    COVERAGE_TARGET_PCT = 84.35

2da434b#diff-49bb55812e565341f0e92d70ff51b302R26-R27

The .platform() function seems flaky for CPU vendor detection: https://stackoverflow.com/a/4842467/1000145

[acatangiu]

Yes, it seems the AMD coverage check only works intermittently. We've only recently added AMD as a target and that check is not required yet (for reasons like this).