Skip to content

fix(remotegpu): select Lupine server using all pod container requests - #3166

Merged
moezdil merged 2 commits into
Project-HAMi:masterfrom
sanket-jadhav-cse:fix/remotegpu-pod-wide-server-selection
Oct 5, 2026
Merged

moezdil merged 2 commits into
Project-HAMi:masterfrom
sanket-jadhav-cse:fix/remotegpu-pod-wide-server-selection

Conversation

@sanket-jadhav-cse

@sanket-jadhav-cse sanket-jadhav-cse commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

RemoteGPUDevices.Fit previously selected the initial Lupine server using only the first RemoteGPU container request.

Because later containers are pinned to that server, a multi-container pod could be rejected when the first container fit a smaller server but a later container required a larger server. The result could therefore depend on container order.

This change checks each candidate Lupine server against every RemoteGPU-requesting init, native sidecar, and app container independently before selecting the server. Each container's card-count and memory requirements are evaluated together.

Existing server pinning behavior is preserved.

Which issue(s) this PR fixes:

Fixes #3165

Special notes for your reviewer:

Added regression coverage for:

  • container-order-independent server selection
  • init, native sidecar, and app containers
  • per-container card-count and memory requirements
  • multi-container allocation staying on one Lupine server
  • existing single-container behavior

Validation completed with focused RemoteGPU race tests, make test, make verify, make build, go mod verify, formatting, and diff checks.

No dependency or public API changes.

AI assistance was used during the investigation and implementation. I reviewed and verified the changes and test results myself.

Does this PR introduce a user-facing change?:

Yes. Valid multi-container RemoteGPU pods that could previously be rejected because of container-order-dependent Lupine server selection can now be placed on a server that satisfies all container requests.

Summary by CodeRabbit

  • Bug Fixes
    • Remote GPU server selection now accounts for requests from every container, including init containers and sidecars.
    • Server selection no longer depends on container order when a server can meet the pod’s combined requirements.
    • Each container’s GPU count and memory requirements are checked independently, with clearer outcomes when available devices do not meet those requirements.
    • Once a server is selected for a pod, later container requests are checked against that same server.

Signed-off-by: sanket-jadhav-cse <sj546400@gmail.com>
@hami-robot hami-robot Bot added the kind/bug Something isn't working label Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
CLAUDE.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9090506a-df46-4a9f-86b2-b23bfde4298c
📥 Commits

Reviewing files that changed from the base of the PR and between 7ff0776 and 7ffaa6b.

📒 Files selected for processing (2)
  • pkg/device/remotegpu/device.go
  • pkg/device/remotegpu/device_test.go

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

RemoteGPU fitting now gathers requests from a pod’s init and regular containers. It checks each request against candidate servers, including device count and memory requirements. Unit and scheduler integration tests cover container order and different container types.

Changes

RemoteGPU server selection

Layer / File(s) Summary
Collect and check pod requests
pkg/device/remotegpu/device.go
Fit collects per-container remote-GPU requests and checks each candidate server against them. Device count and memory are evaluated separately for each request.
Validate per-container server selection
pkg/device/remotegpu/device_test.go, pkg/scheduler/remotegpu_integration_test.go
Tests cover container-order-independent selection, per-container count and memory requirements, and requests from init containers, restartable init sidecars, and app containers.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested labels: enhancement

Merge Risk: ⚪ Minimal · up to 7ffaa

RemoteGPU server selection now accounts for every container in the pod, so selection no longer depends on container order. No merge-blocking issues were identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 7ff07

The change preserves single-server placement and whole-server allocation while removing container-order-dependent rejection. No newly weakened isolation was identified. Server-side authorization and some failure-recovery scenarios remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A pod submitter’s resource requests influence server selection. The intended allocation and exposure unit remains one whole Lupine server, whose endpoint exposes its GPUs rather than only the requested subset. Pod-wide selection may choose a larger server, but larger-server authority was already reachable when the demanding container was evaluated first.

Trust Boundaries and Controls

  • observed — The scheduler-side controls retain single-server pinning, reject candidates with conflicting used or reserved healthy cards, and allocate every healthy card on the selected server. The parent source also included lower-memory healthy cards in whole-server allocations, so their inclusion is not a newly introduced bypass. Downstream authorization and enforcement on such cards remain unverified.

Resilience and Maintainability Implications

  • inferred — The inspected lifecycle retains UID-keyed holds and release paths. On annotation-persistence failure, the local hold has no immediate release in the inspected Filter branch and recovery depends on refresh or expiry. This is outside the changed fitting logic and is not retained as a PR-introduced concern; an outer cleanup path was not established.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. 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 clearly describes the main change: selecting a Lupine server based on all pod container requests.
Linked Issues check ✅ Passed Issue [#3165] requires Fit to select a Lupine server that satisfies each requesting container independently. The reviewed change checks init, sidecar, and app container requests with each request’s …
Out of Scope Changes check ✅ Passed The reviewed changes remain within issue [#3165]. The added failure-reason accounting and tests relate to candidate rejection during the required server selection. No unrelated changes are identified.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

A rabbit checks each card in view,
Then gathers every request in queue.
Init and app containers align,
The fitting server meets each design.
No matter which request comes first,
The pod’s needs are checked throughout.

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

@coderabbitai
coderabbitai Bot requested a review from moezdil October 4, 2026 10:27
@moezdil moezdil self-assigned this Oct 4, 2026
@codecov

codecov Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 77.33% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/device/remotegpu/device.go 92.41% <100.00%> (+1.82%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pkg/device/remotegpu/device.go Outdated
Signed-off-by: sanket-jadhav-cse <sj546400@gmail.com>
@moezdil

moezdil commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@hami-robot hami-robot Bot added the lgtm label Oct 5, 2026
@hami-robot

hami-robot Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: moezdil, sanket-jadhav-cse

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

The pull request process is described 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

@hami-robot hami-robot Bot added the approved label Oct 5, 2026
@hami-robot

hami-robot Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: moezdil, sanket-jadhav-cse

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

The pull request process is described 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

@moezdil
moezdil merged commit 717f016 into Project-HAMi:master Oct 5, 2026
16 of 18 checks passed

This branch was successfully deployed

1 active deployment
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.

RemoteGPU: pick the Lupine server using every container's request, not just the first

2 participants