[hodaaaaaaaaaa]

Problem

Binpack seats time-sliced workers on a shared GPU until its host memory runs out. Nothing bounds how many share the GPU in time, so even with free GPUs elsewhere, three or six time-sliced full fine-tuning workers can land on one H100, and each waits through the others' turns.

Change

--max-seats-per-claim (OPEN_RL_MAX_SEATS_PER_CLAIM, default 0 for no limit) caps the workers on one shared claim. A claim at the limit is not offered for placement, and the ledger's compare-and-swap refuses any write that would add a seat past it, so two workers placed at the same time cannot both take the last seat. A worker that finds every eligible claim full moves on to the next ledger with room, and only cuts its own claim when none accepts it. With no free device it stays Pending, with the limit named in its reason, until a GPU or a seat frees up, or fails at the placement timeout like any worker waiting on capacity. A seat released on a shared claim wakes the waiting workers.

[droot]

Looks good to me. Will defer to @ShubyM for the final approval.

Thanks for getting this in!

[droot]

1. [Medium] Multi-candidate race when a join loses the last seat on one claim while another claim still has room

Look at joinExistingClaim in workload_controller.go:527-538:

func (r *WorkloadReconciler) joinExistingClaim(ctx context.Context, worker *openrlv1alpha1.Workload, request placement.Request, fleet *placement.Fleet, current string)
(*placement.Claim, *openrlv1alpha1.Seat, error) {
    target := placement.SelectClaim(request, fleet)
    if target == nil || target.Name == current {
        return nil, nil, nil
    }
    _, seat, err := r.ensureSeat(ctx, target.Name, newSeat(worker, request), false)
    if err == errBookingContended {
        return nil, nil, nil
    }

And in assign() (under binpack, the default strategy):

    case "", placement.StrategyBinPack:
        target, seat, err := r.joinExistingClaim(ctx, worker, request, fleet, "")
        if err != nil {
            return nil, nil, "", err
        }
        if target != nil {
            log.FromContext(ctx).Info("packing onto an existing claim", "claim", target.Name, "worker", worker.Name)
            return target, seat, "JoinedClaim: " + target.Name, nil
        }
        return r.cutDedicatedClaim(ctx, worker, request, fleet)

What happens under concurrency:
Suppose MaxSeatsPerClaim = 2, and two claims claim-a (1/2 seats) and claim-b (1/2 seats) are allocated on a full 2-GPU cluster.

• Workers w-3 and w-4 reconcile concurrently (MaxConcurrentReconciles = 4) against the same fleet snapshot.
• SelectClaim is deterministic (breaks ties by claim.Name), so both w-3 and w-4 pick claim-a.
• w-3 wins the CAS on claim-a (bringing claim-a to 2/2 seats).
• w-4's ensureSeat on claim-a hits the new check len(claimLedger.Spec.Seats) >= r.MaxSeatsPerClaim and returns errBookingContended.
• joinExistingClaim swallows errBookingContended and returns (nil, nil, nil).
• Under binpack, assign() immediately falls through to r.cutDedicatedClaim(), cutting a dedicated claim for w-4 even though claim-b (1/2 seats) was eligible!
• If the cluster is full, w-4's dedicated pod goes Unschedulable, and fallBackToSharing moves w-4 onto claim-b on the next pass (extra pod/claim churn, plus a transient
scale-up signal to the cluster autoscaler—which design.md specifically notes binpack is meant to avoid).
• Under fallBackToSharing (when w-4 already has an unschedulable dedicated claim and w-3 + w-4 race to fall back onto claim-a), w-4 stays Pending for an entire
RetryInterval (10s) even though claim-b is sitting right there with an open seat.

Note: This single-shot SelectClaim behavior existed previously for exclusive/tombstone contention, but with MaxSeatsPerClaim (e.g. 2), deterministic tie-breaking guarantees
that every concurrent burst of ≥ 2 workers will collide on the same target claim when filling its last seat.

• Suggestion (Optional or Follow-up): Either remove target from fleet.Claims (or re-read the fleet) and loop in joinExistingClaim when err == errBookingContended, or note that
losing a race falls through to cutDedicatedClaim (which design.md line 795 already acknowledges: "a booking that loses the last seat to a race falls through to a dedicated
claim"). Since the author explicitly documented this trade-off in design.md, this is by design, though worth keeping in mind if transient claim cuts under bursts become
noticeable.
──────

2. [Low] ensureSeat comment and design.md stale statements

1. ledger.go:46-50:
   Lines 48–49 immediately above ensureSeat still say:
   // There is no seat ceiling: host memory is the limit on parked workers,
   // checked at selection and verified against a fresh fleet read after booking.
   This comment directly contradicts the new r.MaxSeatsPerClaim check added inside ensureSeat 50 lines below. It should be updated to mention MaxSeatsPerClaim.

2. design.md:
   • Line 366 (Section 2, Node eligibility):
   │ "There is no seat-count label: the node's allocatable host memory is the ceiling on how many workers park there (section 4)." (Still accurate for labels, though section
   4
   │ now has --max-seats-per-claim).

   • Line 800 (Section 6, Shared-ledger eligibility) & Line 284 of placement.go:284-285:
   │ "the advisory host-memory check passes -- the one ceiling on how many workers may park on the node."
   │ (Minor phrasing: host memory is the node-wide ceiling, while --max-seats-per-claim is the per-claim ceiling.)

   • Line 1000 (Section 8, table of operator-tunable parameters): Consider whether --max-seats-per-claim should be mentioned alongside --placement-strategy in the operator
   flags or deployment manifest (scheduler/deploy/base/01-scheduler.yaml).

──────

[droot]

Findings, most important first.

1. The placement timeout fails workers that are only queued. A worker that finds every ledger at the limit parks on its own unallocated claim with an Unschedulable pod. That path goes through reportPlacement, which calls fail once expired returns true. The shipped manifest sets the timeout to 15 minutes. So with a limit of 2 on two GPUs, the fifth job is marked Failed after 15 minutes even if a seat frees a minute later. Before this PR a shareable worker almost always found a seat, so expiry meant "impossible request". Now it fires on ordinary queueing. The PR body, flag help and design.md all promise an indefinite wait. Fix: treat "waiting on the seat limit" as a wait, not a capacity verdict, the way errClaimTerminating already is, or state the real behavior in the docs. This matters for the fleet demo, where jobs beyond capacity are the normal case.
2. No wake-up when a seat frees. The controller watches ResourceClaims and Nodes but not ClaimLedgers. Releasing a seat on a shared claim leaves the claim object untouched, so nothing enqueues the waiting worker. It only retries on the retry interval. The new test drives reconciles by hand so it cannot see this. A ClaimLedger watch with wakeAll would close it.
3. The heal path can exceed the limit. The CAS check is gated on !createMissing, but ensurePlacementClaim at workload_controller.go:292 re-books with createMissing=true for whatever Status.ClaimName points at, including a shared claim a joiner is on. If that joiner's seat is gone while status is intact, it is appended past the cap. Narrow case, but the invariant should hold on every write path: check "does this write add a seat" rather than "which caller got here".
4. Reason string misleads. Explain appends "shared claims hold at most N workers" to every WaitingForCapacity reason when the flag is set, including for LoRA and multi-GPU workers that never share. Guard on the same conditions SelectClaim uses.
5. Stale comments. ledger.go:46 still says "There is no seat ceiling", directly above the ceiling. The SelectClaim doc comment and the design.md bullet after the new seat-limit bullet still call host memory "the one ceiling".
6. No flag validation. Negative values silently mean no limit. A value of 1 silently disables sharing, since every allocated ledger already holds its founder. Reject negatives and document that 1 equals exclusive placement.
7. Limit lives in two places. The request-side limit is stamped only in Reconcile, while the ledger side reads the reconciler field with a different count. Any future Request constructor loses the selection-side check. Set it once in a wrapper around requestFrom.
8. Not wired in the deploy manifest. The other placement knobs appear in the scheduler Deployment env block; this one does not. A zero-valued entry would make it discoverable.

[hodaaaaaaaaaa]

Thanks for the review, @droot. Pushed 1f3022d addressing your comments:

• Placement timeout: Kept the timeout. Waiting on a seat is waiting on capacity, so --placement-timeout applies here too rather than letting workers wait forever. Updated the flag help, reconciler comment, design.md, and PR description to state this explicitly.
• Wake-up when a seat frees: Added a ClaimLedger watch (wakeAll) filtered to seat removals and ledger deletions so bookings don't wake waiting workers.
• Heal path exceeding the limit: Removed !createMissing from the cap check in ensureSeat so every write that adds a seat enforces the limit, while adopting or replacing an existing seat is unaffected (TestBookingRefusesASeatPastTheLimit).
• Reason string: Explain now appends the limit only when req.Shareable && req.Devices == 0, matching SelectClaim.
• Stale comments: Updated ledger.go, SelectClaim, and design.md (the line-366 note in design.md is about node labels, so left as-is).
• Flag validation: Negative values now exit at startup; help text notes that 1 means exclusive placement.
• Limit in two places & deploy manifest: Left as-is. requestFrom only translates Workload fields (and no other caller runs selection), and 01-scheduler.yaml omits --placement-strategy too.
• Multi-candidate race: When a ledger refuses a booking, joinExistingClaim now drops it from candidates and re-runs SelectClaim before cutting a dedicated claim.