From 98f294512f4b982ca0d1bb492193af30cfd60a14 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 14 Aug 2026 21:14:26 +0100 Subject: [PATCH] code optimization Signed-off-by: kerthcet --- api/v1alpha1/nodepool_types.go | 28 ++++++++++++++--- api/v1alpha1/sandbox_types.go | 2 +- api/v1alpha1/sandboxset_types.go | 2 +- .../bases/nebula.inftyai.com_nodepools.yaml | 30 ++++++++++++------- .../bases/nebula.inftyai.com_sandboxes.yaml | 1 - .../bases/nebula.inftyai.com_sandboxsets.yaml | 1 - docs/architecture.md | 10 ++++--- .../controller/nodepool_validation_test.go | 29 ++++++++++-------- 8 files changed, 69 insertions(+), 34 deletions(-) diff --git a/api/v1alpha1/nodepool_types.go b/api/v1alpha1/nodepool_types.go index 463a53c..85fe88d 100644 --- a/api/v1alpha1/nodepool_types.go +++ b/api/v1alpha1/nodepool_types.go @@ -15,7 +15,7 @@ import ( // candidates = Providers x (each provider's Regions) x {this capacityType}, // available now, minus blocklist // region nests per provider // IF candidates non-empty: -// pick one via Strategy (LowestPrice | Ordered | Weighted) // inner: rank candidates +// pick one via Strategy (Ordered today; see Strategy) // inner: rank candidates // DONE // // else fall through to the next capacity tier // @@ -33,6 +33,11 @@ import ( // The Weighted strategy requires a weight on every provider ref. This is a // static property of the spec, so it is enforced at admission by the CEL rule // below rather than surfaced as a status condition after the fact. +// +// The rule is currently UNREACHABLE — Strategy's enum admits only Ordered, so no +// object can carry Weighted for it to check. It is retained rather than deleted so +// that widening the enum is a one-line change that cannot silently ship without its +// weight validation; the cost is one always-true CEL evaluation per admission. // +kubebuilder:validation:XValidation:rule="self.strategy != 'Weighted' || self.providers.all(p, has(p.weight))",message="strategy Weighted requires a weight on every provider" // (AWS once required at least one region here, because an omitted list meant "the // client's default region" and its client has none. Omitted now means "every region @@ -58,7 +63,14 @@ type NodePoolSpec struct { // Strategy is the INNER axis: how to rank providers within the active // capacity tier. It never overrides the capacity tier ordering. - // +kubebuilder:validation:Enum=LowestPrice;Ordered;Weighted + // + // Only Ordered is accepted today. LowestPrice and Weighted are defined as + // constants (and the Weighted weight rule is already enforced above) but are + // deliberately kept OUT of the enum until the ranking is implemented: admitting + // a value the placement walk silently ignores would let a pool claim a policy it + // does not get, which is worse than rejecting it at admission. Widening the enum + // is the one change needed to enable them once selectPlacement ranks. + // +kubebuilder:validation:Enum=Ordered // +kubebuilder:default=Ordered Strategy PlacementStrategy `json:"strategy,omitempty"` @@ -77,7 +89,8 @@ type ProviderSpec struct { Name string `json:"name"` // Weight is the relative share of new placements for the Weighted strategy. - // Ignored by other strategies. + // Ignored by other strategies, which today means ignored entirely: Strategy + // accepts only Ordered, so setting this has no effect until Weighted is enabled. // +kubebuilder:validation:Minimum=1 // +optional Weight *int32 `json:"weight,omitempty"` @@ -118,14 +131,21 @@ type ProviderSpec struct { } // PlacementStrategy ranks providers WITHIN a capacity tier (the inner axis). +// +// Only StrategyOrdered is admitted by NodePoolSpec.Strategy's enum today. The other +// two are declared here so the vocabulary is stable and testable ahead of the +// ranking implementation, NOT because they can be requested — see Strategy. type PlacementStrategy string const ( // StrategyLowestPrice picks the lowest $/hr provider in the active tier. + // NOT YET ACCEPTED by the Strategy enum. StrategyLowestPrice PlacementStrategy = "LowestPrice" - // StrategyOrdered uses the Providers list order as strict priority. + // StrategyOrdered uses the Providers list order as strict priority. The only + // strategy accepted today, and the default. StrategyOrdered PlacementStrategy = "Ordered" // StrategyWeighted spreads placements to match per-provider weights. + // NOT YET ACCEPTED by the Strategy enum. StrategyWeighted PlacementStrategy = "Weighted" ) diff --git a/api/v1alpha1/sandbox_types.go b/api/v1alpha1/sandbox_types.go index 2b0947f..2eb1c28 100644 --- a/api/v1alpha1/sandbox_types.go +++ b/api/v1alpha1/sandbox_types.go @@ -230,7 +230,7 @@ type SandboxStatus struct { } // +kubebuilder:object:root=true -// +kubebuilder:resource:scope=Namespaced,shortName={sb,sbx} +// +kubebuilder:resource:scope=Namespaced,shortName=sb // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Phase",type=string,JSONPath=`.status.phase` // +kubebuilder:printcolumn:name="Pool",type=string,JSONPath=`.spec.nodePoolRef` diff --git a/api/v1alpha1/sandboxset_types.go b/api/v1alpha1/sandboxset_types.go index 279980e..c7b9152 100644 --- a/api/v1alpha1/sandboxset_types.go +++ b/api/v1alpha1/sandboxset_types.go @@ -141,7 +141,7 @@ type SandboxSetStatus struct { } // +kubebuilder:object:root=true -// +kubebuilder:resource:scope=Namespaced,shortName={sbs,sbxs} +// +kubebuilder:resource:scope=Namespaced,shortName=sbs // +kubebuilder:subresource:status // +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.replicas,selectorpath=.status.selector // +kubebuilder:printcolumn:name="Desired",type=integer,JSONPath=`.spec.replicas` diff --git a/config/crd/bases/nebula.inftyai.com_nodepools.yaml b/config/crd/bases/nebula.inftyai.com_nodepools.yaml index 9307987..9c5d824 100644 --- a/config/crd/bases/nebula.inftyai.com_nodepools.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodepools.yaml @@ -57,8 +57,8 @@ spec: CapacityTypes (in listed order): // outer: hard tier\n\t candidates = Providers x (each provider's Regions) x {this capacityType},\n\t available now, minus blocklist // region nests per provider\n\t - \ IF candidates non-empty:\n\t pick one via Strategy (LowestPrice - | Ordered | Weighted) // inner: rank candidates\n\t DONE\n\t + \ IF candidates non-empty:\n\t pick one via Strategy (Ordered + today; see Strategy) // inner: rank candidates\n\t DONE\n\t \ // else fall through to the next capacity tier\n\nRegion is a per-provider axis (see ProviderSpec.Regions), nested under each\nprovider because a region name only means something to one provider. It\nwidens the candidate @@ -71,11 +71,15 @@ spec: capacity tier; it never crosses tiers.\n\nThe Weighted strategy requires a weight on every provider ref. This is a\nstatic property of the spec, so it is enforced at admission by the CEL rule\nbelow rather than surfaced - as a status condition after the fact.\n(AWS once required at least one - region here, because an omitted list meant \"the\nclient's default region\" - and its client has none. Omitted now means \"every region\nthe provider - serves\", which is a valid — if broad — AWS policy, so the rule is gone.\nSee - ProviderSpec.Regions.)" + as a status condition after the fact.\n\nThe rule is currently UNREACHABLE + — Strategy's enum admits only Ordered, so no\nobject can carry Weighted + for it to check. It is retained rather than deleted so\nthat widening + the enum is a one-line change that cannot silently ship without its\nweight + validation; the cost is one always-true CEL evaluation per admission.\n(AWS + once required at least one region here, because an omitted list meant + \"the\nclient's default region\" and its client has none. Omitted now + means \"every region\nthe provider serves\", which is a valid — if broad + — AWS policy, so the rule is gone.\nSee ProviderSpec.Regions.)" properties: capacityTypes: default: @@ -168,7 +172,8 @@ spec: weight: description: |- Weight is the relative share of new placements for the Weighted strategy. - Ignored by other strategies. + Ignored by other strategies, which today means ignored entirely: Strategy + accepts only Ordered, so setting this has no effect until Weighted is enabled. format: int32 minimum: 1 type: integer @@ -183,10 +188,15 @@ spec: description: |- Strategy is the INNER axis: how to rank providers within the active capacity tier. It never overrides the capacity tier ordering. + + Only Ordered is accepted today. LowestPrice and Weighted are defined as + constants (and the Weighted weight rule is already enforced above) but are + deliberately kept OUT of the enum until the ranking is implemented: admitting + a value the placement walk silently ignores would let a pool claim a policy it + does not get, which is worse than rejecting it at admission. Widening the enum + is the one change needed to enable them once selectPlacement ranks. enum: - - LowestPrice - Ordered - - Weighted type: string required: - providers diff --git a/config/crd/bases/nebula.inftyai.com_sandboxes.yaml b/config/crd/bases/nebula.inftyai.com_sandboxes.yaml index 505be87..e98b198 100644 --- a/config/crd/bases/nebula.inftyai.com_sandboxes.yaml +++ b/config/crd/bases/nebula.inftyai.com_sandboxes.yaml @@ -13,7 +13,6 @@ spec: plural: sandboxes shortNames: - sb - - sbx singular: sandbox scope: Namespaced versions: diff --git a/config/crd/bases/nebula.inftyai.com_sandboxsets.yaml b/config/crd/bases/nebula.inftyai.com_sandboxsets.yaml index 4df3fa4..9de1e6f 100644 --- a/config/crd/bases/nebula.inftyai.com_sandboxsets.yaml +++ b/config/crd/bases/nebula.inftyai.com_sandboxsets.yaml @@ -13,7 +13,6 @@ spec: plural: sandboxsets shortNames: - sbs - - sbxs singular: sandboxset scope: Namespaced versions: diff --git a/docs/architecture.md b/docs/architecture.md index 6eb08d5..ac889a2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -174,10 +174,12 @@ Follow one GPU Pod from creation to teardown: joined token rather than one region name). An empty expansion still yields one unconstrained `""` candidate so the walk runs. - `Ordered`, `LowestPrice`, and `Weighted` are API values, but the current inner - ranking is still listed order. The placement flow is already structured so - price or weight ranking can replace the inner ordering without changing the - rest of the controller. + `Ordered` is the only strategy the API accepts, and the inner ranking is listed + order. `LowestPrice` and `Weighted` exist as constants but are deliberately kept + out of the enum until the ranking is implemented — admitting a strategy the walk + ignores would let a pool claim a policy it does not get. The placement flow is + already structured so price or weight ranking can replace the inner ordering + without changing the rest of the controller; widening the enum is the switch. 4. **Create the ledger first.** Before the Pod can bind, the controller creates a deterministic NodeClaim named from the Pod namespace/name. The claim records diff --git a/internal/controller/nodepool_validation_test.go b/internal/controller/nodepool_validation_test.go index 72892ed..d3a25a2 100644 --- a/internal/controller/nodepool_validation_test.go +++ b/internal/controller/nodepool_validation_test.go @@ -63,26 +63,31 @@ var _ = Describe("NodePool spec validation", func() { Expect(err.Error()).To(ContainSubstring("must have at most 8 items")) }) - It("rejects a Weighted pool with a provider missing a weight", func() { - pool := newWeightedPool("weighted-missing", + // Strategy admits only Ordered today (see NodePoolSpec.Strategy). These assert + // the restriction itself, because it is the enum — not the Weighted weight CEL + // rule — that now rejects the other two. The weight rule is retained but + // unreachable, so it has no admission behaviour left to test: a Weighted pool + // WITH weights on every provider is rejected just the same, which is what the + // second spec below pins. + It("rejects a Weighted pool even with a weight on every provider", func() { + pool := newWeightedPool("weighted-ok", nebulav1alpha1.ProviderSpec{Name: "modal", Weight: weight(3)}, - nebulav1alpha1.ProviderSpec{Name: "runpod"}, // no weight + nebulav1alpha1.ProviderSpec{Name: "runpod", Weight: weight(1)}, ) err := k8sClient.Create(ctx, pool) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("strategy Weighted requires a weight on every provider")) + Expect(err.Error()).To(ContainSubstring(`Unsupported value: "Weighted"`)) }) - It("admits a Weighted pool with a weight on every provider", func() { - pool := newWeightedPool("weighted-ok", - nebulav1alpha1.ProviderSpec{Name: "modal", Weight: weight(3)}, - nebulav1alpha1.ProviderSpec{Name: "runpod", Weight: weight(1)}, - ) - Expect(k8sClient.Create(ctx, pool)).To(Succeed()) - Expect(k8sClient.Delete(ctx, pool)).To(Succeed()) + It("rejects a LowestPrice pool", func() { + pool := newPool("lowest-price", nebulav1alpha1.StrategyLowestPrice, + nebulav1alpha1.ProviderSpec{Name: "modal"}) + err := k8sClient.Create(ctx, pool) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(`Unsupported value: "LowestPrice"`)) }) - It("admits a non-Weighted pool regardless of weights", func() { + It("admits an Ordered pool regardless of weights", func() { pool := &nebulav1alpha1.NodePool{ ObjectMeta: metav1.ObjectMeta{Name: "ordered-noweights"}, Spec: nebulav1alpha1.NodePoolSpec{