feat: switch to a per-device metrics model for vsock and rng devices - #5599
aerosouund wants to merge 10 commits into
Conversation
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.
|
Interesting thoughts for sure.
How does this fit with your previous statement of storing a single I think it sounds like a good way forward. Let me know if i misunderstood or you have any other opinions |
Yes, that's what I meant. It seems it would simplify your implementation for the rng device quite much |
3cd82a4 to
602c7a0
Compare
|
Hello @Manciukic I have switched to single instance model in the rng device using a |
8fee710 to
0d0d059
Compare
50c057d to
9b6b94c
Compare
|
@Manciukic |
Manciukic
added
the
Status: WIP
Manciukic
left a comment
Manciukic
left a comment
There was a problem hiding this comment.
I only skimmed through it but I think the approach is the right one. A few things to iron out:
- We can improve all those unit tests that are reading the old values, doing an operation, and reading the new value and comparing. This was needed because of the global metrics. They should all now become simple assertions on the value of the metric after the operation. We don't need that macro anymore as well
- We should split changes to different devices in different commits to make it simpler to review. The same for unrelated changes and the improvement to tests
- Please ensure the PR passes the formatter and buildchecks by running the devtool (I left instructions in one of the comments)
- We should do the same for all other devices to enable testing in parallel. If you have the time that'd be highly appreciated!
Thanks again for the contribution and sorry for the late reply!
Codecov Report❌ Patch coverage is Please upload reports for the commit fc7fe89 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## main #5599 +/- ##
==========================================
- Coverage 83.34% 83.19% -0.16%
==========================================
Files 277 277
Lines 31679 31603 -76
==========================================
- Hits 26404 26291 -113
- Misses 5275 5312 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
FYI I've kicked off the CI build: https://buildkite.com/firecracker/firecracker-pr/builds/15702 |
|
@Manciukic |
04af008 to
82034ed
Compare
ea5ea2e to
eb7d3bd
Compare
cbadb04 to
3914f89
Compare
|
@Manciukic Apart from the threads i have:
Regarding msix/mmds logger globals, i haven't taken a look at them in the context of this PR. I can do that if you'd like, or in a follow up which would also include the change of |
0dfa129 to
75fa321
Compare
3a8efc5 to
f01c9ba
Compare
|
Independently verified the current head (f01c9ba) on a clean checkout:
So the metrics rework itself is parallel-clean and clippy-clean. |
|
@Manciukic |
- turn the METRICS static variable into an rwlock of an option of an arc of BalloonDeviceMetrics. There can only be one balloon device per guest. the device creates the arc during init and sets it as METRICS - replace access to METRICS for incrementing with self.metrics - serialize an empty BalloonDeviceMetrics instance when METRICS has not been set so the schema stays stable - replace check_metric_after_block with traditional asserts - make report_balloon_event_fail take an arc of BalloonDeviceMetrics as an argument to avoid having to reset the METRICS instance after every test. given that each device will instantiate the metrics instance during the start of every test - remove the comparison of the empty local metrics instance with the flushed global instance in test_balloon_dev_metrics Signed-off-by: aerosouund <aerosound161@gmail.com>
- delete the BlockMetricsPerDevice type and make METRICS an rwlock of a btreemap directly, keyed on the block device id - make a device create its own arc of BlockDeviceMetrics during init then add it to the METRICS map - replace access to METRICS for incrementing with self.metrics - replace check_metric_after_block with traditional asserts - change values in tests with asserts that check the total not the delta Signed-off-by: aerosouund <aerosound161@gmail.com>
- delete the PmemMetricsPerDevice type - turn METRICS into an rwlock of the metrics btreemap directly - make a device create its own metrics instance then add it to the METRICS map which is keyed on the device ids - delete test_max_pmem_dev_metrics test because if the we remove reliance on the METRICS instance, it just becomes a test that checks counters incremenet for a group of metrics instance in a vector. meaning that it adds nothing over test_single_pmem_dev_metrics - replace test_single_pmem_dev_metrics with test_pmem_metrics_aggregation which exercises aggregate() and the serialized output on local instances instead of the global Signed-off-by: aerosouund <aerosound161@gmail.com>
- turn the METRICS static variable into an rwlock of an option of an arc of EntropyDeviceMetrics. There can only be one rng device per guest. the device creates the arc during init and sets it as METRICS - replace access to METRICS for incrementing with self.metrics - serialize an empty EntropyDeviceMetrics instance when METRICS has not been set so the schema stays stable - replace check_metric_after_block with traditional asserts - change test values to assert the absolute value not the delta - depend on a local instance in test_rng_dev_metrics Signed-off-by: aerosouund <aerosound161@gmail.com>
- delete the VhostUserMetricsPerDevice type and make METRICS an rwlock of a btreemap directly, keyed on the vhost-user device id - make a device create its own arc of VhostUserDeviceMetrics during init then add it to the METRICS map instead of going through alloc() - drop the now unused per-device allocation helper and its tests Signed-off-by: aerosouund <aerosound161@gmail.com>
- delete the NetMetricsPerDevice type and make METRICS an rwlock of a btreemap directly, keyed on the net device id field. - during net device initialization push the an arc of device metrics to the metrics map. - replace access to METRICS for incrementing with self.metrics - replace check_metric_after_block with traditional asserts that check the absolute count not the delta - remove reliance on the global metrics instance in test_max_net_dev_metrics and test_single_net_dev_metrics by running tests against local instances Signed-off-by: aerosouund <aerosound161@gmail.com>
- turn the METRICS static variable into an rwlock of an option of an arc of VirtioMemDeviceMetrics. There can only be one mem device per guest. the device creates the arc during init and sets it as METRICS - replace access to METRICS for incrementing with self.metrics - add derive default to VirtioMemDeviceMetrics - serialize an empty VirtioMemDeviceMetrics instance when METRICS has not been set so the schema stays stable - make the device tests assert absolute metric counts instead of a captured baseline plus a delta Signed-off-by: aerosouund <aerosound161@gmail.com>
- turn the METRICS static variable to an Rwlock of option of arc of VsockDeviceMetrics. There can only be one vsock device per guest. - the vsock multiplexer is the central piece and the entity that gets created first among the different vsock structs. make it create an arc of VsockDeviceMetrics and set that as the METRICS variable. - thread that arc through the vsock device, its connections and the persist paths instead of reaching for the global. - remove check_metrics_after_block use and replace it with traditional asserts. update the assertion values in select tests to make it reflect the real value not the delta Signed-off-by: aerosouund <aerosound161@gmail.com>
- it has been replaced by pure asserts Signed-off-by: aerosouund <aerosound161@gmail.com>
- its not in use anywhere after the previous commits Signed-off-by: aerosouund <aerosound161@gmail.com>
3 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.