fix(runtime): compute real container CPU% from two samples#49
Conversation
ContainerStatsOneShot returns PreCPUStats=0, so the single-sample delta was the container's whole-lifetime CPU time over whole-lifetime system time — a garbage 'current CPU%' on the dashboard. Take two ContainerStatsOneShot samples one second apart and derive the percentage from the A->B delta; extract the pure math into cpuPercent() for unit testing. List() now samples all running containers concurrently so the added latency is ~one interval regardless of container count. Memory now matches 'docker stats' by subtracting MemoryStats.Stats[inactive_file] from Usage. Verified live against Colima: a single-core busy-loop container reports ~101% (was a meaningless lifetime average before). Unit tests cover the math + guards; a Docker-gated integration test (skipped under -short / no daemon) exercises the real path.
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDocker container resource sampling was reworked to use two time-separated ChangesDocker stats sampling rework
Estimated code review effort: 3 (Moderate) | ~25 minutes Related PRs: None identified. Suggested labels: runtime, testing, enhancement Suggested reviewers: None identified. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 58.81% 61.91% +3.10%
==========================================
Files 8 8
Lines 1748 1783 +35
==========================================
+ Hits 1028 1104 +76
+ Misses 639 597 -42
- Partials 81 82 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/runtime/runtime_test.go (1)
191-286: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on Docker calls in the integration test path.
dialTestDockerandTestDockerStatsIntegrationReportsLiveCPUusecontext.Background()for every Docker call (Ping,pullImage,ContainerCreate,ContainerStart,sampleStats,p.stats). If the daemon stalls (e.g., a wedged socket rather than an outright connection refusal), the test hangs instead of failing/skipping, tying up the CI runner instead of surfacing a clear failure.🐛 Proposed fix: bound the whole test with a timeout context
func TestDockerStatsIntegrationReportsLiveCPU(t *testing.T) { if testing.Short() { t.Skip("skipping docker integration test in -short mode") } cli := dialTestDocker(t) defer cli.Close() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runtime/runtime_test.go` around lines 191 - 286, The Docker integration path in dialTestDocker and TestDockerStatsIntegrationReportsLiveCPU uses unbounded context.Background() calls, so a stalled daemon can hang the test. Introduce a timeout-scoped context for the entire test flow and pass it through Ping, pullImage, ContainerCreate, ContainerStart, sampleStats, and p.stats, while keeping cleanup on a fresh background context if needed. Use the existing dialTestDocker and TestDockerStatsIntegrationReportsLiveCPU entry points to thread the bounded context consistently.internal/runtime/runtime.go (1)
291-321: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the stats fan-out. Per-index slot writes are race-safe, so the concurrency model itself is fine. But this spawns one goroutine per container with no ceiling, and each holds a Docker connection for ~1s across two
ContainerStatsOneShotreads. On a host with many managed containers that's a burst of simultaneous connections that can exhaust the client's transport pool and hammer the daemon. Cap it with a semaphore.♻️ Bound concurrency with a semaphore
out := make([]ContainerInfo, len(containers)) var wg sync.WaitGroup + sem := make(chan struct{}, 16) // cap concurrent stats sampling for i := range containers { wg.Add(1) go func(i int) { defer wg.Done() + sem <- struct{}{} + defer func() { <-sem }() c := containers[i]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runtime/runtime.go` around lines 291 - 321, The List path in runtime.go is fanning out one goroutine per container with no limit, which can overwhelm the Docker client/daemon when stats are sampled. Update the container iteration in the List-related logic so the existing p.stats calls still run concurrently but are bounded by a semaphore or worker cap, keeping the out[i] writes and WaitGroup flow intact while limiting simultaneous ContainerStatsOneShot requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/runtime/runtime_test.go`:
- Around line 191-286: The Docker integration path in dialTestDocker and
TestDockerStatsIntegrationReportsLiveCPU uses unbounded context.Background()
calls, so a stalled daemon can hang the test. Introduce a timeout-scoped context
for the entire test flow and pass it through Ping, pullImage, ContainerCreate,
ContainerStart, sampleStats, and p.stats, while keeping cleanup on a fresh
background context if needed. Use the existing dialTestDocker and
TestDockerStatsIntegrationReportsLiveCPU entry points to thread the bounded
context consistently.
In `@internal/runtime/runtime.go`:
- Around line 291-321: The List path in runtime.go is fanning out one goroutine
per container with no limit, which can overwhelm the Docker client/daemon when
stats are sampled. Update the container iteration in the List-related logic so
the existing p.stats calls still run concurrently but are bounded by a semaphore
or worker cap, keeping the out[i] writes and WaitGroup flow intact while
limiting simultaneous ContainerStatsOneShot requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa9c627c-4afe-4e39-bb3f-cb7fa82407c7
📒 Files selected for processing (2)
internal/runtime/runtime.gointernal/runtime/runtime_test.go
The Docker integration test skips on GitHub-hosted runners (no reachable daemon), so the two-sample orchestration was uncovered in CI. Extract the pure logic behind a one-method statsClient seam — sampleFromResponse, combineSamples, statsForContainers, toContainerInfo — and cover them with Docker-free unit tests (fake statsClient). runtime.go patch coverage now 98.48%; the integration test is unchanged and still verifies the live path on the mini.
1 participant
Problem
The dashboard showed garbage container CPU%.
DockerProvider.statsread a singleContainerStatsOneShotsample, but Docker returnsPreCPUStats = 0in one-shot mode — socpuDeltawas the container's entire lifetime CPU time andsystemDeltathe entire system time. The ratio is a lifetime average vs. total system time, not the instantaneous usage.Fix
ContainerStatsOneShotsamplescpuSampleInterval(1s) apart and derive CPU% from the A→B delta (respectsctxcancellation).cpuPercent(preTotal, curTotal, preSystem, curSystem, onlineCPUs)(deltas in float64 so a backwards counter trips the guard instead of underflowing) — unit-tested.List()samples all running containers concurrently (goroutine +WaitGroup, each writing its own slot), so added latency is ~one interval regardless of container count, not O(N·1s).docker statsby subtractingMemoryStats.Stats["inactive_file"]fromUsage(guarded).Verification (live, on Colima / Docker 29.5.2)
-short/ when no daemon): creates a uniquely-named throwaway busybox busy-loop container, runs the real two-samplestats(), asserts CPU% is finite/>0/<= onlineCPUs*100+slack, and force-removes it (never touches other containers).Observed live: a single-core busy loop reports ~101% (correct).
go build/vet/test -racegreen;test -race -shortgreen with the integration test correctly skipped.Summary by CodeRabbit
Bug Fixes
Tests