Skip to content
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

[v1.11] CI: revamp rootfs artifacts #4858

Merged
merged 17 commits into from
Nov 18, 2024
Merged

Conversation

pb8o
Copy link
Contributor

@pb8o pb8o commented Oct 16, 2024

Changes

  • Update rootfs to Ubuntu 24.04
  • Avoid the need to store an ext4 image
  • Avoid embedding SSH keys in the rootfs. We still bake in a key before running tests.
  • New set of kernels for debugging/tracing. Also some add a basic tracing helper.

Reason

Not hardcoding the key in the rootfs decreases the changes the key is reused in some other context.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.

  • This functionality cannot be added in rust-vmm.

@pb8o pb8o added Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Awaiting author Indicates that an issue or pull request requires author action labels Oct 16, 2024
@pb8o pb8o self-assigned this Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.07%. Comparing base (83aec7b) to head (3b7c357).
Report is 17 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4858   +/-   ##
=======================================
  Coverage   84.07%   84.07%           
=======================================
  Files         251      251           
  Lines       28067    28067           
=======================================
  Hits        23597    23597           
  Misses       4470     4470           
Flag Coverage Δ
5.10-c5n.metal 84.64% <ø> (ø)
5.10-m5n.metal 84.62% <ø> (ø)
5.10-m6a.metal 83.94% <ø> (+<0.01%) ⬆️
5.10-m6g.metal 80.75% <ø> (ø)
5.10-m6i.metal 84.62% <ø> (ø)
5.10-m7g.metal 80.75% <ø> (ø)
6.1-c5n.metal 84.64% <ø> (-0.01%) ⬇️
6.1-m5n.metal 84.62% <ø> (-0.01%) ⬇️
6.1-m6a.metal 83.93% <ø> (ø)
6.1-m6g.metal 80.75% <ø> (+<0.01%) ⬆️
6.1-m6i.metal 84.62% <ø> (+<0.01%) ⬆️
6.1-m7g.metal 80.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pb8o pb8o force-pushed the ci-no-ssh-keys branch 8 times, most recently from b392ac5 to 39f7388 Compare October 22, 2024 08:29
@pb8o pb8o marked this pull request as ready for review October 22, 2024 08:30
@pb8o pb8o changed the title CI: don't embed SSH keys in the rootfs CI: revamp rootfs artifacts Oct 22, 2024
Copy link
Contributor

@Manciukic Manciukic left a comment

Choose a reason for hiding this comment

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

lgtm, just a few minor comments.
It would be nice to run shellcheck on all these bash files to catch some common issues (quoting paths, unused vars, etc).

.buildkite/pipeline_cross.py Show resolved Hide resolved
resources/rebuild.sh Outdated Show resolved Hide resolved
resources/rebuild.sh Outdated Show resolved Hide resolved
resources/rebuild.sh Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tools/sandbox.py Show resolved Hide resolved
resources/guest_configs/ci.config Show resolved Hide resolved
docs/getting-started.md Outdated Show resolved Hide resolved
tests/framework/microvm_helpers.py Show resolved Hide resolved
@pb8o pb8o force-pushed the ci-no-ssh-keys branch 5 times, most recently from c780883 to 778e931 Compare October 23, 2024 18:38
@pb8o pb8o force-pushed the ci-no-ssh-keys branch 5 times, most recently from 299e8c4 to 2ea16d9 Compare November 6, 2024 14:50
@pb8o pb8o added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Awaiting author Indicates that an issue or pull request requires author action labels Nov 6, 2024
@pb8o pb8o changed the title CI: revamp rootfs artifacts [v1.11] CI: revamp rootfs artifacts Nov 6, 2024
resources/rebuild.sh Outdated Show resolved Hide resolved
resources/guest_configs/ci.config Show resolved Hide resolved
.buildkite/pipeline_cross.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
@pb8o
Copy link
Contributor Author

pb8o commented Nov 14, 2024

Can we validate that to avoid surprises?

@kalyazin validated here:

% cat not.config
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG=n
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG=n
% cat ../guest_configs/microvm-kernel-ci-x86_64-6.1.config ../guest_configs/ci.config not.config >.config
% make -j3 olddefconfig
.config:3236:warning: override: reassigning to symbol IKCONFIG
.config:3238:warning: override: reassigning to symbol MSDOS_PARTITION
.config:3239:warning: override: reassigning to symbol SQUASHFS_ZSTD
.config:3241:warning: override: reassigning to symbol DEVMEM
.config:3243:warning: override: reassigning to symbol IKCONFIG
.config:3244:warning: override: reassigning to symbol IKCONFIG
.config:3245:warning: override: reassigning to symbol IKCONFIG
.config:3246:warning: override: reassigning to symbol IKCONFIG
#
# configuration written to .config
#
% grep CONFIG_IKCONFIG .config
# CONFIG_IKCONFIG is not set

pb8o added 16 commits November 14, 2024 16:36
This avoids the need to store and download the image from S3.

Signed-off-by: Pablo Barbáchano <[email protected]>
Enabling ftrace in our kernels changed the performance of several tests,
so it was reverted.

Make a new set of kernels that will not be used for performance tests.

While doing this, simplify our guest kernel config customization that
relied on patches and use file concatenation instead. Turns out `make
olddefconfig` produces the same result and we avoid the complexity of
dealing with patches.

Signed-off-by: Pablo Barbáchano <[email protected]>
Compress squashfs with zstd since that now we have
CONFIG_SQUASHFS_ZSTD=y in all our guest kernels.

In my tests it is 78MB vs 85MB (an 8.2% reduction)

Signed-off-by: Pablo Barbáchano <[email protected]>
socat v1.8.0 in Ubuntu 24.04 has a bug when using `UDP-LISTEN` without
specifying the address family. It looks like:

    E xioopen_ipdgram_listen(): unknown address family 0

We can work-around it by specifying IPv4.

See http://www.dest-unreach.org/socat/CHANGES v1.8.0.1

Signed-off-by: Pablo Barbáchano <[email protected]>
Update guest rootfs to Ubuntu 24.04

Signed-off-by: Pablo Barbáchano <[email protected]>
Generate SSH key after downloading artifacts, and add it to the rootfs.

This avoids having an SSH key hardcoded in the rootfs. Downside is that
we have to rebuild the rootfs, but that is fast.

Signed-off-by: Pablo Barbáchano <[email protected]>
This returns a Popen object instead of waiting for the command to
finish. It may be useful when we need to incrementally read the output
of a long running process in the guest, without having to use screen.

Signed-off-by: Pablo Barbáchano <[email protected]>
So that we don't have to install it in the future.

Signed-off-by: Pablo Barbáchano <[email protected]>
For now it's a very simple one, but we can use it as a base to provide
more complicated ones in the future.

Signed-off-by: Pablo Barbáchano <[email protected]>
Use the new CI artifacts prepared for v1.11

Signed-off-by: Pablo Barbáchano <[email protected]>
It's more hassle to keep this as a separate tool than including it in
the tests, and we avoid having to treat it specially.

Also this way we can run it in parallel easily.

Signed-off-by: Pablo Barbáchano <[email protected]>
This is so we use less IO overall sending snapshot to/from S3.

- Punch holes in the memory snapshots
- Decrease guest memory from 1GB to 512MB as it's not important to the
  test.

This decreases around 10x:

Before: 27GB * 22 runs ~ 594GB
After: 2.7GB * 22 runs ~ 59.4GB

Signed-off-by: Pablo Barbáchano <[email protected]>
Also fix for when the patch kernel version is <100.

Signed-off-by: Pablo Barbáchano <[email protected]>
It is unlikely that we will ever support more than one rootfs.

Signed-off-by: Pablo Barbáchano <[email protected]>
Stage it so next time we rebuild the devctr it's there, and we can use
it to replace gzip, since it compresses better and faster.

Signed-off-by: Pablo Barbáchano <[email protected]>
Add debugging information to debug kernels, but split it out by default.

Signed-off-by: Pablo Barbáchano <[email protected]>
cargo-deny raised its MSRV to 1.81.0 in 0.16.2, while we are still at
1.79.0.

Pinning cargo-deny to last working version that works with our version
of Rust.

Signed-off-by: Pablo Barbáchano <[email protected]>
@pb8o pb8o merged commit e719d60 into firecracker-microvm:main Nov 18, 2024
7 checks passed
@pb8o pb8o deleted the ci-no-ssh-keys branch November 18, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants