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

Add integration tests for /cpu-config #3623

Conversation

mattschlebusch
Copy link
Contributor

Changes

Add integration tests to call the PUT /cpu-config API endpoint.

Reason

  • Improves coverage and validates core end-to-end functionality of the API.

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

Jonathan Woollett-Light and others added 30 commits April 19, 2023 11:28
A bitflags like crate to support efficient
implementation of CPUID functionality.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Reworked the CPUID crate to support extensive CPU templates,
simplifying the existing flow and adding accessor functions and types.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Consistently use `vendor id` over `manufactured id` when referring to
the brand string in CPUID.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
`AmdCpuid::normalize` passes through CPUID leaves from the host, if the
host does not meet the AMD specification it is possible to enter an
indefinite loop, to avoid this we verify the vendor id is
`AuthenticAMD` before attempting to pass through leaves.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Increases timeout in test coverage test.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Moved `cpuid-templates` into a module under
`src/vmm/src/vstate/vcpu/x86_64` to avoid compiling them on Arm.
Updated coverage to take this into account.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Additional tests for `cpuid::common::get_cpuid`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Fixes behaviour of `RawCpuid::push` and `RawCpuid::pop`, and removes
`RawCpuid::resize`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Simple tests for `CpuidTrait::get` and `CpuidTrait::get_mut`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Change `Intel(4)` to `Intel(R)` from the Intel brand string under
cpuid.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Passes portions of the host CPUID through to the guest depending on the
CPUID template.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Adds unit test to `get_max_cpus_per_package_test()` within
`src/vmm/src/cpuid/normalize.rs`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Removes unused `From` implementations on `Leaf`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Removes unused serde `Serialize` and `Deserialize` implementations on
`RawCpuid`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Adds additional test case covering clone of zero length `RawCpuid`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Fixes to issues encountered when rebasing the `feature/cpu-templates`
feature branch onto `main`.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Reduce occurrences of the `unsafe` keyword and adds additional
documentation where present.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
The previous implementation could return multiple mutable references to
the same value, this fix corrects this.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Includes moving cpuid module as a sub-module
for guest_config

Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
The functionality is unused so it can be removed.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
The functionality is unused so it can be removed.

Signed-off-by: Jonathan Woollett-Light <jcawl@amazon.co.uk>
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Added parameters only. Instantiation of configuration
to be down in later changes to include CPU template
modifications.

Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
zulinx86 and others added 8 commits April 24, 2023 16:00
Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Currently, when converting `kvm_bindings::CpuId` to `Cpuid`, it is
relaying `RawCpuid` as an intermediate type. To remove this, this commit
adds bidirectional direct type conversion between them.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
`RawCpuid` was relayed as an intermediate type when conversion between
`kvm_bindings::CpuId` and `Cpuid`. Deleting `RawCpuid` reduces a bunch
of lines and unsafe code.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
Using .get() instead of [] on the CPU model name dictionary
allows for corresponding global test properties to contain "Unknown"
instead of failing if tests are run on an unknown CPU model.

Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
c7g support in spectre_meltdown_checker.sh script
was merged:
speed47/spectre-meltdown-checker#447
so we can now run it on c7g 5.10 host.

Note: this still leaves c7g 4.14 host untested, because
c7g host 4.14 requires a modified 5.10 guest to boot,
and we only have those modifications in place for 4.14
at the moment.

Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
@mattschlebusch mattschlebusch marked this pull request as ready for review April 25, 2023 09:33
@mattschlebusch mattschlebusch 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 Apr 25, 2023
@mattschlebusch mattschlebusch requested a review from a team April 25, 2023 11:46
Copy link
Contributor

@kalyazin kalyazin left a comment

Choose a reason for hiding this comment

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

A general comment. Looks like this change implements tests similar to those we have under pytest: runs a Firecracker binary and executes some HTTP requests. Is there a reason to implement such tests in Rust instead by bringing new development dependencies? Is that merely because you had to convert static templates into json, and an interface for that is only available in Rust? Can we reuse existing json replicas of static templates from our repo?
I know @JonathanWoollett-Light has some ideas on migrating the integration test suite from Python to Rust, but I don't think there has been a conclusion yet.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
@mattschlebusch
Copy link
Contributor Author

A general comment. Looks like this change implements tests similar to those we have under pytest: runs a Firecracker binary and executes some HTTP requests. Is there a reason to implement such tests in Rust instead by bringing new development dependencies? Is that merely because you had to convert static templates into json, and an interface for that is only available in Rust? Can we reuse existing json replicas of static templates from our repo?
I know @JonathanWoollett-Light has some ideas on migrating the integration test suite from Python to Rust, but I don't think there has been a conclusion yet.

To summarize my intentions and approach here:

  • Our Rust code-base has utility functions, like building testing data but also serialization and deserialization that is useful, but also benefits from being re-used in our test code wherever possible.
  • Code coverage - While not applicable here yet, we could far more easily improve our coverage if we wired our integration tests using our Rust codebase.

Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
@kalyazin
Copy link
Contributor

Our Rust code-base has utility functions, like building testing data but also serialization and deserialization that is useful, but also benefits from being re-used in our test code wherever possible.

I would have to say that ser/des functionality is not required for this test since we can use json versions of the static templates from the repo directly. Please speak up if you have concerns with this approach.
Python test code can be reused in the same way as Rust. Is there a Rust-specific advantage?
I can also see bringing in tokio which is certainly something new to our code base.

Code coverage - While not applicable here yet, we could far more easily improve our coverage if we wired our integration tests using our Rust codebase.

While I agree that code coverage can be improved by counting in integration tests, I do not think that this is the right approach for raising coverage. Usually line/branch coverage is useful to measure how code is covered with unit tests. Adding coverage coming from integration tests makes it significantly harder to see what is covered by unit tests alone. Also since integration tests tend to touch many lines of code, we may have a false sense of good coverage. What we should be aiming for integration tests is functional coverage, meaning covering all real-world use cases. Line/branch coverage is much less relevant as it is impractical to write integration tests for many error conditions.

@@ -0,0 +1,65 @@
// Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing an integration test in Rust, and then triggering it from pytest, why not just make it in pytest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a similar thread with @kalyazin, but I will summarize my take:

  • Easier debugging with Rust workspace dependency.
  • Smaller (or no) reliance on Docker environment updates with changing/updating dependencies.
  • Existing Rust code is tested more thoroughly.
  • Improves possibilities of clearer code coverage reporting.

@mattschlebusch
Copy link
Contributor Author

I would have to say that ser/des functionality is not required for this test since we can use json versions of the static templates from the repo directly. Please speak up if you have concerns with this approach.

It is not absolutely required, but it is a repeated use of the code and is certainly a benefit to repeatedly use code in different scenarios as a means of testing it. I used the JSON ser/deser as an example, but this would go for any other functions that could potentially be used in integration tests.

Python test code can be reused in the same way as Rust. Is there a Rust-specific advantage?

Python test code is only implemented exclusively for the purposes of running our tests. While the Rust code is used on critical performance paths and is developed as a consequence of a feature's development in any event. Our Rust specific advantage to my mind is that Firecracker is implemented in Rust itself, and therefore makes writing tests easier (because the code exists). We can also run tests directly if written in Rust's workspace, cargo test --test <test name> which is faster, doesn't require Docker environment manipulations when there are new dependencies or updates involved, and with reliance purely on the Rust workspace, this integrates more easily with IDEs, so debugging with breakpoints is much easier.

I can also see bringing in tokio which is certainly something new to our code base.

That is true. In this context, it's not strictly required though, it just makes testing in a multi-threaded context a bit easier. It is also only a dev dependency which I don't consider an issue. I stand to be corrected on that though.

@ShadowCurse ShadowCurse mentioned this pull request May 9, 2023
9 tasks
@mattschlebusch mattschlebusch removed the Status: Awaiting review Indicates that a pull request is ready to be reviewed label May 10, 2023
@mattschlebusch mattschlebusch marked this pull request as draft May 10, 2023 09:06
@kalyazin kalyazin deleted the branch firecracker-microvm:feature/cpu-templates May 12, 2023 18:04
@kalyazin kalyazin closed this May 12, 2023
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.

5 participants