firecracker-microvm / firecracker-microvm/firecracker
Redesign metrics system with thread-safety in mind
- Dominant language
- Rust
- Stars
- 36.7k
- Forks
- 2.6k
- Avg merge
- 3d 3h
- Merged PRs (30d)
- 58
Description
The metrics system is not fully thread-safe at the moment, due to some issues:
1. `IncMetrics` inner state is mutated on serialisation. This causes race conditions when the `write()` function is called from multiple threads. See: https://github.com/firecracker-microvm/firecracker/pull/2893
2. While `SharedIncMetrics` use atomics, they always use `Relaxed` ordering. While on x86 memory access has Acquire-Release semantics, on Arm this is not the case. Hence, the process of writing metrics to file may use outdated values.
3. Metrics are written from the signal handler, which may cause a deadlock if a thread is preempted by a signal while it was holding the metrics file lock.
Problems 1 and 3 can be fully solved by removing metrics usage from the signal handler. One option here is to have a special file used for logging the exit reason and the latest metrics values, similar to a coredump. We should also enforce that `METRICS.write()` is called from a single thread (and therefore removing the `lazy_static` declaration).
Problem 2 could be solved in two ways: by using tighter ordering constraints (need some further dive deep and may incur some overhead due to CPU reordering constraints and prevention of certain compiler optimisations) or by redesigning the metrics system to use per-thread values (this would also solve problem 1).
Another thing to keep in mind is potential need for having device-specific instances of a metric. For example, `METRICS.net.tx_bytes_count` may have sense to be reported per-device instance instead of being aggregated.
Contributor guide
Assessment
This issue has not been assessed yet.