fix(scheduler): bound-check MIG template/instance index parsed from UUID annotation#2088
fix(scheduler): bound-check MIG template/instance index parsed from UUID annotation#2088mesutoezdil wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mesutoezdil 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 |
|
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:
📝 WalkthroughWalkthroughThe scheduler now validates MIG UUID parsing and template or instance indices before updating MIG usage. Tests cover stale, unparsable, out-of-range, and mismatched indices, confirming usage accounting continues without unsafe writes or panics. ChangesMIG usage safety
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested labels: Suggested reviewers: 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation and bounds checking for MIG template and instance indices extracted from UUIDs in pkg/scheduler/scheduler.go to prevent out-of-bounds panics, along with a corresponding unit test. The review feedback suggests two improvements: validating that the extracted template index matches the already initialized MIG usage index when the usage list is already populated, and separating the parsing error check from the bounds check to ensure clearer log messages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/scheduler/scheduler_test.go (1)
188-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
assert.Trueand correct theassert.Equalargument order.The
testify/assertpackage expects the signatureassert.Equal(t, expected, actual). Additionally,assert.True(t, ok)is more idiomatic thanassert.Equal(t, ok, true).(Note: The rest of the file also uses this pattern, so this is just a minor stylistic suggestion for the newly added test).
♻️ Proposed refactor
- assert.Equal(t, ok, true) - assert.Equal(t, v.Devices.DeviceLists[0].Device.Used, int32(1)) - assert.Equal(t, len(v.Devices.DeviceLists[0].Device.MigUsage.UsageList), 0) + assert.True(t, ok) + assert.Equal(t, int32(1), v.Devices.DeviceLists[0].Device.Used) + assert.Equal(t, 0, len(v.Devices.DeviceLists[0].Device.MigUsage.UsageList))🤖 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/scheduler/scheduler_test.go` around lines 188 - 190, Update the newly added assertions in the test to use assert.True(t, ok) for the boolean result and pass expected values before actual values in the assert.Equal calls, including the Device.Used and MigUsage.UsageList checks.pkg/scheduler/scheduler.go (1)
649-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove variable naming and error logging clarity.
The current fix effectively prevents panics, but there are two minor maintainability improvements to consider:
- Variable naming: Local variables in Go should use
camelCase.Instanceis capitalized, which is typically reserved for exported package-level symbols.- Log clarity: When
tmpIdxis out of bounds but parses successfully,errisnil. This causes the current code to log<nil>at the end of the error message. Separating the parse error check from the bounds check prevents this.Consider this combined refactor to address both:
♻️ Proposed refactor
- tmpIdx, Instance, err := device.ExtractMigTemplatesFromUUID(udevice.UUID) - if err != nil || tmpIdx < 0 || tmpIdx >= len(d.Device.MigTemplate) { - klog.Errorf("invalid mig template index in uuid %s: %v", udevice.UUID, err) - continue - } + tmpIdx, instanceIdx, err := device.ExtractMigTemplatesFromUUID(udevice.UUID) + if err != nil { + klog.Errorf("failed to parse mig template from uuid %s: %v", udevice.UUID, err) + continue + } + if tmpIdx < 0 || tmpIdx >= len(d.Device.MigTemplate) { + klog.Errorf("invalid mig template index %d in uuid %s", tmpIdx, udevice.UUID) + continue + } if len(d.Device.MigUsage.UsageList) == 0 { device.PlatternMIG(&d.Device.MigUsage, d.Device.MigTemplate, tmpIdx) } - if Instance < 0 || Instance >= len(d.Device.MigUsage.UsageList) { + if instanceIdx < 0 || instanceIdx >= len(d.Device.MigUsage.UsageList) { klog.Errorf("invalid mig instance in uuid %s", udevice.UUID) continue } - d.Device.MigUsage.UsageList[Instance].InUse = true + d.Device.MigUsage.UsageList[instanceIdx].InUse = true🤖 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/scheduler/scheduler.go` around lines 649 - 661, Update the MIG UUID handling around ExtractMigTemplatesFromUUID: rename the local Instance variable to camelCase, handle and log extraction errors separately, then validate tmpIdx bounds with a clear index-specific message that does not include a nil error. Preserve the existing continue behavior for invalid UUIDs or template indices.
🤖 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/scheduler/scheduler_test.go`:
- Around line 188-190: Update the newly added assertions in the test to use
assert.True(t, ok) for the boolean result and pass expected values before actual
values in the assert.Equal calls, including the Device.Used and
MigUsage.UsageList checks.
In `@pkg/scheduler/scheduler.go`:
- Around line 649-661: Update the MIG UUID handling around
ExtractMigTemplatesFromUUID: rename the local Instance variable to camelCase,
handle and log extraction errors separately, then validate tmpIdx bounds with a
clear index-specific message that does not include a nil error. Preserve the
existing continue behavior for invalid UUIDs or template indices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a94f4b4-2352-40a0-a53e-f8c4d92f9b93
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/scheduler/scheduler_test.go (1)
226-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
nvidia.NvidiaGPUDeviceconstant.Consider using the existing
nvidia.NvidiaGPUDeviceconstant instead of the hardcoded"NVIDIA"string to maintain consistency with the device initialization above.🛠️ Proposed refactor
podMap.AddPod(&corev1.Pod{ ObjectMeta: metav1.ObjectMeta{UID: "1111", Name: "test1", Namespace: "default"}, }, "node1", device.PodDevices{ - "NVIDIA": device.PodSingleDevice{ + nvidia.NvidiaGPUDevice: device.PodSingleDevice{ []device.ContainerDevice{{Idx: 0, UUID: "GPU0[0-0]", Usedmem: 100, Usedcores: 10}}, }, }) podMap.AddPod(&corev1.Pod{ ObjectMeta: metav1.ObjectMeta{UID: "2222", Name: "test2", Namespace: "default"}, }, "node1", device.PodDevices{ - "NVIDIA": device.PodSingleDevice{ + nvidia.NvidiaGPUDevice: device.PodSingleDevice{ []device.ContainerDevice{{Idx: 0, UUID: "GPU0[1-0]", Usedmem: 100, Usedcores: 10}}, }, })🤖 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/scheduler/scheduler_test.go` around lines 226 - 234, Replace the hardcoded "NVIDIA" key in the pod device map with the existing nvidia.NvidiaGPUDevice constant, matching the device initialization above while leaving the surrounding test data 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 `@pkg/scheduler/scheduler_test.go`:
- Around line 226-234: Replace the hardcoded "NVIDIA" key in the pod device map
with the existing nvidia.NvidiaGPUDevice constant, matching the device
initialization above while leaving the surrounding test data unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c40e742-8c81-4a06-b3de-e5f1af6e40e0
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/scheduler.go
2e846bc to
b92e3b2
Compare
b92e3b2 to
c9f1852
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/scheduler/scheduler_test.go (1)
349-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse appropriate boolean assertions and correct parameter order.
For cleaner test output and to prevent swapping "expected" and "actual" values in failure messages, update the
testify/assertusages:
- Use
assert.Truefor boolean checks instead ofassert.Equal(..., true).- Follow the standard
assert.Equal(t, expected, actual)argument order convention.♻️ Proposed refactor
- assert.Equal(t, ok, true) + assert.True(t, ok) dev := v.Devices.DeviceLists[0].Device - assert.Equal(t, dev.Used, int32(2)) - assert.Equal(t, len(dev.MigUsage.UsageList), 1) - assert.Equal(t, dev.MigUsage.UsageList[0].InUse, true) + assert.Equal(t, int32(2), dev.Used) + assert.Equal(t, 1, len(dev.MigUsage.UsageList)) + assert.True(t, dev.MigUsage.UsageList[0].InUse)🤖 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/scheduler/scheduler_test.go` around lines 349 - 353, Update the assertions in the affected scheduler test to use assert.True for boolean values, including ok and InUse, and correct all assert.Equal calls to pass expected values before actual values. Preserve the existing numeric and length expectations while applying the standard testify argument order.
🤖 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/scheduler/scheduler_test.go`:
- Around line 349-353: Update the assertions in the affected scheduler test to
use assert.True for boolean values, including ok and InUse, and correct all
assert.Equal calls to pass expected values before actual values. Preserve the
existing numeric and length expectations while applying the standard testify
argument order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7c31673-0c3a-45b8-a62e-c77a7670353a
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/scheduler.go
…UID annotation getNodesUsage parses a MIG template index and instance number from a pod's UUID annotation and used both without validation. A stale or mismatched index (node MIG config changed, or two pods disagreeing on the active template) panicked the scheduler's usage-scrape goroutine instead of being skipped. Reject an out-of-range index, an index that disagrees with the device's already-recorded MIG template, and an out-of-range instance. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
c9f1852 to
c794a37
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/scheduler_test.go`:
- Around line 349-353: In the test assertions around dev.MigUsage.UsageList,
replace both assert.Assert calls with the appropriate testify boolean assertion,
using assert.True for the expected true conditions while preserving the existing
checks and messages.
- Around line 289-292: Replace the invalid assert.Assert calls in the scheduler
test with the appropriate testify boolean assertions: use True for ok and False
for the UsageList entry’s InUse value, preserving the existing test
expectations.
- Line 238: Replace the undefined assert.Assert call in the scheduler test with
testify's assert.True, preserving the existing t and ok arguments and assertion
behavior.
- Line 188: In the test assertion around the ok result, replace the undefined
assert.Assert call with testify/assert’s assert.True, preserving t and ok as the
assertion arguments.
🪄 Autofix (Beta)
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: d4a1e4b0-790e-4310-9484-3b0f311670f6
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/scheduler/scheduler.go
3 participants
/kind bug
getNodesUsage reads a MIG template index and instance number from a pod's UUID annotation and used both without checking them.
A stale or mismatched index can panic the scheduler's usage-scrape goroutine instead of just being skipped.
This PR adds bounds and consistency checks: an out-of-range index is rejected, and an index that disagrees with the device's already-recorded MIG template is rejected too.
Two new tests: one for an out-of-range index, one for two pods that disagree on the template index.
Confirmed both panic on the old code and pass after the fix. go test with race, go vet, and golangci-lint all pass.
Summary by CodeRabbit