From 04000a8999b3d6f077266685d0592c2d1a652cc4 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 23 Sep 2026 13:48:05 +0100 Subject: [PATCH 1/4] support specifying regions via pod annotation Signed-off-by: kerthcet --- README.md | 13 +- api/v1alpha1/groupversion_info.go | 5 + api/v1alpha1/nodepool_types.go | 24 +-- .../bases/nebula.inftyai.com_nodepools.yaml | 24 +-- docs/add-a-provider.md | 2 +- docs/architecture.md | 28 ++- docs/metrics.md | 14 +- .../controller/nodeclaim_controller_test.go | 18 +- internal/controller/placement_metrics_test.go | 23 +-- .../pod_placement_controller_test.go | 129 +++++++++++- internal/controller/pod_placement_helpers.go | 54 +++++- pkg/metrics/placement.go | 1 + pkg/provider/aws/aws.go | 116 ++++++----- pkg/provider/aws/aws_test.go | 183 +++++++++++++++++- pkg/provider/catalog/base.go | 16 +- pkg/provider/fake/fake.go | 76 ++++++++ pkg/provider/fake/fake_test.go | 64 ++++++ pkg/provider/modal/modal.go | 92 ++++++--- pkg/provider/modal/modal_test.go | 131 ++++++++++++- pkg/provider/provider.go | 23 +-- pkg/provider/regions.go | 28 +++ pkg/provider/regions_test.go | 66 +++++++ pkg/vnode/handler_test.go | 2 +- test/e2e/e2e_test.go | 121 ++++++++++++ 24 files changed, 1037 insertions(+), 216 deletions(-) create mode 100644 pkg/provider/regions.go create mode 100644 pkg/provider/regions_test.go diff --git a/README.md b/README.md index 080c659..c8d2181 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,8 @@ metadata: nebula.inftyai.com/enabled: "true" # opt in nebula.inftyai.com/nodepool: gpu # which NodePool to place against nebula.inftyai.com/accelerator-type: h100 # GPU type (case-insensitive) + annotations: + nebula.inftyai.com/regions: eu,uk # optional: narrow within the pool spec: containers: - name: workload @@ -99,12 +101,6 @@ spec: nvidia.com/gpu: "8" # GPU count ``` -The accelerator **type** rides on the label and is matched case-insensitively -against the provider catalog (`pkg/provider/catalog/data`); the **count** rides on -the standard `nvidia.com/gpu` resource limit, so scheduling and provisioning read -the same number. Do not set `nodeName` or a provider `nodeSelector` yourself — the -placement controller owns those. - ## Quick start ```bash @@ -126,10 +122,7 @@ One virtual node appears per provider whose credentials are present: kubectl get nodes -l nebula.inftyai.com/provider ``` -Then define a [NodePool](#defining-a-nodepool) and [opt a workload in](#opting-a-workload-in). - -To build and deploy from source instead, see [docs/deploy.md](docs/deploy.md). The -[docs](docs/README.md) cover the rest. +See [docs](docs/README.md) for more detailed instructions. ## License diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index 684c739..7d45a17 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -99,6 +99,11 @@ const ( // via util.AcceleratorRequest. AcceleratorTypeLabel = "nebula.inftyai.com/accelerator-type" + // RegionsAnnotation narrows ONE workload to a comma-separated list of broad + // geographies ("eu", or "eu,uk"), which is how a Pod expresses data residency without + // an operator carving out a NodePool per jurisdiction. Case and spacing are free. + RegionsAnnotation = "nebula.inftyai.com/regions" + // EndpointAnnotation carries the reachable address of the external instance (a DNS // name, an IP, or a URL, in the provider's own form). It is the only way to reach // the workload, and PodIP cannot hold it — the API server validates PodIP as a diff --git a/api/v1alpha1/nodepool_types.go b/api/v1alpha1/nodepool_types.go index cf64934..eedfd82 100644 --- a/api/v1alpha1/nodepool_types.go +++ b/api/v1alpha1/nodepool_types.go @@ -147,27 +147,15 @@ type ProviderSpec struct { // +optional Weight *int32 `json:"weight,omitempty"` - // Regions CONSTRAINS where this provider may place, in the provider's own - // vocabulary. It lives here per provider because region names are - // provider-namespaced. Three levels: + // Regions CONSTRAINS where this provider may place. Three levels: // - omitted/empty => every region the provider serves. For a region-simple // provider (Modal) this sends no region at all, its widest and cheapest mode. - // - a geography GROUP token ("us", "eu", "ap", ...) => that geography's regions. - // The recommended way to ask for breadth with a residency boundary. + // - a GEOGRAPHY ("us", "eu", "ap", ...) => that geography's regions here. The + // recommended way to ask for breadth with a residency boundary, and the only + // level a workload can also request per-Pod. // - a literal region name ("us-east-1" on AWS, "us-east" on Modal) => just that. - // Only the provider knows its own geography, so it resolves which level a value is - // (see provider.Provider's ExpandRegions). Group tokens are shared across - // providers; the regions behind them are not. - // - // A non-group value is passed through UNVALIDATED, because region names change - // faster than Nebula ships: a bad one fails at provision time with the provider's - // own error, which beats refusing a region that launched last week. It is also the - // escape hatch for AWS opt-in regions, which no group contains. - // - // Unconstrained is the widest and costliest setting: every region becomes a - // failover candidate and gets swept by the poll loop. Prefer a group unless the - // workload needs global reach. Entry count is uncapped (a group already expands to - // many); maxLength bounds each entry. + // Geographies are shared across providers; the regions behind them are not, so only + // the provider resolves which level a value is (provider.Provider's ExpandRegions). // +optional // +kubebuilder:validation:items:MaxLength=32 Regions []string `json:"regions,omitempty"` diff --git a/config/crd/bases/nebula.inftyai.com_nodepools.yaml b/config/crd/bases/nebula.inftyai.com_nodepools.yaml index 9fb4152..6a82b8e 100644 --- a/config/crd/bases/nebula.inftyai.com_nodepools.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodepools.yaml @@ -169,27 +169,15 @@ spec: type: string regions: description: |- - Regions CONSTRAINS where this provider may place, in the provider's own - vocabulary. It lives here per provider because region names are - provider-namespaced. Three levels: + Regions CONSTRAINS where this provider may place. Three levels: - omitted/empty => every region the provider serves. For a region-simple provider (Modal) this sends no region at all, its widest and cheapest mode. - - a geography GROUP token ("us", "eu", "ap", ...) => that geography's regions. - The recommended way to ask for breadth with a residency boundary. + - a GEOGRAPHY ("us", "eu", "ap", ...) => that geography's regions here. The + recommended way to ask for breadth with a residency boundary, and the only + level a workload can also request per-Pod. - a literal region name ("us-east-1" on AWS, "us-east" on Modal) => just that. - Only the provider knows its own geography, so it resolves which level a value is - (see provider.Provider's ExpandRegions). Group tokens are shared across - providers; the regions behind them are not. - - A non-group value is passed through UNVALIDATED, because region names change - faster than Nebula ships: a bad one fails at provision time with the provider's - own error, which beats refusing a region that launched last week. It is also the - escape hatch for AWS opt-in regions, which no group contains. - - Unconstrained is the widest and costliest setting: every region becomes a - failover candidate and gets swept by the poll loop. Prefer a group unless the - workload needs global reach. Entry count is uncapped (a group already expands to - many); maxLength bounds each entry. + Geographies are shared across providers; the regions behind them are not, so only + the provider resolves which level a value is (provider.Provider's ExpandRegions). items: maxLength: 32 type: string diff --git a/docs/add-a-provider.md b/docs/add-a-provider.md index 58f33d8..851db4b 100644 --- a/docs/add-a-provider.md +++ b/docs/add-a-provider.md @@ -26,7 +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. 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. | +| `ExpandRegions(declared, narrowTo)` | Turn a pool's `regions` into the region candidates placement will walk, optionally narrowed to the geographies one workload asked for. | 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 8cabea9..1678f75 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -50,11 +50,15 @@ mapping), see [docs/status.md](status.md). **Non-goals in the current implementation** -- 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. +- Uniform geographic coverage. `provider.Geographies` is a flat, provider-neutral + vocabulary of broad tokens (`us`, `eu`, `ap`, `uk`, `ca`, `me`, `sa`, `af`, `mx`) + and that is the whole shared namespace — a provider's own region names are the + second level and are never vocabulary. +- Regions a provider's geography table does not list. That table is the only authority + on which geography holds which region, so such a region is reachable only by naming + it literally in the NodePool, never through a Pod's `regions` annotation. AWS's + opt-in regions are left out on purpose: EC2 answers `OptInRequired`, which classifies + as an auth failure and would blocklist the whole provider. - 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 @@ -147,6 +151,11 @@ Follow one GPU Pod from creation to teardown: `nvidia.com/gpu` resource. The Pod remains the source of truth for image, command, env, ports, CPU, memory, accelerator type, and accelerator count. + A workload that cares where it runs adds `nebula.inftyai.com/regions`, a + comma-separated list of `provider.Geographies` tokens. It only ever narrows + what the pool already allows — a Pod cannot reach a geography its NodePool + left out. + 2. **Gate at admission.** The mutating webhook adds the scheduling gate `nebula.inftyai.com/provider-selection` and a key-only `Exists` toleration for the virtual-node taint `nebula.inftyai.com/provider:NoSchedule`. The webhook @@ -160,10 +169,11 @@ 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 ExpandRegions(provider.regions): # provider-local axis + for each region in ExpandRegions(provider.regions, podGeographies): 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 providers with no region in the requested geographies skip candidates blocked by failover blocklist choose the first remaining candidate ``` @@ -173,7 +183,9 @@ Follow one GPU Pod from creation to teardown: 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. + unconstrained `""` candidate so the walk runs — but only when the Pod requested + no geography. Under a narrowing request an empty expansion means *this provider + cannot reach there*, so the candidate is skipped rather than run unconstrained. `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 @@ -301,6 +313,8 @@ Responsibilities: - resolve the selected NodePool from the Pod's `nebula.inftyai.com/nodepool` label; - parse the accelerator type/count from Pod label plus `nvidia.com/gpu`; +- parse the requested geographies from `nebula.inftyai.com/regions` and narrow + each provider's regions to them; - select the first currently usable candidate across capacity tier, provider, and provider-local region; - consult the shared failover blocklist before selecting a candidate; diff --git a/docs/metrics.md b/docs/metrics.md index 4e33cda..4c75f50 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -67,21 +67,15 @@ which is the whole reason for splitting them: | `reason` | Means | Clears when | | --- | --- | --- | | `no_pool` | The Pod names a NodePool that does not exist, or carries no pool label. | A human fixes the Pod (or the workload generating it). | -| `invalid_request` | The accelerator request is malformed — e.g. `nvidia.com/gpu` with no accelerator-type label. It is *not* treated as CPU-only. | A human fixes the Pod spec. | +| `invalid_request` | The request is malformed — `nvidia.com/gpu` with no accelerator-type label (*not* treated as CPU-only), or a `nebula.inftyai.com/regions` annotation naming no known geography. | A human fixes the Pod spec. | | `all_blocked` | A servable candidate exists, but failover is holding every one of them off. | By itself — the Pod is already requeued for the block's expiry. | | `no_candidate` | No provider in the pool can serve this request at all. | An operator adds a provider, or a provider registers. | | `stale_claim` | A NodeClaim from a prior same-named Pod has not been reaped yet. | By itself, in seconds. A sustained rate means the NodeClaim backstop is stuck. | The skip `reason` is likewise closed: `provider_unregistered`, -`capacity_type_unsupported`, `accelerator_unsupported`, `blocked`. Only `blocked` clears -on its own. One reconcile can file several skips — the walk visits every candidate before -giving up. - -`nebula_placement_deferrals_total` counts **deferrals, not Pods**. A gated Pod is -reconciled again on every requeue and resync, so one Pod stuck for an hour contributes -many increments. The rate is therefore a measure of placement pressure, not a population: -for "how many Pods are stuck right now" read the SchedulingGated Pod count from -kube-state-metrics, and use this series to explain *why*. +`capacity_type_unsupported`, `accelerator_unsupported`, `egress_policy_unsupported`, +`no_available_regions`, `blocked`. Only `blocked` clears on its own. One reconcile can file +several skips — the walk visits every candidate before giving up. ## Provisioning diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index 0e17b46..6bc200b 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -85,9 +85,13 @@ 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 { +// ExpandRegions passes the declaration through and fails closed on narrowing — the +// region-simple behaviour, as pkg/provider/fake has. Tests that need group expansion set +// expandRegions. +func (f *fakeProvider) ExpandRegions(declared, narrowTo []string) []string { + if len(narrowTo) > 0 { + return nil + } if f.expandRegions != nil { return f.expandRegions(declared) } @@ -98,10 +102,12 @@ func (f *fakeProvider) ClassifyProvisionError(error, string, string) provider.Bl } // resolver returns a Providers func that resolves only the given provider. -func resolver(provs ...*fakeProvider) func(string) (provider.Provider, bool) { +// It takes the interface, not *fakeProvider, so a test can register a REAL adapter where the +// thing under test is that adapter's own behaviour (see the region-narrowing tests). +func resolver(provs ...provider.Provider) func(string) (provider.Provider, bool) { return func(name string) (provider.Provider, bool) { for _, p := range provs { - if p.name == name { + if p.Name() == name { return p, true } } @@ -125,7 +131,7 @@ func testScheme(t *testing.T) *runtime.Scheme { // newClaimReconciler wires a NodeClaimReconciler over a fake client seeded with // objs. Any fakeProviders passed are registered as the reconciler's resolver so // the teardown backstop can reach them. -func newClaimReconciler(t *testing.T, objs []client.Object, provs ...*fakeProvider) (*NodeClaimReconciler, client.Client) { +func newClaimReconciler(t *testing.T, objs []client.Object, provs ...provider.Provider) (*NodeClaimReconciler, client.Client) { t.Helper() s := testScheme(t) c := fake.NewClientBuilder(). diff --git a/internal/controller/placement_metrics_test.go b/internal/controller/placement_metrics_test.go index 0bd1326..53543d0 100644 --- a/internal/controller/placement_metrics_test.go +++ b/internal/controller/placement_metrics_test.go @@ -31,6 +31,7 @@ import ( nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/failover" "github.com/InftyAI/Nebula/pkg/metrics" + "github.com/InftyAI/Nebula/pkg/provider" "github.com/InftyAI/Nebula/pkg/util" ) @@ -103,7 +104,7 @@ func TestPlacement_DeferralReasons(t *testing.T) { pool string // the pool label expected on the metric want string // build returns the objects to seed and the providers to register. - build func() ([]client.Object, []*fakeProvider, Blocklister) + build func() ([]client.Object, []provider.Provider, Blocklister) }{{ // The Pod names a pool that does not exist. The pool label is deliberately the // placeholder, NOT the unresolved name — that string is a user-controlled Pod @@ -111,22 +112,22 @@ func TestPlacement_DeferralReasons(t *testing.T) { name: "missing pool", pool: "none", want: metrics.DeferNoPool, - build: func() ([]client.Object, []*fakeProvider, Blocklister) { + build: func() ([]client.Object, []provider.Provider, Blocklister) { return []client.Object{gatedPod("d1", "default", "uid-d1", "ghost-pool", "H100")}, - []*fakeProvider{{name: "p1"}}, nil + []provider.Provider{&fakeProvider{name: "p1"}}, nil }, }, { // nvidia.com/gpu with no accelerator-type label: malformed, not CPU-only. name: "invalid accelerator request", pool: "pool", want: metrics.DeferInvalidRequest, - build: func() ([]client.Object, []*fakeProvider, Blocklister) { + build: func() ([]client.Object, []provider.Provider, Blocklister) { pod := gatedPod("d1", "default", "uid-d1", "pool", "") pod.Spec.Containers[0].Resources.Limits = corev1.ResourceList{ util.NvidiaGPUResource: resource.MustParse("1"), } pool := poolWith("pool", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, "p1") - return []client.Object{pod, pool}, []*fakeProvider{{name: "p1"}}, nil + return []client.Object{pod, pool}, []provider.Provider{&fakeProvider{name: "p1"}}, nil }, }, { // Servable, but every candidate is blocked: self-clearing, and the caller @@ -134,10 +135,10 @@ func TestPlacement_DeferralReasons(t *testing.T) { name: "all candidates blocked", pool: "pool", want: metrics.DeferAllBlocked, - build: func() ([]client.Object, []*fakeProvider, Blocklister) { + build: func() ([]client.Object, []provider.Provider, Blocklister) { pod := gatedPod("d1", "default", "uid-d1", "pool", "H100") pool := poolWith("pool", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, "p1") - return []client.Object{pod, pool}, []*fakeProvider{{name: "p1"}}, + return []client.Object{pod, pool}, []provider.Provider{&fakeProvider{name: "p1"}}, &fakeBlocklist{blocked: []failover.Candidate{{Provider: "p1"}}} }, }, { @@ -145,17 +146,17 @@ func TestPlacement_DeferralReasons(t *testing.T) { name: "no servable candidate", pool: "pool", want: metrics.DeferNoCandidate, - build: func() ([]client.Object, []*fakeProvider, Blocklister) { + build: func() ([]client.Object, []provider.Provider, Blocklister) { pod := gatedPod("d1", "default", "uid-d1", "pool", "H100") pool := poolWith("pool", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, "p1") - return []client.Object{pod, pool}, []*fakeProvider{{name: "p1", gpus: []string{"A100"}}}, nil + return []client.Object{pod, pool}, []provider.Provider{&fakeProvider{name: "p1", gpus: []string{"A100"}}}, nil }, }, { // A claim from a prior same-named Pod has not been reaped yet. name: "stale claim", pool: "pool", want: metrics.DeferStaleClaim, - build: func() ([]client.Object, []*fakeProvider, Blocklister) { + build: func() ([]client.Object, []provider.Provider, Blocklister) { pod := gatedPod("d1", "default", "uid-new", "pool", "H100") pool := poolWith("pool", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, "p1") stale := &nebulav1alpha1.NodeClaim{ @@ -164,7 +165,7 @@ func TestPlacement_DeferralReasons(t *testing.T) { PodRef: nebulav1alpha1.PodReference{Namespace: "default", Name: "d1", UID: "uid-old"}, }, } - return []client.Object{pod, pool, stale}, []*fakeProvider{{name: "p1"}}, nil + return []client.Object{pod, pool, stale}, []provider.Provider{&fakeProvider{name: "p1"}}, nil }, }} diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index e65b679..01adce6 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -73,7 +73,7 @@ func (b *fakeBlocklist) BlockedUntil(c failover.Candidate) (time.Duration, bool) } // newPlacementReconciler wires a PodPlacementReconciler over a fake client. -func newPlacementReconciler(t *testing.T, objs []client.Object, provs ...*fakeProvider) (*PodPlacementReconciler, client.Client) { +func newPlacementReconciler(t *testing.T, objs []client.Object, provs ...provider.Provider) (*PodPlacementReconciler, client.Client) { t.Helper() s := testScheme(t) _ = clientgoscheme.AddToScheme(s) @@ -480,7 +480,7 @@ 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 + // The pool declares a GEOGRAPHY, not a region. Placement must walk the concrete // regions the provider expands it into — and must record a CONCRETE one on the claim, // never the token: the claim's region feeds ProvisionRequest.Region, which the adapter // turns into a regional API endpoint, and "us" is not one. @@ -512,17 +512,134 @@ func TestPlacement_ExpandsRegionGroupIntoConcreteCandidates(t *testing.T) { } 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 + // A region-simple provider passes nil through, 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}) + got := regionsFor(prov, nebulav1alpha1.ProviderSpec{Name: provider.ProviderModal}, nil) if !slices.Equal(got, []string{""}) { t.Fatalf("regionsFor(nil) = %v, want one empty candidate", got) } } +func TestRequestedGeographies(t *testing.T) { + cases := []struct { + name string + anno string + want []string + valid bool + }{{ + name: "absent is no narrowing", + anno: "", + valid: true, + }, { + // Not the same as a typo: there is nothing here to honour or reject. + name: "whitespace only reads as absent", + anno: " ", + valid: true, + }, { + name: "one geography", + anno: "eu", + want: []string{"eu"}, + valid: true, + }, { + name: "case and spacing are free, and repeats collapse", + anno: " EU , uk ,eu", + want: []string{"eu", "uk"}, + valid: true, + }, { + // Matches what an adapter does with a token it cannot resolve: drop it. The Pod + // still asked for somewhere real, so honour that rather than failing the lot. + name: "an unknown token alongside a known one is dropped", + anno: "eu,atlantis", + want: []string{"eu"}, + valid: true, + }, { + // The case that must NOT come back as "no narrowing": empty reads as + // unconstrained downstream, so a typo would place the Pod anywhere on earth. + name: "nothing resolvable is an invalid request", + anno: "atlantis", + }, { + // A provider region name is not vocabulary here, however real it is. It would + // resolve on one provider and not the next, making placement depend on the order + // the pool happens to list them. + name: "a provider region name is not vocabulary", + anno: "us-east-1", + }} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pod := gatedPod("p1", "default", "uid-1", "pool-a", "") + if tc.anno != "" { + pod.Annotations = map[string]string{nebulav1alpha1.RegionsAnnotation: tc.anno} + } + got, ok := requestedGeographies(pod) + if ok != tc.valid { + t.Fatalf("ok = %v, want %v", ok, tc.valid) + } + if tc.valid && !slices.Equal(got, tc.want) { + t.Fatalf("got %v, want %v", got, tc.want) + } + }) + } +} + +func TestPlacement_PodAnnotationNarrowsToTheRequestedJurisdiction(t *testing.T) { + // The pool is unconstrained, so AWS offers all 17 default-enabled regions. The Pod asks + // for "uk", which AWS serves from London alone — so the claim must carry eu-west-2 and + // not the first region of the walk. This is the whole point of the annotation: data + // residency for ONE workload, without an operator carving out a per-jurisdiction pool. + // + // CPU-only on purpose: it keeps MapAccelerator (and so the catalog) out of the path, so + // the real adapter's region table can be exercised with no client and no CSV. + pod := gatedPod("p1", "default", "uid-1", "pool-a", "") + pod.Annotations = map[string]string{nebulav1alpha1.RegionsAnnotation: "uk"} + pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, + provider.ProviderAWS) + r, c := newPlacementReconciler(t, []client.Object{pod, pool}, awsprovider.New(nil, nil, nil)) + + reconcilePod(t, r, "default", "p1") + + if region := getClaim(t, c, "default-p1").Spec.Region; region != "eu-west-2" { + t.Fatalf("expected the narrowing to select London, got %q", region) + } +} + +func TestPlacement_NarrowingThatEliminatesEveryRegionLeavesPodGated(t *testing.T) { + // AWS reaches Africa only through an opt-in region, so "af" resolves to nothing there + // (see regionsByGeography). The provider must be SKIPPED, not widened: an empty + // expansion under a narrowing means no candidate, and the alternative — falling back to + // the unconstrained set — would place an af-only workload in Ohio. + pod := gatedPod("p1", "default", "uid-1", "pool-a", "") + pod.Annotations = map[string]string{nebulav1alpha1.RegionsAnnotation: "af"} + pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, + provider.ProviderAWS) + r, c := newPlacementReconciler(t, []client.Object{pod, pool}, awsprovider.New(nil, nil, nil)) + + reconcilePod(t, r, "default", "p1") + + if got := getPod(t, c, "default", "p1"); !hasGateNamed(got) { + t.Fatal("expected the Pod to stay gated when no region serves the request") + } +} + +func TestPlacement_UnresolvableRegionAnnotationLeavesPodGated(t *testing.T) { + // A typo is an invalid request, not an empty narrowing: the Pod stays gated until a + // human fixes it. Placing it would mean ignoring a residency constraint. + pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") + pod.Annotations = map[string]string{nebulav1alpha1.RegionsAnnotation: "europe"} + pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, + provider.ProviderModal) + prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"H100"}} + r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov) + + reconcilePod(t, r, "default", "p1") + + if got := getPod(t, c, "default", "p1"); !hasGateNamed(got) { + t.Fatal("expected the Pod to stay gated on an unresolvable region request") + } +} + 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 @@ -531,7 +648,7 @@ func TestRegionsFor_AgreesWithAWSSweepExpansion(t *testing.T) { // 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}) + nebulav1alpha1.ProviderSpec{Name: provider.ProviderAWS, Regions: declared}, nil) sweepSide := awsprovider.ExpandRegions(declared) if !slices.Equal(placementSide, sweepSide) { t.Errorf("declared %v: placement walks %v but the sweep covers %v", diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index 049ce98..b600fd2 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -19,6 +19,8 @@ package controller import ( "context" "hash/fnv" + "slices" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -104,6 +106,16 @@ func (r *PodPlacementReconciler) selectPlacement(ctx context.Context, pod *corev return placement{}, false, 0 } + // The Pod's region ask, read ONCE: regionsFor runs per provider ref, so parsing inside it + // would repeat this deferral log for every provider in the pool. + narrowTo, ok := requestedGeographies(pod) + if !ok { + metrics.RecordDeferral(pool.Name, metrics.DeferInvalidRequest) + log.Info("no requested region is a known geography; leaving Pod gated", + "regions", pod.Annotations[nebulav1alpha1.RegionsAnnotation]) + return placement{}, false, 0 + } + var soonest time.Duration // 0 = no blocked-but-servable candidate seen for _, tier := range capacityTiers(pool) { // outer: capacity for _, ref := range pool.Spec.Providers { // provider (Ordered = listed order) @@ -143,7 +155,16 @@ func (r *PodPlacementReconciler) selectPlacement(ctx context.Context, pod *corev continue } } - for _, region := range regionsFor(prov, ref) { // inner: region + // Empty can only mean the Pod's narrowing eliminated every region this + // provider reaches: without one, regionsFor always yields a candidate. + regions := regionsFor(prov, ref, narrowTo) + if len(regions) == 0 { + metrics.RecordCandidateSkip(ref.Name, tier, "", metrics.SkipNoAvailableRegions) + log.V(1).Info("skipping candidate: no available region serves the requested geographies", + "provider", ref.Name, "capacityType", tier, "regions", narrowTo) + continue + } + for _, region := range regions { // 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. @@ -247,22 +268,47 @@ func servesEgress(prov provider.Provider, policy *nebulav1alpha1.EgressPolicy) b // literally — so only the provider can resolve it, and ExpandRegions does (see // provider.Provider for the three levels). // +// narrowTo is the Pod's own ask (RegionsAnnotation), which subsets that expansion. +// // The empty-string fallback covers expansion yielding nothing: a region-simple provider // whose pool declared no regions still needs ONE candidate, or `range` runs zero times and // the provider is silently unplaceable. That candidate means "send no region, place freely" — // Modal's normal and cheapest mode. // +// It is gated on there being NO narrowing, and that guard is load-bearing: under a narrowing +// an empty expansion means the intersection eliminated every region, so falling back to "" +// would place a Pod that asked for "us" anywhere on earth. Empty means no candidate there, +// and the caller skips the provider. +// // This and awsRegionSource (cmd/main.go) are the only readers of ProviderSpec.Regions and // MUST expand it identically: a region provisioned into but not swept is absent from List, // and absence is reported as Terminated on a live, billing instance. -func regionsFor(prov provider.Provider, ref nebulav1alpha1.ProviderSpec) []string { - regions := prov.ExpandRegions(ref.Regions) - if len(regions) == 0 { +func regionsFor(prov provider.Provider, ref nebulav1alpha1.ProviderSpec, narrowTo []string) []string { + regions := prov.ExpandRegions(ref.Regions, narrowTo) + if len(regions) == 0 && len(narrowTo) == 0 { return []string{""} // unconstrained on a region-simple provider } return regions } +// requestedGeographies reads the Pod's RegionsAnnotation into the narrowing ExpandRegions +// takes. Absent means no narrowing, the common case. +func requestedGeographies(pod *corev1.Pod) (narrowTo []string, ok bool) { + raw := strings.TrimSpace(pod.Annotations[nebulav1alpha1.RegionsAnnotation]) + if raw == "" { + return nil, true + } + tokens := strings.Split(raw, ",") + out := make([]string, 0, len(tokens)) + for _, t := range tokens { + t = strings.ToLower(strings.TrimSpace(t)) + if provider.IsGeography(t) && !slices.Contains(out, t) { + out = append(out, t) + } + } + return out, len(out) > 0 +} + // blockedUntil reports whether the (provider, accelerator, tier, region) // candidate is currently excluded by the failover blocklist and, if so, how long // until it frees (for the requeue hint). accelerator is the request's pool diff --git a/pkg/metrics/placement.go b/pkg/metrics/placement.go index 9cc5c4d..873d1d3 100644 --- a/pkg/metrics/placement.go +++ b/pkg/metrics/placement.go @@ -52,6 +52,7 @@ const ( SkipCapacityUnsupported = "capacity_type_unsupported" SkipAcceleratorUnsupported = "accelerator_unsupported" SkipEgressUnsupported = "egress_policy_unsupported" + SkipNoAvailableRegions = "no_available_regions" SkipBlocked = "blocked" ) diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index 7b7eb7c..1571c56 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -40,7 +40,6 @@ import ( "context" "errors" "fmt" - "sort" "strings" "sync" "time" @@ -71,26 +70,31 @@ const spotPollInterval = 10 * time.Second // container became healthy" — that is the 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 ("us" is not an endpoint, London -// is eu-west-2), so the mapping is data. +// regionsByGeography maps a geography token to the AWS regions it encompasses. // -// Only DEFAULT-enabled regions are listed. Opt-in ones (af-south-1, ap-east-1, ca-west-1, -// eu-south-1, me-central-1, …) are excluded because clientFor resolves a GPU AMI and the -// default VPC's subnets on first use and does not cache failures — a region the account has -// not enabled would fail and retry every poll tick, forever, for a region nobody asked for. -// An operator who HAS enabled one names it explicitly; literal names pass through untouched. -// -// GovCloud (us-gov-*) and China (cn-*) are absent for a stronger reason: separate IAM -// partitions, so one credential set cannot reach them at all. -// -// Kept sorted so the failover walk order within a group is stable and reviewable. -var regionGroups = map[string][]string{ +// The empty entries are the opt-in regions, left out on purpose: EC2 answers +// OptInRequired for an account that has not enabled one, which translate.go maps to +// ErrAuth and so blocklists the WHOLE provider. A pool can still name such a region +// literally, but narrowing intersects this table — so it cannot be reached through the +// regions annotation. +var regionsByGeography = 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"}, + "eu": {"eu-central-1", "eu-north-1", "eu-west-1", "eu-west-3"}, + "uk": {"eu-west-2"}, + "ap": { + "ap-northeast-1", "ap-northeast-2", "ap-northeast-3", + "ap-south-1", "ap-southeast-1", "ap-southeast-2", + }, + "af": {}, + "me": {}, + "mx": {}, +} + +// regionsIn returns the list of AWS regions within a given geography token. +func regionsIn(geography string) []string { + return regionsByGeography[geography] } // ErrSpotCapacity is a marker the Client wraps onto a Spot-tier capacity failure @@ -264,34 +268,46 @@ 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: +// ExpandRegions implements provider.Provider. EC2 region names are not geographies, so AWS +// resolves them through regionsByGeography. Three levels, in the order they are checked: // -// nil/[] => every default-enabled region (the union of regionGroups) -// ["us"] => that group's regions +// nil/[] => every default-enabled region +// ["us"] => the default-enabled regions in that geography // ["us-east-1"] => itself, verbatim and unvalidated // -// A non-group value is a literal region name, NOT validated against any list: EC2 gains -// regions faster than this table is edited, so validating would reject a region that -// exists, while an impossible name simply fails at clientFor with AWS's own error. 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, so the -// failover walk is reproducible. -// -// Unconstrained is wide: ~17 regions per capacity tier, each walked as a candidate and -// swept by List/Offerings every tick. Prefer a group unless the workload needs global reach. -// -// It delegates to the package-level ExpandRegions, which cmd/main.go's region source also -// needs — it must expand each pool BEFORE unioning across pools (a pool declaring nothing -// means "all", a meaning lost if raw lists were unioned first). -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. +// narrowTo filters the expansion to only include regions within the requested geographies. +func (p *Provider) ExpandRegions(declared, narrowTo []string) []string { + return narrowRegions(ExpandRegions(declared), narrowTo) +} + +// narrowRegions keeps the regions that fall inside at least one requested geography. An +// empty narrowTo is the no-op. +func narrowRegions(expanded, narrowTo []string) []string { + if len(narrowTo) == 0 { + return expanded + } + want := make(map[string]bool) + for _, token := range narrowTo { + token = strings.ToLower(strings.TrimSpace(token)) + // The token must be a recognized geography; otherwise it is ignored. + if !provider.IsGeography(token) { + continue + } + for _, r := range regionsByGeography[token] { + want[r] = true + } + } + var out []string + for _, r := range expanded { + if want[r] { + out = append(out, r) + } + } + return out +} + +// ExpandRegions is the package-level function that mirrors Provider.ExpandRegions, expanding +// declared regions into the full set of EC2 regions, with geographies resolved. func ExpandRegions(declared []string) []string { seen := make(map[string]bool) var out []string @@ -302,16 +318,10 @@ func ExpandRegions(declared []string) []string { 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] { + // Unconstrained: every default-enabled region. + for _, g := range provider.Geographies { + for _, r := range regionsByGeography[g] { add(r) } } @@ -319,8 +329,8 @@ func ExpandRegions(declared []string) []string { } for _, d := range declared { d = strings.TrimSpace(d) - if group, ok := regionGroups[strings.ToLower(d)]; ok { - for _, r := range group { + if token := strings.ToLower(d); provider.IsGeography(token) { + for _, r := range regionsIn(token) { add(r) } continue diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index 9ea5217..f1a72f4 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -613,21 +613,27 @@ func TestExpandRegions(t *testing.T) { }{{ name: "nil is unconstrained: every default-enabled region", declared: nil, + // Grouped by geography, in provider.Geographies order — so London trails sa-east-1, + // under "uk", rather than sitting with the eu-* names it is spelled like. 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", + "eu-central-1", "eu-north-1", "eu-west-1", "eu-west-3", "sa-east-1", + "eu-west-2", "us-east-1", "us-east-2", "us-west-1", "us-west-2", }, }, { name: "empty behaves as nil", declared: []string{}, + // Grouped by geography, in provider.Geographies order — so London trails sa-east-1, + // under "uk", rather than sitting with the eu-* names it is spelled like. 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", + "eu-central-1", "eu-north-1", "eu-west-1", "eu-west-3", "sa-east-1", + "eu-west-2", "us-east-1", "us-east-2", "us-west-1", "us-west-2", }, }, { @@ -635,9 +641,10 @@ func TestExpandRegions(t *testing.T) { declared: []string{"us"}, want: []string{"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, }, { + // London is absent on purpose: it is under "uk" alone (see regionsByGeography). 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"}, + want: []string{"eu-central-1", "eu-north-1", "eu-west-1", "eu-west-3"}, }, { name: "literal region passes through", declared: []string{"us-east-1"}, @@ -680,18 +687,125 @@ func TestExpandRegions(t *testing.T) { // 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) { + if m := p.ExpandRegions(tc.declared, nil); !slices.Equal(m, got) { t.Fatalf("method %v != function %v", m, got) } }) } } +func TestExpandRegions_NarrowTo(t *testing.T) { + cases := []struct { + name string + declared []string + narrowTo []string + want []string + }{{ + name: "no narrowing is the no-op", + declared: []string{"us"}, + want: []string{"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, + }, { + name: "group token narrows the unconstrained expansion", + narrowTo: []string{"us"}, + want: []string{"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, + }, { + // Only geographies reach here, so a Modal region name is not a narrowing this + // adapter could honour even by accident -- it is dropped, not forwarded. + name: "a Modal region name is dropped", + declared: []string{"us"}, + narrowTo: []string{"us-east"}, + want: nil, + }, { + name: "uk selects London", + narrowTo: []string{"uk"}, + want: []string{"eu-west-2"}, + }, { + // The partition is the point: "eu" is the EEA, so it must not reach London even + // though eu-west-2 is spelled like the rest of the group. Wanting both means + // asking for both. + name: "eu excludes London", + narrowTo: []string{"eu"}, + want: []string{"eu-central-1", "eu-north-1", "eu-west-1", "eu-west-3"}, + }, { + // An EC2 region name is not a geography, so the per-workload path drops it. The + // declaration is the only place a literal region belongs. + name: "an AWS region name is dropped, not matched literally", + narrowTo: []string{"us-east-1"}, + want: nil, + }, { + name: "several tokens union", + narrowTo: []string{"ca", "sa"}, + want: []string{"ca-central-1", "sa-east-1"}, + }, { + name: "case and whitespace are normalized", + narrowTo: []string{" US "}, + want: []string{"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, + }, { + // Africa is opt-in only, so no default-enabled region falls under it. Empty + // means no candidate: placement skips the ref rather than widening. + name: "a geography AWS serves only via opt-in narrows to nothing", + narrowTo: []string{"af"}, + want: nil, + }, { + name: "an unresolvable token narrows to nothing", + narrowTo: []string{"usa"}, + want: nil, + }, { + name: "a disjoint request narrows to nothing", + declared: []string{"eu"}, + narrowTo: []string{"us"}, + want: nil, + }, { + // The accepted cost of a default-enabled-only table: eu-south-1 IS in Europe, but + // nothing here says so, so a request for "eu" cannot select it. The admin's literal + // declaration still places it -- it is only per-workload narrowing that loses it. + name: "an opt-in region declared by the pool is not narrowable", + declared: []string{"eu-south-1", "eu-west-1"}, + narrowTo: []string{"eu"}, + want: []string{"eu-west-1"}, + }} + p := newTestProvider(&fakeClient{}) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := p.ExpandRegions(tc.declared, tc.narrowTo) + if !slices.Equal(got, tc.want) { + t.Fatalf("ExpandRegions(%v, %v)\n got %v\nwant %v", + tc.declared, tc.narrowTo, got, tc.want) + } + // The invariant that makes narrowing safe without any membership check: + // whatever comes out is a subset of what the SWEEP covers for the same + // declaration. A narrowing that escaped it would provision into a region + // List never polls, and absence from List reports a live instance as + // Terminated. + for _, r := range got { + if !slices.Contains(ExpandRegions(tc.declared), r) { + t.Fatalf("narrowed region %q is outside the swept expansion %v", + r, ExpandRegions(tc.declared)) + } + } + }) + } +} + +// TestExpandRegions_NarrowToTakesVocabularyOnly covers the IsGeography gate itself, which no +// other case can reach: a narrowing resolves only because the token is VOCABULARY, not merely +// because this adapter happens to have a table entry for it. Without the gate, adding a key +// here would silently give one provider a narrowing token its siblings drop. +func TestExpandRegions_NarrowToTakesVocabularyOnly(t *testing.T) { + regionsByGeography["jp"] = []string{"ap-northeast-1"} + defer delete(regionsByGeography, "jp") + + if got := narrowRegions(ExpandRegions(nil), []string{"jp"}); got != nil { + t.Errorf("narrowTo [jp] resolved to %v; only provider.Geographies tokens may narrow", 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. + // cache build failures, so one reached by an expansion would fail its AMI/subnet + // resolution and retry on EVERY poll tick, forever, for a region nobody asked for. + // So they are absent from regionsByGeography itself, which is the only thing an + // expansion or a narrowing consults; a pool names one literally instead. 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", @@ -701,13 +815,62 @@ func TestRegionGroups_ExcludeOptInRegions(t *testing.T) { 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) + t.Errorf("opt-in region %q must not expand (it is disabled by default)", r) + } + for geography, regions := range regionsByGeography { + if slices.Contains(regions, r) { + t.Errorf("opt-in region %q is listed under %q; the table holds default-enabled "+ + "regions only, or narrowing can select one clientFor will retry forever", r, geography) + } } } - // Separate IAM partitions: one credential set cannot reach them at all. + // Separate IAM partitions: one credential set cannot reach them at all, so they are + // absent from regionsByGeography entirely, not merely from the default-enabled set. 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) + t.Errorf("region %q is in another IAM partition and must not expand", r) + } + } + for geography, regions := range regionsByGeography { + for _, r := range regions { + if strings.HasPrefix(r, "us-gov-") || strings.HasPrefix(r, "cn-") { + t.Errorf("region %q under %q is in another IAM partition", r, geography) + } + } + } +} + +// TestRegionsByGeography_IsResolvable is the seam between AWS's half of the region model and +// the shared one: a key outside provider.Geographies is unreachable, since no declaration or +// narrowing resolves to it and its regions can never be placed into. +func TestRegionsByGeography_IsResolvable(t *testing.T) { + for geography := range regionsByGeography { + if !provider.IsGeography(geography) { + t.Errorf("%q is not a provider.Geographies token, so nothing can resolve to it", geography) + } + } + // And every geography must appear, mapping to nothing when AWS reaches it only through + // opt-in regions. A MISSING key and an empty one behave identically, so this is about the + // table staying a complete record of what was checked. + for _, g := range provider.Geographies { + if _, ok := regionsByGeography[g]; !ok { + t.Errorf("geography %q has no entry; use an empty one if AWS serves it no "+ + "default-enabled region", g) + } + } + // The lists ARE the walk order — nothing sorts them downstream — and they must partition: + // a region under two geographies would be walked twice by a declaration naming both. + home := map[string]string{} + for geography, regions := range regionsByGeography { + if !slices.IsSorted(regions) { + t.Errorf("%q is not sorted: %v", geography, regions) + } + for _, r := range regions { + if other, dup := home[r]; dup { + t.Errorf("%q is under both %q and %q; placement would attempt it twice", + r, other, geography) + } + home[r] = geography } } } diff --git a/pkg/provider/catalog/base.go b/pkg/provider/catalog/base.go index 84ec8b9..427dd2f 100644 --- a/pkg/provider/catalog/base.go +++ b/pkg/provider/catalog/base.go @@ -53,8 +53,8 @@ type Lookup interface { // only while a provider's catalog price is all-in; one that meters CPU/memory separately // overrides it and adds those components. // -// Lifecycle, Capabilities and ClassifyProvisionError are genuinely provider-specific and -// are not provided here. +// Lifecycle, Capabilities, ClassifyProvisionError and ExpandRegions are genuinely +// provider-specific and are not provided here. type Base struct { // ProviderName is this provider's stable identifier (e.g. "modal"), used both // as Name() and as the key into the catalog. @@ -74,18 +74,6 @@ func (b Base) Offerings(context.Context) ([]provider.Offering, error) { return b.Catalog.Offerings(b.ProviderName), nil } -// ExpandRegions passes the declared regions through unchanged: one candidate each, tokens -// used verbatim as region names. Right for a provider whose own vocabulary already spans -// both levels the pool speaks AND whose provision reports capacity failures synchronously, -// so walking candidates actually buys a retry in the next region. nil stays nil, which -// every adapter reads as "unconstrained". -// -// Both halves have real overriders, in opposite directions: AWS expands a group token into -// many candidates ("us" is not a callable region), while Modal collapses everything into -// ONE candidate because its create cannot fail over. Check which 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 this // provider's own ids, using the catalog as the mapping table: matching rows contribute // their AcceleratorIDs in catalog order — PRIMARY first, then interchangeable alternates, diff --git a/pkg/provider/fake/fake.go b/pkg/provider/fake/fake.go index baf4878..e5efd76 100644 --- a/pkg/provider/fake/fake.go +++ b/pkg/provider/fake/fake.go @@ -29,6 +29,7 @@ package fake import ( "context" "fmt" + "strings" "sync" corev1 "k8s.io/api/core/v1" @@ -44,6 +45,15 @@ import ( // enabled, so a real cluster never places onto it by accident. const ProviderName = "fake" +// regionsByGeography is the fake's region table. Two geographies are enough to cover both +// halves of narrowing — one that resolves to several regions, one to a single region — and +// every geography absent here resolves to nothing, which is the elimination path. The names +// are deliberately unlike any real provider's so they can never be mistaken for live ones. +var regionsByGeography = map[string][]string{ + "us": {"us-fake-1", "us-fake-2"}, + "eu": {"eu-fake-1"}, +} + // compile-time assertion that Provider satisfies the interface. var _ provider.Provider = (*Provider)(nil) @@ -82,6 +92,72 @@ func (p *Provider) Capabilities() provider.Capabilities { } } +// ExpandRegions expands geography tokens through regionsByGeography and narrows the +// result to the requested geographies, mirroring the AWS adapter. Only that shape gives +// placement a NAMED region per candidate, which is what lets the e2e suite assert which +// region a Pod landed in rather than just that it landed. +// +// The fake names regions without partitioning anything behind them: Provision, Get and +// List ignore the region entirely, so no instance behaves differently per region. +func (p *Provider) ExpandRegions(declared, narrowTo []string) []string { + expanded := expandRegions(declared) + if len(narrowTo) == 0 { + return expanded + } + want := make(map[string]bool) + for _, token := range narrowTo { + token = strings.ToLower(strings.TrimSpace(token)) + // Only the shared vocabulary narrows; anything else is dropped, never + // forwarded, because every Provision error is terminal. + if !provider.IsGeography(token) { + continue + } + for _, r := range regionsByGeography[token] { + want[r] = true + } + } + var out []string + for _, r := range expanded { + if want[r] { + out = append(out, r) + } + } + return out +} + +// expandRegions resolves each declared token, forwarding a non-geography verbatim. An +// empty declaration means unconstrained, which walks the whole vocabulary in order. +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) + } + if len(declared) == 0 { + for _, g := range provider.Geographies { + for _, r := range regionsByGeography[g] { + add(r) + } + } + return out + } + for _, d := range declared { + d = strings.TrimSpace(d) + if token := strings.ToLower(d); provider.IsGeography(token) { + for _, r := range regionsByGeography[token] { + add(r) + } + continue + } + add(d) + } + return out +} + // Provision records one instance for the claim and reports it Running at once. // Idempotent on ClaimName: a repeat returns the existing instance's id rather // than creating a second (matching the real adapters' contract). diff --git a/pkg/provider/fake/fake_test.go b/pkg/provider/fake/fake_test.go index c9c76d8..710a71a 100644 --- a/pkg/provider/fake/fake_test.go +++ b/pkg/provider/fake/fake_test.go @@ -132,6 +132,70 @@ func TestTerminateIsIdempotent(t *testing.T) { } } +// The fake backs the e2e suite, so its region behaviour has to be the real thing: a +// geography expands, a literal passes through, and a narrowing request subsets the +// expansion. If any of this drifted toward a pass-through, e2e would sign off on a +// narrowing the real adapters never perform. +func TestExpandRegions(t *testing.T) { + tests := []struct { + name string + declared []string + narrowTo []string + want []string + }{{ + name: "unconstrained walks the vocabulary in order", + want: []string{"eu-fake-1", "us-fake-1", "us-fake-2"}, + }, { + name: "a geography expands to its regions", + declared: []string{"us"}, + want: []string{"us-fake-1", "us-fake-2"}, + }, { + name: "a non-geography is forwarded verbatim", + declared: []string{"somewhere-else-1"}, + want: []string{"somewhere-else-1"}, + }, { + name: "narrowing subsets the expansion", + declared: []string{"us", "eu"}, + narrowTo: []string{"eu"}, + want: []string{"eu-fake-1"}, + }, { + // The pool is the ceiling: narrowing can never reach outside it. + name: "narrowing cannot widen past the declaration", + declared: []string{"eu"}, + narrowTo: []string{"us"}, + want: nil, + }, { + // Empty under a narrowing means "this provider cannot reach there", which + // placement skips rather than running unconstrained. + name: "a geography the fake does not serve resolves to nothing", + narrowTo: []string{"ap"}, + want: nil, + }, { + name: "a region name is not vocabulary, so it narrows to nothing", + narrowTo: []string{"us-fake-1"}, + want: nil, + }, { + name: "narrowing tokens are trimmed and case-folded", + narrowTo: []string{" US "}, + want: []string{"us-fake-1", "us-fake-2"}, + }} + + p := New() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := p.ExpandRegions(tt.declared, tt.narrowTo) + if len(got) != len(tt.want) { + t.Fatalf("ExpandRegions(%v, %v) = %v, want %v", tt.declared, tt.narrowTo, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Fatalf("ExpandRegions(%v, %v) = %v, want %v", tt.declared, tt.narrowTo, got, tt.want) + } + } + }) + } +} + func TestMapAcceleratorFromCatalog(t *testing.T) { p := New() // A GPU in the fixed catalog resolves (case-insensitively); one that is not diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index bd50bba..9d3e888 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -317,36 +317,78 @@ func New(client Client, cat catalog.Lookup) *Provider { // where 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. -// -// The opposite of AWS, because Modal cannot fail over. An AWS CreateFleet reports a -// shortage synchronously, so walking regions one at a time lets the next be tried. -// Sandboxes.Create instead ACCEPTS immediately and returns a real id with the GPU maybe -// still queued. No error means ClassifyProvisionError never runs, nothing is blocklisted, -// and placement is never re-driven — so the first region walked would be the only one ever -// tried, shrinking the pool to one region and discarding the rest. -// -// Handing Modal the full set moves the choice to the party that can act on it: its -// scheduler takes several regions and picks with a live view of capacity. -// -// The cost is that the candidate's region is a joined token, so a blocklist entry covers -// the whole set. That loses nothing today, 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)) +// regionsByGeography maps a geography token to the Modal regions it encompasses. +var regionsByGeography = map[string][]string{ + "us": {"us", "us-east", "us-central", "us-south", "us-west"}, + "eu": {"eu", "eu-west", "eu-north", "eu-south"}, + "ap": {"ap", "ap-northeast", "ap-southeast", "ap-south", "ap-melbourne", "jp", "au"}, + "uk": {"uk"}, + "ca": {"ca"}, + "me": {"me"}, + "sa": {"sa"}, + "af": {"af"}, + "mx": {"mx"}, +} + +// narrowRegions intersects a pool's declaration with the requested geographies, dropping any +// Modal cannot resolve. An empty result means no candidate. +func narrowRegions(declared, narrowTo []string) []string { + requested := make([]string, 0, len(narrowTo)) + want := make(map[string]bool) + for _, t := range narrowTo { + t = strings.ToLower(strings.TrimSpace(t)) + if !provider.IsGeography(t) { + continue + } + inside, ok := regionsByGeography[t] + if !ok { + continue + } + requested = append(requested, t) + for _, r := range inside { + want[r] = true + } + } + switch { + case len(requested) == 0: + return nil // nothing Modal can resolve + case len(declared) == 0: + return requested // unconstrained pool: the request becomes the constraint + } + var out []string for _, d := range declared { - d = strings.TrimSpace(d) - if d == "" || seen[d] { + if want[strings.ToLower(strings.TrimSpace(d))] { + out = append(out, d) + } + } + return out +} + +// dedupeRegions trims and dedupes, preserving first-seen order so the joined token is +// stable across reconciles. +func dedupeRegions(regions []string) []string { + seen := make(map[string]bool) + out := make([]string, 0, len(regions)) + for _, r := range regions { + r = strings.TrimSpace(r) + if r == "" || seen[r] { continue } - seen[d] = true - regions = append(regions, d) + seen[r] = true + out = append(out, r) + } + return out +} + +// ExpandRegions implements provider.Provider. It resolves the pool's whole declaration to at +// most ONE candidate, carrying every declared region in it, rather than one per region. +func (p *Provider) ExpandRegions(declared, narrowTo []string) []string { + regions := dedupeRegions(declared) + if len(narrowTo) > 0 { + regions = dedupeRegions(narrowRegions(regions, narrowTo)) } if len(regions) == 0 { - return nil // unconstrained: the widest and cheapest case + return nil } return []string{strings.Join(regions, regionSeparator)} } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index d7ecb9b..3da4728 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -940,7 +940,7 @@ func TestExpandRegions_CollapsesToOneCandidate(t *testing.T) { want: []string{"eu-west" + regionSeparator + "us-east"}, }} { t.Run(tc.name, func(t *testing.T) { - got := p.ExpandRegions(tc.declared) + got := p.ExpandRegions(tc.declared, nil) if !slices.Equal(got, tc.want) { t.Fatalf("ExpandRegions(%v) = %v, want %v", tc.declared, got, tc.want) } @@ -952,6 +952,133 @@ func TestExpandRegions_CollapsesToOneCandidate(t *testing.T) { } } +func TestExpandRegions_NarrowTo(t *testing.T) { + p := newTestProvider(&fakeClient{}) + for _, tc := range []struct { + name string + declared []string + narrowTo []string + want []string + }{{ + name: "an unconstrained pool takes the request as its constraint", + narrowTo: []string{"us"}, + want: []string{"us"}, + }, { + name: "a declaration matching the request survives whole", + declared: []string{"us"}, + narrowTo: []string{"us"}, + want: []string{"us"}, + }, { + // The pool is narrower than the request, which is the ordinary case now that only + // broad geographies can be requested: the declaration is what survives. + name: "a declaration inside the request survives it", + declared: []string{"us-east"}, + narrowTo: []string{"us"}, + want: []string{"us-east"}, + }, { + // Modal files Japan under Asia-Pacific while naming it "jp", so no spelling rule + // could relate the two. regionsByGeography carries the membership as data. + name: "jp survives a request for ap though its name does not say so", + declared: []string{"jp"}, + narrowTo: []string{"ap"}, + want: []string{"jp"}, + }, { + // The cost of broad-only requests: Modal's narrow names are real regions a pool + // may declare, but a workload cannot ASK for one. It asks for "ap" instead. + name: "a Modal narrow region name is not requestable", + declared: []string{"ap"}, + narrowTo: []string{"jp"}, + want: nil, + }, { + name: "several survivors stay ONE candidate", + declared: []string{"us-east", "eu-west", "ap-south"}, + narrowTo: []string{"us", "eu"}, + want: []string{"us-east" + regionSeparator + "eu-west"}, + }, { + // A geography's own name is in its region list, so both halves of the declaration + // survive and Modal's scheduler gets to choose between them. + name: "a broad region and a narrow one under it both survive", + declared: []string{"us", "us-east"}, + narrowTo: []string{"us"}, + want: []string{"us" + regionSeparator + "us-east"}, + }, { + name: "case and whitespace are normalized", + narrowTo: []string{" US "}, + want: []string{"us"}, + }, { + name: "a disjoint request yields no candidate", + declared: []string{"us-east"}, + narrowTo: []string{"eu"}, + want: nil, + }, { + // The reason regionsByGeography exists. "us-east-1" is an AWS region name; forwarded, + // it would reach Sandboxes.Create and either fail the Pod terminally -- before + // the walk ever reached the AWS ref that WOULD have served it -- or be ignored, + // placing the sandbox anywhere and breaking the residency that was asked for. + name: "an AWS region name is dropped, never forwarded to Modal", + narrowTo: []string{"us-east-1"}, + want: nil, + }, { + name: "an unknown token is dropped", + declared: []string{"us"}, + narrowTo: []string{"usa"}, + want: nil, + }} { + t.Run(tc.name, func(t *testing.T) { + got := p.ExpandRegions(tc.declared, tc.narrowTo) + if !slices.Equal(got, tc.want) { + t.Fatalf("ExpandRegions(%v, %v) = %v, want %v", + tc.declared, tc.narrowTo, got, tc.want) + } + // An empty result with a narrowTo means NO CANDIDATE. Reading it as + // unconstrained -- which is what nil means without one -- would place a Pod + // that asked for one geography anywhere Modal runs. + if len(got) > 1 { + t.Fatalf("produced %d candidates; Modal cannot fail over", len(got)) + } + }) + } +} + +// TestExpandRegions_NarrowToTakesVocabularyOnly covers the IsGeography gate itself, which no +// other case can reach: a narrowing resolves only because the token is VOCABULARY, not merely +// because this adapter has a table entry for it. Modal makes the stake concrete — "jp" IS a +// region it serves, so without the gate an unconstrained pool would hand it straight to the +// API as a candidate, honouring a token every sibling provider drops. +func TestExpandRegions_NarrowToTakesVocabularyOnly(t *testing.T) { + regionsByGeography["jp"] = []string{"jp"} + defer delete(regionsByGeography, "jp") + + p := newTestProvider(&fakeClient{}) + if got := p.ExpandRegions(nil, []string{"jp"}); got != nil { + t.Errorf("narrowTo [jp] resolved to %v; only provider.Geographies tokens may narrow", got) + } +} + +// TestRegionsByGeography_IsResolvable is the seam between Modal's half of the region model +// and the shared one. A key outside provider.Geographies is unreachable, and a geography +// missing its OWN name is worse than unreachable: Modal's expansion is the identity, so a +// pool declaring that geography would place fine while a Pod requesting it would be filtered +// out — the same word meaning two things on one provider. +func TestRegionsByGeography_IsResolvable(t *testing.T) { + for geography, regions := range regionsByGeography { + if !provider.IsGeography(geography) { + t.Errorf("%q is not a provider.Geographies token, so nothing can resolve to it", geography) + } + if !slices.Contains(regions, geography) { + t.Errorf("geography %q does not list itself; Modal serves all nine broad names, "+ + "and the self-entry is what the identity expansion relies on", geography) + } + } + // Every geography must be resolvable here, or a NodePool declaring one places while the + // matching request drops -- the divergence above, in the other direction. + for _, g := range provider.Geographies { + if _, ok := regionsByGeography[g]; !ok { + t.Errorf("geography %q has no Modal entry", g) + } + } +} + // 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 @@ -968,7 +1095,7 @@ func TestExpandRegions_RoundTripsThroughProvision(t *testing.T) { f := &fakeClient{createID: "sb-1"} p := newTestProvider(f) - candidates := p.ExpandRegions(declared) + candidates := p.ExpandRegions(declared, nil) // Placement's own fallback when expansion is empty: one unconstrained candidate. if len(candidates) == 0 { candidates = []string{""} diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 1a44466..67f72e8 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -119,28 +119,11 @@ type Provider interface { // only the provider knows its geography: // // - nil/empty => unconstrained: every region this provider serves. - // - a GROUP token ("us", "eu", "ap") => that geography's regions. + // - a GEOGRAPHY ("us", "eu", "ap" — see Geographies) => its regions here. // - anything else => a literal region name, passed through UNVALIDATED. // - // That last case is deliberate: region names change faster than this code, so an - // unrecognized one is forwarded and a genuinely bad name fails at provision time with - // the provider's own error. Better than refusing a region that shipped last week. - // - // Expanding HERE, at the pool boundary, keeps everything downstream single-valued — - // NodeClaimSpec.Region, ProvisionRequest.Region and the blocklist key — so a capacity - // failure blocks the one candidate that failed, not the group it came from. - // - // How many candidates a declaration becomes depends on whether the provider can FAIL - // OVER between regions. One that reports a shortage synchronously (AWS) returns one - // candidate per region, so the next is tried. One that just queues the request with no - // error (Modal) must not: nothing would re-drive placement, so only the first - // candidate would ever be tried. It returns ONE opaque candidate carrying the whole - // set and lets its own scheduler choose. - // - // Pure (no API calls, no ctx), because the result feeds both placement's candidate walk - // and the List/Offerings fan-out, and those MUST agree: a region provisioned into but - // not swept is absent from List, which reports a live instance as Terminated. - ExpandRegions(declared []string) []string + // Narrowing with narrowTo restricts the result to regions within the specified geographies. + ExpandRegions(declared, narrowTo []string) []string // ClassifyProvisionError maps a Provision error to the granularity at which // the failing placement should be blocklisted. This keeps failover precise: diff --git a/pkg/provider/regions.go b/pkg/provider/regions.go new file mode 100644 index 0000000..8e24978 --- /dev/null +++ b/pkg/provider/regions.go @@ -0,0 +1,28 @@ +/* +Copyright 2026 The InftyAI Team. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package provider + +import "slices" + +// Geographies is Nebula's region vocabulary: the broad geographies a NodePool or a Pod +// may name. +var Geographies = []string{"af", "ap", "ca", "eu", "me", "mx", "sa", "uk", "us"} + +// IsGeography reports whether token is part of the vocabulary. +func IsGeography(token string) bool { + return slices.Contains(Geographies, token) +} diff --git a/pkg/provider/regions_test.go b/pkg/provider/regions_test.go new file mode 100644 index 0000000..be61c18 --- /dev/null +++ b/pkg/provider/regions_test.go @@ -0,0 +1,66 @@ +/* +Copyright 2026 The InftyAI Team. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package provider + +import ( + "slices" + "testing" +) + +func TestIsGeography(t *testing.T) { + for _, tc := range []struct { + token string + want bool + }{ + {"us", true}, + {"uk", true}, // not folded into "eu"; see Geographies + {"af", true}, + // Provider region names are NOT vocabulary, at either provider. This is what makes + // the Pod-facing path broad-only: an adapter never sees one of these in narrowTo. + {"us-east-1", false}, // AWS + {"us-east", false}, // Modal + {"jp", false}, // Modal + {"usa", false}, + {"US", false}, // callers lowercase first + {"", false}, + } { + if got := IsGeography(tc.token); got != tc.want { + t.Errorf("IsGeography(%q) = %v, want %v", tc.token, got, tc.want) + } + } +} + +// TestGeographies_AreFlatAndSorted pins the two properties every provider table leans on: a +// geography is a bare token (anything with a "-" is a provider's own name and belongs in a +// provider table, not here), and the list is sorted so a reader can find one. +func TestGeographies_AreFlatAndSorted(t *testing.T) { + if !slices.IsSorted(Geographies) { + t.Errorf("Geographies is not sorted: %v", Geographies) + } + for _, g := range Geographies { + if g == "" { + t.Error("Geographies holds an empty token") + } + for _, c := range g { + if c == '-' || (c >= 'A' && c <= 'Z') { + t.Errorf("geography %q is not a bare lowercase token; provider region names "+ + "live in the provider's own table", g) + break + } + } + } +} diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index e105eee..cb7218a 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -127,7 +127,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) ExpandRegions(declared, _ []string) []string { return declared } func (f *fakeProvider) ClassifyProvisionError(_ error, accel, region string) provider.BlockScope { f.classifyAccel = accel f.classifyRegion = region diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index ef5f236..d7d1d2d 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -58,6 +58,14 @@ const ( // excludes nebula-system (see config/webhook/selector_patch.yaml), so a Pod // there would never get the scheduling gate and placement would never run. fakeWorkloadNS = "nebula-e2e-workload" + + // Region-narrowing fixtures. They share fakeWorkloadNS but need their own pool, + // because that pool declares regions while fakePoolName deliberately declares + // none (its spec covers the unconstrained path). + fakeRegionPoolName = "e2e-fake-region-pool" + regionServedPod = "e2e-fake-region-served" + regionUnreachablePod = "e2e-fake-region-unreachable" + regionUnknownPod = "e2e-fake-region-unknown" ) var _ = Describe("Manager", Ordered, func() { @@ -125,6 +133,9 @@ var _ = Describe("Manager", Ordered, func() { _, _ = utils.Run(exec.Command("kubectl", "delete", "pod", fakeWorkloadPod, "-n", fakeWorkloadNS, "--ignore-not-found=true")) _, _ = utils.Run(exec.Command("kubectl", "delete", "nodepool", fakePoolName, "--ignore-not-found=true")) + // The region Pods go with the namespace; their pool is cluster-scoped. + _, _ = utils.Run(exec.Command("kubectl", "delete", "nodepool", fakeRegionPoolName, + "--ignore-not-found=true")) _, _ = utils.Run(exec.Command("kubectl", "delete", "ns", fakeWorkloadNS, "--ignore-not-found=true")) By("cleaning up the sync-benchmark batch, pool, and namespace") @@ -435,6 +446,116 @@ spec: _, _ = utils.Run(exec.Command("kubectl", "delete", "-f", manifestFile, "--ignore-not-found=true")) }) + It("should honour the region annotation, and gate the Pod when it cannot", func() { + // The annotation narrows placement WITHIN the pool, so three Pods against one + // pool cover the whole contract: a request the pool can serve lands in that + // exact region, a request for a geography the pool cannot reach stays gated, + // and a token outside the geography vocabulary stays gated too. The last two + // are the ones worth having in e2e — a regression there does not error, it + // silently places the workload in the wrong jurisdiction. + + By("creating the workload namespace (idempotent: the placement spec may have made it)") + _, _ = utils.Run(exec.Command("kubectl", "create", "ns", fakeWorkloadNS)) + cmd := exec.Command("kubectl", "label", "--overwrite", "ns", fakeWorkloadNS, + "pod-security.kubernetes.io/enforce=restricted") + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed to label the workload namespace") + + By("creating a pool spanning both of the fake provider's geographies, plus three Pods") + pod := func(name, regions string) string { + return fmt.Sprintf(`--- +apiVersion: v1 +kind: Pod +metadata: + name: %s + namespace: %s + labels: + nebula.inftyai.com/enabled: "true" + nebula.inftyai.com/nodepool: %s + annotations: + nebula.inftyai.com/regions: %s +spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + containers: + - name: main + image: registry.k8s.io/pause:3.10 + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL +`, name, fakeWorkloadNS, fakeRegionPoolName, regions) + } + manifest := fmt.Sprintf(`apiVersion: nebula.inftyai.com/v1alpha1 +kind: NodePool +metadata: + name: %s +spec: + providers: + - name: %s + regions: + - us + - eu + capacityTypes: + - OnDemand + strategy: Ordered +`, fakeRegionPoolName, fakeProviderName) + + pod(regionServedPod, "eu") + + pod(regionUnreachablePod, "ap") + + pod(regionUnknownPod, "atlantis") + manifestFile := filepath.Join("/tmp", "nebula-fake-region-workload.yaml") + Expect(os.WriteFile(manifestFile, []byte(manifest), 0o644)).To(Succeed()) + _, err = utils.Run(exec.Command("kubectl", "apply", "-f", manifestFile)) + Expect(err).NotTo(HaveOccurred(), "Failed to apply the NodePool + Pods") + + By("verifying the served request lands in the requested geography's region") + // The pool allows us AND eu, and us sorts first in the walk — so eu-fake-1 can + // only be the narrowing at work, not the pool's own ordering. + verifyRegion := func(g Gomega) { + claim := fmt.Sprintf("%s-%s", fakeWorkloadNS, regionServedPod) + out, err := utils.Run(exec.Command("kubectl", "get", "nodeclaim", claim, + "-o", "jsonpath={.spec.region}")) + g.Expect(err).NotTo(HaveOccurred(), "NodeClaim not created") + g.Expect(out).To(Equal("eu-fake-1"), "claim records the wrong region") + } + Eventually(verifyRegion).Should(Succeed()) + + By("verifying that Pod is ungated and bound") + Eventually(func(g Gomega) { + out, err := utils.Run(exec.Command("kubectl", "get", "pod", regionServedPod, + "-n", fakeWorkloadNS, "-o", "jsonpath={.spec.nodeName}")) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(out).To(Equal(fakeVirtualNode), "Pod not bound to the fake virtual node") + }).Should(Succeed()) + + // Consistently, not Eventually: the assertion is that nothing happens. The + // served Pod above is already placed, so the controller has demonstrably run + // this pool — a still-gated Pod here is a decision, not a slow start. + for _, tc := range []struct{ pod, why string }{ + {regionUnreachablePod, "the pool reaches no region in the requested geography"}, + {regionUnknownPod, "the requested token is not a geography"}, + } { + By("verifying the Pod stays gated because " + tc.why) + Consistently(func(g Gomega) { + out, err := utils.Run(exec.Command("kubectl", "get", "pod", tc.pod, + "-n", fakeWorkloadNS, "-o", "jsonpath={.spec.schedulingGates[*].name}")) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(out).To(ContainSubstring("nebula.inftyai.com/provider-selection"), + "Pod was placed despite an unservable region request") + }, 15*time.Second, 3*time.Second).Should(Succeed()) + + claim := fmt.Sprintf("%s-%s", fakeWorkloadNS, tc.pod) + _, err := utils.Run(exec.Command("kubectl", "get", "nodeclaim", claim)) + Expect(err).To(HaveOccurred(), "a gated Pod must not own a NodeClaim") + } + + By("cleaning up the region workloads") + _, _ = utils.Run(exec.Command("kubectl", "delete", "-f", manifestFile, "--ignore-not-found=true")) + }) + It("should sync a batch of workloads within the time budget", Label("perf"), func() { // A benchmark, not a latency SLO: it scales one Deployment to N replicas and // reports how long the whole sync path takes per workload, asserting only a From 8e8921bb412f2686708b87379c4d03240e19d174 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 23 Sep 2026 15:21:03 +0100 Subject: [PATCH 2/4] address comments Signed-off-by: kerthcet --- cmd/main.go | 4 +- docs/metrics.md | 4 ++ .../controller/nodeclaim_controller_test.go | 26 ++++++++---- internal/controller/placement_metrics_test.go | 19 +++++++++ .../pod_placement_controller_test.go | 32 +++++++-------- internal/controller/pod_placement_helpers.go | 40 +++---------------- pkg/provider/aws/aws_test.go | 2 +- pkg/provider/modal/modal.go | 7 ++-- pkg/provider/modal/modal_test.go | 13 +++--- pkg/provider/provider.go | 9 +++-- 10 files changed, 83 insertions(+), 73 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index caa132a..a52e24e 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -678,8 +678,8 @@ func awsRegionSource(c client.Client) awsprovider.RegionSource { // 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. + // This is the same expansion selectPlacement applies through the + // adapter's ExpandRegions; both must agree, so both call this one function. regions = append(regions, awsprovider.ExpandRegions(ps.Regions)...) } } diff --git a/docs/metrics.md b/docs/metrics.md index 4c75f50..9591ced 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -77,6 +77,10 @@ The skip `reason` is likewise closed: `provider_unregistered`, `no_available_regions`, `blocked`. Only `blocked` clears on its own. One reconcile can file several skips — the walk visits every candidate before giving up. +`nebula_placement_deferrals_total` counts **deferrals, not Pods**: a gated Pod adds one on +every reconcile that fails to place it. Read the rate as placement pressure. For how many +Pods are stuck right now, count SchedulingGated Pods in kube-state-metrics. + ## Provisioning What the external call cost, and how it failed. diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index 6bc200b..ec933ca 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -18,6 +18,7 @@ package controller import ( "context" + "slices" "testing" "time" @@ -85,15 +86,26 @@ func (f *fakeProvider) MapAccelerator(c string, _ int32) ([]string, bool) { return nil, false } -// ExpandRegions passes the declaration through and fails closed on narrowing — the -// region-simple behaviour, as pkg/provider/fake has. Tests that need group expansion set -// expandRegions. +// ExpandRegions is region-simple like Modal: declared tokens are their own geographies, an +// unconstrained pool takes the narrowing as its constraint, and no declaration at all is +// one unpinned candidate. Tests that need group expansion set expandRegions. func (f *fakeProvider) ExpandRegions(declared, narrowTo []string) []string { - if len(narrowTo) > 0 { - return nil - } if f.expandRegions != nil { - return f.expandRegions(declared) + declared = f.expandRegions(declared) + } + switch { + case len(narrowTo) > 0 && len(declared) == 0: + return narrowTo + case len(narrowTo) > 0: + var out []string + for _, d := range declared { + if slices.Contains(narrowTo, d) { + out = append(out, d) + } + } + return out + case len(declared) == 0: + return []string{""} } return declared } diff --git a/internal/controller/placement_metrics_test.go b/internal/controller/placement_metrics_test.go index 53543d0..4e4cb71 100644 --- a/internal/controller/placement_metrics_test.go +++ b/internal/controller/placement_metrics_test.go @@ -32,6 +32,7 @@ import ( "github.com/InftyAI/Nebula/pkg/failover" "github.com/InftyAI/Nebula/pkg/metrics" "github.com/InftyAI/Nebula/pkg/provider" + awsprovider "github.com/InftyAI/Nebula/pkg/provider/aws" "github.com/InftyAI/Nebula/pkg/util" ) @@ -239,6 +240,24 @@ func TestPlacement_CandidateSkipReasons(t *testing.T) { } } +// Its own test, not a fourth provider above: the annotation would apply to every provider +// there. The real AWS adapter pins the real cause — "af" is opt-in-only, so an +// unconstrained pool still reaches no region. +func TestPlacement_NarrowingToNoRegionFilesNoAvailableRegions(t *testing.T) { + noRegions := skipLabels(provider.ProviderAWS, nebulav1alpha1.CapacityOnDemand, "", metrics.SkipNoAvailableRegions) + before := counterVal(t, metrics.CandidateSkips, noRegions) + + pod := gatedPod("r1", "default", "uid-r1", "pool", "") + pod.Annotations = map[string]string{nebulav1alpha1.RegionsAnnotation: "af"} + pool := poolWith("pool", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, provider.ProviderAWS) + r, _ := newPlacementReconciler(t, []client.Object{pod, pool}, awsprovider.New(nil, nil, nil)) + reconcilePod(t, r, "default", "r1") + + if got := counterVal(t, metrics.CandidateSkips, noRegions) - before; got != 1 { + t.Fatalf("no_available_regions skips delta = %v, want 1", got) + } +} + // The pool a Pod asks for is a Pod LABEL: user-controlled and unbounded. An unresolvable // one must never reach a metric label, or a mislabeled workload could mint a time series // per typo and blow up the registry. diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index 01adce6..c54ea6f 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -511,18 +511,6 @@ func TestPlacement_ExpandsRegionGroupIntoConcreteCandidates(t *testing.T) { } } -func TestRegionsFor_UnconstrainedOnRegionSimpleProviderYieldsOneCandidate(t *testing.T) { - // A region-simple provider passes nil through, 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}, nil) - if !slices.Equal(got, []string{""}) { - t.Fatalf("regionsFor(nil) = %v, want one empty candidate", got) - } -} - func TestRequestedGeographies(t *testing.T) { cases := []struct { name string @@ -623,6 +611,19 @@ func TestPlacement_NarrowingThatEliminatesEveryRegionLeavesPodGated(t *testing.T } } +func TestPlacement_DeclarationReachingNoRegionLeavesPodGated(t *testing.T) { + pod := gatedPod("p1", "default", "uid-1", "pool-a", "") + pool := poolWithRegions("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, + provider.ProviderAWS, "af") + r, c := newPlacementReconciler(t, []client.Object{pod, pool}, awsprovider.New(nil, nil, nil)) + + reconcilePod(t, r, "default", "p1") + + if got := getPod(t, c, "default", "p1"); !hasGateNamed(got) { + t.Fatal("expected the Pod to stay gated when the pool's declaration reaches no region") + } +} + func TestPlacement_UnresolvableRegionAnnotationLeavesPodGated(t *testing.T) { // A typo is an invalid request, not an empty narrowing: the Pod stays gated until a // human fixes it. Placing it would mean ignoring a residency constraint. @@ -640,15 +641,14 @@ func TestPlacement_UnresolvableRegionAnnotationLeavesPodGated(t *testing.T) { } } -func TestRegionsFor_AgreesWithAWSSweepExpansion(t *testing.T) { - // The two readers of ProviderSpec.Regions — placement's regionsFor and the AWS +func TestPlacementExpansion_AgreesWithAWSSweep(t *testing.T) { + // The two readers of ProviderSpec.Regions — selectPlacement 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}, nil) + placementSide := awsprovider.New(nil, nil, nil).ExpandRegions(declared, nil) sweepSide := awsprovider.ExpandRegions(declared) if !slices.Equal(placementSide, sweepSide) { t.Errorf("declared %v: placement walks %v but the sweep covers %v", diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index b600fd2..595a6eb 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -106,8 +106,8 @@ func (r *PodPlacementReconciler) selectPlacement(ctx context.Context, pod *corev return placement{}, false, 0 } - // The Pod's region ask, read ONCE: regionsFor runs per provider ref, so parsing inside it - // would repeat this deferral log for every provider in the pool. + // The Pod's region ask, read ONCE: region expansion runs per provider ref, so parsing + // there would repeat this deferral log for every provider in the pool. narrowTo, ok := requestedGeographies(pod) if !ok { metrics.RecordDeferral(pool.Name, metrics.DeferInvalidRequest) @@ -155,9 +155,10 @@ func (r *PodPlacementReconciler) selectPlacement(ctx context.Context, pod *corev continue } } - // Empty can only mean the Pod's narrowing eliminated every region this - // provider reaches: without one, regionsFor always yields a candidate. - regions := regionsFor(prov, ref, narrowTo) + // Empty means the pool's declaration, or the Pod's narrowing of it, reaches + // no region this provider can place in. The expansion must match + // awsRegionSource's (cmd/main.go); see its comment. + regions := prov.ExpandRegions(ref.Regions, narrowTo) if len(regions) == 0 { metrics.RecordCandidateSkip(ref.Name, tier, "", metrics.SkipNoAvailableRegions) log.V(1).Info("skipping candidate: no available region serves the requested geographies", @@ -262,35 +263,6 @@ func servesEgress(prov provider.Provider, policy *nebulav1alpha1.EgressPolicy) b return prov.Capabilities().SupportsEgressPolicy } -// 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). -// -// narrowTo is the Pod's own ask (RegionsAnnotation), which subsets that expansion. -// -// The empty-string fallback covers expansion yielding nothing: a region-simple provider -// whose pool declared no regions still needs ONE candidate, or `range` runs zero times and -// the provider is silently unplaceable. That candidate means "send no region, place freely" — -// Modal's normal and cheapest mode. -// -// It is gated on there being NO narrowing, and that guard is load-bearing: under a narrowing -// an empty expansion means the intersection eliminated every region, so falling back to "" -// would place a Pod that asked for "us" anywhere on earth. Empty means no candidate there, -// and the caller skips the provider. -// -// This and awsRegionSource (cmd/main.go) are the only readers of ProviderSpec.Regions and -// MUST expand it identically: a region provisioned into but not swept is absent from List, -// and absence is reported as Terminated on a live, billing instance. -func regionsFor(prov provider.Provider, ref nebulav1alpha1.ProviderSpec, narrowTo []string) []string { - regions := prov.ExpandRegions(ref.Regions, narrowTo) - if len(regions) == 0 && len(narrowTo) == 0 { - return []string{""} // unconstrained on a region-simple provider - } - return regions -} - // requestedGeographies reads the Pod's RegionsAnnotation into the narrowing ExpandRegions // takes. Absent means no narrowing, the common case. func requestedGeographies(pod *corev1.Pod) (narrowTo []string, ok bool) { diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index f1a72f4..94d6d43 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -795,7 +795,7 @@ func TestExpandRegions_NarrowToTakesVocabularyOnly(t *testing.T) { regionsByGeography["jp"] = []string{"ap-northeast-1"} defer delete(regionsByGeography, "jp") - if got := narrowRegions(ExpandRegions(nil), []string{"jp"}); got != nil { + if got := narrowRegions(ExpandRegions(nil), []string{"jp"}); len(got) != 0 { t.Errorf("narrowTo [jp] resolved to %v; only provider.Geographies tokens may narrow", got) } } diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 9d3e888..fda78da 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -386,10 +386,11 @@ func (p *Provider) ExpandRegions(declared, narrowTo []string) []string { regions := dedupeRegions(declared) if len(narrowTo) > 0 { regions = dedupeRegions(narrowRegions(regions, narrowTo)) + if len(regions) == 0 { + return nil + } } - if len(regions) == 0 { - return nil - } + // With no declaration this joins to "": unpinned, Modal's widest and cheapest candidate. return []string{strings.Join(regions, regionSeparator)} } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 3da4728..0db9e9c 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -917,15 +917,14 @@ func TestExpandRegions_CollapsesToOneCandidate(t *testing.T) { declared []string want []string }{{ - name: "no declaration stays unconstrained", + // [""], not nil: nil is no candidate, and placement would skip Modal entirely. + name: "no declaration is one unpinned candidate", declared: nil, - want: nil, + want: []string{""}, }, { - // 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", + name: "a declaration of only blanks is unconstrained, not a blank region", declared: []string{"", " "}, - want: nil, + want: []string{""}, }, { name: "a single region is one candidate holding it", declared: []string{"us"}, @@ -1050,7 +1049,7 @@ func TestExpandRegions_NarrowToTakesVocabularyOnly(t *testing.T) { defer delete(regionsByGeography, "jp") p := newTestProvider(&fakeClient{}) - if got := p.ExpandRegions(nil, []string{"jp"}); got != nil { + if got := p.ExpandRegions(nil, []string{"jp"}); len(got) != 0 { t.Errorf("narrowTo [jp] resolved to %v; only provider.Geographies tokens may narrow", got) } } diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 67f72e8..95a15fc 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -123,6 +123,9 @@ type Provider interface { // - anything else => a literal region name, passed through UNVALIDATED. // // Narrowing with narrowTo restricts the result to regions within the specified geographies. + // + // An empty result means no candidate: placement skips this provider. A provider that + // can place without a region returns [""] for it — one candidate, unpinned. ExpandRegions(declared, narrowTo []string) []string // ClassifyProvisionError maps a Provision error to the granularity at which @@ -215,9 +218,9 @@ type ProvisionRequest struct { // capacity failure blocklist just that region; a provider that cannot fail over // (Modal) may encode several for its own scheduler, and only that adapter parses it. // - // Empty means "no region constraint" — common, not a fallback: a pool declaring no - // regions leaves it empty, which on Modal is the widest and cheapest option (pinning - // costs 1.5-1.75x). AWS cannot honour it, but its ExpandRegions never produces it. + // Empty means "no region constraint", and arrives only if this provider's ExpandRegions + // returned [""] — so an adapter that cannot honour it never sees it. On Modal it is the + // widest and cheapest option (pinning costs 1.5-1.75x). Region string // Egress is the pool's outbound policy, or nil for Open. Placement has already checked // that this provider can enforce it (Capabilities.SupportsEgressPolicy), so an adapter From 37f483aaff4fa2d4770838b089d10a45eaf8a3ce Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 23 Sep 2026 15:25:57 +0100 Subject: [PATCH 3/4] update Signed-off-by: kerthcet --- docs/metrics.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/metrics.md b/docs/metrics.md index 9591ced..4c75f50 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -77,10 +77,6 @@ The skip `reason` is likewise closed: `provider_unregistered`, `no_available_regions`, `blocked`. Only `blocked` clears on its own. One reconcile can file several skips — the walk visits every candidate before giving up. -`nebula_placement_deferrals_total` counts **deferrals, not Pods**: a gated Pod adds one on -every reconcile that fails to place it. Read the rate as placement pressure. For how many -Pods are stuck right now, count SchedulingGated Pods in kube-state-metrics. - ## Provisioning What the external call cost, and how it failed. From b3f5cf99c6565e4c5436627339a17268c141f097 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 23 Sep 2026 15:38:26 +0100 Subject: [PATCH 4/4] polish comment Signed-off-by: kerthcet --- docs/architecture.md | 13 ++++--------- pkg/provider/aws/aws.go | 6 +----- pkg/provider/provider.go | 4 +--- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 1678f75..cddbe3a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -50,15 +50,10 @@ mapping), see [docs/status.md](status.md). **Non-goals in the current implementation** -- Uniform geographic coverage. `provider.Geographies` is a flat, provider-neutral - vocabulary of broad tokens (`us`, `eu`, `ap`, `uk`, `ca`, `me`, `sa`, `af`, `mx`) - and that is the whole shared namespace — a provider's own region names are the - second level and are never vocabulary. -- Regions a provider's geography table does not list. That table is the only authority - on which geography holds which region, so such a region is reachable only by naming - it literally in the NodePool, never through a Pod's `regions` annotation. AWS's - opt-in regions are left out on purpose: EC2 answers `OptInRequired`, which classifies - as an auth failure and would blocklist the whole provider. +- Uniform geographic coverage. Regions come in two levels: broad geographies + (`provider.Geographies`: `us`, `eu`, `ap`, `uk`, `ca`, `me`, `sa`, `af`, `mx`) and a + provider's own region names (`us-east-1`). A NodePool may declare either; a Pod's + `regions` annotation takes geographies only. - 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 diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index 1571c56..dd6c72f 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -72,11 +72,7 @@ const provisionTimeout = 2 * time.Minute // regionsByGeography maps a geography token to the AWS regions it encompasses. // -// The empty entries are the opt-in regions, left out on purpose: EC2 answers -// OptInRequired for an account that has not enabled one, which translate.go maps to -// ErrAuth and so blocklists the WHOLE provider. A pool can still name such a region -// literally, but narrowing intersects this table — so it cannot be reached through the -// regions annotation. +// The empty entries are the opt-in regions, disabled by default in AWS accounts. var regionsByGeography = map[string][]string{ "us": {"us-east-1", "us-east-2", "us-west-1", "us-west-2"}, "ca": {"ca-central-1"}, diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 95a15fc..aad2b6e 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -218,9 +218,7 @@ type ProvisionRequest struct { // capacity failure blocklist just that region; a provider that cannot fail over // (Modal) may encode several for its own scheduler, and only that adapter parses it. // - // Empty means "no region constraint", and arrives only if this provider's ExpandRegions - // returned [""] — so an adapter that cannot honour it never sees it. On Modal it is the - // widest and cheapest option (pinning costs 1.5-1.75x). + // Empty means "no region constraint". Region string // Egress is the pool's outbound policy, or nil for Open. Placement has already checked // that this provider can enforce it (Capabilities.SupportsEgressPolicy), so an adapter