fix(worker): scope node-devices feasibility to each podset's own flavor

[thxCode]

What type of PR is this?

/kind bug
/area worker

What this PR does / why we need it:

The per-accelerator AdmissionCheck (gate 3) flattened two things that must stay correlated, and the
result is the failure an admission gate can least afford: a legal answer, no error raised
anywhere, and a workload that never runs.

• parseFamilyDemands folded every PodSet's demand into one flat list.
• collectCards flattened every candidate node's accelerators into one list with no record of
  which flavor — or which accelerator model — each card belonged to.

nodeDevicesFeasibility then fitted the merged demands against the merged cards, resting on an
assumption its own doc comment stated: "already scoped to one flavor pool by label". That holds
for a single-flavor Workload and fails for two reachable shapes:

1. A multi-PodSet Workload. Kueue assigns a ResourceFlavor per PodSet, so a Workload whose
   roles want different accelerator models carries two. Measured before the fix, a Workload
   demanding 2 cards under an exhausted flavor and 2 under a free one parsed to a single merged
   demand (exclusive cards=4) and was reported Ready, satisfied entirely from the free
   flavor's cards. Kueue admits it and the starved role's Pods stay Pending forever.
2. A node carrying two accelerator models. It publishes one Devices object holding both
   models' cards, so even a single-flavor demand was judged against models its flavor does not
   cover. This one needs no multi-role workload at all — it is reachable today.

What changes

• A demand carries the flavor assigned to its PodSet, and merges with another only when that flavor
  also agrees — so two roles on one flavor still share a budget while two on different flavors
  do not.
• A card carries the assigned flavors that cover it: those whose selector matched its node and
  whose nodeLabels pin its own accelerator key. Coverage is plain equality on the flavor
  reference in both directions — no sentinel, and no "empty means any".
• The per-card budget stays global to the Workload, so a card a mixed-model node exposes to two
  flavors is still spent once.
• The node batch a flavor pins fails closed when it cannot be read: an absent .count
  label means "any batch" — a flavor that states none must not be narrowed to none — while a value
  that is present but not a positive card count covers no card at all, because Kueue admits
  that flavor's Pods only onto nodes carrying the exact value and no node can carry an unreadable
  one.
• A demand whose PodSet carries no flavor assignment is held explicitly, with a verdict naming
  that as the reason. The fit would hold it anyway, but a pool-wide stall reported as "no node has
  enough free cards" about a pool short of nothing is a false negative reached indirectly.
• Verdicts name the role they are about, so a prefill/decode deployment held on capacity says
  which half fell short.

Behaviour for a single-PodSet Workload is unchanged, verdict messages included: the existing
case table runs untouched.

Which issue(s) this PR links to:

None — found while specifying the cross-role PD work.

Special notes for your reviewer:

• This PR is self-contained: it needs no spec on main to be reviewable. It was found while
  specifying cross-role PD (prefill/decode) workloads, where two roles of one deployment can be
  assigned two ResourceFlavors — but the second defect above needs no multi-role workload at all,
  which is why the fix ships on its own rather than waiting for a CRD that does not exist yet. The
  design document for that wider work stays on a branch until its implementation starts.
• Evidence, in order — each step is executed, not argued:
  1. A probe reproduced the wrong verdict before any production change, with a control assertion
     (an empty pool must Retry) proving the query itself worked.
  2. Mutation: removing the eligibility filter turns the two defect cases red; disabling the
     explicit unassigned-flavor hold turns its cases red with exactly the false message the hold
     exists to prevent — "no node has enough free cards" against four completely free cards.
  3. A test drives the whole gate through Reconcile, because the three pieces (reading a PodSet's
     assigned flavor / scoping a node's cards / filtering the fit) can each be right while the
     wiring between them is wrong.
• An independent pass was run on one narrow question, chosen because it is the one a self-review
  structurally cannot answer: read each test case's code only — no name, no comment — infer what it
  actually exercises, then report every place that disagrees with what it claims. The author has a
  prior about their own comments; the reviewer does not. It found two disagreements, both in
  cases predating this change, and both fixed here (the third and fourth bullets below).
• Four test-quality repairs ride along, all of the same shape — a case claiming one thing while
  exercising another: one flavor fixture carried no bare accelerator feature key, and one
  admission fixture carried no PodSetAssignment.Name — Kueue's scheduler writes it from the
  PodSet's name and the CRD defaults it to main, but the fake client does neither. Either
  omission makes the gate answer Retry for an empty or unresolvable card population rather than
  for the units shortage those cases are built to test: still green, exercising nothing. Both now
  assert why the workload is held, not only that it is, so the reason cannot drift silently
  again.
  3. A population case claimed a card was excluded from the partitioned population but asserted
  only the verdict state — and an empty profile ledger yields the same state, so the claim
  went unpinned. This change makes it worse by adding coverage as a third route to that state, so
  the case now asserts the message. Verified by mutation: forcing servesFamily(Partitioned) true
  now fails it on the message; before, it passed.
  4. A partition case named for two cards gave the first seven free placements, so both requested
  instances came from it and the second was never examined. One placement each now forces the
  traversal — the loop this change added a per-card exclusion to.
• The assumption assignedFlavor rests on needs no guard from this PR — it is already enforced.
  It resolves a PodSet's card population from the single flavor its assignment carries, which holds
  while a ClusterQueue covers exactly one resource. An earlier revision added a unit guard in
  node_queue_test.go; review pointed out it duplicated
  TestNodeQueueReconciler_FillsAndSortsByCount, which already asserts one resource group and one
  covered resource, for both a CPU-only and an accelerated queue, through the full reconcile
  path. Checked rather than taken on faith: widening an accelerated queue's CoveredResources
  turns that reconciler case red. So the guard is deleted, and what remains is stated at the
  function — including what an admin's own queue breaking the assumption would do: a CPU flavor
  pins no accelerator key and covers no card, so the demand is held with Retry. The choice of
  flavor is arbitrary there; the direction is not — a flavor that reaches no card can only withhold
  capacity, never invent it.
• One assumption stated rather than defended in code, because it is unreachable today:
  assignedFlavor takes the lexicographically smallest reference when a PodSet is assigned more
  than one flavor. buildResourceGroups gives an accelerated queue exactly one covered resource,
  so a PodSet gets exactly one flavor; the tie-break exists only to keep the verdict stable rather
  than to handle a shape this repository can produce. It is documented at the function.
• Not in scope, deliberately: whether a nil Status.Admission can accompany a reserved
  Workload. It cannot — Kueue's SetQuotaReservation stores the admission and then unconditionally
  dereferences admission.ClusterQueue, so a nil one panics inside Kueue. The code is fail-safe
  there regardless, and the comment records why the state is unreachable rather than merely that
  it is checked.
• The label predicate that decides a role's card population now has one home. Reading the
  accelerator key off a ResourceFlavor's nodeLabels was a private copy of
  nodefeature.ExtractAcceleratableNodeKeys' predicate, and that copy had already drifted once in
  this PR's own review history (it read a period as a metadata suffix, discarding a legal group id).
  It is now nodefeature.ExtractAcceleratableKeys, a labels-map extractor next to the labels
  writer, called from both places. Mutating the predicate turns tests red in both packages,
  which is what proves the controller reads the shared one rather than a stale copy.
• ⚠️ Answering a review finding turned up a defect neither reviewer reported: applyVerdict's
  no-op guard never worked. It compared the verdict against the object Kueue's PatchStatus
  hands an update function, and PatchStatus replaces that object with a bare server-side-apply
  skeleton carrying no status at all — so the check was never found, the guard passed everything
  through, and every reconcile re-applied. An apply of identical content is a no-op on the API
  server, so nothing was ever visible. It was found by mutating the fix and watching the mutation
  survive: the review argued the guard skips a write and pins a stale message, which is what it
  was written to do and not what it did. The comparison now reads the fetched Workload, and it
  includes the message — the two belong together, because a working guard comparing only the state
  would pin the first cause onto the Workload for as long as the state held, now that a verdict
  names its cause and its role.
• The greedy fit's bound is stated where it commits. Each demand takes the first cards it may
  use and never gives one back — exact while the populations are disjoint or nested the way this
  operator's flavors make them (a card carries one model; one model's flavors differ by node batch).
  Where an admin's selectors overlap asymmetrically, a broad demand can take the card a narrow one
  also needed. Not fixed by a matching, for one reason: no ordering can flip the direction of the
  error. A demand only ever takes cards it may itself use, so the fit can report a shortage that
  is not there — never room that is not there.
• Eight assignment fixtures were keyed by the bare word credits, which is not a resource name
  this operator writes. Kueue accounts the manufacturer's credits resource (the chart's
  transformation replaces the raw accelerator keys with it), so every one of those fixtures
  exercised a fallback path instead of the lookup production takes. They now use the real name
  through nodefeature, and the shorthand is pinned as not a credits resource in a new table
  test, so the next fixture cannot repeat it silently. Same shape as the missing
  PodSetAssignment.Name above: a fixture that is not what production produces answers a
  different question, in green.
• A flavor pinning several accelerator keys now resolves to no key at all, rather than to the
  smallest of them. This arrived as a test-determinism complaint, and the test was the symptom: the
  choice was arbitrary. Such a flavor covers genuinely ambiguous cards, so scoping a role to one
  of them is a guess, and the answer that cannot be wrong is to cover nothing and hold the demand.
  It also makes the property testable, which no arrangement of the previous test could — every draw
  over an ambiguous flavor returns the same empty string, so one pass is a verdict instead of a coin
  flip.
• Verified: make lint clean, go vet clean, go test ./pkg/worker/... and -race green.

Does this PR introduce a user-facing change?

Fixed the per-accelerator admission check judging a Workload's device feasibility against every
card of every assigned flavor. A PodSet's demand is now fitted only against the cards of the
ResourceFlavor that PodSet was assigned, so a request is no longer reported placeable because a
different flavor — or a different accelerator model on the same node — had free cards. Retry
verdicts now name the PodSet whose demand fell short.

[Copilot]

🟡 Changes recommended

Flavor scopes can still include nodes outside Kueue’s assigned device-count batch, producing false Ready verdicts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Scopes accelerator feasibility by PodSet flavor and adds role-aware verdicts with regression coverage.

Changes:

• Correlates demands and cards with assigned flavors.
• Adds explicit handling for missing flavor assignments.
• Expands unit and reconcile-level tests.

Standards: One correctness blocker remains around dropped .count selectors.

Spec: Single-PodSet and partition-ledger messages do not fully match the stated behavior.

Over-engineering: Lean already.

File summaries

File	Description
node_devices_admission.go	Implements flavor-scoped feasibility and role-aware verdicts.
node_devices_admission_test.go	Adds flavor, mixed-model, and reconciliation regressions.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Single-PodSet messages regress and one partition Retry path still omits the affected role.

Review details

Suppressed comments (2)

pkg/worker/controllers/worker/node_devices_admission.go:92

• Every production demand now records its PodSet name, so an ordinary one-PodSet workload passes ["main"] to the message helpers and changes messages such as the Ready verdict to include for role "main". This contradicts the PR's explicit guarantee that single-PodSet verdict messages remain unchanged. Keep the PodSet provenance for assignment validation, but render role qualifiers only when the workload actually has multiple PodSets.

			d.podSets = []kueue.PodSetReference{ps.Name}

pkg/worker/controllers/worker/node_devices_admission.go:531

• The ledger-not-ready branch still returns the constant message without d.podSets, so a multi-role partition workload held because one flavor's placement ledger is missing does not identify the affected role. This leaves one Retry path inconsistent with the PR's role-specific verdict requirement; route this branch through a helper that appends forRoles(d.podSets) as the other partition verdicts do.

		return kueue.CheckStateRetry, partitionNoCardsMessage(d.profile, d.podSets)
	case ledgerReady == 0:
		return kueue.CheckStateRetry, partitionLedgerNotReadyMessage
	}
	return kueue.CheckStateRetry, partitionVerdictMessage(kueue.CheckStateRetry, d.profile, d.podSets)

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[gpustack-codereview]

🔍 OpenCodeReview found 1 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)

[Copilot]

🟡 Changes recommended

Valid dotted accelerator IDs and nondeterministic candidate ordering can produce incorrect Retry verdicts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Fail-open count parsing, incomplete role attribution, and a probabilistic regression test remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

pkg/worker/controllers/worker/node_devices_admission.go:478

• withoutSoleRole counts only PodSets that produced accelerator demands, not all PodSets in the Workload. A multi-PodSet Workload with one GPU role and one CPU-only role therefore loses the GPU role from a shortage verdict, despite the PR's requirement to identify the affected role. Pass the Workload's actual PodSet count (or an explicit multi-role flag) into feasibility instead of inferring its shape from demands.

	demands = withoutSoleRole(demands)

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[thxCode]

Round 3 triage — 3 findings, all verified real against source, all fixed in ab638e40. Two had inline threads (replied there); the third had none, so it is recorded here.

Suppressed comment on node_devices_admission.go:478 — withoutSoleRole infers the Workload's shape from the demands. Correct, and it was the most valuable of the three.

The role clause was stripped whenever the demands named at most one podset. A Workload whose accelerator role sits beside a CPU-only one parses to exactly one demand carrying one name — indistinguishable from a plain Pod's — so the role was dropped from the verdict, which is precisely the name a prefill/decode deployment needs. The rule now comes from len(wl.Spec.PodSets) in Reconcile, once, so the capacity verdict and the unassigned-flavor hold agree.

Two consequences worth stating:

• The strip left nodeDevicesFeasibility. That function cannot know a Workload's shape, and the test that "pinned" the single-role wording was calling it directly with hand-built demands — pinning the wording but not the rule. It is now driven through Reconcile, which is the only place that knows.
• unassignedRoles had conflated "which demands lack a flavor" with "which roles to name": it found the hold by role name. A single-podset Workload no longer carries one, so leaving it whole would have silently stopped holding those Workloads at all — a Retry reporting a capacity shortage against a pool short of nothing. It is now two functions, and the mutation in that direction turns the hold's cases red.

Nothing was rejected this round. Findings across the three rounds: 3 + 2 + 3, all real, no false positives.

One observation on the tooling, not on the review: Code Review (open-code-review) has produced no findings on this PR at all. Its run on 398d50e2 completed and self-reported "0 finding(s); 2 of 3 selected item(s) failed"; its runs on ea82edf8 and 44cc93ef both ended cancelled (the latter 30m11s into the OpenCodeReview step, with a 60-minute job timeout) and so left no comment. A cancelled round is indistinguishable from a clean one unless you go looking for the run.

[Copilot]

🟢 Approval recommended

The implementation is well-covered and the only finding is a non-blocking documentation mismatch.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Same-family requests spanning multiple accelerator bases can still be incorrectly reported Ready.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Greedy card selection can return Retry for feasible workloads with overlapping flavor coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The effective no-op guard can preserve a missing retry delay and cause immediate Kueue retry loops.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Partial SSA payloads can prune unchanged AdmissionCheck verdicts when another owned check changes.

Review details

Suppressed comments (1)

pkg/worker/controllers/worker/node_devices_admission.go:962

• When this controller owns multiple AdmissionChecks, an unchanged check is omitted from the server-side-apply payload whenever a sibling check needs updating. Since this field owner previously applied the omitted list item, SSA can prune its owned state/message/requeue fields, resetting a settled check as the sibling changes. Compute whether any verdict differs first, but whenever a patch is needed populate the apply skeleton with every entry in checks; a two-check regression test should cover one unchanged and one changed entry.

				if cur := kueueadmissioncheck.FindAdmissionCheck(wl.Status.AdmissionChecks, name); cur != nil &&
					cur.State == acs.State && cur.Message == acs.Message &&
					ptr.Equal(cur.RequeueAfterSeconds, acs.RequeueAfterSeconds) {
					continue

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[gpustack-codereview]

The < tie-break over psa.Flavors silently picks an arbitrary flavor when a podset legitimately carries two acceleratable credits resources (credits.gpustack.ai/<manufacturer> for two manufacturers). This is reachable: the Pod webhook forbids two families but not two bases (this file's own podSetFamilyDemands comment acknowledges "two manufacturers' bases"), and an admin-authored ClusterQueue can cover both manufacturers' credits (the assignedFlavor doc already concedes admin queues covering "more" reach this path). Because podSetFamilyDemands keys demands by ResourceFamily only — not manufacturer — a two-manufacturer podset collapses into one demand whose cards sums both models but whose single flavor is whichever ref sorts first, scoping the merged demand to one manufacturer's cards while counting the other's. Note the asymmetry with flavorAcceleratorKey a few lines below, which deliberately fails closed on ambiguity (returns empty → held) precisely because "a model chosen arbitrarily" is the wrong answer. Recommend the same treatment here: when more than one acceleratable credits resource is present (and they resolve to different flavors), return empty so the demand is held with an explicit 'ambiguous assignment' verdict rather than fitted against an arbitrary population.