firecracker-microvm / firecracker-microvm/firecracker

Redesign metrics system with thread-safety in mind

Open
#2,899 9 comments 0 reactions 0 assignees View on GitHub
Status: Parked Type: Enhancement
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.