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

test: Enable to run CI with custom CPU templates for testing / debugging purpose #4944

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

zulinx86
Copy link
Contributor

@zulinx86 zulinx86 commented Dec 5, 2024

Changes

Enable us to run CI with custom CPU templates for testing / debugging purpose.

  • Added tests/data/custom_cpu_templates dir and pick JSON files under the dir as custom CPU templates unconditionally to run the CI.
  • Added the two new options to the buildkite steps generation scripts:
    • --additional-prepend: allows us to inject or download custom CPU templates before running tests.
    • --keywords / -k: allows us to filter tests to run
  • Documented how to run CI with custom CPU templates in the test README.

Reason

To speed up the testing / debugging cycle of custom CPU templates.

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

  • [ ] I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • [ ] I have mentioned all user-facing changes in CHANGELOG.md.
  • [ ] If a specific issue led to this PR, this PR closes the issue.
  • [ ] When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • [ ] I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

pb8o and others added 15 commits December 3, 2024 18:50
There is some information in the Microvm class that we don't save in the
snapshot. Some tests do depend on those, so to make the booted/restored
case homogenous, make room in the snapshot to save metadata that we can
then restore.

Signed-off-by: Pablo Barbáchano <[email protected]>
Some further simplifications:
- Simplify the "_host" tests as they will provide the same result in
  both A/B situations.
- Add a second MicrovmFactory fixture with the "A" firecracker.
- Add a uvm_any fixture that includes all CPU templates and also a
  boot/restored dimension.

This decreases the need for separate tests. For example the guest test
test_check_vulnerability_files_ab can now test all variants:

2 (restored/booted) * 3 (kernels) * 9 (cpu templates) = 54 tests

Signed-off-by: Pablo Barbáchano <[email protected]>
Use a new uvm_any_booted fixture to make the test simpler.

Signed-off-by: Pablo Barbáchano <[email protected]>
Use the new uvm_any fixture to make the tests simpler

Signed-off-by: Pablo Barbáchano <[email protected]>
All tests in the file are x86_64 specific, so do the skip at the
top-level, once.

Signed-off-by: Pablo Barbáchano <[email protected]>
Make the common feature flags to get some clarity on what are the
differences between the CPUs.

Signed-off-by: Pablo Barbáchano <[email protected]>
This is the same test spread over two files. Putting it together in a
new file so they don't drift over time.

Signed-off-by: Pablo Barbáchano <[email protected]>
This is a fairly common repeated pattern, so extract it into a method to
make writing tests simpler.

Signed-off-by: Pablo Barbáchano <[email protected]>
Rewrite test_rng using uvm_any.

Signed-off-by: Pablo Barbáchano <[email protected]>
This fixture is not used anymore, as it is mostly superseded by
cpu_template_any.

Signed-off-by: Pablo Barbáchano <[email protected]>
Since we only have one rootfs, simplify the fixture to return a single
value. This will have also not show it as part of the test instance
name.

Signed-off-by: Pablo Barbáchano <[email protected]>
Add a parameter so that we can control vcpu number and memory.

Right now it uses a hardcoded value, but it can be overridden per test.

Signed-off-by: Pablo Barbáchano <[email protected]>
Previously to test custom CPU templates for temporary testing purpose in
development, we needed to modify Python code (especially
tests/framework/utils_cpu_templates.py) manually. That is very
cumbersome and slows down our development / test / debug cycle, since it
requires us to make a commit and push it manually.

With this change, we become able to inject custom CPU templates into
test (not only from local but also from buildkite) without modifying
python code.

Signed-off-by: Takahiro Itazuri <[email protected]>
We usually run the CI through the python scripts generating buildkite
pipeline steps. To enable to inject custom CPU templates through the
python scripts, adds a new command line argument `--additional-prepend`
that specifies additional commands to prepend to the generated steps.

Signed-off-by: Takahiro Itazuri <[email protected]>
Previously, when using the python scripts generating buildkite steps, we
were only able to run the entire tests. However, in some cases, we would
like to run only a subset of them. Adds a new argument `-k` (or
`--keywords`) to filter pytest tests to run.

Signed-off-by: Takahiro Itazuri <[email protected]>
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.07%. Comparing base (81bae9f) to head (6d1470c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4944   +/-   ##
=======================================
  Coverage   84.07%   84.07%           
=======================================
  Files         251      251           
  Lines       28059    28059           
=======================================
  Hits        23592    23592           
  Misses       4467     4467           
Flag Coverage Δ
5.10-c5n.metal 84.65% <ø> (ø)
5.10-m5n.metal 84.63% <ø> (+<0.01%) ⬆️
5.10-m6a.metal 83.94% <ø> (+<0.01%) ⬆️
5.10-m6g.metal 80.74% <ø> (ø)
5.10-m6i.metal 84.62% <ø> (-0.01%) ⬇️
5.10-m7g.metal 80.74% <ø> (ø)
6.1-c5n.metal 84.65% <ø> (ø)
6.1-m5n.metal 84.63% <ø> (ø)
6.1-m6a.metal 83.94% <ø> (+<0.01%) ⬆️
6.1-m6g.metal 80.74% <ø> (-0.01%) ⬇️
6.1-m6i.metal 84.62% <ø> (-0.01%) ⬇️
6.1-m7g.metal 80.74% <ø> (ø)

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
Copy link
Contributor

pb8o commented Dec 5, 2024

I think the issue with this approach is we will only run the tests that explicitly use a CPU template. I think what we want there is to run all tests with the specified template. Many will fail because they don't apply, but we will have to investigate and see if it's expected or not. The idea roughly is:

./tools/devtool test -- --cpu-template my-template.json functional

That will apply my-template.json for all tests that use a microvm. In addition it should probably also override the cpu_templates fixture to only return that template, or disable it completely maybe.

@zulinx86 zulinx86 force-pushed the custom_cpu_template_test_framework branch from 27b6f86 to 7fbd036 Compare December 5, 2024 09:28
@zulinx86
Copy link
Contributor Author

zulinx86 commented Dec 5, 2024

I think the issue with this approach is we will only run the tests that explicitly use a CPU template. I think what we want there is to run all tests with the specified template. Many will fail because they don't apply, but we will have to investigate and see if it's expected or not. The idea roughly is:

./tools/devtool test -- --cpu-template my-template.json functional

That will apply my-template.json for all tests that use a microvm. In addition it should probably also override the cpu_templates fixture to only return that template, or disable it completely maybe.

Let me confirm what we want first. Do we want to run all tests (not only tests normally running with CPU templates) with the given CPU template?

If no, I think making custom_cpu_template_params() return the custom CPU template looks good. Implementing --cpu-template option was what I tried first, but I didn't come up with a good way to make it return the custom CPU template. Maybe because I'm not good at pytest :/ Do you have any idea to implement it in a neat way?

Add documentation explaining how to run python integration tests with
custom CPU templates.

Signed-off-by: Takahiro Itazuri <[email protected]>
@zulinx86 zulinx86 force-pushed the custom_cpu_template_test_framework branch from 7fbd036 to 6d1470c Compare December 5, 2024 09:40
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.

2 participants