feat: switch to a per-device metrics model for vsock and rng devices #5599
Conversation
aerosouund
force-pushed
the
vmm-parallel
branch
4 times, most recently
from
756a589 to
ddcbe7a
Compare
to allow for running vmm tests in parallel Signed-off-by: aerosouund <[email protected]>
aerosouund
force-pushed
the
vmm-parallel
branch
from
ddcbe7a to
3c724e5
Compare
Manciukic
left a comment
Manciukic
left a comment
There was a problem hiding this comment.
Thanks for the contibution!
I only had a quick look, but I think we can simplify the logic for devices that only allow a single instance. Maybe a simple RwLock<Arc<Metrics>> would do the job.
Once we align on the solution for single-entity devices, we should also tackle all other devices using global metrics (like mem and pmem which we recently added), and simplify their unit tests as well.
Please keep different devices in different commits for clarity (and feel free to tackle one at a time if you prefer).
I'm also not a big fan of the current per-device metrics for net and block sharing the same metrics Arc if the ID happens to be the same, which defeats the purpose of not sharing metrics. As in real life we can't have devices with the same ID, I'd rather have the device create the object and replace whatever is in the hashmap rather than the other way around.
| assert_eq!(b"tap0\0\0\0\0\0\0\0\0\0\0\0\0", &tap.if_name); | ||
| assert_eq!("tap0", tap.if_name_as_str()); | ||
| let tap_name_str = tap.if_name_as_str(); | ||
|
|
||
| // Check that it starts with "tap" and the remainder is numeric. | ||
| assert!( | ||
| Regex::new(r"^tap\d+$").unwrap().is_match(tap_name_str), | ||
| "Generated tap name '{}' does not match expected pattern", | ||
| tap_name_str | ||
| ); |
There was a problem hiding this comment.
this change looks unrelated. Why is it needed?
| pub struct EntropyMetricsPerDevice { | ||
| pub metrics: BTreeMap<String, Arc<EntropyDeviceMetrics>>, | ||
| } | ||
|
|
||
| impl EntropyMetricsPerDevice { | ||
| pub fn alloc(device_id: String) -> Arc<EntropyDeviceMetrics> { | ||
| Arc::clone( | ||
| METRICS | ||
| .write() | ||
| .unwrap() | ||
| .metrics | ||
| .entry(device_id) | ||
| .or_insert_with(|| Arc::new(EntropyDeviceMetrics::default())), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| static METRICS: RwLock<EntropyMetricsPerDevice> = RwLock::new(EntropyMetricsPerDevice { | ||
| metrics: BTreeMap::new(), | ||
| }); | ||
|
|
||
| #[derive(Debug, Serialize, Default)] | ||
| pub struct EntropyDeviceMetrics { |
There was a problem hiding this comment.
I don't think we should copy the whole "PerDevice" pattern for devices that only support a single instance.
The main idea of the change is correct: decoupling the device object from the global METRICS so unit tests are independent. However, we can probably simplify this part to just hold a single Arc in the RwLock, with the Arc being created by the device (and not the other way around like in net and block, as that will make it being reused).
| check_metric_after_block!( | ||
| &METRICS.entropy_event_fails, | ||
| &dev.metrics.entropy_event_fails, | ||
| 1, | ||
| dev.process_rate_limiter_event() | ||
| ); |
There was a problem hiding this comment.
We should not need this macro hack anymore, and we could simplify this code block back to just checking the metric is now 1.
dev.process_rate_limiter_event();
assert_eq!(dev.metrics.entropy_event_fails, 1);
2 participants
Changes
Building on the original PR and addressing the reviewer comments.
With the following remarks:
Reason
Closes #4709
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.