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

Fix cross-kernel snapshot/restore tests #4632

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

pb8o
Copy link
Contributor

@pb8o pb8o commented Jun 3, 2024

Changes

fix: ensure snapshot_restore cross kernel test runs

Since PR#3896 this test always succeeded because it did not find any snapshot directories.

Rewrite and fix the test so that

  • we see one test case for each directory, instead of one test per CPU template, so it is clear what is being restored.
  • switch to creating the microvm through the API rather than a JSON file so that the Microvm/Snapshot classes can account for all the devices.

In addition, remove the combinations that were added while this test was falsely working but never worked:

  • x86 snapshot in host 6.1 -> restore in host 5.10
  • arm m6g all combinations, m7g host 4.14 and host 5.10

Also add templates that were not being exercised before: T2A, T2CL

Reason

The test was succeeding falsely.

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 Type: Bug Indicates an unexpected problem or unintended behavior Priority: High Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled Type: Documentation Indicates a need for improvements or additions to documentation labels Jun 3, 2024
@pb8o pb8o self-assigned this Jun 3, 2024
@pb8o pb8o changed the title Fix xrestore tests Fix cross-kernel snapshot/restore tests Jun 3, 2024
@pb8o pb8o added the Status: Awaiting author Indicates that an issue or pull request requires author action label Jun 3, 2024
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.10%. Comparing base (8dbfbfd) to head (8e90208).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4632   +/-   ##
=======================================
  Coverage   82.10%   82.10%           
=======================================
  Files         255      255           
  Lines       31280    31280           
=======================================
  Hits        25681    25681           
  Misses       5599     5599           
Flag Coverage Δ
4.14-c5n.metal 79.59% <ø> (ø)
4.14-c7g.metal ?
4.14-m5n.metal 79.58% <ø> (+<0.01%) ⬆️
4.14-m6a.metal 78.80% <ø> (ø)
4.14-m6g.metal 76.62% <ø> (ø)
4.14-m6i.metal 79.57% <ø> (ø)
4.14-m7g.metal 76.62% <ø> (?)
5.10-c5n.metal 82.11% <ø> (ø)
5.10-c7g.metal ?
5.10-m5n.metal 82.09% <ø> (ø)
5.10-m6a.metal 81.41% <ø> (ø)
5.10-m6g.metal 79.40% <ø> (ø)
5.10-m6i.metal 82.09% <ø> (ø)
5.10-m7g.metal 79.40% <ø> (?)
6.1-c5n.metal 82.11% <ø> (ø)
6.1-m5n.metal 82.09% <ø> (-0.01%) ⬇️
6.1-m6a.metal 81.40% <ø> (-0.01%) ⬇️
6.1-m6g.metal 79.40% <ø> (ø)
6.1-m6i.metal 82.09% <ø> (+<0.01%) ⬆️
6.1-m7g.metal 79.40% <ø> (?)

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 fix-xrestore-tests branch 4 times, most recently from b643012 to ee88c2a Compare June 3, 2024 14:42
@pb8o pb8o force-pushed the fix-xrestore-tests branch 2 times, most recently from 27b0b11 to a402224 Compare June 4, 2024 09:03
@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 Jun 4, 2024
roypat
roypat previously approved these changes Jun 4, 2024
ShadowCurse
ShadowCurse previously approved these changes Jun 4, 2024
@zulinx86
Copy link
Contributor

zulinx86 commented Jun 5, 2024

I really love this PR!! Just for future reference, let me post before/after for "we see one test case for each directory, instead of one test per CPU template, so it is clear what is being restored."

Before:

integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[C3] PASSED [ 33%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[T2] PASSED [ 66%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[T2S] PASSED [100%]

After:

integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[4.14.336_C3_guest_snapshot] PASSED [ 11%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[5.10.217_C3_guest_snapshot] PASSED [ 22%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[5.10.217-no-acpi_C3_guest_snapshot] PASSED [ 33%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[4.14.336_T2_guest_snapshot] PASSED [ 44%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[5.10.217-no-acpi_T2_guest_snapshot] PASSED [ 55%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[5.10.217_T2_guest_snapshot] PASSED [ 66%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[4.14.336_T2S_guest_snapshot] PASSED [ 77%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[5.10.217-no-acpi_T2S_guest_snapshot] PASSED [ 88%]
integration_tests/functional/test_snapshot_restore_cross_kernel.py::test_snap_restore_from_artifacts[5.10.217_T2S_guest_snapshot] PASSED [100%]

zulinx86
zulinx86 previously approved these changes Jun 5, 2024
Copy link
Contributor

@zulinx86 zulinx86 left a comment

Choose a reason for hiding this comment

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

We might want to split the first commit into two (one for rewrite and another one for removal of some combinations).

pb8o added 3 commits June 5, 2024 13:13
Since PR#3896 this test always succeeded because it did not find any
snapshot directories.

Rewrite and fix the test so that

- we see one test case for each directory, instead of one test per CPU
  template, so it is clear what is being restored.

- switch to creating the microvm through the API rather than a JSON file
  so that the Microvm/Snapshot classes can account for all the devices.

Signed-off-by: Pablo Barbáchano <pablob@amazon.com>
Remove the combinations that were added while this test was
falsely working but never worked:

- x86 snapshot in host 6.1 -> restore in host 5.10
- arm m6g all combinations, m7g host 4.14 and host 5.10

Signed-off-by: Pablo Barbáchano <pablob@amazon.com>
Re-use the logic that other tests use to get the list of templates.

This starts testing T2A and T2CL templates when applicable.

Signed-off-by: Pablo Barbáchano <pablob@amazon.com>
@pb8o pb8o dismissed stale reviews from zulinx86, ShadowCurse, and roypat via db8fe97 June 5, 2024 11:13
@pb8o pb8o merged commit e3893d3 into firecracker-microvm:main Jun 5, 2024
6 checks passed
@pb8o pb8o deleted the fix-xrestore-tests branch June 5, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Bug Indicates an unexpected problem or unintended behavior Type: Documentation Indicates a need for improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants