Skip to content

fix(scheduler): graceful handling of asymmetric GPU topology to prevent panics - #2217

Closed
Norway-02 wants to merge 3 commits into
Project-HAMi:masterfrom
Norway-02:fix-scheduler-panic-asymmetric-topology
Closed

fix(scheduler): graceful handling of asymmetric GPU topology to prevent panics#2217
Norway-02 wants to merge 3 commits into
Project-HAMi:masterfrom
Norway-02:fix-scheduler-panic-asymmetric-topology

Conversation

@Norway-02

@Norway-02 Norway-02 commented Jul 30, 2026

Copy link
Copy Markdown

Graceful Handling of Asymmetric GPU Topology

Motivation

The scheduler currently assumes GPU P2P topology information is perfectly symmetric and terminates when this assumption is violated. Although asymmetric topology reports are unexpected (and originally treated as an internal invariant assertion), handling them gracefully improves scheduler robustness while preserving observability through warning logs. Because the scheduler processes topology data from across the cluster, terminating on inconsistent runtime data causes a denial-of-service for all GPU scheduling, making robust fallback preferable to a hard panic.

Changes

  • Replace the panic in calculateGPUPairScore with graceful handling of asymmetric P2P topology information.
  • Log a warning containing both GPU UUIDs and observed link counts when inconsistent topology information is detected.
  • Evaluate only the links consistently reported by both GPUs instead of terminating the scheduler, avoiding assumptions about missing topology information while allowing scheduling to continue.
  • Preserve existing behavior for symmetric topologies.
  • Verify that Links map lookups safely return nil slices without panicking, and that existing nil checks on device pointers prevent other nil dereferences in this path.

Testing

Added regression tests covering:

  • symmetric topology
  • asymmetric topology
  • zero links
  • missing map entries / nil slices

Verified with:

go test ./pkg/device/nvidia/...

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of replicated GPU devices so device metadata, health, topology, paths, memory, and compute capabilities are preserved.
    • GPU pairing now tolerates asymmetric or missing NVLink information without failing, scoring only available common links.
    • Added safer handling for edge cases with empty or incomplete GPU connectivity data.

Signed-off-by: Norway-02 <anshulkhetade02@gmail.com>
Signed-off-by: Norway-02 <anshulkhetade02@gmail.com>
…nt panics

Signed-off-by: Norway-02 <anshulkhetade02@gmail.com>
@hami-robot

hami-robot Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Norway-02
Once this PR has been reviewed and has the lgtm label, please assign fouof for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions Bot added the kind/bug Something isn't working label Jul 30, 2026
@hami-robot hami-robot Bot added the size/L label Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The changes preserve complete device data when creating replicas, switch selected Device methods to pointer receivers, and update replica tests. GPU pair scoring now tolerates asymmetric or missing NVLink data by logging warnings and scoring shared links.

Changes

Device replication

Layer / File(s) Summary
Replica construction and device receivers
pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go, pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go
Replica devices are newly allocated, protobuf fields are merged from the source device, and selected methods use pointer receivers.
Replica preservation tests
pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map_test.go, pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go
Tests validate protobuf and non-protobuf fields on replicated devices and use pointers for per-iteration test cases.

NVLink scoring

Layer / File(s) Summary
Asymmetric link scoring
pkg/device/nvidia/calculate_score.go
Asymmetric directional links produce a warning and are scored using the shared link count instead of triggering an internal error.
NVLink edge-case coverage
pkg/device/nvidia/calculate_score_test.go
Tests cover mismatched, empty, nil, and missing NVLink relationships.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: dsfans2014

Poem

I’m a rabbit with replicas neat,
Merging fields so devices meet.
NVLinks may point askew,
But shared paths still score true.
Hop, hop—tests now guard the queue!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main scheduler change: handling asymmetric GPU topology gracefully to avoid panics.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/device-plugin/nvidiadevice/nvinternal/rm/device_map.go`:
- Around line 345-355: Deep-copy orig.Paths when constructing replicatedDevice
so each replica owns an independent path slice and mutations cannot affect the
original or sibling replicas. Update the replica construction near
devices.insert and add a regression test verifying path mutations remain
isolated across replicas.

In `@pkg/device/nvidia/calculate_score.go`:
- Around line 214-227: The scoring loop around the directional link slices in
the GPU score calculation must match links by identity rather than comparing
equal-index entries. Use a stable link identity or ordering derived from the
existing link representation, preserve only links present in both directions,
and score those matched pairs. Add a regression test covering the same shared
links returned in reversed orders.
🪄 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: b8437b9f-a885-4ccf-ba59-78c93c821228

📥 Commits

Reviewing files that changed from the base of the PR and between 05e6c80 and 68b73bf.

📒 Files selected for processing (6)
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go
  • pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go
  • pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map_test.go
  • pkg/device-plugin/nvidiadevice/nvinternal/rm/devices.go
  • pkg/device/nvidia/calculate_score.go
  • pkg/device/nvidia/calculate_score_test.go

Comment thread pkg/device-plugin/nvidiadevice/nvinternal/rm/device_map.go
Comment thread pkg/device/nvidia/calculate_score.go
@mesutoezdil

mesutoezdil commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

this runs in the device plugin at registration, not the scheduler,
so the claimed cluster-wide dos cannot occur..

the links map is built symmetrically from deterministic pci topology, and 2 of the 3 commits are unrelated changes to forked nvinternal code we keep in sync with upstream.

additionally, the pr does not contains no ai assistance disclosure as required by contributing.md.

can be closed, as this adds review load without fixing a reachable issue.

@Norway-02

Copy link
Copy Markdown
Author

I apologize, you are completely right on all points. I misunderstood the architectural boundaries (device plugin vs scheduler) and the determinism of the topology build. I also accidentally included unrelated local commits, and missed the AI disclosure requirement. I'm closing this immediately to avoid wasting any more of your time. Thank you for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants