feat: InstanceType Inactive hold + aggregation phase filtering#32
Merged
Merged
Conversation
… Stop to bool - add InstanceTypeSpec.DisplayName (maxLength=64) and Inactive fields - change InstanceSpec.Stop from *bool to bool; drop ptr wrappers at call sites - regenerate protobuf, deepcopy, CRD, openapi and applyconfiguration Task T0 of instancetype-inactive-and-aggregation-phase. Signed-off-by: thxCode <thxcode0824@gmail.com>
- map HoldAndDrain to Draining when reserved, else Inactive; Hold to Inactive - add local clusterQueueHasReserved mirroring the node queue check - pass the full ClusterQueue and simplify the draining check in computeStatus - cover all four summary branches in a new table test Task T1 of instancetype-inactive-and-aggregation-phase. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add syncInactive converging Spec.Inactive and the queue StopPolicy per a forward-before-mirror truth table: Inactive drives the Hold<->None pair, a stopped queue backfills Inactive=true one-way, HoldAndDrain is never downgraded and never cleared on None (memoryless, non-oscillating) - run the sync before computeStatus; skip the status refresh on a write - update the reconciler doc to reflect the Hold<->None ownership - cover all six truth-table rows plus post-convergence stability Task T2 of instancetype-inactive-and-aggregation-phase. Signed-off-by: thxCode <thxcode0824@gmail.com>
… policy - stop a running instance when its type is gone or being deleted, or when its backing ClusterQueue is HoldAndDrain (a pool drain or a teardown that evicts admitted workloads); a Hold-Inactive type keeps its running Pods and a new Instance under it simply stays pending - read the backing ClusterQueue StopPolicy directly rather than the InstanceType phase: the phase collapses Hold and drained-HoldAndDrain to Inactive and a fast drain skips a durable Draining, so it cannot drive the stop - watch the ClusterQueue and re-enqueue the type's Instances on a StopPolicy change; narrow the InstanceType watch to fire only on the deletion (teardown) signal, keeping the stop level-based - add InstanceTypePhaseDraining and refine the InstanceTypePhaseInactive doc - cover hold-keeps-running, drain-stops, and a fully-drained HoldAndDrain stopping a pod-less instance; align the case-2 e2e and its drain-recycle reference to the StopPolicy-based log and watch Task T3 of instancetype-inactive-and-aggregation-phase. Signed-off-by: thxCode <thxcode0824@gmail.com>
There was a problem hiding this comment.
Pull request overview
Enhances GPUStack Operator’s worker control plane and workergateway aggregation to support an administrative InstanceType.Spec.Inactive hold (mapped to Kueue ClusterQueue StopPolicy=Hold), adds DisplayName, refines draining vs inactive status derivation, and ensures aggregated capacity totals count only active instance types.
Changes:
- Add
Inactive+DisplayNametoInstanceType, implementInactive↔StopPolicyconvergence, and deriveDrainingvsInactivefrom queue stop policy + reservation state. - Update
Instancestop behavior to key off the backingClusterQueueStopPolicy(distinguishing admin Hold vs drain/teardown) and watchClusterQueuefor StopPolicy transitions. - Update workergateway aggregation to record per-candidate phase and exclude non-Active candidates from
OnceMaxRequest/Remainingtotals while retaining them in listings; plus grouping ignores per-clusterInactive/DisplayName.
Reviewed changes
Copilot reviewed 23 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workergateway/service/types.go | Adds candidate Phase field to aggregated instance type payload. |
| pkg/workergateway/service/helper.go | Implements phase filtering in tier/item aggregation and normalizes grouping identity across clusters. |
| pkg/workergateway/service/helper_test.go | Adds coverage for phase filtering, tier identity stability, and grouping behavior. |
| pkg/worker/webhooks/worker/instancetype.go | Defaults/guards CPU descriptors for agnostic pools and defaults DisplayName (≤64 runes). |
| pkg/worker/webhooks/worker/instancetype_test.go | Updates/extends webhook tests for CPU-agnostic behavior and DisplayName defaulting/truncation. |
| pkg/worker/webhooks/worker/instance.go | Updates webhook logic for InstanceSpec.Stop migration to bool. |
| pkg/worker/webhooks/worker/instance_test.go | Updates tests to reflect Stop as a bool. |
| pkg/worker/extensionapis/worker/instance_type_flavor.go | Removes Manufacturer=generic sentinel for CPU-agnostic flavor spec. |
| pkg/worker/extensionapis/worker/instance_type_flavor_test.go | Adjusts tests for absence of manufacturer sentinel. |
| pkg/worker/controllers/worker/nodequeue.go | Prevents NodeQueue reconciler from reactivating admin Hold; only reactivates HoldAndDrain when drained empty. |
| pkg/worker/controllers/worker/nodequeue_test.go | Adds test asserting admin Hold remains sticky across flavor return. |
| pkg/worker/controllers/worker/instancetype.go | Adds syncInactive truth-table convergence and updates status computation via ClusterQueue summarizer. |
| pkg/worker/controllers/worker/instancetype_test.go | Adds tests for Inactive↔StopPolicy truth table convergence and stability. |
| pkg/worker/controllers/worker/instance.go | Stops instances on HoldAndDrain (not Hold), and adds ClusterQueue watch to re-enqueue on StopPolicy changes. |
| pkg/worker/controllers/worker/instance_test.go | Updates tests to drive stop behavior via backing ClusterQueue StopPolicy. |
| pkg/worker/apistatus/cluster_queue.go | Derives Draining/Inactive from StopPolicy and reservation presence; falls back to condition-based summary when not held. |
| pkg/worker/apistatus/cluster_queue_test.go | Adds unit tests for StopPolicy-first phase derivation. |
| pkg/kubeclients/applyconfiguration/worker/v1alpha1/instancetypespec.go | Adds apply-config builders for DisplayName and Inactive. |
| deploy/gpustack-operator/chart/files/cleanup.sh | Extends uninstall cleanup to delete worker-provisioned Secrets. |
| api/worker/zz_generated.openapi.go | Regenerates OpenAPI schema for new InstanceType fields. |
| api/worker/v1alpha1/zz_generated.deepcopy.go | Regenerates deepcopy after InstanceSpec.Stop type change. |
| api/worker/v1alpha1/zz_generated.crds.go | Regenerates CRDs for DisplayName/Inactive and Stop nullability removal. |
| api/worker/v1alpha1/instance.go | Migrates InstanceSpec.Stop from *bool to bool. |
| api/worker/v1alpha1/instance_type.go | Adds InstanceTypeSpec.DisplayName and InstanceTypeSpec.Inactive. |
| api/worker/v1alpha1/generated.proto | Regenerates API proto for new InstanceTypeSpec fields. |
| api/worker/v1alpha1/generated.pb.go | Regenerates protobuf Go bindings for API changes. |
| .claude/skills/gpustack-operator-e2e/references/drain-recycle.md | Updates e2e reference docs for stop-on-drain behavior (StopPolicy-driven). |
| .claude/skills/gpustack-operator-e2e/cases/case-2.sh | Updates e2e case assertions/log expectations to stop-on-drain behavior. |
| .claude/skills/_e2e-lib/scripts/teardown.sh | Extends teardown to delete worker-provisioned Secrets. |
Files not reviewed (6)
- api/worker/v1alpha1/generated.pb.go: Generated file
- api/worker/v1alpha1/generated.proto: Generated file
- api/worker/v1alpha1/zz_generated.crds.go: Generated file
- api/worker/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/worker/zz_generated.openapi.go: Generated file
- pkg/kubeclients/applyconfiguration/worker/v1alpha1/instancetypespec.go: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 30 changed files in this pull request and generated 3 comments.
Files not reviewed (6)
- api/worker/v1alpha1/generated.pb.go: Generated file
- api/worker/v1alpha1/generated.proto: Generated file
- api/worker/v1alpha1/zz_generated.crds.go: Generated file
- api/worker/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/worker/zz_generated.openapi.go: Generated file
- pkg/kubeclients/applyconfiguration/worker/v1alpha1/instancetypespec.go: Generated file
- Default webhook: clear manufacturer/product/family and skip enrichment for a CPU-manufacturer-agnostic type (awareness off and not acceleratable), so no arbitrary flavor identity is stamped onto the collapsed pool - Default webhook: default DisplayName to Product (preserving an admin value); the agnostic guard path leaves it empty - flavor catalog: drop the Manufacturer=generic sentinel from the CPU-agnostic "generic" row, aligning it with the derived InstanceType - rewrite the awareness-off webhook test and add DisplayName + empty-manufacturer cases Task T4 of instancetype-inactive-and-aggregation-phase. Signed-off-by: thxCode <thxcode0824@gmail.com>
- add Phase to AggregatedInstanceTypeOnceMaxRequestCandidate, populated at every candidate-construction site (Next + the three Handle sites) - tier Recompute counts only Active candidates toward OnceMaxRequest/Remaining; non-Active candidates stay listed but contribute zero and never seed - item Recompute skips all-zero (e.g. all-inactive) tiers from winner seeding so they cannot mask a fully-sliced active tier tying them at primary zero - recompute every tier before sorting/aggregation on all paths (batch Result and the streaming cross-tier/new-tier Handle paths) so a non-Active candidate never leaks raw capacity nor mis-orders tiers by an unfiltered seed - normalize the cross-cluster grouping key (zero Inactive/DisplayName) for the itemIndexer and the Handle spec comparison; keep the first-seen DisplayName - match Handle tier identity on the stable Candidates[0].Accelerator.OnceMaxRequest Task T5 of instancetype-inactive-and-aggregation-phase. Signed-off-by: thxCode <thxcode0824@gmail.com>
thxCode
force-pushed
the
spec/instancetype-inactive-and-aggregation-phase
branch
from
474b7ab to
af70052
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 30 changed files in this pull request and generated 1 comment.
Files not reviewed (6)
- api/worker/v1alpha1/generated.pb.go: Generated file
- api/worker/v1alpha1/generated.proto: Generated file
- api/worker/v1alpha1/zz_generated.crds.go: Generated file
- api/worker/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/worker/zz_generated.openapi.go: Generated file
- pkg/kubeclients/applyconfiguration/worker/v1alpha1/instancetypespec.go: Generated file
- The worker's webhook serving cert (<release>-worker-cert) and the delegated editable-settings store (gpustack-settings) are provisioned at runtime, not by helm templating, so `helm uninstall` left them behind in the namespace. - Delete both by fixed name in the post-delete cleanup hook, and mirror the same step into the e2e teardown script (its documented single source of truth). Delete by name, never a label sweep, so a co-located standalone GPUStack app's own secrets are untouched. Found while tearing down the live e2e for the instancetype-inactive-and-aggregation-phase spec. Signed-off-by: thxCode <thxcode0824@gmail.com>
…hip with the shipped model - rewrite the running-instance stop paragraph: the stop reads the backing ClusterQueue StopPolicy (HoldAndDrain, or the InstanceType deleted/gone), an admin Hold keeps running Pods, and a ClusterQueue watch drives it while the InstanceType watch is narrowed to the deletion signal - record the InstanceTypeReconciler's admin Inactive<->StopPolicy sync and its one-way sticky backfill, and split StopPolicy ownership across the two reconcilers (Hold<->None vs HoldAndDrain) in both the prose and the diagram Signed-off-by: thxCode <thxcode0824@gmail.com>
thxCode
force-pushed
the
spec/instancetype-inactive-and-aggregation-phase
branch
from
af70052 to
405d713
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 31 changed files in this pull request and generated 3 comments.
Files not reviewed (6)
- api/worker/v1alpha1/generated.pb.go: Generated file
- api/worker/v1alpha1/generated.proto: Generated file
- api/worker/v1alpha1/zz_generated.crds.go: Generated file
- api/worker/v1alpha1/zz_generated.deepcopy.go: Generated file
- api/worker/zz_generated.openapi.go: Generated file
- pkg/kubeclients/applyconfiguration/worker/v1alpha1/instancetypespec.go: Generated file
Comment on lines
+528
to
531
| if tier.Candidates[0].Accelerator.OnceMaxRequest.Equal(candidate.Accelerator.OnceMaxRequest) { | ||
| // If the once max request has not changed, update the candidate in place. | ||
| tier.Candidates[index[2]] = *candidate | ||
|
|
Comment on lines
611
to
615
| candidate := &AggregatedInstanceTypeOnceMaxRequestCandidate{ | ||
| Cluster: evt.Cluster, | ||
| Name: instType.Name, | ||
| Phase: instType.Status.Phase, | ||
| Accelerator: instType.Status.Accelerator, |
Comment on lines
+349
to
352
| draining := ptr.Deref(cq.Spec.StopPolicy, kueue.None) == kueue.HoldAndDrain | ||
|
|
||
| var st workercore.InstanceTypeStatus | ||
| if !draining { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind enhancement
/kind api-change
/area worker
/area workergateway
What this PR does / why we need it:
Introduces an administrative
Inactiveswitch onInstanceTypeplus a set of coordinated changes across the API types, the worker control plane, the admission webhook, and the workergateway aggregation:Inactivehold.InstanceTypeSpec.Inactiveholds the backing KueueClusterQueue(StopPolicy=Hold): new admission is blocked, already-running workloads keep running (neverHoldAndDrainfor this path). Clearing the flag reactivates the queue (StopPolicy=None).HoldorHoldAndDrain) is mirrored back intoSpec.Inactive=true; the mirror never clears onNone, so a pool that lost all its nodes (or was torn down and later recovered) staysInactiveuntil an admin explicitly clears it. Ownership is cleanly split — the InstanceType path only ever togglesHold↔None;HoldAndDrainis owned solely byNodeQueueReconciler(teardown / no-flavors drain).Drainingwhile aHoldAndDrainqueue still holds reservations andInactiveonce held/drained — derived fromSpec.StopPolicy+ reservation state.InstanceReconcilerreads the backingClusterQueue'sStopPolicy(not the InstanceType phase) before (re)creating a Pod:HoldAndDrain(a pool drain or a teardown that evicts admitted workloads) stops the Instance;Holdkeeps running Pods; aClusterQueuewatch re-enqueues on aStopPolicychange so the stop stays level-based. This also closes a pre-existing gap where an evicted Pod under a draining queue was recreated and left stuckPending.DisplayName(≤64 runes) defaults toProductin the Default webhook (truncated to the field max).manufacturer/product/familystamped on it; the InstanceTypeFlavor catalog drops the matchingManufacturer=genericsentinel so the catalog and the derived type agree.Phasefiltering.AggregatedInstanceTypeOnceMaxRequestCandidaterecords a per-candidatePhase; onlyPhase=="Active"candidates contribute to fleetOnceMaxRequest/Remaining, while inactive candidates remain listed. Cross-cluster grouping ignoresInactive/DisplayName.InstanceSpec.Stopmigrates from*booltobool.helm uninstall(and the gated cleanup hook) now also removes the worker-provisioned Secrets it leaves behind (<release>-worker-cert,gpustack-settings), deleted by fixed name so a co-located standalone GPUStack app's Secrets are untouched.Which issue(s) this PR fixes:
None — spec-driven change.
Special notes for your reviewer:
InstanceTypeReconcilertoggles onlyHold↔None;NodeQueueReconcilerownsHoldAndDrain. The forward direction (Inactive → StopPolicy) is evaluated before the one-way mirror; all writes areDeepEqual/value-guarded and idempotent, and the sync truth table is verified per-row for convergence without oscillation.observedGeneration): a recovered pool staysInactiveuntil an admin clears it — see the spec Non-Goals.StopPolicy, not the phase — the phase collapsesHoldand a fully-drainedHoldAndDraintoInactive, and a fast drain skips a durableDraining, so the phase cannot drive the stop. Teardown (deletion) still stops immediately via the InstanceType's deletion timestamp.make generateidempotent (clean tree),make lint, andmake test(-race) all green; additionally validated end-to-end on a live Kubernetes cluster — an adminHoldkeeps a running Instance's Pod, a pool drain stops the Instance (not recreate), and the InstanceType status reportsInactive/Draining.Does this PR introduce a user-facing change?