Various small test fixes#5887
Conversation
We were previously only searching for ubuntu rootfs'. Add AL2023 to the list too. Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>
With the Kconfig update in commit 8a7e8a0, CONFIG_CRYPTO_MANAGER_DISABLE_TESTS was changed from `y` to `not set`. Crypto self-tests are required as part of the FIPS-enabled boot tests and we will use the same Kconfig for testing FIPS-enabled VMs, so instead of disabling tests at config level again, we set `cryptomgr.notests` kernel boot arg to skip them. To validate the fix, A/B tests were run: - Firstly with the Kconfig option set to `y` vs `not set` to validate the regression - Then with the boot arg set, to validate that the regression disappears Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>
Before each test run, we copy artifacts from the build/artifacts/ path to /srv/test_artifacts. This directory was not cleared up though, which led to issues with artifact A/B testing, where A's artifacts persisted and were selected by pytest over B's artifacts. To prevent this, clear the test artifacts dir at the beginning of each run. Signed-off-by: James Curtis <jxcurtis@amazon.co.uk>
JamesC1305
added
the
Status: Awaiting review
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5887 +/- ##
=======================================
Coverage 82.89% 82.89%
=======================================
Files 277 277
Lines 30051 30051
=======================================
Hits 24912 24912
Misses 5139 5139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| from framework.microvm import MicroVMFactory | ||
|
|
||
| kernels = list(kernels("vmlinux-*")) | ||
| rootfs = list(disks("ubuntu*ext4")) |
There was a problem hiding this comment.
maybe we can just keep *.ext4 so we can put whatever we want in there?
There was a problem hiding this comment.
Can do, I thought it would be better to be explicit, to prevent other rootfs' from being used, but I don't feel strongly about that
There was a problem hiding this comment.
sandbox is for development, I think it's nice if it picks up whatever is in the artifacts folder. But maybe as you say, it may lead to unexpected behaviours. Maybe we should tweak the default logic to be predictable, or just leave it as you made it here, that's fine as well.
There was a problem hiding this comment.
Wdym by tweaking default logic to be predictable? Are you suggesting being more permissive but ensuring we default to AL2023/Ubuntu? That sounds reasonable to me.
The other thing that bugs me with the sandbox is that it takes devctr paths rather than host paths, so you have to prepend /firecracker to all the paths. Personally I think host paths make more sense, since we're launching from host rather than devctr. I don't know how you feel abt that, if you think it's something worth changing
There was a problem hiding this comment.
yes, that's what I meant. I agree also on paths! Any improvement to the tool is welcome!
3 participants
Changes
devtool sandbox/srv/test_artifactson each test runReason
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
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.