From e6761f1b9d22f7bd1efca5584cb0c92549a0799f Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 13 Aug 2026 17:06:57 +0100 Subject: [PATCH 1/3] support regions in modal Signed-off-by: kerthcet --- README.md | 8 +- api/v1alpha1/nodepool_types.go | 54 +++++---- cmd/main.go | 9 +- .../bases/nebula.inftyai.com_nodepools.yaml | 58 +++++---- docs/add-a-provider.md | 6 +- docs/architecture.md | 14 ++- .../controller/nodeclaim_controller_test.go | 11 ++ .../pod_placement_controller_test.go | 64 ++++++++++ internal/controller/pod_placement_helpers.go | 34 ++++-- pkg/provider/aws/aws.go | 111 ++++++++++++++++- pkg/provider/aws/aws_test.go | 113 +++++++++++++++++- pkg/provider/aws/client.go | 5 +- pkg/provider/catalog/base.go | 17 +++ pkg/provider/modal/client.go | 3 + pkg/provider/modal/modal.go | 93 +++++++++++--- pkg/provider/modal/modal_test.go | 69 +++++++++++ pkg/provider/provider.go | 61 ++++++++-- pkg/vnode/handler_test.go | 1 + 18 files changed, 631 insertions(+), 100 deletions(-) diff --git a/README.md b/README.md index e88781b..6f97cdd 100644 --- a/README.md +++ b/README.md @@ -53,11 +53,11 @@ metadata: name: gpu spec: providers: - - name: modal # NeoCloud; region-simple, no regions needed - - name: aws # hyperscaler; region-aware, at least one required + - name: modal # NeoCloud; regions omitted = place anywhere (cheapest) + - name: aws # hyperscaler; "us" expands to every US region regions: - - us-east-1 - - us-west-1 + - us + - eu-west-1 # or name one region exactly capacityTypes: # prefer cheap Spot, fall back to OnDemand - Spot - OnDemand diff --git a/api/v1alpha1/nodepool_types.go b/api/v1alpha1/nodepool_types.go index 835a1c5..463a53c 100644 --- a/api/v1alpha1/nodepool_types.go +++ b/api/v1alpha1/nodepool_types.go @@ -34,7 +34,10 @@ import ( // 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. // +kubebuilder:validation:XValidation:rule="self.strategy != 'Weighted' || self.providers.all(p, has(p.weight))",message="strategy Weighted requires a weight on every provider" -// +kubebuilder:validation:XValidation:rule="self.providers.all(p, p.name != 'aws' || (has(p.regions) && size(p.regions) > 0))",message="provider aws requires at least one region" +// (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 +// the provider serves", which is a valid — if broad — AWS policy, so the rule is gone. +// See ProviderSpec.Regions.) type NodePoolSpec struct { // Providers is the ordered set of NeoClouds this pool is allowed to use. // A Pod bound to this pool can only ever be placed on a provider in this @@ -79,26 +82,37 @@ type ProviderSpec struct { // +optional Weight *int32 `json:"weight,omitempty"` - // Regions constrains this provider to a subset of its regions, in the - // provider's OWN vocabulary (e.g. ["us-east-1","eu-west-2"] for AWS). Region - // is provider-namespaced — there is no cross-provider region vocabulary — so - // it lives here per provider, not on the pool. Two cases: - // - omitted/empty => the provider's configured default region (the region - // its client resolved from env/config/instance metadata at startup). This - // is the no-surprise default for region-simple providers (Modal, RunPod), - // which have a single region and ignore this field. - // - explicit list => exactly those regions. - // AWS is the exception: it is region-aware with no meaningful single default, - // so a `- name: aws` entry MUST list at least one region. That is enforced at - // admission by the CEL rule on NodePoolSpec (a per-provider requirement, so it - // belongs on the spec where all provider entries are visible, not as a blanket - // MinItems that would burden region-simple providers). An "all regions" - // wildcard is intentionally NOT supported yet: it only makes sense once the - // price-ranking optimizer can expand it against the provider's catalog and - // choose among the results, so it is reserved for then. At most 8 regions; - // maxLength bounds each entry. + // Regions constrains where this provider may place, in the provider's OWN + // vocabulary. Region is provider-namespaced — there is no cross-provider region + // vocabulary — so it lives here per provider, not on the pool. It is a + // CONSTRAINT, not a list of regions to use, and it has three levels: + // - omitted/empty => unconstrained: every region the provider serves. For a + // region-simple provider (Modal) this means "send no region and let the + // provider place freely", which is also its widest and cheapest mode. + // - a geography GROUP token ("us", "eu", "ap", ...) => that geography's + // regions. This is the recommended way to ask for breadth with a data + // residency boundary. + // - a literal region name ("us-east-1" on AWS, "us-east" on Modal) => exactly + // that region. + // The provider resolves which level a value is, since only it knows its own + // geography (see provider.Provider's ExpandRegions). Group tokens are shared + // across providers but the regions behind them are not: "eu" is eu-west-1 and + // friends on AWS, while London is eu-west-2 there and there is no "uk" group. + // + // A value that is not a group token is passed to the provider UNVALIDATED. + // Region names change faster than Nebula ships, so an unrecognized one is + // forwarded rather than rejected: a genuinely bad name fails at provision time + // with the provider's own error, which is better than refusing a region that + // launched last week. It is also the escape hatch for AWS opt-in regions, which + // no group contains. + // + // Unconstrained on a region-aware provider is the widest setting and costs + // something: every region becomes a placement candidate to walk on failover, and + // every region is swept by the observability poll loop. Prefer a group unless the + // workload genuinely needs global reach. There is no cap on the number of entries + // (a group already expands to many, so capping the declaration would be + // arbitrary); maxLength bounds each entry. // +optional - // +kubebuilder:validation:MaxItems=8 // +kubebuilder:validation:items:MaxLength=32 Regions []string `json:"regions,omitempty"` } diff --git a/cmd/main.go b/cmd/main.go index 82e3fff..1d52aa3 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -485,7 +485,14 @@ func awsRegionSource(c client.Client) awsprovider.RegionSource { for i := range pools.Items { for _, ps := range pools.Items[i].Spec.Providers { if ps.Name == provider.ProviderAWS { - regions = append(regions, ps.Regions...) + // Expand PER POOL, before unioning. ProviderSpec.Regions is a + // constraint, not a list: an omitted one means "every region", and + // unioning the raw lists first would collapse that to "nothing" — + // the swept set would miss regions placement provisions into, and + // List's absence is reported as Terminated on live instances. + // This is the same expansion regionsFor applies on the placement + // side; both must agree, so both call this one function. + regions = append(regions, awsprovider.ExpandRegions(ps.Regions)...) } } } diff --git a/config/crd/bases/nebula.inftyai.com_nodepools.yaml b/config/crd/bases/nebula.inftyai.com_nodepools.yaml index 867afa9..9307987 100644 --- a/config/crd/bases/nebula.inftyai.com_nodepools.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodepools.yaml @@ -71,7 +71,11 @@ 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." + 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.)" properties: capacityTypes: default: @@ -127,28 +131,39 @@ spec: type: string regions: description: |- - Regions constrains this provider to a subset of its regions, in the - provider's OWN vocabulary (e.g. ["us-east-1","eu-west-2"] for AWS). Region - is provider-namespaced — there is no cross-provider region vocabulary — so - it lives here per provider, not on the pool. Two cases: - - omitted/empty => the provider's configured default region (the region - its client resolved from env/config/instance metadata at startup). This - is the no-surprise default for region-simple providers (Modal, RunPod), - which have a single region and ignore this field. - - explicit list => exactly those regions. - AWS is the exception: it is region-aware with no meaningful single default, - so a `- name: aws` entry MUST list at least one region. That is enforced at - admission by the CEL rule on NodePoolSpec (a per-provider requirement, so it - belongs on the spec where all provider entries are visible, not as a blanket - MinItems that would burden region-simple providers). An "all regions" - wildcard is intentionally NOT supported yet: it only makes sense once the - price-ranking optimizer can expand it against the provider's catalog and - choose among the results, so it is reserved for then. At most 8 regions; - maxLength bounds each entry. + Regions constrains where this provider may place, in the provider's OWN + vocabulary. Region is provider-namespaced — there is no cross-provider region + vocabulary — so it lives here per provider, not on the pool. It is a + CONSTRAINT, not a list of regions to use, and it has three levels: + - omitted/empty => unconstrained: every region the provider serves. For a + region-simple provider (Modal) this means "send no region and let the + provider place freely", which is also its widest and cheapest mode. + - a geography GROUP token ("us", "eu", "ap", ...) => that geography's + regions. This is the recommended way to ask for breadth with a data + residency boundary. + - a literal region name ("us-east-1" on AWS, "us-east" on Modal) => exactly + that region. + The provider resolves which level a value is, since only it knows its own + geography (see provider.Provider's ExpandRegions). Group tokens are shared + across providers but the regions behind them are not: "eu" is eu-west-1 and + friends on AWS, while London is eu-west-2 there and there is no "uk" group. + + A value that is not a group token is passed to the provider UNVALIDATED. + Region names change faster than Nebula ships, so an unrecognized one is + forwarded rather than rejected: a genuinely bad name fails at provision time + with the provider's own error, which is better than refusing a region that + launched last week. It is also the escape hatch for AWS opt-in regions, which + no group contains. + + Unconstrained on a region-aware provider is the widest setting and costs + something: every region becomes a placement candidate to walk on failover, and + every region is swept by the observability poll loop. Prefer a group unless the + workload genuinely needs global reach. There is no cap on the number of entries + (a group already expands to many, so capping the declaration would be + arbitrary); maxLength bounds each entry. items: maxLength: 32 type: string - maxItems: 8 type: array weight: description: |- @@ -179,9 +194,6 @@ spec: x-kubernetes-validations: - message: strategy Weighted requires a weight on every provider rule: self.strategy != 'Weighted' || self.providers.all(p, has(p.weight)) - - message: provider aws requires at least one region - rule: self.providers.all(p, p.name != 'aws' || (has(p.regions) && size(p.regions) - > 0)) status: description: NodePoolStatus surfaces the current placement picture for observability. diff --git a/docs/add-a-provider.md b/docs/add-a-provider.md index 68e8948..b56b8d3 100644 --- a/docs/add-a-provider.md +++ b/docs/add-a-provider.md @@ -6,8 +6,9 @@ drives everything through the `provider.Provider` interface and a price/availabi catalog, so a new provider is an adapter package plus a little wiring — no changes to the placement controller, virtual kubelet, or NodeClaim controller. -Use `pkg/provider/modal` (region-simple NeoCloud) and `pkg/provider/aws` -(region-aware hyperscaler) as references. +Use `pkg/provider/modal` (NeoCloud, coarse regions, all optional) and +`pkg/provider/aws` (hyperscaler, a region is mandatory on every call) as +references. ## 1. Implement the adapter @@ -25,6 +26,7 @@ Create `pkg/provider//` and implement `provider.Provider` | `Offerings(ctx)` | Price/availability rows for the optimizer (see the catalog below). | | `MapAccelerator(canonical, count)` | Translate a canonical accelerator (type + count) to the provider's own id; `ok=false` if unsupported. | | `ClassifyProvisionError(err, accel, region)` | Map a Provision failure to the `BlockScope` failover should blocklist (a capacity error → that {accel, tier, region}; an auth/quota error → the whole provider). | +| `ExpandRegions(declared)` | Turn a pool's `regions` into the concrete regions to try. `catalog.Base` passes them through unchanged, which is right whenever the provider's own region names already include the group tokens (`us`, `eu`, `ap`) a pool may write — Modal's do. Override only if they don't, as `pkg/provider/aws` does with a static table. | The Pod is the single source of truth for the workload shape; `ProvisionRequest` carries only what the Pod cannot express (the optimizer's capacity tier and the diff --git a/docs/architecture.md b/docs/architecture.md index d4aa234..1e6ea22 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -49,10 +49,16 @@ mapping), see [docs/status.md](status.md). **Non-goals in the current implementation** -- Provider-neutral geography. Region is provider-specific (`ProviderSpec.Regions`) - and there is no global "us-east" vocabulary across clouds. -- "All regions" expansion. AWS pool entries must list explicit regions; an - all-regions wildcard is reserved for a richer price/availability optimizer. +- Provider-neutral geography. `ProviderSpec.Regions` accepts shared *group* tokens + (`us`, `eu`, `ap`), but the regions behind them are per-provider and the narrower + names are each cloud's own vocabulary — there is no global region namespace. Which + level a value is, is resolved by the provider (`ExpandRegions`); an omitted list + means every region it serves. +- Price-ranked region choice. Within a capacity tier the expanded regions are walked + in order, not ranked: the catalog carries no per-region prices, so a wide + declaration cannot yet prefer the cheapest region. Modal is the sharper case — a + pinned region there costs 1.5x (group) or 1.75x (narrow) over its unconstrained + default, which the catalog does not model. - Bin-packing multiple unrelated Pods onto one external instance. The current model is one workload Pod to one external instance. - In-place migration. Recovery from reclaim, failure, or spec changes is diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index a535688..552f3b6 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -47,6 +47,8 @@ type fakeProvider struct { terminateErr error // if set, Terminate fails gpus []string // accelerators MapAccelerator offers; nil = offer any spot bool // Capabilities().SupportsSpot (placement skips Spot without it) + // expandRegions overrides ExpandRegions; nil = pass the declaration through. + expandRegions func([]string) []string } func (f *fakeProvider) Name() string { return f.name } @@ -76,6 +78,15 @@ func (f *fakeProvider) MapAccelerator(c string, _ int32) ([]string, bool) { } return nil, false } + +// ExpandRegions passes the declaration through, matching catalog.Base's default (the +// region-simple behaviour). Tests that need group expansion set expandRegions. +func (f *fakeProvider) ExpandRegions(declared []string) []string { + if f.expandRegions != nil { + return f.expandRegions(declared) + } + return declared +} func (f *fakeProvider) ClassifyProvisionError(error, string, string) provider.BlockScope { return provider.BlockScope{} } diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index 4ff0313..e2d372a 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -18,6 +18,7 @@ package controller import ( "context" + "slices" "testing" "time" @@ -33,6 +34,7 @@ import ( nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/failover" "github.com/InftyAI/Nebula/pkg/provider" + awsprovider "github.com/InftyAI/Nebula/pkg/provider/aws" "github.com/InftyAI/Nebula/pkg/util" ) @@ -476,6 +478,68 @@ func TestPlacement_CapacityIsOuterAxis(t *testing.T) { } } +func TestPlacement_ExpandsRegionGroupIntoConcreteCandidates(t *testing.T) { + // The pool declares a GROUP token, not a region. Placement must walk the concrete + // regions the provider expands it into — and must stamp a CONCRETE one on the Pod, + // never the token: RegionAnnotation feeds ProvisionRequest.Region, which the + // adapter turns into a regional API endpoint, and "us" is not one. + pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") + pool := poolWithRegions("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, + provider.ProviderModal, "us") + prov := &fakeProvider{ + name: provider.ProviderModal, gpus: []string{"H100"}, + expandRegions: func(declared []string) []string { + if slices.Equal(declared, []string{"us"}) { + return []string{"us-east-1", "us-west-2"} + } + return declared + }, + } + // The first expanded region is blocked, so the walk must reach the second — proving + // the group really became multiple candidates rather than one opaque value. + r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov) + r.Blocklist = &fakeBlocklist{blocked: []failover.Candidate{ + {Provider: provider.ProviderModal, Accelerator: "H100:1", + CapacityType: nebulav1alpha1.CapacityOnDemand, Region: "us-east-1"}, + }} + + reconcilePod(t, r, "default", "p1") + + got := getPod(t, c, "default", "p1") + if region := got.Annotations[nebulav1alpha1.RegionAnnotation]; region != "us-west-2" { + t.Fatalf("expected the group to expand and fail over to us-west-2, got %q", region) + } +} + +func TestRegionsFor_UnconstrainedOnRegionSimpleProviderYieldsOneCandidate(t *testing.T) { + // A region-simple provider passes nil through (catalog.Base's default), so + // expansion yields nothing. regionsFor must still emit ONE candidate — the empty + // region, meaning "send no region" — or `range` would run zero times and the + // provider would be silently unplaceable with no error anywhere. + prov := &fakeProvider{name: provider.ProviderModal} + got := regionsFor(prov, nebulav1alpha1.ProviderSpec{Name: provider.ProviderModal}) + if !slices.Equal(got, []string{""}) { + t.Fatalf("regionsFor(nil) = %v, want one empty candidate", got) + } +} + +func TestRegionsFor_AgreesWithAWSSweepExpansion(t *testing.T) { + // The two readers of ProviderSpec.Regions — placement's regionsFor and the AWS + // RegionSource in cmd/main.go — MUST expand a declaration identically. If the + // sweep covers less than placement provisions into, the missing region's instances + // are absent from List, and applyState maps absence to Terminated: a live, billing + // fleet reported as gone. Both go through ExpandRegions; this pins that they do. + for _, declared := range [][]string{nil, {"us"}, {"eu"}, {"us-east-1"}, {"us", "me-central-1"}} { + placementSide := regionsFor(awsprovider.New(nil, nil, nil), + nebulav1alpha1.ProviderSpec{Name: provider.ProviderAWS, Regions: declared}) + sweepSide := awsprovider.ExpandRegions(declared) + if !slices.Equal(placementSide, sweepSide) { + t.Errorf("declared %v: placement walks %v but the sweep covers %v", + declared, placementSide, sweepSide) + } + } +} + func TestPlacement_SkipsSpotWhenProviderHasNoSpotTier(t *testing.T) { // Modal has no user-facing preemptible capacity (SupportsSpot=false). The pool // asks for Spot first, but that candidate is unservable, so the walk falls diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index 73ab8da..8684d9d 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -134,7 +134,7 @@ func (r *PodPlacementReconciler) selectPlacement(ctx context.Context, pod *corev continue } } - for _, region := range regionsFor(ref) { // inner: region + for _, region := range regionsFor(prov, ref) { // inner: region if until, blocked := r.blockedUntil(ref.Name, accelerator, tier, region); blocked { // Servable but failed recently; try the next region, then the next // tier, and remember when this one frees so we can requeue for it. @@ -192,17 +192,29 @@ func servesCapacity(prov provider.Provider, tier nebulav1alpha1.CapacityType) bo return prov.Capabilities().SupportsSpot } -// regionsFor is the inner axis for one provider ref: the regions to try, in listed -// order. An empty/omitted list means "the provider's configured default region", -// represented as a single empty-string candidate so the walk runs once for -// region-simple providers (Modal, RunPod). AWS is required by admission to list at -// least one region (the CEL rule on NodePoolSpec). An "all regions" wildcard is not -// supported yet (see ProviderSpec.Regions), so there is nothing to expand here. -func regionsFor(ref nebulav1alpha1.ProviderSpec) []string { - if len(ref.Regions) == 0 { - return []string{""} // omitted => provider default region (region-simple providers) +// regionsFor is the inner axis for one provider ref: the concrete regions to try, in +// expansion order. The pool's declaration is a CONSTRAINT, not a list of regions — +// it may be omitted (unconstrained), name a geography group ("us"), or name regions +// literally — so only the provider can resolve it, and ExpandRegions does (see +// provider.Provider for the three levels). +// +// The empty-string fallback survives for the case where expansion yields nothing: a +// region-simple provider whose pool declared no regions (Modal returns nil unchanged) +// still needs ONE candidate or `range` would run zero times and the provider would be +// silently unplaceable. That empty candidate means "send no region; let the provider +// place freely" — which for Modal is both its normal mode and its cheapest. +// +// This and awsRegionSource (cmd/main.go) are the only two readers of +// ProviderSpec.Regions and MUST expand it identically: a region placement provisions +// into but the sweep does not cover is absent from List, and absence is reported as +// Terminated on a live, billing instance. Routing both through ExpandRegions is what +// makes divergence impossible. +func regionsFor(prov provider.Provider, ref nebulav1alpha1.ProviderSpec) []string { + regions := prov.ExpandRegions(ref.Regions) + if len(regions) == 0 { + return []string{""} // unconstrained on a region-simple provider } - return ref.Regions + return regions } // blockedUntil reports whether the (provider, accelerator, tier, region) diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index b8b430c..958b48c 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -40,6 +40,7 @@ import ( "context" "errors" "fmt" + "sort" "strings" "sync" "time" @@ -73,6 +74,37 @@ const spotPollInterval = 10 * time.Second // poll loop's job). const provisionTimeout = 2 * time.Minute +// regionGroups maps a NodePool geography token to the EC2 regions it covers. A +// group token is NOT an EC2 region name and cannot be derived from one by string +// surgery ("us" is not a callable endpoint, and London is eu-west-2, not uk-*), so +// the mapping is data. Every value here is a DEFAULT-enabled region: opt-in regions +// (launched after 2019-03-20 and disabled until an operator enables them — af-south-1, +// ap-east-1/2, ap-south-2, ap-southeast-3/4/5/6/7, ca-west-1, eu-central-2, +// eu-south-1/2, il-central-1, me-central-1, me-south-1, mx-central-1) are +// deliberately EXCLUDED, so a group or an unconstrained pool can only ever expand to +// regions the account can actually use. +// +// That exclusion is what keeps the wide cases cheap. clientFor resolves a GPU AMI and +// the default VPC's subnets on first use and deliberately does not cache failures, so +// a region the account has not enabled would fail that resolution and retry on every +// poll tick (every spotPollInterval), logging forever, for a region nobody asked for. +// An operator who HAS enabled one names it explicitly — a literal region name is +// passed through untouched, so nothing here restricts them. +// +// GovCloud (us-gov-*) and China (cn-*) are absent for a stronger reason: they are +// separate IAM partitions, so one credential set cannot reach them at all (see +// NewSDKClient's security contract). They are not a group member under any token. +// +// Kept sorted so expansion order — and therefore the failover walk order within a +// group — is stable and reviewable rather than an accident of map iteration. +var regionGroups = map[string][]string{ + "us": {"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, + "eu": {"eu-central-1", "eu-north-1", "eu-west-1", "eu-west-2", "eu-west-3"}, + "ap": {"ap-northeast-1", "ap-northeast-2", "ap-northeast-3", "ap-south-1", "ap-southeast-1", "ap-southeast-2"}, + "ca": {"ca-central-1"}, + "sa": {"sa-east-1"}, +} + // ErrSpotCapacity is a marker the Client wraps onto a Spot-tier capacity failure // (alongside provider.ErrNoCapacity) so ClassifyProvisionError — which the // interface hands only the error, not the request — can recover that the failing @@ -222,9 +254,10 @@ type Provider struct { // provisioned into). cat is the catalog.Lookup seam so tests can inject a fake. // // There is deliberately NO default region: every request carries its own region -// (admission requires each aws pool to list ≥1 region, and placement stamps it onto -// the ProvisionRequest), and observed instances report their region from the -// region-pinned client — so nothing needs a fallback, and no AWS_REGION env is read. +// (ExpandRegions turns even an omitted pool declaration into concrete regions, and +// placement stamps one onto the ProvisionRequest), and observed instances report +// their region from the region-pinned client — so nothing needs a fallback, and no +// AWS_REGION env is read. func New(newClient ClientFactory, cat catalog.Lookup, regionSource RegionSource) *Provider { return &Provider{ Base: catalog.Base{ProviderName: provider.ProviderAWS, Catalog: cat}, @@ -251,6 +284,78 @@ func newSingleRegion(client Client, cat catalog.Lookup, region string) *Provider return p } +// ExpandRegions implements provider.Provider, overriding catalog.Base's pass-through: +// EC2 region names do not contain the pool's group tokens, so AWS needs the +// regionGroups table. Three levels, in the order they are checked: +// +// nil/[] => every default-enabled region (the union of regionGroups) +// ["us"] => that group's regions +// ["us-east-1"] => itself, verbatim and unvalidated +// +// A value that is not a group token is a literal region name. It is NOT checked +// against any list: EC2 gains regions faster than this table is edited, so a +// hardcoded validation would reject a region that exists, whereas passing an +// impossible name through fails at clientFor with AWS's own error. Forwarding is the +// safer direction of wrongness. That is also the escape hatch for opt-in regions, +// which no group contains. +// +// The result is deduped (["us", "us-east-1"] is 4 regions, not 5) and order-stable: +// group order follows the table, and a literal keeps its declared position, so the +// failover walk is reproducible. +// +// Unconstrained is the widest case and worth understanding before using it: ~17 +// regions per capacity tier, each a candidate placement walks in turn, and each swept +// by List/Offerings on every poll tick. Prefer a group when the workload does not +// genuinely need global reach. +// It delegates to the package-level ExpandRegions so the same resolution is +// available to the region source in cmd/main.go, which must expand each pool's +// declaration BEFORE unioning across pools (a pool declaring nothing means "all", a +// meaning that would be lost if raw lists were unioned first) and therefore cannot +// wait for a constructed Provider. +func (p *Provider) ExpandRegions(declared []string) []string { return ExpandRegions(declared) } + +// ExpandRegions is Provider.ExpandRegions as a package-level function; see that +// method for the semantics. It is exported because the NodePool-backed RegionSource +// in cmd/main.go must apply the identical expansion, and it needs it per-pool at a +// point where no Provider is in hand. +func ExpandRegions(declared []string) []string { + seen := make(map[string]bool) + var out []string + add := func(r string) { + if r == "" || seen[r] { + return + } + seen[r] = true + out = append(out, r) + } + // Unconstrained: every default-enabled region. Walk the group table in sorted key + // order so the union is deterministic (Go randomizes map iteration). + if len(declared) == 0 { + groups := make([]string, 0, len(regionGroups)) + for g := range regionGroups { + groups = append(groups, g) + } + sort.Strings(groups) + for _, g := range groups { + for _, r := range regionGroups[g] { + add(r) + } + } + return out + } + for _, d := range declared { + d = strings.TrimSpace(d) + if group, ok := regionGroups[strings.ToLower(d)]; ok { + for _, r := range group { + add(r) + } + continue + } + add(d) // a literal region name: forwarded unvalidated + } + return out +} + // sweepRegions returns the regions List and Offerings fan out across: the union of // the NodePool-declared set (regionSource) and every region already in the lazy // client cache. The cache half is what makes teardown survive a NodePool edit — an diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index 639de64..7443ecf 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -21,6 +21,8 @@ import ( "errors" "fmt" "reflect" + "slices" + "strings" "testing" "time" @@ -304,8 +306,8 @@ func TestProvision_EmptyRegionIsError(t *testing.T) { // There is NO default region: a request that omits one cannot build a client, so // Provision errors rather than silently guessing. In production every request - // carries a region (admission requires each aws pool to list ≥1; placement stamps - // it), so this only guards a malformed request. + // carries a region (ExpandRegions never yields an empty one; placement stamps it), + // so this only guards a malformed request. if _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ ClaimName: "claim-def", }); err == nil { @@ -546,6 +548,113 @@ func TestCapabilities(t *testing.T) { } } +func TestExpandRegions(t *testing.T) { + cases := []struct { + name string + declared []string + want []string + }{{ + name: "nil is unconstrained: every default-enabled region", + declared: nil, + want: []string{ + "ap-northeast-1", "ap-northeast-2", "ap-northeast-3", "ap-south-1", "ap-southeast-1", "ap-southeast-2", + "ca-central-1", + "eu-central-1", "eu-north-1", "eu-west-1", "eu-west-2", "eu-west-3", + "sa-east-1", + "us-east-1", "us-east-2", "us-west-1", "us-west-2", + }, + }, { + name: "empty behaves as nil", + declared: []string{}, + want: []string{ + "ap-northeast-1", "ap-northeast-2", "ap-northeast-3", "ap-south-1", "ap-southeast-1", "ap-southeast-2", + "ca-central-1", + "eu-central-1", "eu-north-1", "eu-west-1", "eu-west-2", "eu-west-3", + "sa-east-1", + "us-east-1", "us-east-2", "us-west-1", "us-west-2", + }, + }, { + name: "group token expands", + declared: []string{"us"}, + want: []string{"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, + }, { + name: "group token is case-insensitive", + declared: []string{"EU"}, + want: []string{"eu-central-1", "eu-north-1", "eu-west-1", "eu-west-2", "eu-west-3"}, + }, { + name: "literal region passes through", + declared: []string{"us-east-1"}, + want: []string{"us-east-1"}, + }, { + // An opt-in region belongs to no group, so naming it explicitly is the only + // way to reach it — that escape hatch must keep working. + name: "opt-in region passes through though no group contains it", + declared: []string{"me-central-1"}, + want: []string{"me-central-1"}, + }, { + // Region names outlive this table, so an unknown value is FORWARDED, not + // rejected: it fails later with AWS's own error instead of Nebula refusing a + // region that shipped after this code did. + name: "unknown value is forwarded unvalidated", + declared: []string{"us-east-9", "not-a-region"}, + want: []string{"us-east-9", "not-a-region"}, + }, { + // A group and one of its own members must not yield a duplicate: placement + // walks the result, so a repeat would attempt the same region twice. + name: "group plus a member of it dedupes", + declared: []string{"us", "us-east-1"}, + want: []string{"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, + }, { + name: "declared order is preserved across groups and literals", + declared: []string{"ca", "us-east-1", "sa"}, + want: []string{"ca-central-1", "us-east-1", "sa-east-1"}, + }, { + name: "whitespace is trimmed and empties dropped", + declared: []string{" us-east-1 ", "", " "}, + want: []string{"us-east-1"}, + }} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := ExpandRegions(tc.declared) + if !slices.Equal(got, tc.want) { + t.Fatalf("ExpandRegions(%v)\n got %v\nwant %v", tc.declared, got, tc.want) + } + // The method must agree with the package function: cmd/main.go's region + // source calls the function while placement calls the method, and the two + // diverging is exactly the bug that reports a live fleet as Terminated. + p := newTestProvider(&fakeClient{}) + if m := p.ExpandRegions(tc.declared); !slices.Equal(m, got) { + t.Fatalf("method %v != function %v", m, got) + } + }) + } +} + +func TestRegionGroups_ExcludeOptInRegions(t *testing.T) { + // Opt-in regions are disabled until an operator enables them. clientFor does not + // cache build failures, so one in a group would fail its AMI/subnet resolution and + // retry on EVERY poll tick, forever, for a region nobody asked for. Guard the + // table against a well-meant future addition. + optIn := []string{ + "af-south-1", "ap-east-1", "ap-east-2", "ap-south-2", + "ap-southeast-3", "ap-southeast-4", "ap-southeast-5", "ap-southeast-6", "ap-southeast-7", + "ca-west-1", "eu-central-2", "eu-south-1", "eu-south-2", + "il-central-1", "me-central-1", "me-south-1", "mx-central-1", + } + all := ExpandRegions(nil) + for _, r := range optIn { + if slices.Contains(all, r) { + t.Errorf("opt-in region %q must not be in any group (it is disabled by default)", r) + } + } + // Separate IAM partitions: one credential set cannot reach them at all. + for _, r := range all { + if strings.HasPrefix(r, "us-gov-") || strings.HasPrefix(r, "cn-") { + t.Errorf("region %q is in another IAM partition and must not be in a group", r) + } + } +} + func TestOfferings_StampsRegionAndFiltersByLiveProbe(t *testing.T) { // The region offers only g4dn.xlarge and p5.48xlarge (not g4dn.metal or // p4de.24xlarge), so those two rows stay available and the rest, though still diff --git a/pkg/provider/aws/client.go b/pkg/provider/aws/client.go index 0ee0629..7f0c584 100644 --- a/pkg/provider/aws/client.go +++ b/pkg/provider/aws/client.go @@ -188,8 +188,9 @@ var _ Client = (*sdkClient)(nil) // and default-VPC subnets on demand. // // There is NO default region and NO region env (AWS_REGION is not read): every -// request carries its own region (admission requires each aws pool to list ≥1 region; -// placement stamps it), so a fallback would be dead config. This constructor fails +// request carries its own region (ExpandRegions resolves the pool's declaration — +// omitted, a group token, or literal names — into concrete regions, and placement +// stamps one), so a fallback would be dead config. This constructor fails // ONLY if the catalog cannot load — never on region config. // // Security contract (and why ONE credential set spans all regions): diff --git a/pkg/provider/catalog/base.go b/pkg/provider/catalog/base.go index 485b171..24c1ea7 100644 --- a/pkg/provider/catalog/base.go +++ b/pkg/provider/catalog/base.go @@ -76,6 +76,23 @@ func (b Base) Offerings(context.Context) ([]provider.Offering, error) { return b.Catalog.Offerings(b.ProviderName), nil } +// ExpandRegions passes the pool's declared regions through unchanged. This is the +// right default for a provider whose OWN vocabulary already spans both levels the +// pool speaks: Modal accepts "us" and "eu" as first-class placement values (its +// broad regions) alongside narrower ones, so there is nothing for Nebula to expand — +// the token IS the region name, and forwarding it verbatim is both correct and +// future-proof as the provider adds regions. +// +// nil stays nil, which every adapter must read as "unconstrained": send no region +// and let the provider place freely. On Modal that is also the cheapest option — a +// pinned region carries a 1.5x (broad) or 1.75x (narrow) price multiplier, so +// constraining placement there is a deliberate cost, not a free preference. +// +// A region-AWARE provider whose region names do not contain the group tokens (AWS: +// "us" is not a prefix of an EC2 region name you can call) must override this. See +// the AWS adapter. +func (b Base) ExpandRegions(declared []string) []string { return declared } + // MapAccelerator translates a canonical accelerator request (type + count) into // this provider's own accelerator ids using the catalog as the mapping table. It // finds the offering rows whose AcceleratorType matches (case-insensitively) and diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index fdc0d77..dffdb4b 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -137,6 +137,9 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string CPU: spec.CPU, MemoryMiB: spec.MemoryMiB, EncryptedPorts: spec.Ports, + // Nil leaves Modal's SchedulerPlacement unset entirely (the SDK only builds one + // when Regions is non-empty), which is the unconstrained, un-multiplied case. + Regions: spec.Regions, Timeout: spec.Timeout, Tags: spec.Tags, ReadinessProbe: probe, diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 569effc..699495d 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -20,11 +20,21 @@ limitations under the License. // Modal's shape drives several adapter decisions: // - Lifecycle is create/terminate only. A Modal Sandbox is spun up and later // terminated; there is no stop/resume, so Capabilities.SupportsStop=false. -// - Modal does not expose a user-facing spot/preemptible tier, so -// SupportsSpot=false. Placement consults that trait (servesCapacity in the -// capacity-tier loop) and skips a Spot candidate here rather than downgrading -// it silently, so only OnDemand (or default-tier) ProvisionRequests reach this -// adapter and it never has to interpret CapacityType. +// - The Go SDK exposes no spot/preemptible knob, so SupportsSpot=false. (Modal's +// own API does have one — SchedulerPlacement's lifecycle, surfaced in Python as +// nonpreemptible — but the pinned Go client only ever builds SchedulerPlacement +// from Regions, so there is no way to ask for it from here.) Placement consults +// the trait (servesCapacity in the capacity-tier loop) and SKIPS a Spot candidate +// rather than downgrading it silently, so only OnDemand (or default-tier) +// ProvisionRequests reach this adapter and it never interprets CapacityType. +// - Region is OPTIONAL here, unlike AWS: Modal's scheduler places freely when no +// region is given, and that unconstrained mode is both the widest capacity pool +// and the cheapest — a pinned region costs 1.5x (broad, e.g. "us") or 1.75x +// (narrow, e.g. "us-east") on the whole compute bill. So this adapter forwards a +// region only when the pool asked for one, and the empty case is not a fallback +// but the preferred path. The vocabulary is Modal's own, and it already spans +// both levels NodePool speaks, so no expansion table is needed (catalog.Base's +// pass-through ExpandRegions serves it). // - Modal Sandboxes carry native tags, so NativeTags=true and the ClaimName // is stored as a tag rather than smuggled into the instance name. // - There is no preemption push; detection is poll-based like every provider. @@ -107,6 +117,24 @@ type SandboxSpec struct { // routes to the first of them (see firstPort) — one token routes to one port. // Empty leaves both the exposed set and the routed port to Modal's own default. Ports []int + // Regions constrains where Modal may place the sandbox, in Modal's own + // vocabulary — a broad region ("us", "eu", "ap") or a narrow one ("us-east", + // "eu-west", "jp"). It comes from ProviderSpec.Regions via ProvisionRequest.Region + // and is forwarded unvalidated, since Modal owns that vocabulary and gains regions + // faster than this adapter ships. + // + // EMPTY IS THE PREFERRED VALUE and the one to reach for unless a workload has a + // real placement requirement. Modal prices a pinned region at a multiplier over + // its unconstrained default — 1.5x for a broad region, 1.75x for a narrow one, + // applied to the whole compute bill (GPU + CPU + memory) — and an unconstrained + // sandbox also draws on Modal's widest capacity pool, which is what makes it both + // cheapest and most available. So this field trades money and availability for + // locality; it is a data-residency knob, not a performance one. + // + // It is a slice because Modal's scheduler accepts several and picks among them, + // but placement resolves one region per candidate (so a capacity failure blocks + // only what failed), so today it carries at most one. + Regions []string // Timeout is the sandbox's maximum lifetime. It MUST be non-zero: Modal treats // a zero timeout as its 5-minute default, which would terminate a real // workload almost immediately. The adapter always sets it (from the Pod's @@ -323,10 +351,29 @@ func (p *Provider) List(ctx context.Context) ([]provider.Instance, error) { // matching shared sentinel (e.g. fmt.Errorf("...: %w", provider.ErrNoCapacity)); // ClassifyError honours those first and falls back to string heuristics for raw // API messages, so no Modal-specific matching is duplicated here. -func (p *Provider) ClassifyProvisionError(err error, accelerator, _ string) provider.BlockScope { - // Region is ignored: Modal is region-simple (no region axis), so the block's - // Region stays nil — see BlockScope's three-state rule. - return provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, accelerator) +// +// It also confines the block to the failing region, the same way the AWS adapter +// does, now that a pool can pin a Modal sandbox to one. Modal's regions are +// independent capacity pools, so "no H100 in us-east" must not disqualify the same +// request in eu-west — without this the first regional shortage would block every +// region the pool lists. The unconstrained case keeps the region-simple behaviour: +// an empty region leaves Region nil, which per BlockScope's three-state rule matches +// only a candidate whose region is also empty, so the block neither widens across an +// axis the request never used nor leaks onto region-pinned candidates. +func (p *Provider) ClassifyProvisionError(err error, accelerator, region string) provider.BlockScope { + // No failure, no block. ClassifyError already returns the zero scope here, but + // the region decoration below would repopulate it into a non-empty scope that + // recordBlock would install — so the guard has to come first, as it does in AWS. + if err == nil { + return provider.BlockScope{} + } + scope := provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, accelerator) + // DenyAll already covers every region (auth fails everywhere), so narrowing it + // would contradict the category. + if region != "" && !scope.DenyAll { + scope.Region = ®ion + } + return scope } // findByClaim returns the sandbox tagged with claimName, or nil if none. @@ -375,12 +422,17 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq } spec := SandboxSpec{ - Image: c.Image, - Command: append(append([]string{}, c.Command...), c.Args...), - Env: env, - CPU: cpuCores(&c), - MemoryMiB: memoryMiB(&c), - Ports: containerPorts(&c), + Image: c.Image, + Command: append(append([]string{}, c.Command...), c.Args...), + Env: env, + CPU: cpuCores(&c), + MemoryMiB: memoryMiB(&c), + Ports: containerPorts(&c), + // An empty request region stays an empty slice, not a one-element [""]: that + // is the unconstrained case (no region declared on the pool), and it must + // reach Modal as "no placement constraint" — its widest pool and its + // un-multiplied price. See SandboxSpec.Regions. + Regions: regionsOf(req.Region), Timeout: sandboxTimeout(pod), Tags: tags, ReadinessProbe: c.ReadinessProbe, @@ -406,6 +458,17 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq return spec, nil } +// regionsOf lifts placement's single chosen region into the slice Modal's API takes. +// An empty region means "unconstrained" and must produce a nil slice rather than +// [""], since a one-element slice holding the empty string would ask Modal to place +// in a region named "" — turning the cheapest, widest case into a malformed request. +func regionsOf(region string) []string { + if region == "" { + return nil + } + return []string{region} +} + // cpuCores reads the container's CPU request as fractional physical cores (Modal's // unit). It prefers requests, falling back to limits, and returns 0 (→ Modal // default) when neither is set. diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 0cce7d2..0554ea4 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -513,6 +513,75 @@ func TestProvision_NoProbeLeavesSpecUnset(t *testing.T) { // client derives it, so the routed port can never name one outside the set). No // declared port is not "no endpoint" — every workload is credentialed — it means Modal // picks, defaulting to 8080. +func TestProvision_CarriesRegion(t *testing.T) { + for _, tc := range []struct { + name string + region string + want []string + }{{ + // The unconstrained case, and the one that must NOT become []string{""}: an + // empty region means "no placement constraint", which is Modal's widest pool + // and its un-multiplied price. A one-element slice holding "" would instead ask + // Modal to place in a region named "". + name: "no region leaves placement unconstrained", + region: "", + want: nil, + }, { + name: "broad region is forwarded", + region: "us", + want: []string{"us"}, + }, { + // Modal owns this vocabulary and gains regions faster than the adapter ships, + // so a value it does not recognize is forwarded rather than rejected here. + name: "narrow region is forwarded verbatim", + region: "us-east", + want: []string{"us-east"}, + }} { + t.Run(tc.name, func(t *testing.T) { + f := &fakeClient{createID: "sb-1"} + p := newTestProvider(f) + req := provider.ProvisionRequest{ClaimName: "claim-a", Region: tc.region} + if _, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), req); err != nil { + t.Fatalf("Provision: %v", err) + } + if !slices.Equal(f.lastSpec.Regions, tc.want) { + t.Fatalf("spec.Regions = %v, want %v", f.lastSpec.Regions, tc.want) + } + }) + } +} + +func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { + p := newTestProvider(&fakeClient{}) + + // A capacity shortage in one region must not disqualify the others: Modal's + // regions are independent pools, so without this the first regional failure would + // block every region the pool lists. + got := p.ClassifyProvisionError(provider.ErrNoCapacity, "H100:1", "us-east") + if got.Region == nil || *got.Region != "us-east" { + t.Fatalf("expected the block confined to us-east, got Region=%v", got.Region) + } + + // Unconstrained: no region axis was used, so Region stays nil — which per + // BlockScope's three-state rule matches only an empty-region candidate, and so + // cannot leak onto region-pinned ones. + if got := p.ClassifyProvisionError(provider.ErrNoCapacity, "H100:1", ""); got.Region != nil { + t.Fatalf("expected nil Region for an unconstrained request, got %q", *got.Region) + } + + // Auth fails in every region, so DenyAll must not be narrowed to one. + if got := p.ClassifyProvisionError(provider.ErrAuth, "H100:1", "us-east"); got.Region != nil { + t.Fatalf("DenyAll must not be confined to a region, got %q", *got.Region) + } + + // No error, no block. recordBlock installs any non-empty scope it is handed, so + // decorating the zero scope with a region would turn "nothing failed" into a live + // block on that region. + if got := p.ClassifyProvisionError(nil, "H100:1", "us-east"); got != (provider.BlockScope{}) { + t.Fatalf("a nil error must classify to the zero scope, got %+v", got) + } +} + func TestProvision_CarriesDeclaredPorts(t *testing.T) { for _, tc := range []struct { name string diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 340e204..ebf6a35 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -3,13 +3,15 @@ // Nebula's provider-agnostic control plane (placement controller, NodeClaim // controller, poll loop) and the heterogeneous cloud APIs underneath. // -// Scope: v1 targets NeoClouds (RunPod, Modal, CoreWeave, Lambda), which are -// region-simple, so region/zone is intentionally NOT modeled yet. Hyperscalers -// (AWS/GCP/Azure) are a planned near-term expansion; when they land, Region/Zone -// become additive fields on the request/Offering/BlockScope structs and the -// optimizer's candidate key widens to include them — the method signatures here -// are designed not to change. Do not hard-code NeoCloud-only assumptions into -// the control plane; keep provider quirks behind Capabilities. +// Scope: region IS modeled, as one axis of the placement candidate key and of +// BlockScope, but it stays OPTIONAL — an empty region means "no constraint, let +// the provider place freely", which is a normal mode (and on Modal the cheapest +// one), not a degenerate fallback. Zone is not modeled: AWS's CreateFleet already +// spreads across the AZs of a region on its own, and no NeoCloud exposes zones. +// Providers differ in how coarse their region vocabulary is, and the pool speaks +// group tokens ("us") on top of that, so translation lives behind ExpandRegions +// rather than in the control plane. Do not hard-code any one provider's geography +// into the control plane; keep provider quirks behind Capabilities/ExpandRegions. // // Design rules learned from SkyPilot / the Nebula design discussion: // - The Pod is the source of truth for the workload shape. Provision takes the @@ -131,6 +133,32 @@ type Provider interface { // returns a single-element slice. MapAccelerator(canonical string, count int32) (providerAcceleratorIDs []string, ok bool) + // ExpandRegions resolves a NodePool's declared region constraint + // (ProviderSpec.Regions) into the concrete regions placement may walk, in this + // provider's own vocabulary. The pool speaks two levels and this is what tells + // them apart, because only the provider knows its own geography: + // + // - nil/empty => unconstrained: every region this provider serves. + // - a GROUP token ("us", "eu", "ap") => that geography's regions. + // - anything else => a literal region name, passed through UNVALIDATED. + // + // The last case is deliberate: region names are the provider's vocabulary and + // change faster than this code, so an unrecognized value is forwarded rather than + // rejected. A genuinely bad name fails at provision time with the provider's own + // error, which beats Nebula refusing a region that shipped last week. + // + // Expansion happens HERE, at the pool boundary, so everything downstream keeps + // seeing exactly one concrete region: ProvisionRequest.Region, RegionAnnotation, + // and the failover blocklist key all stay single-valued, and a capacity failure + // blocks the one region that failed rather than the whole group it came from. + // + // It is a pure function of the declaration (no API calls, no ctx): the result + // feeds both placement's candidate walk and the observability fan-out + // (List/Offerings), and those two MUST agree — a region provisioned into but not + // swept would be absent from List, which reports a live instance as Terminated. + // So it must not be able to fail differently between the two callers. + ExpandRegions(declared []string) []string + // ClassifyProvisionError maps a Provision error to the granularity at which // the failing placement should be blocklisted. This keeps failover precise: // a "no H100 capacity" error blocks only {provider, H100, capacityType, region}, @@ -162,12 +190,19 @@ type ProvisionRequest struct { // This is the one workload-independent decision that cannot be expressed on // the Pod, so it must be passed explicitly. CapacityType nebulav1alpha1.CapacityType - // Region is the provider region the optimizer chose to provision in, in the - // provider's own vocabulary (e.g. AWS "us-east-1"). Like CapacityType it is a - // workload-independent decision absent from the Pod. Empty means "use the - // provider's configured default region" — region-simple NeoClouds (Modal, - // RunPod) ignore it, and a region-aware adapter falls back to the region its - // client was built with (see the AWS adapter's NewSDKClient). + // Region is the ONE concrete region placement chose for this attempt, in the + // provider's own vocabulary (AWS "us-east-1", Modal "us-east"). Like CapacityType + // it is a workload-independent decision absent from the Pod. It is always a single + // resolved region, never a group token or a list: the pool's constraint was already + // expanded by ExpandRegions and walked one candidate at a time, which is what lets + // a capacity failure blocklist exactly the region that failed. + // + // Empty means "no region constraint — let the provider place freely". That is a + // real, common mode, not a fallback: a pool that declares no regions leaves it + // empty, and on Modal that is the widest and cheapest option (a pinned region + // carries a 1.5-1.75x multiplier there). The AWS adapter cannot honour it — EC2 + // endpoints are regional, so it has no "anywhere" call and returns ErrConfig — but + // its ExpandRegions never produces an empty region, so the case does not arise. Region string } diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index 1c6d227..ba54ce2 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -122,6 +122,7 @@ func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { } func (f *fakeProvider) Offerings(context.Context) ([]provider.Offering, error) { return nil, nil } func (f *fakeProvider) MapAccelerator(c string, _ int32) ([]string, bool) { return []string{c}, true } +func (f *fakeProvider) ExpandRegions(declared []string) []string { return declared } func (f *fakeProvider) ClassifyProvisionError(_ error, accel, region string) provider.BlockScope { f.classifyAccel = accel f.classifyRegion = region From 441cc03d4f5119295c7253b75310e014225ee648 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 14 Aug 2026 19:11:07 +0100 Subject: [PATCH 2/3] update doc Signed-off-by: kerthcet --- api/v1alpha1/nodeclaim_types.go | 13 ++- .../bases/nebula.inftyai.com_nodeclaims.yaml | 13 ++- config/manager/kustomization.yaml | 2 +- config/samples/nodepool.yaml | 15 +-- docs/add-a-provider.md | 4 +- docs/architecture.md | 21 +++- pkg/provider/catalog/base.go | 26 ++--- pkg/provider/modal/modal.go | 97 ++++++++++++++++--- pkg/provider/modal/modal_test.go | 94 ++++++++++++++++++ pkg/provider/provider.go | 32 ++++-- 10 files changed, 267 insertions(+), 50 deletions(-) diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index 131fe8f..c7b1cde 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -31,13 +31,20 @@ type NodeClaimSpec struct { // +optional CapacityType CapacityType `json:"capacityType,omitempty"` - // Region is the provider region the placement optimizer selected, in the + // Region is the region candidate the placement optimizer selected, in the // provider's own vocabulary (e.g. AWS "us-east-1"). Stored durably alongside // Provider/CapacityType because it is a provisioning input that cannot be read // off the Pod, and Provision needs it to re-issue the request in the same // region after a controller restart. Immutable, like Provider. Empty means - // "the provider's configured default region" — region-simple providers (Modal, - // RunPod) leave it empty. + // "the provider's configured default region" — a provider with no region + // constraint declared on the pool leaves it empty. + // + // It is not always a single region NAME: a provider whose create cannot fail + // over collapses every region the pool declared into ONE candidate, and stores + // them joined by a provider-private separator (Modal uses "|", so a pool + // declaring us-east and us-west records "us-east|us-west"). Only that provider + // can split the value back, which it does at the API boundary. Treat the field + // as an opaque provider-scoped token rather than parsing it. // +optional Region string `json:"region,omitempty"` diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index 9454426..de62ca2 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -128,13 +128,20 @@ spec: type: string region: description: |- - Region is the provider region the placement optimizer selected, in the + Region is the region candidate the placement optimizer selected, in the provider's own vocabulary (e.g. AWS "us-east-1"). Stored durably alongside Provider/CapacityType because it is a provisioning input that cannot be read off the Pod, and Provision needs it to re-issue the request in the same region after a controller restart. Immutable, like Provider. Empty means - "the provider's configured default region" — region-simple providers (Modal, - RunPod) leave it empty. + "the provider's configured default region" — a provider with no region + constraint declared on the pool leaves it empty. + + It is not always a single region NAME: a provider whose create cannot fail + over collapses every region the pool declared into ONE candidate, and stores + them joined by a provider-private separator (Modal uses "|", so a pool + declaring us-east and us-west records "us-east|us-west"). Only that provider + can split the value back, which it does at the API boundary. Treat the field + as an opaque provider-scoped token rather than parsing it. type: string required: - podRef diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 086bc1e..55fef88 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -5,4 +5,4 @@ kind: Kustomization images: - name: controller newName: inftyai/nebula-controller - newTag: latest + newTag: 0814-01 diff --git a/config/samples/nodepool.yaml b/config/samples/nodepool.yaml index 8295ac8..a6dcc25 100644 --- a/config/samples/nodepool.yaml +++ b/config/samples/nodepool.yaml @@ -9,15 +9,16 @@ spec: providers: - name: aws regions: - - us-east-1 - - us-west-1 - - ap-south-1 - - ap-northeast-1 - - eu-central-1 - - eu-west-1 + - us + - eu + - ap - ca-central-1 - sa-east-1 - # - name: modal + - name: modal + regions: + - us + - eu + - ap-melbourne # - name: runpod # Outer axis: try OnDemand on every provider first, fall back to Spot. capacityTypes: diff --git a/docs/add-a-provider.md b/docs/add-a-provider.md index b56b8d3..6552211 100644 --- a/docs/add-a-provider.md +++ b/docs/add-a-provider.md @@ -25,8 +25,8 @@ Create `pkg/provider//` and implement `provider.Provider` | `List(ctx)` | Every Nebula-owned instance, in as few API calls as possible. This drives the poll loop — preemption is detected by an instance disappearing here. | | `Offerings(ctx)` | Price/availability rows for the optimizer (see the catalog below). | | `MapAccelerator(canonical, count)` | Translate a canonical accelerator (type + count) to the provider's own id; `ok=false` if unsupported. | -| `ClassifyProvisionError(err, accel, region)` | Map a Provision failure to the `BlockScope` failover should blocklist (a capacity error → that {accel, tier, region}; an auth/quota error → the whole provider). | -| `ExpandRegions(declared)` | Turn a pool's `regions` into the concrete regions to try. `catalog.Base` passes them through unchanged, which is right whenever the provider's own region names already include the group tokens (`us`, `eu`, `ap`) a pool may write — Modal's do. Override only if they don't, as `pkg/provider/aws` does with a static table. | +| `ClassifyProvisionError(err, accel, region)` | Map a Provision failure to the `BlockScope` failover should blocklist. Only an **auth** error widens to the whole provider (`DenyAll`); capacity, quota, and unrecognized errors are all scoped to that {accel, tier, region} so failover can route around one candidate instead of fencing off the provider. Delegate to `provider.ClassifyError` for the shared part and decorate only what is provider-specific (e.g. the region axis). | +| `ExpandRegions(declared)` | Turn a pool's `regions` into the region candidates placement will walk. `catalog.Base` passes them through unchanged — one candidate per declared region, token used verbatim. Override for **either** of two independent reasons: the tokens are not callable (`pkg/provider/aws` expands the group `us` into every US EC2 region via a static table, since `us` is not a region you can call), or the provider's create **cannot fail over**, in which case splitting shrinks the capacity pool instead of widening it (`pkg/provider/modal` collapses every declared region into ONE candidate). Note Modal's own names already include the group tokens, so it overrides for the *second* reason alone — the two axes are orthogonal. | The Pod is the single source of truth for the workload shape; `ProvisionRequest` carries only what the Pod cannot express (the optimizer's capacity tier and the diff --git a/docs/architecture.md b/docs/architecture.md index 1e6ea22..6eb08d5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -59,6 +59,17 @@ mapping), see [docs/status.md](status.md). declaration cannot yet prefer the cheapest region. Modal is the sharper case — a pinned region there costs 1.5x (group) or 1.75x (narrow) over its unconstrained default, which the catalog does not model. +- Failover on a provider that *queues*. The blocklist has exactly one writer: the + virtual kubelet's `Provision` error path. A provider that reports a capacity + shortfall synchronously (AWS `CreateFleet`) therefore fails over across zone, + region and tier, but one that ACCEPTS the request and queues for capacity returns + an instance id and no error — so nothing is blocklisted and placement is never + re-driven. Modal is that case: a `[modal, aws]` pool never advances to AWS on + capacity, and the Pod waits in Modal's queue at `Initializing` (which, per + [status](status.md#modal), is indistinguishable from booting). Modal also collapses + its regions into a single candidate, so it has no intra-provider region failover + either. Making failover live for such a provider needs a reserved-by deadline that + synthesizes `ErrNoCapacity` — the classification side already handles it. - Bin-packing multiple unrelated Pods onto one external instance. The current model is one workload Pod to one external instance. - In-place migration. Recovery from reclaim, failure, or spec changes is @@ -148,13 +159,21 @@ Follow one GPU Pod from creation to teardown: ```text for each capacityType in pool.spec.capacityTypes: # outer axis for each provider in pool.spec.providers: # listed order today - for each region in provider.regions or [""]: # provider-local axis + for each region in ExpandRegions(provider.regions): # provider-local axis skip unregistered providers skip providers that do not offer the accelerator type/count + skip providers that cannot serve the tier (Modal has no Spot) skip candidates blocked by failover blocklist choose the first remaining candidate ``` + The inner axis is whatever the provider's `ExpandRegions` returns, which is not + one iteration per declared region: AWS expands a group token into many candidates, + while Modal collapses every declared region into a single candidate carrying them + all (so its inner loop always runs exactly once, and the chosen `region` may be a + 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 diff --git a/pkg/provider/catalog/base.go b/pkg/provider/catalog/base.go index 24c1ea7..b5188d8 100644 --- a/pkg/provider/catalog/base.go +++ b/pkg/provider/catalog/base.go @@ -76,21 +76,23 @@ func (b Base) Offerings(context.Context) ([]provider.Offering, error) { return b.Catalog.Offerings(b.ProviderName), nil } -// ExpandRegions passes the pool's declared regions through unchanged. This is the -// right default for a provider whose OWN vocabulary already spans both levels the -// pool speaks: Modal accepts "us" and "eu" as first-class placement values (its -// broad regions) alongside narrower ones, so there is nothing for Nebula to expand — -// the token IS the region name, and forwarding it verbatim is both correct and -// future-proof as the provider adds regions. +// ExpandRegions passes the pool's declared regions through unchanged: one candidate +// per declared region, and the declaration's tokens used verbatim as region names. +// This is the right default for a provider whose OWN vocabulary already spans both +// levels the pool speaks (so there is nothing to expand — the token IS the region +// name, which stays future-proof as the provider adds regions) AND whose provision +// call reports a capacity failure synchronously, so walking candidates one at a time +// actually buys a retry in the next region. // // nil stays nil, which every adapter must read as "unconstrained": send no region -// and let the provider place freely. On Modal that is also the cheapest option — a -// pinned region carries a 1.5x (broad) or 1.75x (narrow) price multiplier, so -// constraining placement there is a deliberate cost, not a free preference. +// and let the provider place freely. // -// A region-AWARE provider whose region names do not contain the group tokens (AWS: -// "us" is not a prefix of an EC2 region name you can call) must override this. See -// the AWS adapter. +// Both halves have real overriders, in opposite directions. AWS expands a group +// token into many candidates, because "us" is not an EC2 region name you can call. +// Modal collapses the whole declaration into ONE candidate holding every region, +// because its create cannot fail over — splitting would strand the workload in +// whichever region was walked first. Check which of those a new provider resembles +// before inheriting this. func (b Base) ExpandRegions(declared []string) []string { return declared } // MapAccelerator translates a canonical accelerator request (type + count) into diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 699495d..7ff8012 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -32,9 +32,18 @@ limitations under the License. // and the cheapest — a pinned region costs 1.5x (broad, e.g. "us") or 1.75x // (narrow, e.g. "us-east") on the whole compute bill. So this adapter forwards a // region only when the pool asked for one, and the empty case is not a fallback -// but the preferred path. The vocabulary is Modal's own, and it already spans -// both levels NodePool speaks, so no expansion table is needed (catalog.Base's -// pass-through ExpandRegions serves it). +// but the preferred path. The vocabulary is Modal's own and needs no translation +// table, since it already spans both levels a NodePool speaks. +// - A pool's regions are sent in ONE call, not walked one at a time. This follows +// from create being non-failing (above): an accepted-but-queued sandbox reports no +// capacity error, so nothing ever re-drives placement to a second region, and +// splitting the declaration into candidates would strand the workload in whichever +// region happened to be walked first while discarding the others. Modal's +// scheduler is the right chooser here — it accepts several regions and has the +// live capacity view to pick among them — so ExpandRegions collapses the whole +// declaration into one opaque candidate and regionsOf splits it back at the API +// boundary. The trade is blocklist precision, which costs nothing because no +// Modal failure is region-attributable anyway. // - Modal Sandboxes carry native tags, so NativeTags=true and the ClaimName // is stored as a tag rather than smuggled into the instance name. // - There is no preemption push; detection is poll-based like every provider. @@ -131,9 +140,12 @@ type SandboxSpec struct { // cheapest and most available. So this field trades money and availability for // locality; it is a data-residency knob, not a performance one. // - // It is a slice because Modal's scheduler accepts several and picks among them, - // but placement resolves one region per candidate (so a capacity failure blocks - // only what failed), so today it carries at most one. + // It carries EVERY region the pool declared, not one per attempt. Modal's + // scheduler accepts several and picks among them with its own live view of + // capacity, and it must, because a Modal create cannot fail over: it returns an + // accepted id with no capacity error, so nothing here could try a second region + // afterwards. See ExpandRegions and regionsOf for the join/split that carries + // the set through placement's single-region candidate. Regions []string // Timeout is the sandbox's maximum lifetime. It MUST be non-zero: Modal treats // a zero timeout as its 5-minute default, which would terminate a real @@ -229,6 +241,51 @@ func New(client Client, cat catalog.Lookup) *Provider { } } +// regionSeparator joins several Modal regions into the ONE candidate placement +// walks. It is deliberately a character no region name contains, so splitting is +// unambiguous, and deliberately not a comma: the value lands in RegionAnnotation and +// a comma reads like a list a consumer might re-split with different rules. +const regionSeparator = "|" + +// ExpandRegions implements provider.Provider, overriding catalog.Base's +// pass-through. It resolves the pool's whole declaration to at most ONE candidate, +// carrying every declared region in it, rather than one candidate per region. +// +// This is the opposite of AWS, and the reason is that Modal cannot fail over. An +// AWS CreateFleet reports a capacity shortage synchronously, so walking regions one +// at a time is what lets the next region be tried. Sandboxes.Create instead ACCEPTS +// the sandbox immediately and returns a real id with Reserved=false — the GPU may +// still be queued for minutes. There is no error, so ClassifyProvisionError never +// runs, nothing is blocklisted, and placement is never re-driven: the first region +// walked is the only region ever tried. Splitting the declaration into candidates +// would therefore SHRINK the capacity pool to one region and discard the rest. +// +// Handing Modal the full set instead moves the choice to the party that can act on +// it: Modal's scheduler accepts several regions (SchedulerPlacement.Regions) and +// picks among them itself, with its own live view of capacity. So a pool declaring +// several regions gets all of them considered, which is what the operator asked for. +// +// The cost is precision, and it is worth naming: the resulting candidate's region is +// a joined token, so a blocklist entry covers the whole declared set rather than one +// region. That loses nothing today — no Modal failure is region-attributable in the +// first place, since a queued sandbox never reports which region ran dry. +func (p *Provider) ExpandRegions(declared []string) []string { + seen := make(map[string]bool) + regions := make([]string, 0, len(declared)) + for _, d := range declared { + d = strings.TrimSpace(d) + if d == "" || seen[d] { + continue + } + seen[d] = true + regions = append(regions, d) + } + if len(regions) == 0 { + return nil // unconstrained: the widest and cheapest case + } + return []string{strings.Join(regions, regionSeparator)} +} + // Capabilities implements provider.Provider. See the package doc for why each // trait is set the way it is. func (p *Provider) Capabilities() provider.Capabilities { @@ -352,11 +409,14 @@ func (p *Provider) List(ctx context.Context) ([]provider.Instance, error) { // ClassifyError honours those first and falls back to string heuristics for raw // API messages, so no Modal-specific matching is duplicated here. // -// It also confines the block to the failing region, the same way the AWS adapter -// does, now that a pool can pin a Modal sandbox to one. Modal's regions are -// independent capacity pools, so "no H100 in us-east" must not disqualify the same -// request in eu-west — without this the first regional shortage would block every -// region the pool lists. The unconstrained case keeps the region-simple behaviour: +// It also confines the block to the failing CANDIDATE, the same way the AWS adapter +// does, now that a pool can pin a Modal sandbox. Note this is the candidate, not +// necessarily a single region: ExpandRegions hands Modal every declared region at +// once, so the token here may name the whole set and the block then covers all of it. +// That is the honest scope — a failure on a multi-region create tells us nothing +// about which region was short, and there is no narrower attempt left to make. A pool +// that wants per-region blocking has to declare per-region pools. The unconstrained +// case keeps the region-simple behaviour: // an empty region leaves Region nil, which per BlockScope's three-state rule matches // only a candidate whose region is also empty, so the block neither widens across an // axis the request never used nor leaks onto region-pinned candidates. @@ -458,7 +518,12 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq return spec, nil } -// regionsOf lifts placement's single chosen region into the slice Modal's API takes. +// regionsOf turns placement's single region candidate back into the slice Modal's +// API takes. It is the exact inverse of ExpandRegions' join: that collapses the +// pool's whole declaration into ONE candidate (see there for why Modal cannot fail +// over region by region), and this expands it again at the call boundary, so the +// set the operator declared is what Modal's scheduler gets to choose among. +// // An empty region means "unconstrained" and must produce a nil slice rather than // [""], since a one-element slice holding the empty string would ask Modal to place // in a region named "" — turning the cheapest, widest case into a malformed request. @@ -466,7 +531,13 @@ func regionsOf(region string) []string { if region == "" { return nil } - return []string{region} + var out []string + for _, r := range strings.Split(region, regionSeparator) { + if r = strings.TrimSpace(r); r != "" { + out = append(out, r) + } + } + return out } // cpuCores reads the container's CPU request as fractional physical cores (Modal's diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 0554ea4..1d59293 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -536,6 +536,14 @@ func TestProvision_CarriesRegion(t *testing.T) { name: "narrow region is forwarded verbatim", region: "us-east", want: []string{"us-east"}, + }, { + // The whole point of the join: a pool declaring several regions must hand Modal + // ALL of them in the one create call, because that call cannot fail over — it + // returns an accepted id with no capacity error, so no second region would ever + // be attempted. Modal's scheduler picks among these itself. + name: "a multi-region candidate is split back into the full set", + region: "us-east" + regionSeparator + "us-west" + regionSeparator + "eu-west", + want: []string{"us-east", "us-west", "eu-west"}, }} { t.Run(tc.name, func(t *testing.T) { f := &fakeClient{createID: "sb-1"} @@ -551,6 +559,92 @@ func TestProvision_CarriesRegion(t *testing.T) { } } +// TestExpandRegions_CollapsesToOneCandidate pins the axis decision that matters most +// for Modal: a pool's whole region declaration becomes exactly ONE placement +// candidate. Modal's create accepts a sandbox and queues it without a capacity error, +// so nothing re-drives placement afterwards — one candidate per region would mean the +// first region walked is the only one ever tried, silently discarding the rest of the +// operator's declaration. Collapsing hands the full set to Modal's own scheduler. +func TestExpandRegions_CollapsesToOneCandidate(t *testing.T) { + p := newTestProvider(&fakeClient{}) + + for _, tc := range []struct { + name string + declared []string + want []string + }{{ + name: "no declaration stays unconstrained", + declared: nil, + want: nil, + }, { + // Not []string{""}: an empty candidate and no candidate must not be confused, + // and regionsFor supplies the one candidate the walk needs. + name: "a declaration of only blanks is unconstrained, not an empty region", + declared: []string{"", " "}, + want: nil, + }, { + name: "a single region is one candidate holding it", + declared: []string{"us"}, + want: []string{"us"}, + }, { + name: "several regions are ONE candidate, not several", + declared: []string{"us-east", "eu-west"}, + want: []string{"us-east" + regionSeparator + "eu-west"}, + }, { + name: "duplicates collapse and order is the operator's", + declared: []string{"eu-west", "us-east", "eu-west"}, + want: []string{"eu-west" + regionSeparator + "us-east"}, + }} { + t.Run(tc.name, func(t *testing.T) { + got := p.ExpandRegions(tc.declared) + if !slices.Equal(got, tc.want) { + t.Fatalf("ExpandRegions(%v) = %v, want %v", tc.declared, got, tc.want) + } + if len(got) > 1 { + t.Fatalf("ExpandRegions(%v) produced %d candidates; Modal cannot fail "+ + "over, so every extra candidate is a region silently never tried", tc.declared, len(got)) + } + }) + } +} + +// TestExpandRegions_RoundTripsThroughProvision is the invariant that makes the +// collapse safe: whatever ExpandRegions joins, regionsOf must split back to the exact +// declared set by the time it reaches Modal's API. The two are inverses, and this +// asserts it end to end through Provision rather than on the helpers alone — a +// mismatch here would send Modal a region name it has never heard of (the joined +// token), which is precisely the failure a unit test on either half would miss. +func TestExpandRegions_RoundTripsThroughProvision(t *testing.T) { + for _, declared := range [][]string{ + nil, + {"us"}, + {"us-east", "us-west"}, + {"us", "eu", "ap", "jp"}, + } { + f := &fakeClient{createID: "sb-1"} + p := newTestProvider(f) + + candidates := p.ExpandRegions(declared) + // Placement's own fallback when expansion is empty: one unconstrained candidate. + if len(candidates) == 0 { + candidates = []string{""} + } + req := provider.ProvisionRequest{ClaimName: "claim-a", Region: candidates[0]} + if _, err := p.Provision(context.Background(), gpuPod("claim-a", "H100", 1), req); err != nil { + t.Fatalf("Provision: %v", err) + } + + // A nil declaration must stay nil all the way down, not become [""]. + var want []string + if len(declared) > 0 { + want = declared + } + if !slices.Equal(f.lastSpec.Regions, want) { + t.Fatalf("declared %v reached Modal as %v, want %v", declared, f.lastSpec.Regions, want) + } + } +} + func TestClassifyProvisionError_ConfinesToFailingRegion(t *testing.T) { p := newTestProvider(&fakeClient{}) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index ebf6a35..7adc8e5 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -148,9 +148,20 @@ type Provider interface { // error, which beats Nebula refusing a region that shipped last week. // // Expansion happens HERE, at the pool boundary, so everything downstream keeps - // seeing exactly one concrete region: ProvisionRequest.Region, RegionAnnotation, + // seeing exactly one CANDIDATE region: ProvisionRequest.Region, RegionAnnotation, // and the failover blocklist key all stay single-valued, and a capacity failure - // blocks the one region that failed rather than the whole group it came from. + // blocks the one candidate that failed rather than the whole group it came from. + // + // How many candidates a declaration becomes is the provider's call, and it turns + // on whether that provider can FAIL OVER between regions. A provider whose + // provision reports a capacity shortage synchronously (AWS) should return one + // candidate per region, so a shortage in one is retried in the next. A provider + // that accepts a request and queues it with no error (Modal) must NOT: nothing + // would ever re-drive placement, so the first candidate is the only one tried, and + // splitting the declaration would discard every other region the operator asked + // for. Such a provider returns ONE candidate carrying the whole set and lets its + // own scheduler choose — the candidate is then opaque to the control plane, which + // only ever passes it back to the provider that minted it. // // It is a pure function of the declaration (no API calls, no ctx): the result // feeds both placement's candidate walk and the observability fan-out @@ -190,12 +201,17 @@ type ProvisionRequest struct { // This is the one workload-independent decision that cannot be expressed on // the Pod, so it must be passed explicitly. CapacityType nebulav1alpha1.CapacityType - // Region is the ONE concrete region placement chose for this attempt, in the - // provider's own vocabulary (AWS "us-east-1", Modal "us-east"). Like CapacityType - // it is a workload-independent decision absent from the Pod. It is always a single - // resolved region, never a group token or a list: the pool's constraint was already - // expanded by ExpandRegions and walked one candidate at a time, which is what lets - // a capacity failure blocklist exactly the region that failed. + // Region is the ONE candidate placement chose for this attempt, as its own + // ExpandRegions minted it. Like CapacityType it is a workload-independent decision + // absent from the Pod. It is never a raw pool declaration or a group token — that + // was already resolved — and the control plane treats it as an OPAQUE token, + // passing back exactly what the provider produced. + // + // For a provider that fails over region by region this is one concrete region + // (AWS "us-east-1"), which is what lets a capacity failure blocklist exactly the + // region that failed. For one that cannot (Modal), it may encode the several + // regions its scheduler should choose among; only that provider's own code parses + // it. Nothing between here and the adapter inspects the value. // // Empty means "no region constraint — let the provider place freely". That is a // real, common mode, not a fallback: a pool that declares no regions leaves it From 61fba6d1a53f24ee756f9922e7a66811543217aa Mon Sep 17 00:00:00 2001 From: kerthcet Date: Fri, 14 Aug 2026 19:13:16 +0100 Subject: [PATCH 3/3] update the tag Signed-off-by: kerthcet --- config/manager/kustomization.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 55fef88..086bc1e 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -5,4 +5,4 @@ kind: Kustomization images: - name: controller newName: inftyai/nebula-controller - newTag: 0814-01 + newTag: latest