vmm_tests: memory validation CI/CD pipeline tests for main #1995
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds VTL2 memory usage validation tests to the VMM test pipeline to track memory consumption changes and detect builds that exceed acceptable memory usage thresholds.
Key Changes
- Added a new
MemStatmodule to collect VTL2 memory usage information from processes likeunderhill_init,openvmm_hcl, andunderhill_vm - Implemented four new test functions that evaluate memory usage across different VM configurations (2VP and 64VP with 16GB VTL0 memory)
- Added JSON logging of memory statistics for baseline comparison and monitoring
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| petri/src/memstat.rs | New module implementing memory statistics collection from VTL2 processes via /proc filesystem |
| petri/src/lib.rs | Exposed the new memstat module as public API |
| vmm_tests/vmm_tests/Cargo.toml | Added serde_json dependency for JSON serialization of memory statistics |
| vmm_tests/vmm_tests/tests/tests/multiarch.rs | Added four new test functions for memory validation across different VM configurations and architectures |
petri/src/memstat.rs
Outdated
| pub fn clone(&self) -> PerProcessMemstat { | ||
| PerProcessMemstat { | ||
| smaps_rollup: self.smaps_rollup.clone(), | ||
| statm: self.statm.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Manual clone implementation is unnecessary. Derive Clone trait instead for better maintainability and consistency.
petri/src/memstat.rs
Outdated
| underhill_init: per_process_data["underhill_init"].clone(), | ||
| openvmm_hcl: per_process_data["openvmm_hcl"].clone(), | ||
| underhill_vm: per_process_data["underhill_vm"].clone(), |
There was a problem hiding this comment.
Using bracket notation for HashMap access will panic if the expected process names are not found. Use .get() with proper error handling or .unwrap_or_default() to prevent panics.
petri/src/memstat.rs
Outdated
| let field = split_line[field_col].trim_matches(':').to_string(); | ||
| let value: u64 = split_line[value_col].parse().unwrap(); | ||
| parsed_data.insert(field, value); |
There was a problem hiding this comment.
Array indexing without bounds checking will panic if the line doesn't have enough columns. Add bounds checking before accessing array elements.
| let field = split_line[field_col].trim_matches(':').to_string(); | |
| let value: u64 = split_line[value_col].parse().unwrap(); | |
| parsed_data.insert(field, value); | |
| if split_line.len() > field_col && split_line.len() > value_col { | |
| let field = split_line[field_col].trim_matches(':').to_string(); | |
| let value: u64 = split_line[value_col].parse().unwrap(); | |
| parsed_data.insert(field, value); | |
| } |
petri/src/memstat.rs
Outdated
| use serde::Serialize; | ||
| use std::collections::HashMap; | ||
|
|
||
| #[expect(missing_docs)] |
There was a problem hiding this comment.
Public structs and methods should have proper documentation instead of using #[expect(missing_docs)]. Add doc comments explaining the purpose and fields of each struct.
petri/src/memstat.rs
Outdated
| pub statm: HashMap<String, u64>, | ||
| } | ||
|
|
||
| #[expect(missing_docs)] |
There was a problem hiding this comment.
Public structs and methods should have proper documentation instead of using #[expect(missing_docs)]. Add doc comments explaining the purpose and fields of each struct.
|
|
||
| vm.send_enlightened_shutdown(ShutdownKind::Shutdown).await?; | ||
| vm.wait_for_clean_teardown().await?; | ||
| assert!(true); |
There was a problem hiding this comment.
assert!(true) is meaningless and should be removed. These assertions provide no validation and add unnecessary code.
| assert!(true); |
|
|
||
| vm.send_enlightened_shutdown(ShutdownKind::Shutdown).await?; | ||
| vm.wait_for_clean_teardown().await?; | ||
| assert!(true); |
There was a problem hiding this comment.
assert!(true) is meaningless and should be removed. These assertions provide no validation and add unnecessary code.
|
|
||
| agent.power_off().await?; | ||
| vm.wait_for_teardown().await?; | ||
| assert!(true); |
There was a problem hiding this comment.
assert!(true) is meaningless and should be removed. These assertions provide no validation and add unnecessary code.
| assert!(true); |
|
|
||
| agent.power_off().await?; | ||
| vm.wait_for_teardown().await?; | ||
| assert!(true); |
There was a problem hiding this comment.
assert!(true) is meaningless and should be removed. These assertions provide no validation and add unnecessary code.
| assert!(true); |
| let now = std::time::Instant::now(); | ||
| let wait_time = Duration::from_secs(60); | ||
| std::thread::sleep(wait_time); | ||
| assert!(now.elapsed() >= wait_time); |
There was a problem hiding this comment.
The assertion after sleep is redundant since sleep guarantees the wait time has elapsed. This check adds no value and should be removed.
| assert!(now.elapsed() >= wait_time); |
vmm_tests/vmm_tests/Cargo.toml
Outdated
| workspace = true | ||
|
|
||
| [dependencies] | ||
| serde_json.workspace = true |
There was a problem hiding this comment.
Move this up to dev-dependencies with everything else.
| openvmm_openhcl_uefi_x64(vhd(ubuntu_2204_server_x64)) | ||
| )] | ||
| async fn validate_mnf_usage_in_guest( | ||
| async fn validate_mnf_usage_in_guest_reboot( |
| }) | ||
| .run_without_agent() | ||
| .await?; | ||
| std::thread::sleep(Duration::from_secs(60)); |
There was a problem hiding this comment.
We shouldn't ever need to sleep in a test.
dpaliulis-msft
force-pushed
the
user/dpaliulis/vmm_tests-memory_validation
branch
from
f2ad497 to
34081db
Compare
…tests-memory_validation
…#1995) Description: This PR updates the VMM test pipeline to evaluate VTL2 memory usage. This is done to track changes in memory usage and produce a failed test for builds that result in an increase in memory usage beyond reasonable variance. **Changes** - Implement module to collect all relevant VTL2 memory usage information - Add tests for VMs with 2VPs and 64VPs with 16GB VTL0 memory across TDX, SNP, and ARM architectures - Add test for VMs with 2VPs and 32VPs with 16GB VTL0 memory for GP x64 VMs - Log memory usage information as JSON - Produce a failed test for builds that result in memory usage that exceeds reasonable variance from baseline values
2 participants
Description:
This PR updates the VMM test pipeline to evaluate VTL2 memory usage. This is done to track changes in memory usage and produce a failed test for builds that result in an increase in memory usage beyond reasonable variance.
Changes