fix(metrics): normalize AMD core allocation ratio to percentage - #2527
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughScheduler metrics now resolve devices by UUID and convert MiB values through a shared helper. AMD node and container core allocations use normalized percentages. Legacy metrics retain raw core allocations. Tests cover lookup, conversion, and normalization. ChangesAMD core metric normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/scheduler/config/config.go`:
- Around line 281-294: The InitDevices flow must not retain partially
initialized global state when InitDevicesWithConfig fails. Update InitDevices
and/or InitDevicesWithConfig so device.DevicesMap is published only after all
backends succeed, or is cleared before returning an initialization error,
allowing subsequent calls to retry failed backends.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9422fe52-f8f3-41d0-b764-ea036486fe3e
📒 Files selected for processing (6)
cmd/scheduler/main.gocmd/scheduler/metrics.gocmd/scheduler/metrics_test.gopkg/scheduler/config/config.gopkg/scheduler/config/config_test.gopkg/scheduler/nodes_test.go
e14e10f to
00c74e9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/scheduler/metrics_test.go (1)
208-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for node and legacy core metrics.
Line 211 selects only
hami_vgpu_core_allocated_ratio. This test cannot detect a regression inhami_gpu_core_allocated_ratioor a change to the legacy raw core metric.Set
DeviceUsage.Usedcoresto32and assert the node metric reports50. Add a legacy-enabled assertion that the legacy container metric remains32.🤖 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 `@cmd/scheduler/metrics_test.go` around lines 208 - 212, Extend the test around CollectAndCompare to set DeviceUsage.Usedcores to 32 and validate hami_gpu_core_allocated_ratio reports 50. Add a separate legacy-enabled assertion confirming the legacy raw core metric remains 32, while preserving the existing hami_vgpu_core_allocated_ratio check.
🤖 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 `@cmd/scheduler/metrics_test.go`:
- Around line 208-212: Extend the test around CollectAndCompare to set
DeviceUsage.Usedcores to 32 and validate hami_gpu_core_allocated_ratio reports
50. Add a separate legacy-enabled assertion confirming the legacy raw core
metric remains 32, while preserving the existing hami_vgpu_core_allocated_ratio
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42521791-d98c-4073-8fac-e8b52a578d1e
📒 Files selected for processing (2)
cmd/scheduler/metrics.gocmd/scheduler/metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/scheduler/metrics.go
Address review feedback on Project-HAMi#2527: extend the AMD regression test to also assert hami_gpu_core_allocated_ratio == 50 at node level (setting DeviceUsage.Usedcores = 32 on the 64-CU device) and add a legacy-enabled assertion that vGPUCoreAllocated keeps the raw CU count (32) for backward compatibility. Signed-off-by: yxxhero <aiopsclub@163.com>
|
Thanks for the review! Addressed in 58b852c:
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
…tage The container-level hami_vgpu_core_allocated_ratio metric emitted ContainerDevice.Usedcores directly. For AMD devices Usedcores stores physical compute-unit (CU) counts (cuCount = percentage * Totalcore / 100), so the metric reported the raw CU count instead of a normalized percentage: a 50% request on a 64-CU device was stored as 32 and emitted as 32 rather than 50. Node-level normalization already landed via normalizeAMDCoreMetrics, but the container-level metric was missed (issue Project-HAMi#2518). Reuse that helper for the container metric by resolving the matching node device's Totalcore and Type, and leave legacy metrics untouched for backward compatibility. Supporting readability refactors (no behavior change): - Add findNodeDeviceUsage to replace the inline UUID lookup (previously a double loop with a break flag) and drop the dead Totalmem capture. - Add mibToBytes and replace all eight float64(x)*float64(1024)*float64(1024) occurrences (node- and container-level, new and legacy). - Hoist the repeated container label set into a single containerLabels slice and compute usedMemBytes once per container device. Tests: - TestAMDCoreAllocatedRatioNormalization asserts hami_vgpu_core_allocated_ratio == 50 and hami_gpu_core_allocated_ratio == 50 for an AMD container with 32 CUs used on a 64-CU device, and that the legacy vGPUCoreAllocated stays 32. - TestFindNodeDeviceUsage and TestMibToBytes cover the new helpers. Fixes Project-HAMi#2518 Signed-off-by: yxxhero <aiopsclub@163.com>
26cb85a to
33eedd3
Compare
Collect had grown to ~250 lines mixing three independent concerns: node device metrics, resource quota usage, and per-container metrics, each with its own descriptor set. Decompose it into a thin orchestrator that takes a single shared node-usage snapshot and delegates to: - collectNodeMetrics: node-level limits/allocations, sharing, MIG, overview - collectQuotaMetrics: per-namespace resource quota usage - collectContainerMetrics: per-container vGPU metrics (incl. AMD normalization) Each helper owns only the descriptors it emits, replacing the single large shared descriptor block. This is a pure structural relocation with no behavior change: every metric name, help text, label set, and emitted value is byte-identical (verified by diffing the sorted set of NewDesc/send* calls before and after, plus the existing pedantic-registry and CollectAndCompare tests). Signed-off-by: yxxhero <aiopsclub@163.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FouoF, yxxhero The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
The container-level
hami_vgpu_core_allocated_ratiometric emittedContainerDevice.Usedcoresdirectly. For AMD devicesUsedcoresstores physical compute-unit (CU) counts (cuCount = percentage × Totalcore / 100, see the AMD design doc), so the metric reported the raw CU count instead of a normalized percentage.For example, a 50% core request on a 64-CU device is stored as
Usedcores = 32, but the collector emitted:while the expected value is
ceil(32 / 64 × 100) = 50.Node-level normalization for AMD already landed via
normalizeAMDCoreMetrics, but the container-level metric was missed — which is exactly what issue #2518 reports.This PR:
normalizeAMDCoreMetrics(deviceType, total, allocated)helper for the container-levelhami_vgpu_core_allocated_ratio.Totalmem) to also resolve the device'sTotalcoreandType, then normalizes before emitting the metric.TestContainerCoreAllocatedRatioNormalizedForAMD) assertinghami_vgpu_core_allocated_ratio == 50for an AMD container with 32 CUs used on a 64-CU device.Which issue(s) this PR fixes:
Fixes #2518
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
This PR was prepared with AI assistance and is disclosed per CONTRIBUTING.md.
Summary by CodeRabbit