feat: respect CPU and cgroup limits in server-ng and connectors stats#3647
Conversation
CPU set and cgroup memory cap, but server-ng and the connectors runtime kept reporting host-wide sysinfo numbers: a confined instance showed its neighbors' CPU load and a memory total it can never allocate, leaking host sizing to anyone who can read the stats endpoint. Extract the scoping into a shared system_stats crate (scoped_total_cpu_usage, cgroup_scoped_memory) and use it from the legacy server, server-ng and the connectors runtime. The connectors runtime also built a fresh sysinfo System on every call, so its CPU fields (deltas over the previous refresh) were always zero. Keep one process-wide System so readings are real from the second probe on.
fbd018f to
6f2c3fa
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3647 +/- ##
=============================================
- Coverage 73.42% 57.79% -15.64%
Complexity 937 937
=============================================
Files 1285 1284 -1
Lines 140958 124703 -16255
Branches 116848 100634 -16214
=============================================
- Hits 103503 72071 -31432
- Misses 34228 49570 +15342
+ Partials 3227 3062 -165
🚀 New features to boost your workflow:
|
hubcio
left a comment
There was a problem hiding this comment.
one finding lands outside the diff so it goes here: ConnectorRuntimeStats in core/connectors/sdk/src/api.rs now carries cpuset/cgroup-scoped total_cpu_usage / total_memory / available_memory but the fields have no rustdoc - legacy Stats got per-field scoping notes in #3615, worth mirroring the memory ones here (hold the cpu wording until the affinity issue below is fixed, right now it would document a value the servers don't produce). release notes should also mention the value change for server-ng stats: total/available memory become the cgroup cap on confined hosts. wire-compatible, but dashboards and SDK consumers see different numbers.
hubcio
left a comment
There was a problem hiding this comment.
all findings from the earlier review are fixed as of fb6d2f8 - verified each one: boot-time capture_allowed_cpus() snapshot runs on the main thread before any shard pinning in all three binaries, and the .get()-not-get_or_init read means a missed capture falls back to host-global instead of lazily grabbing a pinned thread's mask; System::new() swap landed everywhere including the two pre-existing sites; positive averaging test + None-branch assertion added; cgroup_scoped_memory got the calling-process doc and went private; Pid bound once. cargo test -p system_stats green, fmt + clippy clean.
two leftovers, neither blocking:
ConnectorRuntimeStatsincore/connectors/sdk/src/api.rsstill has no per-field rustdoc ontotal_cpu_usage/total_memory/available_memory, which now report cpuset/cgroup-scoped values on confined hosts. legacyStatsincore/common/src/types/stats/mod.rsgot per-field scoping notes in #3615 - worth mirroring, and the cpu wording is safe to write now that the affinity snapshot landed.- the PR description still names
scoped_total_cpu_usage/cgroup_scoped_memoryas the crate's api (both are private now - the surface isSystemProbe::capture+capture_allowed_cpus) and doesn't mention the affinity boot snapshot or that stats values change for dashboards on confined hosts. worth a refresh before merge, it doubles as the release note.
The server-ng and connectors runtime stats endpoints reported
host-wide sysinfo numbers: a cpuset-confined or memory-capped
process showed its neighbors' CPU load and a memory total it can
never allocate, leaking host sizing on multi-tenant machines. The
legacy server already scoped these (#3615) but kept the logic
inline.
Extract that logic into a shared system_stats crate: SystemProbe
scopes total CPU usage to the allowed core set (snapshotted at
startup, before shard threads pin themselves) and memory totals
to the effective cgroup cap, falling back to host-wide values for
unconfined processes. All three binaries now probe through it.
On confined hosts the reported total_cpu_usage, total_memory and
available_memory now reflect the confinement, so dashboards and
alerts keyed to the old host-wide values will see lower numbers.
The connectors runtime also keeps its sysinfo System alive across
captures, so CPU usage reflects real deltas instead of the zero
first sample of a freshly created System on every request.