perf: remove unused all-device scan in metrics collector - #2166
perf: remove unused all-device scan in metrics collector#2166Gaurav-205 wants to merge 1 commit into
Conversation
Signed-off-by: Gaurav-205 <gauravkhandelwal205@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Gaurav-205 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughScheduled pod snapshots now deep-copy device data while omitting ChangesScheduled pod snapshot isolation
Scheduler metrics cleanup
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 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 |
|
@wawa0210 @lengrongfu hey, can you please review this pr!! |
| } | ||
| klog.V(4).InfoS("Total memory for device", | ||
| "deviceUUID", ctrdevval.UUID, | ||
| "totalMemory", totaldev, |
There was a problem hiding this comment.
Great find! This looks reasonable tbh. The removed scan only feeds a verbose log line and does not affect any emitted metric. One question tho... was this totalMemory log used for any debugging flow, or is the node memory limit metric enough for that case?
There was a problem hiding this comment.
Thanks! I checked the full metrics collector flow. totaldev was only used by that klog.V(4) message; it does not affect emitted metrics, scheduling, or allocation.
For debugging device capacity, the collector already exposes the same per-device total-memory information through hami_gpu_memory_limit_bytes and hami_node_gpu_overview (device_memory_limit). So the verbose log is redundant, and removing the scan does not reduce Prometheus visibility.
If maintainers prefer retaining a V(4) log for troubleshooting, I can add one using the already available per-device data, without restoring the all-node scan.
There was a problem hiding this comment.
also the description doesnt follow the pr template, no "what type of pr is this?" block or /kind line, and no ai disclosure. if any ai tool was used it has to be disclosed, see https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
fyi #2170 is open on this same file, worth watching the order.
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
the loop u deleted broke on the first uuid hit while ranging *nu, which is a map, and the pods node was already sitting in val.NodeID, so what did that V(4) line print if the same uuid ever turned up under 2 nodes?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/pod_test.go (1)
157-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop after a failed scheduled-pod lookup.
assert.Trueis non-fatal; a failed lookup makesgotnil and line 161 panics. Return after the assertion so regressions report the actual failed condition.Proposed fix
got, ok := scheduledPods[pod1.UID] -assert.True(t, ok) +if !assert.True(t, ok) { + return +} // 1. Existing Pod pointer is kept🤖 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 `@pkg/device/pod_test.go` around lines 157 - 161, Make the scheduled-pod lookup assertion in the relevant test fail fast: after checking the ok result for scheduledPods[pod1.UID], return immediately when the lookup fails before dereferencing got in the assert.Same call. Preserve the existing pointer-preservation assertion for successful lookups.
🤖 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 `@pkg/device/pod_test.go`:
- Around line 157-161: Make the scheduled-pod lookup assertion in the relevant
test fail fast: after checking the ok result for scheduledPods[pod1.UID], return
immediately when the lookup fails before dereferencing got in the assert.Same
call. Preserve the existing pointer-preservation assertion for successful
lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef3b76eb-acae-4e01-afec-ac892d3ecb7c
📒 Files selected for processing (2)
pkg/device/pod_test.gopkg/device/pods.go
e62ae08 to
c884077
Compare
3 participants
Summary
Fixes #2165.
During Prometheus metrics collection in
cmd/scheduler/metrics.go, an nested loop was iterating over all cluster nodes and node devices to calculatetotaldevfor every scheduled container device. This computedtotaldevvalue was only logged atklog.V(4)and never used in any emitted metric or calculation.This PR removes the redundant$O(A \times D)$ device scan loop and unused
stringspackage import, eliminating unnecessary CPU and memory allocation overhead during Prometheus scrapes.Changes
cmd/scheduler/metrics.go.stringsimport.Summary by CodeRabbit
Summary
Refactor
Tests
Chores