fix(unraid): report correct CPU utilization on hyper-threaded systems - #6250
fix(unraid): report correct CPU utilization on hyper-threaded systems#6250junkerderprovinz wants to merge 2 commits into
Conversation
🚨 Preview Deployment Blocked - Security ProtectionYour pull request was blocked from triggering preview deployments Why was this blocked?
How to resolve this:Option 1: Get Collaborator Access (Recommended) Option 2: Request Permission Override For Repository Administrators:To disable this security check ( This security measure protects against malicious code execution in preview deployments. Only trusted collaborators should have the ability to trigger deployments. 🛡️ Learn more about this security featureThis protection prevents unauthorized users from:
Preview deployments are powerful but require trust. Only users with repository write access can trigger them. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughCPU utilization reporting now uses whole-system percentage calculations. Unraid reads its overall CPU metric directly, while Docker removes per-core scaling and updates validation cases for the revised formula. ChangesCPU Utilization Calculations
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/integrations/src/unraid/unraid-integration.ts (1)
87-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove now-unused
cpus { percentTotal }sub-query.Since
cpuUtilizationno longer averages per-thread values, the per-CPUcpus { percentTotal }field fetched here is unused dead data in the response.♻️ Proposed fix
cpu { percentTotal - cpus { - percentTotal - } },🤖 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 `@packages/integrations/src/unraid/unraid-integration.ts` around lines 87 - 92, The Unraid CPU query still fetches unused per-CPU data, so remove the now-dead cpus percentTotal sub-selection from the query built in unraid-integration.ts. Update the GraphQL selection around the cpu block so only the top-level cpu percentTotal is requested, and keep the rest of the response shape unchanged in the Unraid integration logic.
🤖 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 `@packages/integrations/src/unraid/unraid-integration.ts`:
- Around line 87-92: The Unraid CPU query still fetches unused per-CPU data, so
remove the now-dead cpus percentTotal sub-selection from the query built in
unraid-integration.ts. Update the GraphQL selection around the cpu block so only
the top-level cpu percentTotal is requested, and keep the rest of the response
shape unchanged in the Unraid integration logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7015b66-fa72-4e1d-80e8-87dac244abd3
📒 Files selected for processing (1)
packages/integrations/src/unraid/unraid-integration.ts
|
Somewhat related (although I understand the issue you're solving here), your previous PR with Docker (#6249) made it so that the CPU can go over 100%. Users are complaining that they are not expecting this kind of reading going over 100%. I think it might be beneficial to define and think about how we want to show CPU usage across the whole homarr to normalize it. I am of the opinion that seeing the strain on the total system |
|
@ajnart Really good point, and I agree this is worth settling consistently across homarr rather than per-widget. The two readings just answer different questions: per-core (can go over 100%) is "how hard is this container working the cores it has", like docker stats or Portainer, so 134% means ~1.34 cores busy, while total-system % (0-100%) is "how much of the whole machine is it using", like Beszel, which is more intuitive at a glance and never surprises anyone. For a dashboard I think you're right that total-system % is the better default: 3.5% is immediately meaningful and nobody has to wonder why a number went past 100%. My suggestion would be to normalize to total-system % everywhere by default (divide by the core count), and keep the per-core detail as an option or a tooltip if we still want it. This PR is really just making the per-core number correct on hyper-threaded Unraid hosts in the first place, so it slots in under either choice. I'm happy to adapt it (and revisit #6249) to emit the normalized total-system % so everything lines up. Just say the word and I'll push the change. Thanks for thinking this through with me! |
|
@junkerderprovinz feel free to push the change here as well, then we'll merge both inside of this PR |
4b4e692 to
1dc4fa0
Compare
|
@ajnart Done! I rebased onto main and pushed the Docker normalization into this PR (1dc4fa0). The change itself is tiny: Tests updated accordingly, plus a new case asserting that multiple fully busy cores can never push the reading past 100% (18/18 passing locally). Thanks for pushing for consistency here, I think the dashboard is better for it! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/request-handler/src/test/docker.spec.ts (1)
79-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a boundary test for exactly 100%.
The test title claims "should never exceed 100%" but only asserts 75% (3 of 4 cores busy). Adding a case where all cores are fully busy (
cpuDelta == systemDelta) would verify the function returns exactly 100 at the boundary, strengthening the guarantee.💡 Suggested additional test
test("should report exactly 100% when all cores are fully busy", () => { const stats = createStats({ cpu_stats: { online_cpus: 4, cpu_usage: { total_usage: 10000 }, system_cpu_usage: 10000 }, }); // All 4 cores busy: (10000 / 10000) * 100 = 100 expect(calculateCpuUsage(stats)).toBe(100); });🤖 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 `@packages/request-handler/src/test/docker.spec.ts` around lines 79 - 86, Add a boundary test alongside the existing calculateCpuUsage coverage in docker.spec.ts for cpuDelta equal to systemDelta, using fully busy cores and asserting calculateCpuUsage returns exactly 100. Keep the existing 75% multi-core test unchanged.
🤖 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 `@packages/request-handler/src/test/docker.spec.ts`:
- Around line 79-86: Add a boundary test alongside the existing
calculateCpuUsage coverage in docker.spec.ts for cpuDelta equal to systemDelta,
using fully busy cores and asserting calculateCpuUsage returns exactly 100. Keep
the existing 75% multi-core test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b83551ab-0fb9-466a-94dd-9392d582872e
📒 Files selected for processing (3)
packages/integrations/src/unraid/unraid-integration.tspackages/request-handler/src/docker.tspackages/request-handler/src/test/docker.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/integrations/src/unraid/unraid-integration.ts
The Unraid integration summed the per-thread CPU percentages and divided the total by the number of physical cores. On hyper-threaded systems there are more logical CPUs (threads) than physical cores, so the result was inflated — roughly doubled — and could even exceed 100% (e.g. a real 43% load reported as ~93%). Use the overall CPU utilization the Unraid API already reports (`metrics.cpu.percentTotal`, 0-100), which is the value Unraid itself displays, instead of recomputing it from the per-thread percentages. Closes homarr-labs#6082
Drop the docker-stats per-core convention (* online_cpus), which produced readings like 134% and surprised users. system_cpu_usage accumulates across all cores, so the plain delta ratio is the share of the whole machine - the same 0-100% scale the Unraid integration now reports, keeping CPU readings consistent across homarr.
1dc4fa0 to
9dcb9f4
Compare
|
Rebased onto the latest |
|
Hi @ajnart — this is rebased onto the latest |
|
Hi @ajnart, following up after a couple of weeks. This is still rebased onto |
Closes #6082
Problem
The Unraid integration reports CPU load that is roughly double the real value and can exceed 100% — a reporter with a real ~43% load saw ~93% in Homarr (and occasionally 105–108%).
Cause
metrics.cpu.cpus[]has one entry per logical CPU (thread), eachpercentTotalbeing 0–100. The sum is divided byinfo.cpu.cores(physical cores). On a hyper-threaded system there are ~2× as many threads as cores (e.g. 24 threads / 12 cores), so the average is divided by the wrong (smaller) number and comes out inflated.Fix
The GraphQL query already fetches
metrics.cpu.percentTotal— the API's own overall CPU utilization (0–100), which is exactly what the Unraid dashboard displays. Use it directly instead of recomputing from the per-thread values:For the reporter's machine this yields ~43%, matching Unraid.
Checklist
tsc --noEmitclean on@homarr/integrations)oxfmtformatted,oxlintcleandev; conventional commit; no shorthand variable namesNo unit test added: the integration test suites run only against a real instance (they
skipIfwithout credentials), and after this change the value is a direct pass-through of the API's own aggregate, so there is no computation left to unit-test. Happy to add a mocked test if you'd prefer.Summary by CodeRabbit