From d9d9ed6d0c1dfbd92805f5522df5d070ff35ad2f Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 29 Jul 2026 15:25:34 +0100 Subject: [PATCH] Support better provisioning in aws Signed-off-by: kerthcet --- api/v1alpha1/groupversion_info.go | 11 + api/v1alpha1/nodeclaim_types.go | 36 +- cmd/main.go | 63 ++- .../bases/nebula.inftyai.com_nodeclaims.yaml | 6 + config/manager/manager.yaml | 12 +- config/samples/deployment.yaml | 4 +- .../samples/nebula_v1alpha1_nodepool_aws.yaml | 20 +- internal/controller/nodeclaim_controller.go | 124 ++--- .../controller/nodeclaim_controller_test.go | 52 +- .../controller/pod_placement_controller.go | 35 +- .../pod_placement_controller_test.go | 94 +++- internal/controller/pod_placement_helpers.go | 94 +++- pkg/failover/blocklist.go | 91 +++- pkg/failover/blocklist_test.go | 138 ++++- pkg/provider/aws/aws.go | 507 ++++++++++++++---- pkg/provider/aws/aws_test.go | 80 ++- pkg/provider/aws/client.go | 101 +++- pkg/provider/aws/translate.go | 15 +- pkg/provider/catalog/base.go | 45 +- pkg/provider/catalog/catalog_test.go | 2 +- pkg/provider/catalog/data/aws.csv | 2 + pkg/provider/errors.go | 32 +- pkg/provider/errors_test.go | 10 +- pkg/provider/fake/fake.go | 3 +- pkg/provider/fake/fake_test.go | 4 +- pkg/provider/modal/modal.go | 6 +- pkg/provider/modal/modal_test.go | 2 +- pkg/provider/provider.go | 58 +- pkg/util/claim.go | 52 +- pkg/util/claim_test.go | 81 +++ pkg/vnode/handler.go | 260 ++++++++- pkg/vnode/handler_test.go | 222 +++++++- pkg/vnode/node.go | 2 +- pkg/vnode/status.go | 28 +- 34 files changed, 1834 insertions(+), 458 deletions(-) create mode 100644 pkg/util/claim_test.go diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go index 5abce62..f756132 100644 --- a/api/v1alpha1/groupversion_info.go +++ b/api/v1alpha1/groupversion_info.go @@ -100,6 +100,17 @@ const ( // default TTL. BlocklistTTLAnnotation = "nebula.inftyai.com/blocklist-ttl" + // EndpointAnnotation carries the reachable address of the external instance + // once it is running (a public DNS name or IP, in the provider's own form). + // It is the ONLY way to reach the workload, so it must be visible on the Pod: + // PodIP cannot hold it because the API server validates PodIP as a literal IP + // and rejects a DNS name (the common AWS case), so the endpoint rides an + // annotation instead. Written by the virtual kubelet when it first observes the + // instance running; absent until then. Unlike the provisioning-input + // annotations above (which the placement controller stamps and VK reads), this + // flows the other way — VK writes it for operators/tooling to read. + EndpointAnnotation = "nebula.inftyai.com/endpoint" + // TerminateInstanceFinalizer is held by every NodeClaim to guarantee teardown. // The virtual kubelet owns the happy path (DeletePod → provider.Terminate, // keyed on the Pod-derived claim name), but its teardown is edge-triggered and diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index a76ca9e..2192bfb 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -59,23 +59,35 @@ type PodReference struct { // // The NodeClaim is a passive teardown ledger, not a status mirror: it does NOT // track finer workload runtime status (CPU/logs/restarts) — the Pod is the -// source of truth for that (see pkg/vnode/status.go). It tracks only the three -// coarse states that matter to its own job as a ledger, keyed off the served -// Pod's phase: Provisioning (instance coming up), Bound (instance running — the -// guard the teardown backstop trusts), and Terminated (instance gone). Finer -// states (e.g. Preempted) are deliberately absent: preemption cannot be detected -// — the provider contract's InstanceState has no Preempted value, and an absent -// instance only tells us it is gone, not why. Reintroduce a phase only when -// something actually sets it. +// source of truth for that (see pkg/vnode/status.go). It tracks only the coarse +// states that matter to its own job as a ledger, keyed off the served Pod's +// phase/reason: Provisioning (allocating — instance does not exist yet), +// Initializing (instance exists and is booting but not yet reachable), Bound +// (instance running — the guard the teardown backstop trusts), and Terminated +// (instance gone). Finer states (e.g. Preempted) are deliberately absent: +// preemption cannot be detected — the provider contract's InstanceState has no +// Preempted value, and an absent instance only tells us it is gone, not why. +// Reintroduce a phase only when something actually sets it. +// +// Only Bound is a teardown guard: neither Provisioning nor Initializing earns the +// "trust a later disappearance" trust, because until the instance is confirmed up +// an absent Pod may be cache lag rather than a real teardown. type NodeClaimPhase string const ( - // NodeClaimProvisioning: the served Pod has been observed but is not yet - // running — the external instance is still being provisioned. The claim does - // NOT earn the Bound teardown guard here: a Pod that vanishes while still + // NodeClaimProvisioning: the served Pod has been observed but the external + // instance does not yet exist — provisioning is still allocating it. The claim + // does NOT earn the Bound teardown guard here: a Pod that vanishes while still // provisioning is treated as possible cache lag (grace window), not a real // teardown, because we never confirmed the instance was actually up. NodeClaimProvisioning NodeClaimPhase = "Provisioning" + // NodeClaimInitializing: the external instance EXISTS at the provider but is not + // yet reachable (e.g. EC2 is "pending", or "running" but its 2/2 status checks + // have not passed). The served Pod is Pending with reason Initializing (see + // pkg/vnode/status.go). Like Provisioning it does NOT earn the Bound guard — the + // instance is not yet confirmed up — but it is surfaced as a distinct phase so + // "allocating" and "booting" are distinguishable on the ledger. + NodeClaimInitializing NodeClaimPhase = "Initializing" // NodeClaimBound: the served Pod has been observed running (present and not in // a terminal phase). This is the durable guard the backstop trusts — a Bound // claim whose Pod later disappears is a real teardown, not cache lag. The claim @@ -116,7 +128,9 @@ type NodeClaimStatus struct { // +kubebuilder:resource:scope=Cluster,shortName=nc // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Provider",type=string,JSONPath=`.spec.provider` +// +kubebuilder:printcolumn:name="Region",type=string,JSONPath=`.spec.region` // +kubebuilder:printcolumn:name="Instance",type=string,JSONPath=`.status.instanceID` +// +kubebuilder:printcolumn:name="CapacityType",type=string,JSONPath=`.spec.capacityType` // +kubebuilder:printcolumn:name="Phase",type=string,JSONPath=`.status.phase` // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` diff --git a/cmd/main.go b/cmd/main.go index 13b5bd0..d70e105 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -32,6 +32,7 @@ import ( clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/certwatcher" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" @@ -218,8 +219,10 @@ func main() { // Register provider backends into the process-wide registry that both // reconcilers resolve through (their Providers field defaults to // provider.Get). Done before SetupWithManager so a pool/claim reconciled at - // startup already sees its provider. - registerProviders(context.Background()) + // startup already sees its provider. The manager's client backs the AWS region + // source (regions are read from NodePools at call time, not env), so it is + // threaded in; the client is only queried at runtime, after the cache has synced. + registerProviders(context.Background(), mgr.GetClient()) // One shared failover blocklist, written by the virtual kubelet handlers on a // Provision failure and read by the placement controller to skip a candidate @@ -329,7 +332,7 @@ func setupVirtualNodes(mgr ctrl.Manager, blocklist vnode.Blocklister) error { // still run for the providers that ARE configured, and a pool referencing an // unregistered provider surfaces as a clear NodePool condition rather than a // crash loop. -func registerProviders(ctx context.Context) { +func registerProviders(ctx context.Context, c client.Client) { if p, err := modal.NewSDKClient(ctx, os.Getenv("MODAL_APP_NAME")); err != nil { setupLog.Info("skipping Modal provider registration", "reason", err.Error()) } else { @@ -337,15 +340,18 @@ func registerProviders(ctx context.Context) { setupLog.Info("registered provider", "provider", p.Name()) } - // AWS. Region is the only NON-SECRET config, read from AWS_REGION (empty => the - // SDK resolves its default from shared config / IMDS). The adapter is otherwise - // self-configuring: it resolves the GPU AMI and default-VPC subnets itself, so - // no launch template or pre-created infra is needed. Credentials are secrets and - // are NEVER read here: the SDK client uses the default credential chain (IRSA / - // instance-role / AWS_ACCESS_KEY_ID delivered via a Secret), so nothing - // sensitive touches a flag or this call. When AWS is not configured (no - // resolvable region, no GPU AMI in the region), registration is a non-fatal skip. - if p, err := awsprovider.NewSDKClient(ctx, os.Getenv("AWS_REGION")); err != nil { + // AWS. There is NO region env/flag: the regions this provider may use are declared + // per-pool in the NodePool (ProviderSpec.Regions) and read at call time via the + // region source below, so a pool added at runtime widens the fan-out without a + // restart. One AWS provider spans every such region (per-region clients are built + // lazily). The adapter is otherwise self-configuring: it resolves each region's + // GPU AMI and default-VPC subnets itself, so no launch template or pre-created + // infra is needed. Credentials are secrets and are NEVER read here: the SDK client + // uses the default credential chain (IRSA / instance-role / AWS_ACCESS_KEY_ID + // delivered via a Secret), and one account-global credential authorizes every + // region. Registration only fails (and is a non-fatal skip) if the price catalog + // cannot load — region config can no longer make it fail. + if p, err := awsprovider.NewSDKClient(ctx, awsRegionSource(c)); err != nil { setupLog.Info("skipping AWS provider registration", "reason", err.Error()) } else { provider.Register(p) @@ -362,3 +368,36 @@ func registerProviders(ctx context.Context) { setupLog.Info("registered provider", "provider", p.Name()) } } + +// awsRegionSource returns the AWS adapter's RegionSource: the union of +// ProviderSpec.Regions across every NodePool that references the "aws" provider. It +// needs no env/flag — regions are the operator's per-pool declaration — and a pool +// added/edited at runtime widens the swept set on the next List tick, no restart +// required. +// +// It is evaluated on each List/Offerings tick. The underlying List is served from +// the manager's informer cache (no API call), so scanning the pools per tick is +// cheap even though it is O(pools); sweepRegions dedupes the result. On a list error +// (cache not yet synced at startup, a transient failure) it returns nil and +// sweepRegions falls back to the regions already provisioned into. It uses a +// background context, not the registration ctx, since it runs long after +// registration returns. +func awsRegionSource(c client.Client) awsprovider.RegionSource { + return func() []string { + var pools nebulav1alpha1.NodePoolList + if err := c.List(context.Background(), &pools); err != nil { + setupLog.V(1).Info("aws region source: list NodePools failed; sweeping provisioned regions only", + "reason", err.Error()) + return nil + } + var regions []string + for i := range pools.Items { + for _, ps := range pools.Items[i].Spec.Providers { + if ps.Name == provider.ProviderAWS { + regions = append(regions, ps.Regions...) + } + } + } + return regions + } +} diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index d3bc033..595a39a 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -20,9 +20,15 @@ spec: - jsonPath: .spec.provider name: Provider type: string + - jsonPath: .spec.region + name: Region + type: string - jsonPath: .status.instanceID name: Instance type: string + - jsonPath: .spec.capacityType + name: CapacityType + type: string - jsonPath: .status.phase name: Phase type: string diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 0a16800..f8b02c0 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -72,14 +72,6 @@ spec: # embedded in the binary. - name: NEBULA_CATALOG_DIR value: /etc/nebula/catalog - # AWS NON-SECRET config (never a credential): the region to operate in. - # The adapter is otherwise self-configuring — it resolves the GPU AMI and - # the default VPC's subnets itself — so no launch template or other infra - # config is needed. This is a plain value here, NOT in a Secret. Empty => - # the SDK resolves its default region (shared config / IMDS); with no - # resolvable region AWS registration is a non-fatal skip. - - name: AWS_REGION - value: "us-east-1" envFrom: # Provider credentials live in a per-provider Secret, one secretRef per # provider — NOT a single shared secret. This matches the "creds-absent → @@ -96,8 +88,8 @@ spec: # AWS credentials (AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY), read by # the SDK's default chain. optional=true so an absent Secret is fine — # in production this is the norm: prefer IRSA / instance role and skip - # this Secret entirely. Non-secret region/launch-template config is set - # as plain env above, never here. + # this Secret entirely. There is no non-secret AWS env: regions come from + # the NodePool, so this Secret is the ONLY AWS config here. - secretRef: name: nebula-aws-credentials optional: true diff --git a/config/samples/deployment.yaml b/config/samples/deployment.yaml index 30efdcc..5a59017 100644 --- a/config/samples/deployment.yaml +++ b/config/samples/deployment.yaml @@ -23,7 +23,7 @@ apiVersion: apps/v1 kind: Deployment metadata: - name: gpu-workload-sample + name: gpu-workload-sample-l4-8 namespace: default labels: app.kubernetes.io/managed-by: nebula @@ -65,4 +65,4 @@ spec: # GPU count. Standard extended resource, so the scheduler's fit check # and provisioning read the same number. 8 => 8x the accelerator-type # above. - nvidia.com/gpu: "1" + nvidia.com/gpu: "8" diff --git a/config/samples/nebula_v1alpha1_nodepool_aws.yaml b/config/samples/nebula_v1alpha1_nodepool_aws.yaml index 46740ea..87400d7 100644 --- a/config/samples/nebula_v1alpha1_nodepool_aws.yaml +++ b/config/samples/nebula_v1alpha1_nodepool_aws.yaml @@ -6,10 +6,10 @@ metadata: name: aws spec: # Place onto AWS EC2. The provider name must match the ProviderLabel value on - # the AWS virtual node ("aws"); the adapter registers whenever the manager has a - # resolvable region (AWS_REGION or the SDK default chain) and credentials (IRSA / - # instance role / keys) — it self-configures the GPU AMI and subnets, so no other - # setup is needed. Otherwise the pool reports Ready=False/UnknownProvider. + # the AWS virtual node ("aws"); the adapter registers whenever the manager has + # credentials (IRSA / instance role / keys) — no region env is needed, the regions + # come from the `regions` field below. It self-configures the GPU AMI and subnets, + # so no other setup is needed. Otherwise the pool reports Ready=False/UnknownProvider. providers: - name: aws # regions is per-provider and in AWS's OWN vocabulary. Unlike region-simple @@ -19,17 +19,23 @@ spec: # are actually offered is resolved by the live probe. regions: - us-east-1 - # - us-west-2 + - us-west-1 + - ap-south-1 + - ap-northeast-1 + - eu-central-1 + - eu-west-1 + - ca-central-1 + - sa-east-1 # Outer axis: prefer cheap interruptible Spot, fall back to OnDemand when Spot # capacity is exhausted. (EC2 has a real Spot tier with a ~2-minute notice; a # Spot no-capacity failure blocks only Spot in that region, so OnDemand is still # tried.) Reserved is omitted — EC2 Reserved is a billing construct, not a # distinct provisioning path here. capacityTypes: - - Spot - OnDemand + - Spot # Inner axis: within the active capacity tier. A single-provider pool makes this # a no-op, but LowestPrice is the natural choice once more AWS regions are listed. - strategy: LowestPrice + strategy: Ordered failover: blocklistTTL: 10m diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index de8edf8..902b47f 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -53,6 +53,14 @@ import ( // because we KNOW the Pod existed and is now gone. const placementGracePeriod = 15 * time.Second +// podReasonInitializing is the Pod status.Reason the virtual node stamps while the +// external instance exists but is not yet reachable (see pkg/vnode/status.go +// reasonInitializing). The claim keys off it to distinguish Initializing from +// Provisioning, since both share the Pending phase. Kept in sync with vnode's +// value by hand — it is a stable, user-facing reason string, not worth an exported +// package coupling. +const podReasonInitializing = "Initializing" + // NodeClaimReconciler reconciles a NodeClaim object. // // Ownership model: the virtual kubelet owns the instance lifecycle — its pod @@ -125,27 +133,12 @@ func (r *NodeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if pod != nil { // The workload is present. We do not mirror the Pod's fine-grained runtime - // status, but we DO reflect the one coarse transition that matters to the - // ledger: the external instance going away. VK reports a vanished instance - // as a terminal Pod phase (Failed/Succeeded, see pkg/vnode/status.go). A Pod - // with restartPolicy: Never lingers in that terminal phase rather than being - // deleted, so keying only off Pod existence would wedge the claim at Bound - // forever — reflect Terminated instead. - if isTerminal(pod.Status.Phase) { - return ctrl.Result{}, r.markTerminated(ctx, &nc) - } - // The Pod exists but is not yet running (VK is still provisioning the - // instance). Do NOT mark Bound yet: Bound is the guard that makes a later - // disappearance trustworthy, and we only earn that trust once the instance - // is actually up. Record the instance id early (it exists the moment - // Provision returned) and reflect Provisioning; the Pod watch re-enqueues us - // when it transitions to Running. - if pod.Status.Phase != corev1.PodRunning { - return ctrl.Result{}, r.markProvisioning(ctx, &nc) - } - // Running: the instance is up. Record that we have observed it placed (this - // is the guard that makes a later disappearance trustworthy) and stop. - return ctrl.Result{}, r.markBound(ctx, &nc) + // status, but we DO reflect the coarse phase that matters to the ledger. + // desiredPhase maps the served Pod onto the claim phase (Terminated / Bound / + // Initializing / Provisioning); markPhase persists it (and the instance id) + // idempotently. An empty desiredPhase means "hold the current phase, just keep + // the id fresh" — the Bound-flap guard (see desiredPhase). + return ctrl.Result{}, r.markPhase(ctx, &nc, r.desiredPhase(&nc, pod)) } // The served Pod is absent. Decide whether this is a real teardown or a @@ -277,61 +270,48 @@ func (r *NodeClaimReconciler) provider(name string) (provider.Provider, bool) { return provider.Get(name) } -// markBound records that the served Pod has been observed running. This is the -// persisted guard the self-delete path trusts: a claim in phase Bound whose Pod -// later disappears is a real teardown, whereas a claim that never reached Bound -// is treated as possible cache lag. It does NOT copy the Pod's fine-grained -// runtime status — the Pod is the source of truth for that — but it does record -// status.InstanceID: the provider id is the one datum that must not be lost, so -// the teardown backstop can reclaim the instance even if VK (which otherwise -// holds the id only in memory) has died. The id is resolved once, best-effort; -// if the provider List is unavailable the Bound transition still proceeds and -// the backstop falls back to re-deriving the id by claim name. Idempotent. - -// markProvisioning records that the served Pod exists but is not yet running. -// The claim sits in phase Provisioning (it never earns the Bound teardown guard -// while the instance is still coming up), but we capture status.InstanceID as -// soon as it is resolvable so the backstop can reclaim a half-provisioned -// instance. Idempotent. -func (r *NodeClaimReconciler) markProvisioning(ctx context.Context, nc *nebulav1alpha1.NodeClaim) error { - changed := false - if nc.Status.Phase != nebulav1alpha1.NodeClaimProvisioning { - nc.Status.Phase = nebulav1alpha1.NodeClaimProvisioning - changed = true - } - if r.recordInstanceID(ctx, nc) { - changed = true - } - if !changed { - return nil - } - return r.patchStatus(ctx, nc) -} - -func (r *NodeClaimReconciler) markBound(ctx context.Context, nc *nebulav1alpha1.NodeClaim) error { - changed := false - if nc.Status.Phase != nebulav1alpha1.NodeClaimBound { - nc.Status.Phase = nebulav1alpha1.NodeClaimBound - changed = true - } - if r.recordInstanceID(ctx, nc) { - changed = true - } - if !changed { - return nil +// desiredPhase maps a present served Pod onto the claim phase to record. The +// claim does NOT mirror the Pod's fine-grained runtime status — it tracks only +// the coarse lifecycle its teardown job needs: +// +// - Terminal Pod (Failed/Succeeded) => Terminated. VK reports a vanished +// instance this way; a restartPolicy:Never Pod lingers terminal rather than +// being deleted, so keying off existence alone would wedge the claim at Bound. +// - Bound already => "" (hold). Bound is the durable teardown guard, and a +// transient Running->Pending flap (e.g. a status-check blip) must NOT strip it +// — losing it would make a later disappearance read as cache lag and skip +// teardown. Returning "" holds the phase while markPhase still refreshes the id. +// - Running => Bound. The instance is confirmed up; earn the guard. +// - Pending with reason Initializing => Initializing (instance exists, booting). +// - Otherwise => Provisioning (still allocating; instance does not exist yet). +// +// Neither Initializing nor Provisioning earns the Bound guard. +func (r *NodeClaimReconciler) desiredPhase(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) nebulav1alpha1.NodeClaimPhase { + switch { + case isTerminal(pod.Status.Phase): + return nebulav1alpha1.NodeClaimTerminated + case r.wasBound(nc): + return "" // hold Bound (or Terminated); never downgrade + case pod.Status.Phase == corev1.PodRunning: + return nebulav1alpha1.NodeClaimBound + case pod.Status.Reason == podReasonInitializing: + return nebulav1alpha1.NodeClaimInitializing + default: + return nebulav1alpha1.NodeClaimProvisioning } - return r.patchStatus(ctx, nc) } -// markTerminated records that the external instance is gone (the served Pod -// reached a terminal phase). Terminal, so once set it never advances. Like -// markBound it best-effort records status.InstanceID for the backstop; the id is -// still worth capturing here because the instance may already be gone from the -// provider but the finalizer path is independent. Idempotent. -func (r *NodeClaimReconciler) markTerminated(ctx context.Context, nc *nebulav1alpha1.NodeClaim) error { +// markPhase persists the claim's coarse phase and best-effort records +// status.InstanceID, in one idempotent status write. An empty phase means "leave +// the phase as-is" (the Bound-flap hold, see desiredPhase); the id is still +// refreshed. The provider id is the one datum that must not be lost — the teardown +// backstop reclaims the instance by it even if VK (which otherwise holds it only +// in memory) has died — so it is captured as soon as it resolves, regardless of +// phase. A no-op (phase unchanged and id already set) writes nothing. +func (r *NodeClaimReconciler) markPhase(ctx context.Context, nc *nebulav1alpha1.NodeClaim, phase nebulav1alpha1.NodeClaimPhase) error { changed := false - if nc.Status.Phase != nebulav1alpha1.NodeClaimTerminated { - nc.Status.Phase = nebulav1alpha1.NodeClaimTerminated + if phase != "" && nc.Status.Phase != phase { + nc.Status.Phase = phase changed = true } if r.recordInstanceID(ctx, nc) { diff --git a/internal/controller/nodeclaim_controller_test.go b/internal/controller/nodeclaim_controller_test.go index 9dbe953..ab93452 100644 --- a/internal/controller/nodeclaim_controller_test.go +++ b/internal/controller/nodeclaim_controller_test.go @@ -62,7 +62,7 @@ func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { return f.list, f.listErr } func (f *fakeProvider) Offerings(context.Context) ([]provider.Offering, error) { return nil, nil } -func (f *fakeProvider) MapAccelerator(c string) (string, bool) { +func (f *fakeProvider) MapAccelerator(c string, _ int32) (string, bool) { if f.gpus == nil { return c, true // offer any accelerator } @@ -73,7 +73,7 @@ func (f *fakeProvider) MapAccelerator(c string) (string, bool) { } return "", false } -func (f *fakeProvider) ClassifyProvisionError(error, string) provider.BlockScope { +func (f *fakeProvider) ClassifyProvisionError(error, string, string) provider.BlockScope { return provider.BlockScope{} } @@ -233,6 +233,54 @@ func TestReconcile_PendingPodDoesNotMarkBound(t *testing.T) { } } +func TestReconcile_InitializingPodMarksInitializing(t *testing.T) { + // A served Pod that is Pending with reason Initializing (instance exists and is + // booting, e.g. EC2 running but <2/2 checks) must move the claim to the distinct + // Initializing phase — NOT Provisioning (which means still allocating) and NOT + // Bound (the guard is earned only when running). + pod := newPod("p1", "default", "uid-1", corev1.PodPending) + pod.Status.Reason = podReasonInitializing + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + prov := &fakeProvider{ + name: "fake", + list: []provider.Instance{{ID: "inst-1", ClaimName: "default-p1"}}, + } + r, c := newClaimReconciler(t, []client.Object{pod, claim}, prov) + + reconcileClaim(t, r, "c1") + + got := getClaim(t, c, "c1") + if got.Status.Phase != nebulav1alpha1.NodeClaimInitializing { + t.Fatalf("expected phase Initializing, got %q", got.Status.Phase) + } + if got.Status.InstanceID != "inst-1" { + t.Fatalf("expected instance id captured, got %q", got.Status.InstanceID) + } +} + +func TestReconcile_BoundClaimDoesNotDowngradeOnStatusFlap(t *testing.T) { + // A claim already Bound whose Pod briefly drops back to Pending (a status-check + // flap surfacing as reason Initializing) must NOT downgrade: Bound is the durable + // teardown guard, and losing it would make a later disappearance read as cache + // lag and skip teardown. The phase holds at Bound. + pod := newPod("p1", "default", "uid-1", corev1.PodPending) + pod.Status.Reason = podReasonInitializing + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + claim.Status.Phase = nebulav1alpha1.NodeClaimBound + prov := &fakeProvider{ + name: "fake", + list: []provider.Instance{{ID: "inst-1", ClaimName: "default-p1"}}, + } + r, c := newClaimReconciler(t, []client.Object{pod, claim}, prov) + + reconcileClaim(t, r, "c1") + + got := getClaim(t, c, "c1") + if got.Status.Phase != nebulav1alpha1.NodeClaimBound { + t.Fatalf("Bound must not downgrade on a transient flap, got %q", got.Status.Phase) + } +} + func TestReconcile_RecordsInstanceIDOnBound(t *testing.T) { // When the served Pod is running, the claim is marked Bound AND its // status.InstanceID is captured (resolved by claim name via List) so the diff --git a/internal/controller/pod_placement_controller.go b/internal/controller/pod_placement_controller.go index 5033dcb..09f8383 100644 --- a/internal/controller/pod_placement_controller.go +++ b/internal/controller/pod_placement_controller.go @@ -38,6 +38,16 @@ import ( // bounded retry is enough. const staleClaimRequeue = 5 * time.Second +// blockRequeueJitter is the width of the window a blocked-everywhere Pod's +// requeue is spread across, ON TOP OF the block's expiry. A failover block is +// keyed by scope (provider/accelerator/tier/region), not by Pod, so every Pod +// contending for the same capacity pool sees the same expiry and, without +// jitter, would requeue at the same instant — then all retry together, all hit +// the still-tight pool together, and all re-block with a fresh TTL in lockstep +// (a synchronized retry storm). A deterministic per-Pod offset in [0, jitter) +// breaks the herd apart while staying stable across a Pod's own retries. +const blockRequeueJitter = 30 * time.Second + // PodPlacementReconciler is the middle of the placement flow: it turns a gated, // opted-in Pod into a placed one. The webhook holds the Pod SchedulingGated // until a provider is chosen; this controller chooses one, records the decision @@ -67,11 +77,13 @@ type PodPlacementReconciler struct { } // Blocklister is the read side of pkg/failover.Blocklist the placement controller -// depends on: it asks whether a candidate placement is currently excluded. The -// concrete type is injected from main; this narrow interface keeps the controller -// decoupled and a nil value a no-op. +// depends on: it asks whether a candidate placement is currently excluded and, +// when it is, how long until it frees (so a Pod stuck on failover can requeue for +// the exact moment a block lapses rather than idling until the periodic resync). +// The concrete type is injected from main; this narrow interface keeps the +// controller decoupled and a nil value a no-op. type Blocklister interface { - Blocked(c failover.Candidate) bool + BlockedUntil(c failover.Candidate) (retryAfter time.Duration, blocked bool) } // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch;update;patch;delete @@ -118,10 +130,23 @@ func (r *PodPlacementReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, nil } - placement, ok := r.selectPlacement(&pod, pool) + placement, ok, retryAfter := r.selectPlacement(ctx, &pod, pool) if !ok { // No provider in the pool can serve this Pod's GPU type right now. Leave it // gated; a later reconcile (pool edit, provider registered) can place it. + // When the only thing standing in the way is a failover block, requeue for + // the moment it lapses — blocklist TTL expiry emits no event, so without this + // the Pod would idle until the periodic resync (hours) even though a servable + // candidate frees in minutes. + if retryAfter > 0 { + // Spread the requeue past the shared expiry by a stable per-Pod offset so + // Pods contending for the same (scope-keyed) block don't all wake and retry + // in lockstep, thundering the same tight pool the instant it frees. + retryAfter += requeueJitter(pod.UID) + log.Info("all servable candidates are blocked; requeuing for the soonest to free", + "pod", pod.Name, "pool", pool.Name, "retryAfter", retryAfter.String()) + return ctrl.Result{RequeueAfter: retryAfter}, nil + } log.Info("no provider in pool can serve the Pod; leaving it gated", "pod", pod.Name, "pool", pool.Name) return ctrl.Result{}, nil diff --git a/internal/controller/pod_placement_controller_test.go b/internal/controller/pod_placement_controller_test.go index 20552b1..23bb667 100644 --- a/internal/controller/pod_placement_controller_test.go +++ b/internal/controller/pod_placement_controller_test.go @@ -37,16 +37,19 @@ import ( // fakeBlocklist is a test Blocklister that reports a fixed set of candidates as // blocked. A candidate is blocked when it matches an entry on every NON-empty // field (empty fields are wildcards), mirroring the real blocklist's coverage. +// A blocked candidate reports retryAfter=until (defaulting to a nonzero duration +// so the placement controller's requeue-on-block path is exercised). type fakeBlocklist struct { blocked []failover.Candidate + until time.Duration } -func (b *fakeBlocklist) Blocked(c failover.Candidate) bool { +func (b *fakeBlocklist) BlockedUntil(c failover.Candidate) (time.Duration, bool) { for _, e := range b.blocked { if e.Provider != "" && e.Provider != c.Provider { continue } - if e.AcceleratorType != "" && e.AcceleratorType != c.AcceleratorType { + if e.AcceleratorID != "" && e.AcceleratorID != c.AcceleratorID { continue } if e.CapacityType != "" && e.CapacityType != c.CapacityType { @@ -55,9 +58,13 @@ func (b *fakeBlocklist) Blocked(c failover.Candidate) bool { if e.Region != "" && e.Region != c.Region { continue } - return true + until := b.until + if until == 0 { + until = time.Minute + } + return until, true } - return false + return 0, false } // newPlacementReconciler wires a PodPlacementReconciler over a fake client. @@ -386,7 +393,7 @@ func TestPlacement_FailsOverToNextRegionWhenBlocked(t *testing.T) { prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"H100"}} r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov) r.Blocklist = &fakeBlocklist{blocked: []failover.Candidate{ - {Provider: provider.ProviderModal, AcceleratorType: "H100", CapacityType: nebulav1alpha1.CapacityOnDemand, Region: "us-east-1"}, + {Provider: provider.ProviderModal, AcceleratorID: "H100", CapacityType: nebulav1alpha1.CapacityOnDemand, Region: "us-east-1"}, }} reconcilePod(t, r, "default", "p1") @@ -425,20 +432,34 @@ func TestPlacement_CapacityIsOuterAxis(t *testing.T) { } } -func TestPlacement_AllCandidatesBlockedLeavesPodGated(t *testing.T) { - // Every (tier, provider, region) candidate is blocked (DenyAll on the provider). - // Placement has nowhere to go, so it leaves the Pod gated for a later retry - // (when a block expires) rather than placing onto a failing candidate. +func TestPlacement_AllCandidatesBlockedRequeuesForBlockExpiry(t *testing.T) { + // Every (tier, provider, region) candidate is blocked (DenyAll on the provider), + // but the candidates are servable — the block is a transient failover exclusion. + // Placement has nowhere to go now, so it leaves the Pod gated, but it must + // requeue for when the soonest block frees (TTL expiry emits no event) rather + // than idling until the periodic resync. pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacitySpot, nebulav1alpha1.CapacityOnDemand}, provider.ProviderModal) prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"H100"}} r, c := newPlacementReconciler(t, []client.Object{pod, pool}, prov) - r.Blocklist = &fakeBlocklist{blocked: []failover.Candidate{ - {Provider: provider.ProviderModal}, // whole-provider block (auth/quota) - }} + r.Blocklist = &fakeBlocklist{ + blocked: []failover.Candidate{{Provider: provider.ProviderModal}}, // whole-provider block (auth/quota) + until: 2 * time.Minute, + } - reconcilePod(t, r, "default", "p1") + res, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: "default", Name: "p1"}, + }) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + // Requeue lands at the block's expiry plus a stable per-Pod jitter (added so + // Pods sharing a scope-keyed block don't wake in lockstep), so it sits in + // [2m, 2m+jitter). The jitter must be strictly bounded, never negative. + if res.RequeueAfter < 2*time.Minute || res.RequeueAfter >= 2*time.Minute+blockRequeueJitter { + t.Fatalf("expected requeue in [2m, 2m+jitter), got %v", res.RequeueAfter) + } got := getPod(t, c, "default", "p1") if !hasGateNamed(got) { @@ -451,6 +472,53 @@ func TestPlacement_AllCandidatesBlockedLeavesPodGated(t *testing.T) { } } +func TestRequeueJitter_StableAndBounded(t *testing.T) { + // The offset must be deterministic per UID (a Pod's own retries must not drift, + // so it isn't a moving target) and stay within [0, blockRequeueJitter). + uids := []types.UID{"uid-1", "uid-2", "uid-3", "", "a-much-longer-pod-uid-value"} + for _, uid := range uids { + first := requeueJitter(uid) + if first < 0 || first >= blockRequeueJitter { + t.Fatalf("jitter(%q) = %v, want [0, %v)", uid, first, blockRequeueJitter) + } + if again := requeueJitter(uid); again != first { + t.Fatalf("jitter(%q) not stable: %v then %v", uid, first, again) + } + } + + // Distinct UIDs should generally land on distinct offsets (that's the whole + // point). Not a hard guarantee for any two, but across a spread they must not + // all collapse to one value. + distinct := map[time.Duration]struct{}{} + for _, uid := range uids { + distinct[requeueJitter(uid)] = struct{}{} + } + if len(distinct) < 2 { + t.Fatalf("jitter collapsed distinct UIDs to %d offset(s); herd would not spread", len(distinct)) + } +} + +func TestPlacement_NoServableCandidateDoesNotRequeue(t *testing.T) { + // The pool's only provider does not offer the requested accelerator, so no + // candidate is servable and none can be freed by a lapsing TTL. Placement leaves + // the Pod gated with NO requeue — a Pod edit, pool edit, or the resync is what + // retries — rather than spinning on a candidate that will never become servable. + pod := gatedPod("p1", "default", "uid-1", "pool-a", "H100") + pool := poolWith("pool-a", []nebulav1alpha1.CapacityType{nebulav1alpha1.CapacityOnDemand}, provider.ProviderModal) + prov := &fakeProvider{name: provider.ProviderModal, gpus: []string{"A100"}} // no H100 + r, _ := newPlacementReconciler(t, []client.Object{pod, pool}, prov) + + res, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: "default", Name: "p1"}, + }) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if res.RequeueAfter != 0 { + t.Fatalf("expected no requeue when no candidate can ever be unblocked, got %v", res.RequeueAfter) + } +} + func TestPlacement_StampsBlocklistTTLAnnotation(t *testing.T) { // The pool's FailoverPolicy.BlocklistTTL must reach the Pod so the VK handler // knows how long to blocklist a placement that fails. diff --git a/internal/controller/pod_placement_helpers.go b/internal/controller/pod_placement_helpers.go index ee99931..5e95a5d 100644 --- a/internal/controller/pod_placement_helpers.go +++ b/internal/controller/pod_placement_helpers.go @@ -18,13 +18,17 @@ package controller import ( "context" + "hash/fnv" + "time" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" @@ -73,43 +77,76 @@ func (r *PodPlacementReconciler) poolFor(ctx context.Context, pod *corev1.Pod) ( // registering, or a block expiring). Provider quirks (e.g. Modal being // OnDemand-only) are still handled at Provision time, not here. // +// On ok=false it also returns retryAfter: the time until the SOONEST currently- +// servable candidate (one skipped ONLY because it is blocklisted) frees, or 0 when +// no candidate can ever be unblocked by a lapsing TTL (every candidate is +// unregistered or does not offer the accelerator). The caller requeues on a +// positive hint so a Pod stuck purely on failover retries the moment a block +// expires, rather than idling until the periodic resync — blocklist TTL expiry +// emits no event of its own. +// // Strategy (LowestPrice/Weighted price-ranking) is a later swap-in for the inner // ordering; today the inner walk is listed order (Ordered), which the caller's // flow does not depend on — only that a (provider, capacityType, region) comes // back. -func (r *PodPlacementReconciler) selectPlacement(pod *corev1.Pod, pool *nebulav1alpha1.NodePool) (placement, bool) { - // Only the accelerator type is needed for matching; the count is a provisioning - // detail the adapter reads. A malformed request is treated as "no accelerator" - // so placement stays a no-op rather than erroring the reconcile — provisioning - // would surface the real error. - accel, _, _ := util.AcceleratorRequest(pod) +func (r *PodPlacementReconciler) selectPlacement(ctx context.Context, pod *corev1.Pod, pool *nebulav1alpha1.NodePool) (placement, bool, time.Duration) { + log := logf.FromContext(ctx).WithName("placement-select").WithValues( + "pod", pod.Namespace+"/"+pod.Name, "pool", pool.Name) + + // The (type, count) together select the concrete offering: a provider resolves + // them through MapAccelerator to its own id (an EC2 instance type on AWS), which + // is what the blocklist keys on so L4x1 and L4x8 (distinct instance types) block + // independently. A malformed request is treated as "no accelerator" so placement + // stays a no-op rather than erroring the reconcile — provisioning would surface + // the real error. + accel, count, _ := util.AcceleratorRequest(pod) + 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) prov, ok := r.provider(ref.Name) if !ok { + log.V(1).Info("skipping candidate: provider not registered", + "provider", ref.Name, "capacityType", tier) continue // unregistered; NodePool status surfaces this separately } // A CPU-only Pod (no accelerator) matches any provider; an accelerator - // Pod only matches a provider whose catalog maps that accelerator. + // Pod only matches a provider whose catalog serves that (type, count). The + // resolved id is also what the block is keyed on, so it is captured here + // from the same lookup that decides servability. + acceleratorID := "" if accel != "" { - if _, offered := prov.MapAccelerator(accel); !offered { + id, offered := prov.MapAccelerator(accel, count) + if !offered { + log.V(1).Info("skipping candidate: provider does not offer the accelerator", + "provider", ref.Name, "accelerator", accel, "count", count) continue } + acceleratorID = id } for _, region := range regionsFor(ref) { // inner: region - if r.blocked(ref.Name, accel, tier, region) { - continue // failed recently; try the next region, then the next tier + if until, blocked := r.blockedUntil(ref.Name, acceleratorID, 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. + log.Info("skipping candidate: blocked by failover blocklist", + "provider", ref.Name, "acceleratorID", acceleratorID, + "capacityType", tier, "region", region, "freesIn", until.String()) + if until > 0 && (soonest == 0 || until < soonest) { + soonest = until + } + continue } + log.Info("selected placement candidate", + "provider", ref.Name, "capacityType", tier, "region", region) return placement{ provider: ref.Name, capacityType: tier, region: region, - }, true + }, true, 0 } } } - return placement{}, false + return placement{}, false, soonest } // capacityTiers is the outer axis to walk: the pool's CapacityTypes in fallback @@ -136,21 +173,36 @@ func regionsFor(ref nebulav1alpha1.ProviderSpec) []string { return ref.Regions } -// blocked reports whether the (provider, accelerator, tier, region) candidate is -// currently excluded by the failover blocklist. It is nil-safe: with no blocklist +// blockedUntil reports whether the (provider, acceleratorID, tier, region) +// candidate is currently excluded by the failover blocklist and, if so, how long +// until it frees (for the requeue hint). acceleratorID is the provider's resolved +// id for the request (see selectPlacement), so a capacity block matches only +// candidates on the same instance type / pool. It is nil-safe: with no blocklist // wired (tests, or a blocklist-less build) nothing is ever blocked. -func (r *PodPlacementReconciler) blocked(provName, accel string, tier nebulav1alpha1.CapacityType, region string) bool { +func (r *PodPlacementReconciler) blockedUntil(provName, acceleratorID string, tier nebulav1alpha1.CapacityType, region string) (time.Duration, bool) { if r.Blocklist == nil { - return false + return 0, false } - return r.Blocklist.Blocked(failover.Candidate{ - Provider: provName, - AcceleratorType: accel, - CapacityType: tier, - Region: region, + return r.Blocklist.BlockedUntil(failover.Candidate{ + Provider: provName, + AcceleratorID: acceleratorID, + CapacityType: tier, + Region: region, }) } +// requeueJitter maps a Pod UID to a stable offset in [0, blockRequeueJitter) to +// desynchronize the requeues of Pods that share a scope-keyed failover block. +// It is deterministic (a hash of the UID, not a random draw) so a given Pod's +// successive retries land at the same offset — the goal is to spread DISTINCT +// Pods apart, not to move one Pod around between attempts. A missing UID hashes +// to a fixed value, which is harmless: real Pods always carry a UID. +func requeueJitter(uid types.UID) time.Duration { + h := fnv.New64a() + _, _ = h.Write([]byte(uid)) + return time.Duration(h.Sum64() % uint64(blockRequeueJitter)) +} + // ensureClaim creates the NodeClaim for this placement if it does not already // exist. The claim is the durable teardown ledger (see NodeClaimReconciler): it // pins the Pod by UID, records the chosen provider and capacity tier, and labels diff --git a/pkg/failover/blocklist.go b/pkg/failover/blocklist.go index 8281da7..de1928f 100644 --- a/pkg/failover/blocklist.go +++ b/pkg/failover/blocklist.go @@ -51,14 +51,18 @@ type entry struct { expiresAt time.Time } -// Candidate is a placement being considered — the (provider, accelerator, tier, +// Candidate is a placement being considered — the (provider, acceleratorID, tier, // region) tuple the blocklist is queried against. It is the query counterpart to a // recorded BlockScope: Blocked reports whether any live entry's scope covers it. +// AcceleratorID is the provider's RESOLVED id for what would serve the request +// (the (type, count) mapped through MapAccelerator), not the bare accelerator +// type, so a block recorded against one instance type/capacity pool matches only +// candidates that share it. type Candidate struct { - Provider string - AcceleratorType string - CapacityType nebulav1alpha1.CapacityType - Region string + Provider string + AcceleratorID string + CapacityType nebulav1alpha1.CapacityType + Region string } // Blocklist is a concurrency-safe, TTL-bounded set of provider blocks. The zero @@ -88,6 +92,15 @@ func newWithClock(c clock.Clock) *Blocklist { // to key on) — the caller is expected to skip zero scopes, but Record is defensive // so a misfire cannot install a permanent or provider-less block. Recording also // opportunistically drops expired entries so the slice cannot grow without bound. +// +// Records COALESCE: if a live entry with the same provider and an identical scope +// already exists, its expiry is extended to the later of its current value and +// now+ttl rather than appending a duplicate. Several Pods failing for the same +// (provider, accelerator, tier, region) reason therefore produce ONE entry, not one +// per failure — the slice is bounded by the number of distinct live scopes, not by +// failure volume. The externally observable behaviour is unchanged: Blocked/ +// BlockedUntil already took the latest covering expiry, and coalescing preserves +// exactly that latest value. func (b *Blocklist) Record(prov string, scope provider.BlockScope, ttl time.Duration) { if prov == "" || ttl <= 0 { return @@ -97,10 +110,22 @@ func (b *Blocklist) Record(prov string, scope provider.BlockScope, ttl time.Dura now := b.clock.Now() b.gcLocked(now) + + expiresAt := now.Add(ttl) + // Coalesce into an existing live entry for the same scope. gcLocked has already + // dropped expired entries, so every entry scanned here is still live. + for i := range b.entries { + if b.entries[i].provider == prov && scopeEqual(b.entries[i].scope, scope) { + if expiresAt.After(b.entries[i].expiresAt) { + b.entries[i].expiresAt = expiresAt // extend; never shorten a live block + } + return + } + } b.entries = append(b.entries, entry{ provider: prov, scope: scope, - expiresAt: now.Add(ttl), + expiresAt: expiresAt, }) } @@ -121,6 +146,33 @@ func (b *Blocklist) Blocked(c Candidate) bool { return false } +// BlockedUntil reports whether c is currently excluded and, if so, how long until +// it could become servable — the duration until the LATEST covering entry lapses +// (c stays blocked while ANY covering entry is live, so it frees only when the +// last one expires). Placement uses this to requeue a gated Pod exactly when a +// candidate frees, instead of leaving it idle until the periodic resync. A false +// result (retryAfter 0) means c is not blocked now. +func (b *Blocklist) BlockedUntil(c Candidate) (retryAfter time.Duration, blocked bool) { + b.mu.Lock() + defer b.mu.Unlock() + + now := b.clock.Now() + b.gcLocked(now) + var latest time.Time + for i := range b.entries { + if b.entries[i].provider == c.Provider && scopeCovers(b.entries[i].scope, c) { + if b.entries[i].expiresAt.After(latest) { + latest = b.entries[i].expiresAt + } + } + } + if latest.IsZero() { + return 0, false + } + // gcLocked has dropped every expired entry, so latest is strictly after now. + return latest.Sub(now), true +} + // gcLocked drops entries that have expired as of now. Caller holds b.mu. func (b *Blocklist) gcLocked(now time.Time) { if len(b.entries) == 0 { @@ -155,7 +207,7 @@ func scopeCovers(scope provider.BlockScope, c Candidate) bool { if scope.DenyAll { return true } - if !ptrMatches(scope.AcceleratorType, c.AcceleratorType) { + if !ptrMatches(scope.AcceleratorID, c.AcceleratorID) { return false } if scope.CapacityType != "" && scope.CapacityType != c.CapacityType { @@ -167,6 +219,31 @@ func scopeCovers(scope provider.BlockScope, c Candidate) bool { return true } +// scopeEqual reports whether two BlockScopes are the SAME block — identical along +// every axis. It is used by Record to coalesce a repeated failure into an existing +// entry. The *string fields are compared by VALUE, not pointer identity: two +// independent failures for the same region allocate distinct *string pointers, so a +// pointer-wise "==" on BlockScope would never coalesce them (and pointer fields make +// "==" unsafe anyway). This is stricter than scopeCovers, which asks "does this block +// COVER that candidate"; here we need exact equality so distinct scopes stay distinct +// entries. +func scopeEqual(a, b provider.BlockScope) bool { + return a.DenyAll == b.DenyAll && + a.CapacityType == b.CapacityType && + ptrEqual(a.AcceleratorID, b.AcceleratorID) && + ptrEqual(a.Region, b.Region) +} + +// ptrEqual reports whether two *string are equal by value: both nil, or both non-nil +// pointing at equal strings. (nil and &"" are DIFFERENT — nil means "axis not +// applicable" while &"" means "wildcard", a distinction BlockScope depends on.) +func ptrEqual(x, y *string) bool { + if x == nil || y == nil { + return x == y // equal only if both nil + } + return *x == *y +} + // ptrMatches applies BlockScope's three-state rule to one axis: nil matches only an // empty candidate value (the axis is not applicable), &"" is a wildcard that matches // anything, and &"v" matches only the exact value. diff --git a/pkg/failover/blocklist_test.go b/pkg/failover/blocklist_test.go index 422e628..84d719a 100644 --- a/pkg/failover/blocklist_test.go +++ b/pkg/failover/blocklist_test.go @@ -35,16 +35,16 @@ func ptr(s string) *string { return &s } func spotH100EastScope() provider.BlockScope { return provider.BlockScope{ - AcceleratorType: ptr("H100"), - CapacityType: nebulav1alpha1.CapacitySpot, - Region: ptr("us-east-1"), + AcceleratorID: ptr("H100"), + CapacityType: nebulav1alpha1.CapacitySpot, + Region: ptr("us-east-1"), } } // cand builds a query Candidate compactly, keeping the table rows within the // line-length limit. func cand(prov, accel string, tier nebulav1alpha1.CapacityType, region string) Candidate { - return Candidate{Provider: prov, AcceleratorType: accel, CapacityType: tier, Region: region} + return Candidate{Provider: prov, AcceleratorID: accel, CapacityType: tier, Region: region} } const ( @@ -100,13 +100,13 @@ func TestBlocked_ScopeMatchIsPrecise(t *testing.T) { func TestBlocked_WildcardFieldsMatchAnyValue(t *testing.T) { fc := testclock.NewFakeClock(baseTime) bl := newWithClock(fc) - // Wildcard AcceleratorType + wildcard Region (&"") => blocks Spot on aws + // Wildcard AcceleratorID + wildcard Region (&"") => blocks Spot on aws // everywhere, any GPU. The pointers are deliberately non-nil-empty: nil would // mean "no such axis" and match only an empty candidate field. bl.Record("aws", provider.BlockScope{ - AcceleratorType: ptr(""), - CapacityType: spot, - Region: ptr(""), + AcceleratorID: ptr(""), + CapacityType: spot, + Region: ptr(""), }, 10*time.Minute) if !bl.Blocked(cand("aws", "H100", spot, "us-east-1")) { @@ -122,12 +122,12 @@ func TestBlocked_WildcardFieldsMatchAnyValue(t *testing.T) { // A nil axis is "not applicable": it matches only a candidate whose field is empty, // never a populated one. This is how a region-simple provider (Modal: nil Region) -// or a CPU-only Pod (nil AcceleratorType) blocks without widening across an axis it +// or a CPU-only Pod (nil AcceleratorID) blocks without widening across an axis it // never had. Contrast &"" (wildcard), which matches any value — see the test above. func TestBlocked_NilFieldMatchesOnlyEmptyCandidate(t *testing.T) { fc := testclock.NewFakeClock(baseTime) bl := newWithClock(fc) - // A region-simple provider's block: nil Region, nil AcceleratorType, Spot only. + // A region-simple provider's block: nil Region, nil AcceleratorID, Spot only. bl.Record("modal", provider.BlockScope{CapacityType: spot}, 10*time.Minute) // A region-simple candidate carries an empty region and (CPU Pod) empty @@ -138,7 +138,7 @@ func TestBlocked_NilFieldMatchesOnlyEmptyCandidate(t *testing.T) { // A populated candidate field is NOT matched by a nil axis: it is out of scope, // not wildcarded in. if bl.Blocked(cand("modal", "H100", spot, "")) { - t.Error("nil AcceleratorType must not match a populated accelerator") + t.Error("nil AcceleratorID must not match a populated accelerator") } if bl.Blocked(cand("modal", "", spot, "us-east-1")) { t.Error("nil Region must not match a populated region") @@ -155,7 +155,7 @@ func TestBlocked_DenyAllBlocksWholeProvider(t *testing.T) { t.Error("DenyAll must block any aws candidate") } // ...but a different provider is untouched (a block never spans providers). - if bl.Blocked(Candidate{Provider: "modal", AcceleratorType: "H100"}) { + if bl.Blocked(Candidate{Provider: "modal", AcceleratorID: "H100"}) { t.Error("DenyAll on aws must not block modal") } } @@ -183,6 +183,40 @@ func TestBlocked_ExpiresAfterTTL(t *testing.T) { } } +func TestBlockedUntil_ReportsTimeToSoonestFree(t *testing.T) { + fc := testclock.NewFakeClock(baseTime) + bl := newWithClock(fc) + c := cand("aws", "H100", spot, "us-east-1") + + // Not blocked: retryAfter 0, blocked false. + if until, blocked := bl.BlockedUntil(c); blocked || until != 0 { + t.Fatalf("unblocked candidate: got (%v, %v), want (0, false)", until, blocked) + } + + // Two covering entries; the candidate frees only when the LATER one lapses. + bl.Record("aws", spotH100EastScope(), 5*time.Minute) + bl.Record("aws", spotH100EastScope(), 12*time.Minute) + until, blocked := bl.BlockedUntil(c) + if !blocked { + t.Fatal("candidate with live covering entries should be blocked") + } + if until != 12*time.Minute { + t.Fatalf("should free when the last covering entry lapses; got %v want 12m", until) + } + + // After the earlier entry lapses, the remaining one still bounds it. + fc.Step(6 * time.Minute) + if until, blocked := bl.BlockedUntil(c); !blocked || until != 6*time.Minute { + t.Fatalf("after 6m: got (%v, %v), want (6m, true)", until, blocked) + } + + // Past the last expiry: unblocked again. + fc.Step(7 * time.Minute) + if until, blocked := bl.BlockedUntil(c); blocked || until != 0 { + t.Fatalf("after all TTLs lapse: got (%v, %v), want (0, false)", until, blocked) + } +} + func TestRecord_NoOpOnInvalidInput(t *testing.T) { fc := testclock.NewFakeClock(baseTime) bl := newWithClock(fc) @@ -192,14 +226,90 @@ func TestRecord_NoOpOnInvalidInput(t *testing.T) { bl.Record("aws", provider.BlockScope{DenyAll: true}, 0) bl.Record("aws", provider.BlockScope{DenyAll: true}, -time.Minute) - if bl.Blocked(Candidate{Provider: "aws", AcceleratorType: "H100"}) { + if bl.Blocked(Candidate{Provider: "aws", AcceleratorID: "H100"}) { t.Error("invalid Record inputs must not block anything") } - if bl.Blocked(Candidate{Provider: "", AcceleratorType: "H100"}) { + if bl.Blocked(Candidate{Provider: "", AcceleratorID: "H100"}) { t.Error("empty-provider block must not exist") } } +func TestRecord_CoalescesIdenticalScope(t *testing.T) { + fc := testclock.NewFakeClock(baseTime) + bl := newWithClock(fc) + + // Several Pods failing for the SAME (provider, accelerator, tier, region) reason, + // each allocating its own scope value (distinct *string pointers), must collapse + // to ONE entry rather than one per failure. + for i := 0; i < 5; i++ { + bl.Record("aws", spotH100EastScope(), 10*time.Minute) + } + if n := len(bl.entries); n != 1 { + t.Fatalf("len(entries) = %d, want 1 (identical scopes must coalesce)", n) + } + if !bl.Blocked(cand("aws", "H100", spot, "us-east-1")) { + t.Error("the coalesced entry must still block its scope") + } +} + +func TestRecord_CoalesceExtendsButNeverShortens(t *testing.T) { + fc := testclock.NewFakeClock(baseTime) + bl := newWithClock(fc) + c := cand("aws", "H100", spot, "us-east-1") + + // A later failure with a LONGER TTL pushes the single entry's expiry out. + bl.Record("aws", spotH100EastScope(), 5*time.Minute) + bl.Record("aws", spotH100EastScope(), 12*time.Minute) + if n := len(bl.entries); n != 1 { + t.Fatalf("len(entries) = %d, want 1", n) + } + if until, _ := bl.BlockedUntil(c); until != 12*time.Minute { + t.Fatalf("expiry should extend to the longer TTL; got %v want 12m", until) + } + + // A subsequent SHORTER TTL must not shorten a still-live block (a fresh short + // failure cannot make an existing longer block lapse early). + bl.Record("aws", spotH100EastScope(), 3*time.Minute) + if until, _ := bl.BlockedUntil(c); until != 12*time.Minute { + t.Fatalf("a shorter later TTL must not shorten the live block; got %v want 12m", until) + } +} + +func TestRecord_DistinctScopesStaySeparate(t *testing.T) { + fc := testclock.NewFakeClock(baseTime) + bl := newWithClock(fc) + + // Scopes differing on any single axis are different blocks and must NOT coalesce. + bl.Record("aws", spotH100EastScope(), 10*time.Minute) // H100/Spot/us-east-1 + bl.Record("aws", provider.BlockScope{ // different region + AcceleratorID: ptr("H100"), CapacityType: spot, Region: ptr("us-west-2"), + }, 10*time.Minute) + bl.Record("aws", provider.BlockScope{ // different tier + AcceleratorID: ptr("H100"), CapacityType: onDemand, Region: ptr("us-east-1"), + }, 10*time.Minute) + bl.Record("modal", spotH100EastScope(), 10*time.Minute) // different provider + + if n := len(bl.entries); n != 4 { + t.Fatalf("len(entries) = %d, want 4 (distinct scopes must not coalesce)", n) + } +} + +func TestRecord_NilAndWildcardScopesDoNotCoalesce(t *testing.T) { + fc := testclock.NewFakeClock(baseTime) + bl := newWithClock(fc) + + // nil (axis not applicable) and &"" (wildcard) are semantically different scopes, + // so scopeEqual must keep them as separate entries. + nilAxes := provider.BlockScope{CapacityType: spot} + wildcardAxes := provider.BlockScope{AcceleratorID: ptr(""), Region: ptr(""), CapacityType: spot} + bl.Record("aws", nilAxes, 10*time.Minute) + bl.Record("aws", wildcardAxes, 10*time.Minute) + + if n := len(bl.entries); n != 2 { + t.Fatalf("len(entries) = %d, want 2 (nil and wildcard scopes are distinct)", n) + } +} + func TestRecord_ExpiredEntriesAreGarbageCollected(t *testing.T) { fc := testclock.NewFakeClock(baseTime) bl := newWithClock(fc) diff --git a/pkg/provider/aws/aws.go b/pkg/provider/aws/aws.go index 983958b..d4c5db4 100644 --- a/pkg/provider/aws/aws.go +++ b/pkg/provider/aws/aws.go @@ -41,9 +41,11 @@ import ( "errors" "fmt" "strings" + "sync" "time" corev1 "k8s.io/api/core/v1" + logf "sigs.k8s.io/controller-runtime/pkg/log" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/provider" @@ -150,30 +152,152 @@ type EC2Instance struct { StatusChecksPassed bool } +// ClientFactory builds a Client bound to one region. It is the seam through which +// the Provider lazily materializes a per-region EC2 client on first use: the real +// factory (NewSDKClient's) resolves that region's GPU AMI and default-VPC subnets; +// a test injects a fake. Credentials are NOT a parameter — the factory resolves +// them from the SDK's default chain, which is account-global, so ONE credential +// set serves every region and only the region endpoint differs. +type ClientFactory func(ctx context.Context, region string) (Client, error) + +// RegionSource reports the regions the adapter should sweep in List and Offerings. +// The NodePool is the source of truth: cmd/main.go backs this with a lister over +// ProviderSpec.Regions across every pool referencing "aws", so the region set is +// DYNAMIC (a pool added at runtime widens the sweep) and needs no env var or flag — +// the region a Pod lands in already flows NodePool -> selectPlacement -> +// RegionAnnotation -> ProvisionRequest.Region, so provisioning never depended on a +// configured set; only the List/Offerings fan-out did, and this supplies it. +// +// It may return an empty slice (no aws pool exists yet, or the cache has not synced): +// sweepRegions then falls back to the regions already in the lazy client cache +// (regions actually provisioned into), so a fleet placed by a prior generation is +// still swept. It must be safe to call concurrently. +type RegionSource func() []string + // Provider is the EC2 implementation of provider.Provider. It embeds catalog.Base // for the generic catalog methods (Name, Offerings, and MapAccelerator — which // resolves the accelerator_id/instance-type mapping straight from the catalog, so // no override is needed here) and implements the EC2-specific lifecycle. +// +// Multi-region: unlike the OnDemand-only NeoClouds, one AWS Provider spans several +// regions. EC2 is region-partitioned (an instance, its AMI, its subnets, and its +// capacity all live in one region), so the Provider holds a region -> Client map, +// each Client pinned to one region, built LAZILY on first use via newClient. The +// map is keyed by region and guarded by mu. A single registry entry ("aws") and a +// single virtual node still front all regions; the region is a per-Pod placement +// choice carried on the ProvisionRequest, not a per-node fact — so it must be an +// axis inside the one adapter, not a provider-per-region. +// +// The region set is NOT configured at construction: it is sourced from the NodePool +// at call time via regionSource (see RegionSource), because ProviderSpec.Regions is +// the operator's declaration of which regions this provider may use and it changes +// at runtime. Provisioning never needed a configured set (the target region rides on +// the request); only the List/Offerings fan-out does, and sweepRegions derives it +// from the NodePool plus the regions already provisioned into (the cache keys). type Provider struct { catalog.Base - client Client - // region is the adapter's configured default region, used when a - // ProvisionRequest does not pin one and stamped onto observed instances that - // do not report their own. - region string + // newClient lazily builds the Client for a region (AMI/subnet resolution). The + // factory seam keeps the adapter SDK-free and unit-testable. + newClient ClientFactory + // regionSource reports the NodePool-declared regions to sweep in List/Offerings. + // May be nil in tests, in which case sweepRegions uses only the cache keys. + regionSource RegionSource + + mu sync.Mutex + clients map[string]Client // region -> Client, populated lazily by clientFor } -// New returns an EC2 Provider backed by client and price catalog. region is the -// default region the client was configured with (used when a request omits one); -// cat is the catalog.Lookup seam so tests can inject a fake. -func New(client Client, cat catalog.Lookup, region string) *Provider { +// New returns an EC2 Provider backed by a client factory and price catalog. +// regionSource supplies the NodePool-declared region set the List/Offerings fan-out +// sweeps (nil is tolerated — the sweep then uses only the regions already +// provisioned into). cat is the catalog.Lookup seam so tests can inject a fake. +// +// There is deliberately NO default region: every request carries its own region +// (admission requires each aws pool to list ≥1 region, and placement stamps it onto +// the ProvisionRequest), and observed instances report their region from the +// region-pinned client — so nothing needs a fallback, and no AWS_REGION env is read. +func New(newClient ClientFactory, cat catalog.Lookup, regionSource RegionSource) *Provider { return &Provider{ - Base: catalog.Base{ProviderName: provider.ProviderAWS, Catalog: cat}, - client: client, - region: region, + Base: catalog.Base{ProviderName: provider.ProviderAWS, Catalog: cat}, + newClient: newClient, + regionSource: regionSource, + clients: make(map[string]Client), } } +// newSingleRegion is a test/back-compat convenience: a Provider serving exactly one +// region backed by an already-built Client (no lazy factory). It underpins the +// existing single-region unit tests, which inject a fakeClient directly. The region +// is the sole swept region (via a constant regionSource), so List/Offerings behave +// as the pre-multi-region tests expect. +func newSingleRegion(client Client, cat catalog.Lookup, region string) *Provider { + p := New( + func(context.Context, string) (Client, error) { return client, nil }, + cat, + func() []string { return []string{region} }, + ) + // Pre-seed the cache so even a stray region lookup returns the fake rather than + // invoking the (constant) factory. + p.clients[region] = client + return p +} + +// sweepRegions returns the regions List and Offerings fan out across: the union of +// the NodePool-declared set (regionSource) and every region already in the lazy +// client cache. The cache half is what makes teardown survive a NodePool edit — an +// instance still running in a region just dropped from every pool is still swept and +// so still observed/reclaimed, rather than being stranded because the region left +// the declared set. Order is not significant (callers concatenate results). +func (p *Provider) sweepRegions() []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 p.regionSource != nil { + for _, r := range p.regionSource() { + add(strings.TrimSpace(r)) + } + } + p.mu.Lock() + for r := range p.clients { + add(r) + } + p.mu.Unlock() + return out +} + +// clientFor returns the Client for region (defaulting an empty region), building +// and caching it on first use. Construction is serialized under mu: a region's +// Client is built at most once, and a concurrent caller for the same or another +// region waits — acceptable because a build is a rare one-time, per-region event +// (an AMI + subnet resolution), not a hot path. A build failure is NOT cached, so +// a transient resolution error (throttle, a not-yet-enabled region) is retried on +// the next call rather than poisoning the region permanently. +func (p *Provider) clientFor(ctx context.Context, region string) (Client, error) { + if region == "" { + // No region to build a client for. Unreachable on the normal path (every + // request carries a region), so this only guards a legacy unqualified instance + // id reaching Terminate/Get — surface it rather than silently guessing. + return nil, fmt.Errorf("aws: no region for client: %w", ErrConfig) + } + p.mu.Lock() + defer p.mu.Unlock() + if c, ok := p.clients[region]; ok { + return c, nil + } + c, err := p.newClient(ctx, region) + if err != nil { + return nil, fmt.Errorf("aws: build client for region %s: %w", region, err) + } + p.clients[region] = c + return c, nil +} + // Capabilities implements provider.Provider. See the package doc for why each // trait is set the way it is. func (p *Provider) Capabilities() provider.Capabilities { @@ -197,32 +321,58 @@ func (p *Provider) Capabilities() provider.Capabilities { // - AWS itself holds the PER-REGION truth: which instance types the configured // region actually offers, queried live via the AvailableInstanceTypes probe. // -// So each static row is stamped with this adapter's region and its Available flag -// is AND-ed with the live probe: a row survives as available only if the catalog -// seeded it available AND the region currently offers its instance type. Rows for -// types the region does not offer are still returned (so the optimizer can see the -// price) but marked unavailable. A probe failure is surfaced as an error rather -// than silently reporting the stale seed as truth. +// So each static row is emitted ONCE PER CONFIGURED REGION, stamped with that +// region and its Available flag AND-ed with the region's live probe: a row survives +// as available only if the catalog seeded it available AND that region currently +// offers its instance type. Rows for types a region does not offer are still +// returned (so the optimizer can see the price) but marked unavailable. +// +// Per-region probe errors are tolerated like List's: a region that fails its probe +// is skipped for this call rather than failing the whole Offerings (its rows simply +// do not appear this time). Only if every region fails is an error returned. func (p *Provider) Offerings(ctx context.Context) ([]provider.Offering, error) { rows := p.Catalog.Offerings(p.ProviderName) if len(rows) == 0 { return nil, nil } - avail, err := p.client.AvailableInstanceTypes(ctx) - if err != nil { - return nil, fmt.Errorf("aws: probe instance-type availability in %s: %w", p.region, err) + log := logf.FromContext(ctx).WithName("aws-offerings") + + regions := p.sweepRegions() + out := make([]provider.Offering, 0, len(rows)*len(regions)) + failed := 0 + for _, region := range regions { + client, err := p.clientFor(ctx, region) + if err != nil { + failed++ + log.Error(err, "build client for offerings failed; skipping region", "region", region) + continue + } + avail, err := client.AvailableInstanceTypes(ctx) + if err != nil { + failed++ + log.Error(err, "probe instance-type availability failed; skipping region", "region", region) + continue + } + for _, o := range rows { + o.Region = region + o.Available = o.Available && avail[o.AcceleratorID] + out = append(out, o) + } } - out := make([]provider.Offering, len(rows)) - for i, o := range rows { - o.Region = p.region - o.Available = o.Available && avail[o.AcceleratorID] - out[i] = o + if failed == len(regions) && failed > 0 { + return nil, fmt.Errorf("aws: offerings probe failed in all %d region(s)", failed) } return out, nil } // Provision implements provider.Provider. The Pod is the source of truth for the // workload; req carries the claim identity, capacity tier, and region. +// +// The returned instance id is the RAW EC2 id ("i-..."), not region-qualified: the +// id is what the NodeClaim ledger records and the user sees, so it stays a clean +// EC2 id. Terminate/Get do not need the region encoded in it — they locate the +// instance by sweeping the swept regions (the same set List covers), since a +// wrong-region lookup is a harmless no-op (see Terminate/Get). func (p *Provider) Provision(ctx context.Context, pod *corev1.Pod, req provider.ProvisionRequest) (string, error) { if pod == nil { return "", errors.New("aws: nil pod") @@ -231,9 +381,17 @@ func (p *Provider) Provision(ctx context.Context, pod *corev1.Pod, req provider. return "", errors.New("aws: empty ClaimName in ProvisionRequest") } - // Idempotency: if an instance already carries this claim tag, return it rather - // than launching a second (guards a retry after a partial create). - if existing, err := p.findByClaim(ctx, req.ClaimName); err != nil { + region := req.Region + client, err := p.clientFor(ctx, region) + if err != nil { + return "", err + } + + // Idempotency: if an instance already carries this claim tag IN THIS REGION, + // return it rather than launching a second (guards a retry after a partial + // create). A claim is placed in exactly one region per attempt, so scanning the + // target region's client is sufficient. + if existing, err := findByClaim(ctx, client, req.ClaimName); err != nil { return "", err } else if existing != nil { return existing.ID, nil @@ -246,39 +404,180 @@ func (p *Provider) Provision(ctx context.Context, pod *corev1.Pod, req provider. // The Provision deadline is enforced generically by the vnode handler (from // Capabilities.ProvisionTimeout), so RunInstance simply honors ctx as it fails // over across zones — no adapter-local WithTimeout here. - return p.client.RunInstance(ctx, spec) + id, err := client.RunInstance(ctx, spec) + if err != nil { + return "", err + } + return id, nil } -// Terminate implements provider.Provider. Idempotent by the Client contract. +// Terminate implements provider.Provider. Idempotent by the Client contract. The +// instanceID is a raw EC2 id, which does not carry its region, so teardown sweeps +// the swept regions and terminates the instance wherever it lives. This is safe +// and cheap: TerminateInstance is idempotent and a wrong-region lookup returns +// InvalidInstanceID.NotFound, which the Client maps to nil — so terminating in a +// region the instance is not in is a harmless no-op. It stops at the first region +// that actually owns the instance. +// +// A legacy region-qualified id ("/i-...") from before this change is still +// honored: splitID peels the region off and it routes straight to that region. func (p *Provider) Terminate(ctx context.Context, instanceID string) error { if instanceID == "" { return nil // nothing provisioned yet; treat as already gone } - return p.client.TerminateInstance(ctx, instanceID) + // Back-compat: a legacy qualified id routes directly to its region. + if region, rawID := splitID(instanceID); region != "" { + client, err := p.clientFor(ctx, region) + if err != nil { + return err + } + return client.TerminateInstance(ctx, rawID) + } + + // Raw id: sweep the regions and terminate wherever it lives. Confirm ownership + // with a Describe first so we only issue TerminateInstance against the region + // that actually has it — and so a region whose client cannot be built does not + // mask a successful terminate elsewhere. + var lastErr error + for _, region := range p.sweepRegions() { + client, err := p.clientFor(ctx, region) + if err != nil { + lastErr = err + continue + } + ec2, err := client.DescribeInstance(ctx, instanceID) + if err != nil { + lastErr = err + continue + } + if ec2 == nil { + continue // not in this region + } + return client.TerminateInstance(ctx, instanceID) + } + // Not found in any region we could reach: already gone (idempotent success), + // unless every reachable region errored, in which case surface that for retry. + return lastErr } -// Get implements provider.Provider. +// Get implements provider.Provider. instanceID is a raw EC2 id, which does not +// carry its region, so the lookup sweeps the swept regions and returns the first +// region's view of the instance. A per-region client-build/describe error is +// tolerated and the sweep continues; only if every region errored (and none held +// the instance) is that error surfaced. +// +// A legacy region-qualified id ("/i-...") routes directly to its region. func (p *Provider) Get(ctx context.Context, instanceID string) (*provider.Instance, error) { - ec2, err := p.client.DescribeInstance(ctx, instanceID) - if err != nil { - return nil, err + if region, rawID := splitID(instanceID); region != "" { + client, err := p.clientFor(ctx, region) + if err != nil { + return nil, err + } + ec2, err := client.DescribeInstance(ctx, rawID) + if err != nil { + return nil, err + } + if ec2 == nil { + return nil, nil // absent => terminated, per interface contract + } + inst := p.toInstance(*ec2) + return &inst, nil } - if ec2 == nil { - return nil, nil // absent => terminated, per interface contract + + var lastErr error + for _, region := range p.sweepRegions() { + client, err := p.clientFor(ctx, region) + if err != nil { + lastErr = err + continue + } + ec2, err := client.DescribeInstance(ctx, instanceID) + if err != nil { + lastErr = err + continue + } + if ec2 == nil { + continue // not in this region + } + inst := p.toInstance(*ec2) + return &inst, nil } - inst := p.toInstance(*ec2) - return &inst, nil + // Absent from every reachable region => terminated (nil,nil), unless a region + // errored and might have held it, in which case surface the error for retry. + return nil, lastErr } -// List implements provider.Provider. +// List implements provider.Provider. It FANS OUT across every region sweepRegions +// yields (the NodePool-declared set unioned with regions already provisioned into) +// and concatenates the results, so the poll loop and the NodeClaim teardown backstop +// see instances wherever they live — including regions this process never +// provisioned into itself (a restart, or an instance placed by a prior leader). +// +// Per-region errors are TOLERATED, not fatal: one region throttling or briefly +// unreachable must not blank the whole list (which the vnode poll loop would read +// as "every tracked pod's instance vanished" and mark them Terminated). A region +// that errors is logged and skipped for this tick; the regions that answered still +// contribute. Only if EVERY region fails is an error returned, so a total outage +// still surfaces rather than silently reporting an empty fleet. +// +// Each instance's ID is the raw EC2 id; a downstream Terminate/Get re-locates it by +// sweeping the same regions (a wrong-region lookup is a harmless no-op), so the id +// stays a clean EC2 id rather than a region-qualified token. func (p *Provider) List(ctx context.Context) ([]provider.Instance, error) { - instances, err := p.client.ListInstances(ctx) - if err != nil { - return nil, err + log := logf.FromContext(ctx).WithName("aws-list") + + type result struct { + region string + instances []provider.Instance + err error + } + regions := p.sweepRegions() + // No region to sweep (no aws NodePool yet and nothing provisioned): an empty + // fleet, not an error. Returning nil here is correct — there are provably no + // Nebula instances to observe — and avoids the "all regions failed" path below + // firing on a zero-length set. + if len(regions) == 0 { + return nil, nil } - out := make([]provider.Instance, 0, len(instances)) - for _, ec2 := range instances { - out = append(out, p.toInstance(ec2)) + results := make([]result, len(regions)) + var wg sync.WaitGroup + for i, region := range regions { + wg.Add(1) + go func(i int, region string) { + defer wg.Done() + client, err := p.clientFor(ctx, region) + if err != nil { + results[i] = result{region: region, err: err} + return + } + raw, err := client.ListInstances(ctx) + if err != nil { + results[i] = result{region: region, err: err} + return + } + insts := make([]provider.Instance, 0, len(raw)) + for _, ec2 := range raw { + insts = append(insts, p.toInstance(ec2)) + } + results[i] = result{region: region, instances: insts} + }(i, region) + } + wg.Wait() + + out := make([]provider.Instance, 0) + failed := 0 + for _, r := range results { + if r.err != nil { + failed++ + log.Error(r.err, "list instances in region failed; skipping this region for this tick", "region", r.region) + continue + } + out = append(out, r.instances...) + } + // Every region failed => surface the outage rather than reporting an empty + // fleet, which would strand or falsely terminate tracked pods. + if failed == len(regions) && failed > 0 { + return nil, fmt.Errorf("aws: list failed in all %d region(s)", failed) } return out, nil } @@ -293,12 +592,14 @@ func (p *Provider) List(ctx context.Context) ([]provider.Instance, error) { // Spot, leaving OnDemand serviceable. ClassifyError does not see the request, // so we detect the Spot case here (ErrNoCapacity wrapped with the spot marker) // and pass the right tier. -// - EC2 capacity is per-region and this adapter is bound to one region (its -// client's), so an accelerator/capacity block is confined to p.region — a -// "no capacity in us-east-1" failure does not disqualify the same request -// against the us-west-2 adapter. Whole-provider blocks (auth/quota) are left -// region-wide, since bad credentials fail in every region. -func (p *Provider) ClassifyProvisionError(err error, accelerator string) provider.BlockScope { +// - EC2 capacity is per-region and this adapter is multi-region, so an +// accelerator/capacity block is confined to the region that ACTUALLY FAILED +// (the region the failing request targeted) — a "no capacity in us-east-1" +// failure does not disqualify the same request in us-west-2. Whole-provider +// blocks (auth/quota) are left region-wide, since bad credentials fail in every +// region. The region is passed in because the error alone does not carry it and +// the adapter has no default region to assume. +func (p *Provider) ClassifyProvisionError(err error, acceleratorID, region string) provider.BlockScope { if err == nil { return provider.BlockScope{} } @@ -306,42 +607,54 @@ func (p *Provider) ClassifyProvisionError(err error, accelerator string) provide if errors.Is(err, ErrSpotCapacity) { tier = nebulav1alpha1.CapacitySpot } - scope := provider.ClassifyError(err, tier, accelerator) - // Accelerator/capacity blocks are per-region (EC2 capacity is regional and - // this adapter is bound to one region); a DenyAll (auth/quota) fails in every - // region, so it stays region-wide (Region left nil). The exact-region pointer - // confines the block to p.region so the same request survives in another region. - // - // p.region is correct ONLY because this adapter is single-region: the client is - // pinned to cfg.Region at construction and runInSubnet ignores spec.Region, so - // every launch lands in p.region regardless of the request's Region — the block - // region and the launch region are the same. If the adapter is ever made - // multi-region (runInSubnet honoring spec.Region, re-resolving AMI/subnets per - // region), this MUST become the request's region (regionOrDefault(req.Region)) — - // which means threading it through ClassifyProvisionError — or failover would - // block the wrong region and keep retrying the one that actually failed. + scope := provider.ClassifyError(err, tier, acceleratorID) + // Confine an accelerator/capacity block to the region that failed. A DenyAll + // (auth/quota) fails in every region, so it stays region-wide (Region left nil). + // An empty region (should not happen — every request carries one) maps to &"" = + // the wildcard, which blocks the accelerator in ALL regions: the safe over-broad + // choice, since a stale-but-effective block beats a block pinned to a guessed + // region that never matches. if !scope.DenyAll { - region := p.region - scope.Region = ®ion + r := region + scope.Region = &r } return scope } -// findByClaim returns the instance tagged with claimName, or nil if none. -func (p *Provider) findByClaim(ctx context.Context, claimName string) (*provider.Instance, error) { - instances, err := p.client.ListInstances(ctx) +// findByClaim returns the instance in this client's region tagged with claimName, +// or nil if none. It scans one region's client (the launch target), since a claim +// is placed in exactly one region per provision attempt. +func findByClaim(ctx context.Context, client Client, claimName string) (*EC2Instance, error) { + instances, err := client.ListInstances(ctx) if err != nil { return nil, err } - for _, ec2 := range instances { - if ec2.Tags[ClaimTagKey] == claimName { - inst := p.toInstance(ec2) - return &inst, nil + for i := range instances { + if instances[i].Tags[ClaimTagKey] == claimName { + return &instances[i], nil } } return nil, nil } +// idSep separates the region prefix from the raw EC2 id in a legacy region-qualified +// instance id ("/"). "/" cannot appear in either a region name or +// an EC2 instance id, so it is an unambiguous delimiter. Current ids are raw EC2 +// ids; this exists only so splitID can still route ids recorded before that change. +const idSep = "/" + +// splitID recognizes a LEGACY region-qualified id ("/i-..."): it returns +// the region and the raw EC2 id. A current, raw EC2 id (no separator) yields an +// empty region, signaling the caller to locate the instance by sweeping regions +// instead. It is the one remaining reader of the old format, kept so ids persisted +// on a NodeClaim before the id stopped being qualified still terminate correctly. +func splitID(instanceID string) (region, rawID string) { + if i := strings.Index(instanceID, idSep); i >= 0 { + return instanceID[:i], instanceID[i+len(idSep):] + } + return "", instanceID +} + // instanceSpecFromPod reads the workload off the Pod (source of truth) and the // accelerator type (from the AcceleratorTypeLabel), maps it to an EC2 instance // type via the catalog, and stamps the claim tag, capacity tier, and region. @@ -364,8 +677,9 @@ func (p *Provider) instanceSpecFromPod(pod *corev1.Pod, req provider.ProvisionRe // nvidia.com/gpu resource. On EC2 both are lookup keys: the instance type is the // one whose (accelerator_type, gpu_count) pair matches, since the GPU count is // baked into the instance type (T4x1 = g4dn.xlarge, T4x8 = g4dn.metal) rather - // than a free knob. So — unlike the identity-mapped NeoClouds — this does NOT - // use the type-only MapAccelerator; it resolves by type AND count. + // than a free knob. MapAccelerator (from the embedded catalog.Base) resolves by + // that (type, count) key straight from the catalog, so no AWS-specific lookup is + // needed here. canonical, count, err := util.AcceleratorRequest(pod) if err != nil { return InstanceSpec{}, fmt.Errorf("aws: %w", err) @@ -374,7 +688,7 @@ func (p *Provider) instanceSpecFromPod(pod *corev1.Pod, req provider.ProvisionRe return InstanceSpec{}, errors.New( "aws: pod requests no accelerator; EC2 GPU provisioning needs an accelerator type and count") } - instanceType, ok := p.instanceTypeFor(canonical, count) + instanceType, ok := p.MapAccelerator(canonical, count) if !ok { return InstanceSpec{}, fmt.Errorf("aws: no EC2 instance type for %s x%d", canonical, count) } @@ -385,39 +699,11 @@ func (p *Provider) instanceSpecFromPod(pod *corev1.Pod, req provider.ProvisionRe Command: append(append([]string{}, c.Command...), c.Args...), Env: env, Spot: req.CapacityType == nebulav1alpha1.CapacitySpot, - Region: p.regionOrDefault(req.Region), + Region: req.Region, Tags: map[string]string{ClaimTagKey: req.ClaimName}, }, nil } -// instanceTypeFor resolves the EC2 instance type serving canonical accelerators -// at the requested count, by matching a catalog offering on BOTH the accelerator -// type (case-insensitively) and gpu_count. This is the AWS-specific replacement -// for the type-only MapAccelerator: because an EC2 instance type has a fixed GPU -// count, (type, count) — not type alone — selects it. Region and capacity tier -// are NOT part of the key here: the same (type, count) maps to the same instance -// type in every region/tier, so the first matching row's AcceleratorID is -// authoritative. Returns ok=false when no offering serves that pair (e.g. an -// unsupported accelerator, or a count with no single-instance shape — there is no -// T4x2 instance type). -func (p *Provider) instanceTypeFor(canonical string, count int32) (instanceType string, ok bool) { - for _, o := range p.Catalog.Offerings(p.ProviderName) { - if o.GPUCount == count && strings.EqualFold(o.AcceleratorType, canonical) && o.AcceleratorID != "" { - return o.AcceleratorID, true - } - } - return "", false -} - -// regionOrDefault returns the requested region, or the adapter's configured -// default when the request does not pin one. -func (p *Provider) regionOrDefault(region string) string { - if region != "" { - return region - } - return p.region -} - // toInstance normalizes an observed EC2 instance into the provider-agnostic // Instance. func (p *Provider) toInstance(ec2 EC2Instance) provider.Instance { @@ -425,10 +711,9 @@ func (p *Provider) toInstance(ec2 EC2Instance) provider.Instance { if ec2.Spot { tier = nebulav1alpha1.CapacitySpot } + // The region-pinned SDK client always stamps its region onto observed instances + // (see sdkClient.observe), so ec2.Region is set; no default fallback is needed. region := ec2.Region - if region == "" { - region = p.region - } return provider.Instance{ ID: ec2.ID, ClaimName: ec2.Tags[ClaimTagKey], diff --git a/pkg/provider/aws/aws_test.go b/pkg/provider/aws/aws_test.go index aca4168..6e4186d 100644 --- a/pkg/provider/aws/aws_test.go +++ b/pkg/provider/aws/aws_test.go @@ -126,7 +126,7 @@ func offering(accel, id string, count int32, tier nebulav1alpha1.CapacityType, p // catalog whose (accelerator_type, gpu_count) pairs map to EC2 instance types. // T4 appears at two counts so the count-aware resolution is exercised. func newTestProvider(f *fakeClient) *Provider { - return New(f, fakeCatalog{rows: []provider.Offering{ + return newSingleRegion(f, fakeCatalog{rows: []provider.Offering{ offering("H100", "p5.48xlarge", 8, nebulav1alpha1.CapacityOnDemand, 98.32), offering("H100", "p5.48xlarge", 8, nebulav1alpha1.CapacitySpot, 34.41), offering("A100-80GB", "p4de.24xlarge", 8, nebulav1alpha1.CapacityOnDemand, 40.96), @@ -171,6 +171,8 @@ func TestProvision_MapsAcceleratorToInstanceType(t *testing.T) { if err != nil { t.Fatalf("Provision: %v", err) } + // The returned id is the raw EC2 id (no region prefix); Terminate/Get re-locate + // it by sweeping regions. if id != "i-1" { t.Fatalf("id = %q, want i-1", id) } @@ -204,6 +206,7 @@ func TestProvision_LowercaseAcceleratorLabel(t *testing.T) { // the canonical catalog row (and thus the right instance type). if _, err := p.Provision(context.Background(), gpuPod("h100", 8), provider.ProvisionRequest{ ClaimName: "claim-lc", + Region: testRegion, }); err != nil { t.Fatalf("Provision: %v", err) } @@ -226,7 +229,7 @@ func TestProvision_CountSelectsInstanceType(t *testing.T) { for _, tc := range cases { f := &fakeClient{runID: "i-t4"} p := newTestProvider(f) - req := provider.ProvisionRequest{ClaimName: "claim-t4"} + req := provider.ProvisionRequest{ClaimName: "claim-t4", Region: testRegion} if _, err := p.Provision(context.Background(), gpuPod("T4", tc.count), req); err != nil { t.Fatalf("Provision(T4 x%d): %v", tc.count, err) } @@ -241,7 +244,7 @@ func TestProvision_UnsupportedCountIsError(t *testing.T) { p := newTestProvider(f) // T4 x2 has no instance type (there is no 2-GPU T4 shape): must error rather // than silently picking the x1 or x8 row. - req := provider.ProvisionRequest{ClaimName: "claim-t4x2"} + req := provider.ProvisionRequest{ClaimName: "claim-t4x2", Region: testRegion} if _, err := p.Provision(context.Background(), gpuPod("T4", 2), req); err == nil { t.Fatal("expected an error for an unsupported (accelerator, count) pair") } @@ -257,6 +260,7 @@ func TestProvision_SpotSetsMarketOption(t *testing.T) { if _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ ClaimName: "claim-spot", CapacityType: nebulav1alpha1.CapacitySpot, + Region: testRegion, }); err != nil { t.Fatalf("Provision: %v", err) } @@ -265,18 +269,21 @@ func TestProvision_SpotSetsMarketOption(t *testing.T) { } } -func TestProvision_EmptyRegionFallsBackToAdapterDefault(t *testing.T) { +func TestProvision_EmptyRegionIsError(t *testing.T) { f := &fakeClient{runID: "i-def"} p := newTestProvider(f) - // An empty request Region means "use the adapter's configured default region". + // There is NO default region: a request that omits one cannot build a client, so + // Provision errors rather than silently guessing. In production every request + // carries a region (admission requires each aws pool to list ≥1; placement stamps + // it), so this only guards a malformed request. if _, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ ClaimName: "claim-def", - }); err != nil { - t.Fatalf("Provision: %v", err) + }); err == nil { + t.Fatal("expected an error for a request with no region") } - if f.lastSpec.Region != testRegion { - t.Fatalf("Region = %q, want adapter default %q", f.lastSpec.Region, testRegion) + if f.runCnt != 0 { + t.Fatalf("RunInstance called %d times, want 0 (no client built)", f.runCnt) } } @@ -286,7 +293,7 @@ func TestProvision_NoAcceleratorIsError(t *testing.T) { // EC2 GPU provisioning is by instance type; a Pod with no accelerator has no // instance type to launch, so it must error rather than silently guessing. - req := provider.ProvisionRequest{ClaimName: "claim-cpu"} + req := provider.ProvisionRequest{ClaimName: "claim-cpu", Region: testRegion} if _, err := p.Provision(context.Background(), gpuPod("", 0), req); err == nil { t.Fatal("expected an error for a Pod requesting no accelerator") } @@ -306,10 +313,12 @@ func TestProvision_Idempotent(t *testing.T) { } p := newTestProvider(f) - id, err := p.Provision(context.Background(), gpuPod("H100", 8), provider.ProvisionRequest{ClaimName: "claim-a"}) + id, err := p.Provision(context.Background(), gpuPod("H100", 8), + provider.ProvisionRequest{ClaimName: "claim-a", Region: testRegion}) if err != nil { t.Fatalf("Provision: %v", err) } + // Raw EC2 id, since idempotent reuse returns the same clean id a fresh launch would. if id != "i-existing" { t.Fatalf("id = %q, want i-existing (idempotent reuse)", id) } @@ -321,7 +330,7 @@ func TestProvision_Idempotent(t *testing.T) { func TestProvision_UnsupportedAccelerator(t *testing.T) { f := &fakeClient{} p := newTestProvider(f) - req := provider.ProvisionRequest{ClaimName: "claim-x"} + req := provider.ProvisionRequest{ClaimName: "claim-x", Region: testRegion} if _, err := p.Provision(context.Background(), gpuPod("TPU-v4", 1), req); err == nil { t.Fatal("expected error for unsupported accelerator") } @@ -341,6 +350,7 @@ func TestGetAndList_NormalizeInstance(t *testing.T) { } p := newTestProvider(f) + // Get takes the raw EC2 id Provision/List hand back; it sweeps regions to find it. got, err := p.Get(context.Background(), "i-1") if err != nil { t.Fatalf("Get: %v", err) @@ -374,23 +384,32 @@ func TestGetAndList_NormalizeInstance(t *testing.T) { if err != nil { t.Fatalf("List: %v", err) } + // List reports the raw EC2 id; a downstream Terminate re-locates it by sweeping + // regions. if len(list) != 1 || list[0].ID != "i-1" { t.Fatalf("List = %+v", list) } } -func TestToInstance_RegionFallsBackToAdapterDefault(t *testing.T) { +func TestToInstance_RegionFromInstance(t *testing.T) { p := newTestProvider(&fakeClient{}) - // An instance that does not report its own region is stamped with the - // adapter's configured region. - inst := p.toInstance(EC2Instance{ID: "i-x", State: stateRunning}) - if inst.Region != testRegion { - t.Fatalf("Region = %q, want adapter default %q", inst.Region, testRegion) + // The region-pinned client always stamps its region onto observed instances, so + // toInstance passes it through verbatim (no default fallback exists anymore). + inst := p.toInstance(EC2Instance{ID: "i-x", State: stateRunning, Region: "ap-south-1"}) + if inst.Region != "ap-south-1" { + t.Fatalf("Region = %q, want ap-south-1 (reported by the instance)", inst.Region) } } func TestTerminate_Idempotent(t *testing.T) { - f := &fakeClient{} + f := &fakeClient{ + instances: []EC2Instance{{ + ID: "i-1", + Tags: map[string]string{ClaimTagKey: "claim-a"}, + State: stateRunning, + Region: testRegion, + }}, + } p := newTestProvider(f) // Empty id: nothing was provisioned; treat as already gone (no client call). @@ -400,6 +419,8 @@ func TestTerminate_Idempotent(t *testing.T) { if len(f.terminated) != 0 { t.Fatalf("Terminate(\"\") called client, want no-op") } + // A raw EC2 id: Terminate sweeps regions, confirms the instance lives in one, and + // terminates it there. if err := p.Terminate(context.Background(), "i-1"); err != nil { t.Fatalf("Terminate: %v", err) } @@ -408,6 +429,21 @@ func TestTerminate_Idempotent(t *testing.T) { } } +// TestTerminate_LegacyQualifiedID covers the back-compat path: an id persisted in the +// old "/i-..." form (before the id stopped being region-qualified) still +// routes straight to that region and terminates the raw id, no sweep needed. +func TestTerminate_LegacyQualifiedID(t *testing.T) { + f := &fakeClient{} + p := newTestProvider(f) + + if err := p.Terminate(context.Background(), testRegion+"/i-legacy"); err != nil { + t.Fatalf("Terminate(legacy): %v", err) + } + if len(f.terminated) != 1 || f.terminated[0] != "i-legacy" { + t.Fatalf("terminated = %v, want [i-legacy]", f.terminated) + } +} + func TestClassifyProvisionError(t *testing.T) { p := newTestProvider(&fakeClient{}) const accel = "H100" @@ -417,10 +453,10 @@ func TestClassifyProvisionError(t *testing.T) { // caller — the provider owns the whole scope). Auth/quota (DenyAll) ignores both. region, wantAccel := testRegion, accel onDemandRegional := provider.BlockScope{ - AcceleratorType: &wantAccel, CapacityType: nebulav1alpha1.CapacityOnDemand, Region: ®ion, + AcceleratorID: &wantAccel, CapacityType: nebulav1alpha1.CapacityOnDemand, Region: ®ion, } spotRegional := provider.BlockScope{ - AcceleratorType: &wantAccel, CapacityType: nebulav1alpha1.CapacitySpot, Region: ®ion, + AcceleratorID: &wantAccel, CapacityType: nebulav1alpha1.CapacitySpot, Region: ®ion, } // A Spot capacity failure the Client wraps with both sentinels: no-capacity @@ -444,7 +480,7 @@ func TestClassifyProvisionError(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := p.ClassifyProvisionError(tt.err, accel) + got := p.ClassifyProvisionError(tt.err, accel, testRegion) if !reflect.DeepEqual(got, tt.want) { t.Fatalf("got %+v, want %+v", got, tt.want) } diff --git a/pkg/provider/aws/client.go b/pkg/provider/aws/client.go index 86d274d..e31eaa2 100644 --- a/pkg/provider/aws/client.go +++ b/pkg/provider/aws/client.go @@ -26,6 +26,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/ec2" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" smithy "github.com/aws/smithy-go" + logf "sigs.k8s.io/controller-runtime/pkg/log" "github.com/InftyAI/Nebula/pkg/provider/catalog" ) @@ -112,45 +113,60 @@ type subnet struct { // compile-time assertion that sdkClient satisfies the adapter's Client seam. var _ Client = (*sdkClient)(nil) -// NewSDKClient builds an EC2-backed Provider for region, ready to register. +// NewSDKClient builds an EC2-backed multi-region Provider, ready to register. // -// It is fully self-configuring: given only credentials and a region, it resolves -// the GPU AMI and the default VPC's subnets itself, so registering AWS in a new -// region needs no pre-created Launch Template, tagged subnets, or any other infra -// — matching the creds-only UX of the NeoCloud adapters. +// regionSource supplies the regions the List/Offerings fan-out sweeps, sourced from +// the NodePool at call time (ProviderSpec.Regions). It is NOT env/flag config: +// regions are the operator's per-pool declaration and change at runtime, and the +// region a Pod lands in already flows NodePool -> placement -> ProvisionRequest, so +// provisioning never needed a configured set. See RegionSource and Provider.sweepRegions. // -// Security contract (fixed regardless of backing): -// - region is the ONLY non-secret config, passed as a plain argument (flag/env). -// Empty means "resolve the SDK's default region" from the standard chain -// (AWS_REGION / shared config / IMDS); the resolved value is echoed on -// observed instances and stamped onto region-scoped blocks. A region that -// resolves to empty is a config skip (ErrConfig). +// Per-region clients are built LAZILY (see Provider.clientFor): this constructor +// only loads the catalog — startup pays no per-region AMI+subnet resolution, no +// region is resolved or validated here (the set is dynamic and comes from the +// NodePool). The first Provision/List into a region resolves that region's GPU AMI +// and default-VPC subnets on demand. +// +// There is NO default region and NO region env (AWS_REGION is not read): every +// request carries its own region (admission requires each aws pool to list ≥1 region; +// placement stamps it), so a fallback would be dead config. This constructor fails +// ONLY if the catalog cannot load — never on region config. +// +// Security contract (and why ONE credential set spans all regions): +// - NO non-secret config is accepted as an argument; regions come from the NodePool +// (API objects), not env/flags. // - Credentials are SECRETS and are NEVER accepted here. The SDK's default -// credential chain (config.LoadDefaultConfig) is used, preferring IRSA / -// instance-role and falling back to AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY -// from the environment (a Kubernetes Secret / .env, not a flag), keeping -// secrets out of process arguments and logs. +// credential chain (config.LoadDefaultConfig, used per-region in the factory) is +// used, preferring IRSA / instance-role and falling back to AWS_ACCESS_KEY_ID / +// AWS_SECRET_ACCESS_KEY from the environment. An IAM principal is ACCOUNT-GLOBAL, +// so the same credentials authorize every region — only the endpoint +// (config.WithRegion) differs per client. No per-region credential is needed. // // The catalog is loaded via catalog.Load() (embedded CSV / mounted ConfigMap), // identical to the other adapters. -func NewSDKClient(ctx context.Context, region string) (*Provider, error) { +func NewSDKClient(_ context.Context, regionSource RegionSource) (*Provider, error) { cat, err := catalog.Load() if err != nil { return nil, fmt.Errorf("aws: load price catalog: %w", err) } - opts := []func(*config.LoadOptions) error{} - if region != "" { - opts = append(opts, config.WithRegion(region)) + factory := func(ctx context.Context, region string) (Client, error) { + return newSDKClientForRegion(ctx, region) } - cfg, err := config.LoadDefaultConfig(ctx, opts...) + return New(factory, cat, regionSource), nil +} + +// newSDKClientForRegion builds one region-pinned sdkClient: it loads SDK config for +// that region (same account-global credential chain, region endpoint overridden), +// then resolves the region's GPU AMI (required) and default-VPC subnets +// (best-effort). This is the ClientFactory body the Provider calls lazily per region. +func newSDKClientForRegion(ctx context.Context, region string) (Client, error) { + cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(region)) if err != nil { - return nil, fmt.Errorf("aws: load SDK config: %w", err) + return nil, fmt.Errorf("aws: load SDK config for %s: %w", region, err) } if cfg.Region == "" { - // Neither the argument nor the SDK chain resolved a region; without one the - // EC2 endpoint is undefined. Treat as unconfigured (non-fatal skip). - return nil, fmt.Errorf("aws: no region resolved: %w", ErrConfig) + return nil, fmt.Errorf("aws: no region resolved for %q: %w", region, ErrConfig) } c := &sdkClient{ @@ -173,7 +189,7 @@ func NewSDKClient(ctx context.Context, region string) (*Provider, error) { } c.subnets = subnets - return New(c, cat, cfg.Region), nil + return c, nil } // gpuAMINameFilter matches the Amazon-owned Amazon-Linux-2 ECS GPU-optimized AMI. @@ -219,11 +235,26 @@ func (c *sdkClient) RunInstance(ctx context.Context, spec InstanceSpec) (string, targets = []subnet{{}} // single attempt, letting EC2 pick the subnet } + log := logf.FromContext(ctx).WithName("aws-run").WithValues( + "region", c.region, "instanceType", spec.InstanceType, "spot", spec.Spot) + log.V(1).Info("starting zone sweep", "candidateZones", len(targets)) + var lastErr error - for _, sn := range targets { + for i, sn := range targets { + // A zone attempt logs its start and its outcome so a capacity sweep across AZs + // is traceable (which zones were tried, in order, and why each was abandoned). + // zone "" means no default VPC was discovered and EC2 is picking the subnet. + zone := sn.az + if zone == "" { + zone = "" + } + log.V(1).Info("zone attempt starting", "zone", zone, "subnet", sn.id, "attempt", i+1, "of", len(targets)) + // Honor the deadline between attempts: a slow first zone must not let the // sweep overrun the provision timeout. if err := ctx.Err(); err != nil { + log.Info("zone sweep aborted before completion; provision deadline reached", + "zone", zone, "attempted", i, "of", len(targets)) if lastErr != nil { return "", lastErr } @@ -232,6 +263,7 @@ func (c *sdkClient) RunInstance(ctx context.Context, spec InstanceSpec) (string, id, err := c.runInSubnet(ctx, spec, userData, sn) if err == nil { + log.Info("zone attempt succeeded", "zone", zone, "subnet", sn.id, "instanceID", id) return id, nil } classified := classifyEC2Error(err, spec.Spot) @@ -245,12 +277,18 @@ func (c *sdkClient) RunInstance(ctx context.Context, spec InstanceSpec) (string, // futile sweep. It still carries ErrNoCapacity, so ClassifyProvisionError // blocks the region correctly. if !errors.Is(classified, errZoneLocal) { + log.Info("zone attempt failed with a non-zone-local error; stopping sweep", + "zone", zone, "subnet", sn.id, "error", classified.Error()) return "", classified } + log.Info("zone attempt failed with a zone-local capacity shortfall; trying next zone", + "zone", zone, "subnet", sn.id, "remainingZones", len(targets)-i-1) lastErr = classified } // Every zone was capacity-starved; return the last capacity error so // ClassifyProvisionError can block the region and region-failover can proceed. + log.Info("zone sweep exhausted; every candidate zone was capacity-starved", + "zonesTried", len(targets)) return "", lastErr } @@ -441,8 +479,19 @@ func (c *sdkClient) ListInstances(ctx context.Context) ([]EC2Instance, error) { // actually passed their 2/2 checks. A failure here is not fatal — the instances // are still returned (StatusChecksPassed stays false), so the poll loop simply // holds them at Pending one more tick rather than the whole List erroring out. + // + // It IS logged (not silently swallowed): a PERSISTENT failure here — most often a + // missing ec2:DescribeInstanceStatus IAM permission, distinct from the + // DescribeInstances grant this same List already relied on — pins every running + // instance at StatusChecksPassed=false forever, so its Pod is stranded at + // Pending/Provisioning even though the instance is healthy. Without this log that + // failure is invisible (List still returns the instances), so surface it. passed, err := c.statusChecksByID(ctx, ids) if err != nil { + logf.FromContext(ctx).WithName("aws-list").Error(err, + "describe instance status checks failed; instances held at Pending until this recovers "+ + "(often a missing ec2:DescribeInstanceStatus permission)", + "region", c.region, "instances", len(ids)) return out, nil } for i := range out { diff --git a/pkg/provider/aws/translate.go b/pkg/provider/aws/translate.go index 0e1d7b7..e3db901 100644 --- a/pkg/provider/aws/translate.go +++ b/pkg/provider/aws/translate.go @@ -148,8 +148,10 @@ func classifyEC2Error(err error, spot bool) error { // wrapNoCapacity wraps err with provider.ErrNoCapacity plus, conditionally, the // Spot-tier marker (ErrSpotCapacity) and the zone-local sweep marker -// (errZoneLocal). errors.Join lets errors.Is find every marker regardless of how -// many apply. +// (errZoneLocal). It builds a single multi-%w chain (Go 1.20+) rather than +// errors.Join: errors.Is still finds every marker, but the rendered message stays +// on ONE line (": "-separated) instead of errors.Join's one-error-per-line form, +// which is far more readable when the chain is surfaced as a Pod Event / log line. func wrapNoCapacity(err error, spot, zoneLocal bool) error { errs := []error{err, provider.ErrNoCapacity} if spot { @@ -158,5 +160,12 @@ func wrapNoCapacity(err error, spot, zoneLocal bool) error { if zoneLocal { errs = append(errs, errZoneLocal) } - return errors.Join(errs...) + // fmt.Errorf with N "%w" verbs wraps every error (so errors.Is/As unwrap them + // all) while joining their messages with ": " on a single line. + format := strings.TrimPrefix(strings.Repeat(": %w", len(errs)), ": ") + args := make([]any, len(errs)) + for i, e := range errs { + args[i] = e + } + return fmt.Errorf(format, args...) } diff --git a/pkg/provider/catalog/base.go b/pkg/provider/catalog/base.go index 106c5dc..1d78392 100644 --- a/pkg/provider/catalog/base.go +++ b/pkg/provider/catalog/base.go @@ -76,23 +76,38 @@ func (b Base) Offerings(context.Context) ([]provider.Offering, error) { return b.Catalog.Offerings(b.ProviderName), nil } -// MapAccelerator translates a canonical accelerator type into this provider's -// own accelerator id using the catalog as the mapping table: it finds the row -// whose AcceleratorType matches (case-insensitively) and returns that row's -// AcceleratorID. When AcceleratorID is blank (a provider whose id equals the -// canonical name) it falls back to the canonical name, so an identity-mapped -// provider needs no per-name data. Because the mapping lives entirely in the -// CSV, a provider whose ids diverge (AWS instance types) does NOT need to -// override this — it just populates the accelerator_id column. Returns ok=false -// when the provider does not offer the accelerator. -func (b Base) MapAccelerator(canonical string) (providerAcceleratorID string, ok bool) { +// MapAccelerator translates a canonical accelerator request (type + count) into +// this provider's own accelerator id using the catalog as the mapping table. It +// finds the offering row whose AcceleratorType matches (case-insensitively) and +// whose GPUCount matches the request, and returns that row's AcceleratorID. When +// AcceleratorID is blank (a provider whose id equals the canonical name) it falls +// back to the canonical name, so an identity-mapped provider needs no per-name +// data. Because the mapping lives entirely in the CSV, a provider whose ids +// diverge (AWS instance types) does NOT need to override this — it just populates +// the accelerator_id and gpu_count columns. +// +// Count matching honours the two catalog shapes (see Offering.GPUCount): a +// provider that bakes the count into the offering (AWS: T4x1=g4dn.xlarge, +// T4x8=g4dn.metal) emits one row per count, so the row whose GPUCount equals the +// request is the match — this is what keeps (L4, 1) and (L4, 8) on DISTINCT ids so +// one's capacity block does not exclude the other. A provider that takes the count +// as a free parameter (Modal) leaves GPUCount 0 on its single row; a 0-count row +// matches any requested count, since count is not a lookup dimension there. +// Returns ok=false when the provider offers no row for that (type, count). +func (b Base) MapAccelerator(canonical string, count int32) (providerAcceleratorID string, ok bool) { for _, o := range b.Catalog.Offerings(b.ProviderName) { - if strings.EqualFold(o.AcceleratorType, canonical) { - if o.AcceleratorID != "" { - return o.AcceleratorID, true - } - return o.AcceleratorType, true + if !strings.EqualFold(o.AcceleratorType, canonical) { + continue + } + // GPUCount 0 => count is not a lookup dimension for this provider (matches any + // requested count); otherwise the row's count must equal the request. + if o.GPUCount != 0 && o.GPUCount != count { + continue + } + if o.AcceleratorID != "" { + return o.AcceleratorID, true } + return o.AcceleratorType, true } return "", false } diff --git a/pkg/provider/catalog/catalog_test.go b/pkg/provider/catalog/catalog_test.go index 44bbcaa..f082af1 100644 --- a/pkg/provider/catalog/catalog_test.go +++ b/pkg/provider/catalog/catalog_test.go @@ -204,7 +204,7 @@ func TestBaseMapAccelerator_CaseInsensitive(t *testing.T) { } for name, tc := range cases { t.Run(name, func(t *testing.T) { - got, ok := base.MapAccelerator(tc.in) + got, ok := base.MapAccelerator(tc.in, 1) if ok != tc.wantOK || got != tc.want { t.Fatalf("MapAccelerator(%q) = (%q, %v), want (%q, %v)", tc.in, got, ok, tc.want, tc.wantOK) } diff --git a/pkg/provider/catalog/data/aws.csv b/pkg/provider/catalog/data/aws.csv index 458e5be..a3df592 100644 --- a/pkg/provider/catalog/data/aws.csv +++ b/pkg/provider/catalog/data/aws.csv @@ -50,6 +50,8 @@ L4,g6.12xlarge,4,OnDemand,4.602,true,,2026-07-27 L4,g6.12xlarge,4,Spot,1.610,true,,2026-07-27 L4,g6.48xlarge,8,OnDemand,13.350,true,,2026-07-27 L4,g6.48xlarge,8,Spot,4.672,true,,2026-07-27 +A100-40GB,p4d.24xlarge,8,OnDemand,32.773,true,,2026-07-29 +A100-40GB,p4d.24xlarge,8,Spot,11.470,true,,2026-07-29 A100-80GB,p4de.24xlarge,8,OnDemand,40.966,true,,2026-07-27 A100-80GB,p4de.24xlarge,8,Spot,14.338,true,,2026-07-27 H100,p5.48xlarge,8,OnDemand,98.320,true,,2026-07-27 diff --git a/pkg/provider/errors.go b/pkg/provider/errors.go index dfd6452..031a84c 100644 --- a/pkg/provider/errors.go +++ b/pkg/provider/errors.go @@ -53,31 +53,33 @@ var ( // a failure does not hot-loop (the blocklist TTL bounds the blast radius). // // It is the single place the SHARED part of a scope is derived — category -// (DenyAll vs capacity-scoped), the capacity tier, and the accelerator — so every -// adapter's ClassifyProvisionError delegates here and then only decorates with -// what is provider-specific (AWS adds its region). No scope is assembled anywhere -// else: the vnode handler resolves the accelerator off the Pod and passes it in, -// rather than mutating the returned scope. +// (DenyAll vs capacity-scoped), the capacity tier, and the accelerator id — so +// every adapter's ClassifyProvisionError delegates here and then only decorates +// with what is provider-specific (AWS adds its region). No scope is assembled +// anywhere else: the vnode handler resolves the accelerator id off the Pod and +// passes it in, rather than mutating the returned scope. // // capacityType is the tier the failing request used; it is stamped onto // accelerator-scoped scopes so the block is precise (a Spot failure does not -// block OnDemand). accelerator is the canonical accelerator the request asked for -// ("" for a CPU-only Pod): on a capacity-scoped block a non-empty accelerator -// becomes an exact-match pointer (narrowing the block to the type that failed), -// while "" leaves AcceleratorType nil ("not applicable") so the block does not -// widen across every accelerator. Auth/quota (DenyAll) ignores both, since bad -// credentials fail for every accelerator and tier. -func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, accelerator string) BlockScope { +// block OnDemand). acceleratorID is the provider's RESOLVED id for what served the +// failing request (the (type, count) mapped through MapAccelerator — an EC2 +// instance type on AWS, the canonical name on Modal; "" for a CPU-only Pod): on a +// capacity-scoped block a non-empty id becomes an exact-match pointer (narrowing +// the block to the instance type / capacity pool that actually failed), while "" +// leaves AcceleratorID nil ("not applicable") so the block does not widen across +// every accelerator. Auth/quota (DenyAll) ignores both, since bad credentials fail +// for every accelerator and tier. +func ClassifyError(err error, capacityType nebulav1alpha1.CapacityType, acceleratorID string) BlockScope { if err == nil { return BlockScope{} } // capacityScope builds an accelerator/tier-scoped block, promoting a non-empty - // accelerator to an exact-match pointer and leaving it nil otherwise. + // accelerator id to an exact-match pointer and leaving it nil otherwise. capacityScope := func() BlockScope { s := BlockScope{CapacityType: capacityType} - if accelerator != "" { - s.AcceleratorType = &accelerator + if acceleratorID != "" { + s.AcceleratorID = &acceleratorID } return s } diff --git a/pkg/provider/errors_test.go b/pkg/provider/errors_test.go index 6e80b6c..6c52ade 100644 --- a/pkg/provider/errors_test.go +++ b/pkg/provider/errors_test.go @@ -27,9 +27,9 @@ import ( func TestClassifyError(t *testing.T) { const tier = nebulav1alpha1.CapacityOnDemand const accel = "H100" - // capacityScope is the expected accelerator-scoped block: the accelerator is + // capacityScope is the expected accelerator-scoped block: the accelerator id is // promoted to an exact-match pointer. - capacityScope := BlockScope{CapacityType: tier, AcceleratorType: ptrStr(accel)} + capacityScope := BlockScope{CapacityType: tier, AcceleratorID: ptrStr(accel)} tests := []struct { name string err error @@ -64,12 +64,12 @@ func TestClassifyError_TierStamped(t *testing.T) { } } -// An empty accelerator (CPU-only Pod) must leave AcceleratorType nil ("not +// An empty accelerator (CPU-only Pod) must leave AcceleratorID nil ("not // applicable"), never a wildcard that would widen the block across every GPU. func TestClassifyError_EmptyAcceleratorStaysNil(t *testing.T) { got := ClassifyError(ErrNoCapacity, nebulav1alpha1.CapacityOnDemand, "") - if got.AcceleratorType != nil { - t.Fatalf("expected nil AcceleratorType for a CPU-only request, got %+v", got.AcceleratorType) + if got.AcceleratorID != nil { + t.Fatalf("expected nil AcceleratorID for a CPU-only request, got %+v", got.AcceleratorID) } if got.DenyAll || got.CapacityType != nebulav1alpha1.CapacityOnDemand { t.Fatalf("expected an OnDemand capacity scope, got %+v", got) diff --git a/pkg/provider/fake/fake.go b/pkg/provider/fake/fake.go index a436606..36fbfec 100644 --- a/pkg/provider/fake/fake.go +++ b/pkg/provider/fake/fake.go @@ -148,7 +148,8 @@ func (p *Provider) List(_ context.Context) ([]provider.Instance, error) { // ClassifyProvisionError never really fails to provision, so any error it is // asked to classify is treated as a whole-provider block on OnDemand — the same // shared derivation the real adapters use. -func (p *Provider) ClassifyProvisionError(err error, accelerator string) provider.BlockScope { +func (p *Provider) ClassifyProvisionError(err error, accelerator, _ string) provider.BlockScope { + // Region is ignored: the fake is region-simple, matching the OnDemand NeoClouds. return provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, accelerator) } diff --git a/pkg/provider/fake/fake_test.go b/pkg/provider/fake/fake_test.go index d727ea2..9981515 100644 --- a/pkg/provider/fake/fake_test.go +++ b/pkg/provider/fake/fake_test.go @@ -120,10 +120,10 @@ func TestMapAcceleratorFromCatalog(t *testing.T) { p := New() // A GPU in the fixed catalog resolves (case-insensitively); one that is not // offered reports ok=false, so selectPlacement skips the fake for it. - if _, ok := p.MapAccelerator("h100"); !ok { + if _, ok := p.MapAccelerator("h100", 1); !ok { t.Fatal("expected H100 to be offered by the fake catalog") } - if _, ok := p.MapAccelerator("TPU-v4"); ok { + if _, ok := p.MapAccelerator("TPU-v4", 1); ok { t.Fatal("did not expect TPU-v4 to be offered by the fake catalog") } } diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index d59961d..4be7d6c 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -230,7 +230,9 @@ func (p *Provider) List(ctx context.Context) ([]provider.Instance, error) { // matching shared sentinel (e.g. fmt.Errorf("...: %w", provider.ErrNoCapacity)); // ClassifyError honours those first and falls back to string heuristics for raw // API messages, so no Modal-specific matching is duplicated here. -func (p *Provider) ClassifyProvisionError(err error, accelerator string) provider.BlockScope { +func (p *Provider) ClassifyProvisionError(err error, accelerator, _ string) provider.BlockScope { + // Region is ignored: Modal is region-simple (no region axis), so the block's + // Region stays nil — see BlockScope's three-state rule. return provider.ClassifyError(err, nebulav1alpha1.CapacityOnDemand, accelerator) } @@ -286,7 +288,7 @@ func (p *Provider) sandboxSpecFromPod(pod *corev1.Pod, req provider.ProvisionReq return SandboxSpec{}, fmt.Errorf("modal: %w", err) } if canonical != "" { - modalGPU, ok := p.MapAccelerator(canonical) + modalGPU, ok := p.MapAccelerator(canonical, count) if !ok { return SandboxSpec{}, fmt.Errorf("modal: unsupported accelerator %q", canonical) } diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index bd23c41..b3912ec 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -278,7 +278,7 @@ func TestClassifyProvisionError(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := p.ClassifyProvisionError(tt.err, "") + got := p.ClassifyProvisionError(tt.err, "", "") if got != tt.want { t.Fatalf("got %+v, want %+v", got, tt.want) } diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index b59fa9b..e50f339 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -86,24 +86,33 @@ type Provider interface { // --- Translation ----------------------------------------------------- - // MapAccelerator translates a canonical Nebula accelerator type ("H100", - // "A100-80GB", "TPU-v4") into this provider's identifier (e.g. RunPod's - // "NVIDIA H100 80GB HBM3"). Returns ok=false if the provider does not offer - // that accelerator. - MapAccelerator(canonical string) (providerAcceleratorID string, ok bool) + // MapAccelerator translates a canonical Nebula accelerator request (type + + // count) into this provider's identifier for what actually serves it (e.g. + // RunPod's "NVIDIA H100 80GB HBM3", or AWS's "g6.48xlarge"). Count is part of + // the key because on some providers the GPU count is baked into the offering + // rather than a free knob: on AWS (L4, 1) and (L4, 8) resolve to DIFFERENT + // instance types (g6.xlarge vs g6.48xlarge) — distinct capacity pools — so the + // id must distinguish them. Providers that attach an arbitrary count to a single + // offering (Modal) ignore count and return the same id for every count. Returns + // ok=false if the provider does not offer that (type, count). The returned id is + // what failover keys a capacity block on, so two requests that share a capacity + // pool must return the same id and two that do not must differ. + MapAccelerator(canonical string, count int32) (providerAcceleratorID string, ok bool) // ClassifyProvisionError maps a Provision error to the granularity at which // the failing placement should be blocklisted. This keeps failover precise: - // a "no H100 capacity" error blocks only {provider, H100, capacityType}, + // a "no H100 capacity" error blocks only {provider, H100, capacityType, region}, // while an auth/quota error blocks the whole provider. See BlockScope. // - // accelerator is the canonical accelerator the failing request asked for ("" - // for a CPU-only Pod). The provider needs it because the error alone does not - // carry it, and the provider is the single owner of the complete scope: it - // stamps the accelerator on a capacity block AND adds any provider-specific - // axis it knows (AWS confines the block to its region). No caller assembles a - // scope piecemeal after this returns. - ClassifyProvisionError(err error, accelerator string) BlockScope + // accelerator and region are the two request facts the error itself does not + // carry: the accelerator the failing request asked for ("" for a CPU-only Pod) + // and the region it targeted ("" for a region-simple provider, or when the + // request did not pin one). The provider is the single owner of the complete + // scope — it stamps the accelerator on a capacity block, confines the block to + // the region that actually failed (a region-aware provider), and widens + // auth/quota to the whole provider. No caller assembles a scope piecemeal after + // this returns. + ClassifyProvisionError(err error, accelerator, region string) BlockScope } // ProvisionRequest carries only the decisions the placement controller made @@ -232,7 +241,7 @@ type Offering struct { // BlockScope is the granularity at which a failed placement is excluded, matched // to what actually failed (SkyPilot's blocklist-granularity rule). It is a partial -// MATCH PATTERN, not a value, and AcceleratorType/Region are THREE-STATE pointers +// MATCH PATTERN, not a value, and AcceleratorID/Region are THREE-STATE pointers // so a pattern can distinguish "any value" from "no value": // // - nil => the axis is NOT APPLICABLE to this block: it matches only a @@ -257,13 +266,18 @@ type Offering struct { // mixes the two: a NodePool candidate is resolved to fully-concrete values before // it is matched against these patterns. type BlockScope struct { - // AcceleratorType: nil => not applicable (CPU-only Pod, or a DenyAll scope); - // &"" => wildcard, matches every accelerator; &"H100" => exactly that one. - // A capacity error carries no accelerator (it is classified from the error - // alone), so the adapter leaves this nil and the vnode handler fills it in - // from the failing Pod's request — narrowing the block to the accelerator that - // actually failed. - AcceleratorType *string + // AcceleratorID: nil => not applicable (CPU-only Pod, or a DenyAll scope); + // &"" => wildcard, matches every accelerator; &"g6.48xlarge" => exactly that + // one. It is the provider's RESOLVED id for what serves the failing request — + // the (type, count) mapped through MapAccelerator (an EC2 instance type on AWS, + // the canonical GPU name on Modal) — NOT the bare accelerator type. Keying on + // the resolved id is what confines a capacity block to the pool that actually + // ran out: an L4x8 (g6.48xlarge) Spot shortage must not exclude L4x1 + // (g6.xlarge), a different instance type and capacity pool, though both are + // "L4". A capacity error carries no accelerator (it is classified from the error + // alone), so the adapter leaves this nil and the vnode handler fills it in from + // the failing Pod's resolved request. + AcceleratorID *string // CapacityType empty => blocks all capacity types. CapacityType nebulav1alpha1.CapacityType // Region: nil => the provider has no region axis (a region-simple provider like @@ -273,7 +287,7 @@ type BlockScope struct { // region-aware provider (AWS) sets it; a region-simple one leaves it nil. Region *string // DenyAll true => block everything on this provider (e.g. auth/quota errors), - // ignoring AcceleratorType/CapacityType/Region. The scope is still this one + // ignoring AcceleratorID/CapacityType/Region. The scope is still this one // provider; it never spans providers. DenyAll bool } diff --git a/pkg/util/claim.go b/pkg/util/claim.go index 2359cee..3298f7b 100644 --- a/pkg/util/claim.go +++ b/pkg/util/claim.go @@ -18,13 +18,53 @@ limitations under the License. // control plane and provider adapters. package util +import ( + "crypto/sha256" + "encoding/hex" +) + +const ( + // maxClaimNameLen is the DNS-subdomain cap Kubernetes enforces on a NodeClaim's + // metadata.name (the claim is a cluster-scoped object). The token must never + // exceed it, or ensureClaim's create fails and the Pod is never placed. + maxClaimNameLen = 253 + // claimHashLen is the hex width of the disambiguating suffix appended only when + // a name is truncated. 10 hex chars = 40 bits, ample against collision among the + // (few) over-length names in one cluster. + claimHashLen = 10 +) + // ClaimName is the instance-identity token Nebula encodes into a provider // instance's name/tag so List/Terminate can find it later without a durable id. -// It is a deterministic function of the served workload's namespace and name, so -// any component that knows the Pod (the virtual kubelet) or the claim's PodRef -// (the teardown backstop) computes the same token. Keep this the single source -// of truth for the convention — the vnode handler and the NodeClaim finalizer -// both depend on producing identical values. +// It is a deterministic, PURE function of the served workload's namespace and +// name, so any component that knows the Pod (the virtual kubelet) or the claim's +// PodRef (the teardown backstop) computes the same token. Keep this the single +// source of truth for the convention — the vnode handler and the NodeClaim +// finalizer both depend on producing identical values. +// +// The common case is a plain "namespace-name" join, kept verbatim: it is the +// historical format, so instances already tagged this way keep matching, and it +// is human-readable in `kubectl get nodeclaim`. +// +// A NodeClaim's metadata.name is capped at maxClaimNameLen (a cluster-scoped +// object name is a DNS subdomain). A long namespace + long Pod name can exceed +// that, and then the join would be an invalid, un-creatable name — the Pod would +// never be placed. Only in that over-length case do we fall back to a bounded, +// collision-resistant form: the join truncated to fit, with a hash of the full, +// canonical "namespace/name" key appended. Hashing the "/"-joined key (— "/" is +// illegal in both a namespace and a name, so the input is unambiguous) keeps the +// suffix stable and distinct even when two different (namespace, name) pairs +// truncate to the same prefix. func ClaimName(namespace, name string) string { - return namespace + "-" + name + joined := namespace + "-" + name + if len(joined) <= maxClaimNameLen { + return joined + } + + // Over the cap: truncate the readable join and append a hash of the canonical + // key so the token stays unique and within the limit. + sum := sha256.Sum256([]byte(namespace + "/" + name)) + suffix := hex.EncodeToString(sum[:])[:claimHashLen] + prefix := joined[:maxClaimNameLen-claimHashLen-1] // room for "-" + return prefix + "-" + suffix } diff --git a/pkg/util/claim_test.go b/pkg/util/claim_test.go new file mode 100644 index 0000000..ec096fe --- /dev/null +++ b/pkg/util/claim_test.go @@ -0,0 +1,81 @@ +/* +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 util + +import ( + "strings" + "testing" +) + +func TestClaimName_ShortStaysReadableAndCompatible(t *testing.T) { + // The common case must remain the historical "namespace-name" join, verbatim: + // instances already tagged this way keep matching the teardown backstop, and it + // stays readable. Any change here silently orphans live instances. + if got := ClaimName("default", "p1"); got != "default-p1" { + t.Fatalf("ClaimName(default, p1) = %q, want default-p1", got) + } +} + +func TestClaimName_Deterministic(t *testing.T) { + // The whole scheme rests on every component deriving the SAME token from the + // same (namespace, name) — the vnode handler tags with it, the backstop matches + // on it. It must be a pure function. + for _, tc := range []struct{ ns, name string }{ + {"default", "p1"}, + {strings.Repeat("n", 120), strings.Repeat("p", 200)}, // over-length branch + } { + want := ClaimName(tc.ns, tc.name) + for i := 0; i < 3; i++ { + if got := ClaimName(tc.ns, tc.name); got != want { + t.Fatalf("ClaimName(%q,%q) is not deterministic: %q != %q", tc.ns, tc.name, got, want) + } + } + } +} + +func TestClaimName_OverLengthIsBoundedAndUnique(t *testing.T) { + // A long namespace + long Pod name would exceed the k8s object-name cap; the + // token must be truncated to fit (else the NodeClaim create fails and the Pod is + // never placed) while staying distinct across different inputs that share a + // truncated prefix. + ns := strings.Repeat("n", 200) + a := ClaimName(ns, strings.Repeat("a", 200)) + b := ClaimName(ns, strings.Repeat("a", 199)+"b") + + if len(a) > maxClaimNameLen { + t.Fatalf("over-length claim name not bounded: len=%d > %d", len(a), maxClaimNameLen) + } + if a == b { + t.Fatal("distinct over-length inputs collided; hash suffix must disambiguate") + } + if !strings.HasPrefix(a, ns+"-") { + t.Fatalf("expected readable prefix retained, got %q", a) + } +} + +func TestClaimName_BoundaryExactlyAtCapIsNotHashed(t *testing.T) { + // A join landing exactly on the cap must stay the plain readable form (the hash + // fallback triggers only strictly above the cap). + name := strings.Repeat("p", maxClaimNameLen-len("ns-")) + got := ClaimName("ns", name) + if len(got) != maxClaimNameLen { + t.Fatalf("expected exact-cap join of len %d, got %d", maxClaimNameLen, len(got)) + } + if got != "ns-"+name { + t.Fatal("a join exactly at the cap must not be hashed") + } +} diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index e42630b..3b8571d 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -18,6 +18,7 @@ package vnode import ( "context" + "encoding/json" "io" "sync" "time" @@ -28,7 +29,10 @@ import ( vkapi "github.com/virtual-kubelet/virtual-kubelet/node/api" "github.com/virtual-kubelet/virtual-kubelet/node/api/statsv1alpha1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" logf "sigs.k8s.io/controller-runtime/pkg/log" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" @@ -85,6 +89,13 @@ const defaultProvisionTimeout = 90 * time.Second type Handler struct { prov provider.Provider + // client patches the endpoint annotation onto the Pod's metadata. VK persists + // only the status subresource (its UpdateStatus never touches metadata), so the + // reachable address — which must be visible on the Pod for anyone to reach the + // workload — needs a metadata write of its own. May be nil in tests, where the + // annotation is set on the in-memory tracked Pod but not persisted. + client kubernetes.Interface + // blocklist records failed placements so the placement controller can fail over // (zone → region → tier). Shared across every provider's handler and the // placement controller. May be nil (a no-op), keeping tests and any @@ -108,19 +119,27 @@ type trackedPod struct { pod *corev1.Pod claimName string instance string + // persistedEndpoint is the endpoint value last written to the Pod's metadata + // annotation via persistEndpoint. The notify callback fires every poll tick, so + // this dedups the metadata patch to the ticks where the reachable address + // actually changed (first appearance, or a re-provision) rather than every tick. + persistedEndpoint string } // NewHandler builds a Handler for the given provider backend. The poll cadence // comes from the provider's Capabilities.PollInterval, falling back to // defaultPollInterval when the provider does not set one. blocklist is the shared // failover blocklist a Provision failure is recorded on; it may be nil (no-op). -func NewHandler(prov provider.Provider, blocklist Blocklister) *Handler { +// client is used to patch the endpoint annotation onto the Pod metadata (VK only +// writes status); it may be nil in tests, where the annotation stays in-memory. +func NewHandler(prov provider.Provider, client kubernetes.Interface, blocklist Blocklister) *Handler { poll := prov.Capabilities().PollInterval if poll <= 0 { poll = defaultPollInterval } return &Handler{ prov: prov, + client: client, blocklist: blocklist, tracked: make(map[string]*trackedPod), nowFn: metav1.Now, @@ -158,6 +177,9 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { Region: pod.Annotations[nebulav1alpha1.RegionAnnotation], } + log := logf.FromContext(ctx).WithName("vnode-handler").WithValues( + "provider", h.prov.Name(), "pod", key(pod.Namespace, pod.Name), "claim", claim) + // Bound the provision call so a wedged backend cannot pin this worker forever. // The provider may raise the deadline via Capabilities.ProvisionTimeout (AWS // does, to leave room for cross-zone failover); zero means "use the default". @@ -168,15 +190,18 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() + log.Info("provisioning external instance", + "capacityType", req.CapacityType, "region", req.Region, "timeout", timeout.String()) id, err := h.prov.Provision(ctx, pod, req) if err != nil { + log.Error(err, "provision failed; Pod marked Failed for failover") // Record the failure on the shared blocklist so placement fails over to the // next candidate (zone → region → tier) instead of hot-looping here. The // provider classifies its own error into the precise BlockScope (a Spot // no-capacity in one region blocks only that, not OnDemand or other regions; // auth/quota blocks the whole provider); the TTL rides on the Pod from the // pool's FailoverPolicy. - h.recordBlock(pod, err) + h.recordBlock(ctx, pod, err) // Surface the failure on the Pod so the placement controller can fail // over, and return the error so the pod controller retries with backoff. h.markStatus(pod, corev1.PodFailed, reasonProvisionFailed, err.Error()) @@ -186,6 +211,7 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { } // Record the instance id before reporting success; teardown relies on it. + log.Info("external instance provisioned", "instanceID", id) h.markStatus(pod, corev1.PodPending, reasonProvisioning, "provisioning external instance") h.store(pod, claim, id) h.emit(pod) @@ -220,12 +246,20 @@ func (h *Handler) DeletePod(ctx context.Context, pod *corev1.Pod) error { } h.mu.Unlock() + log := logf.FromContext(ctx).WithName("vnode-handler").WithValues( + "provider", h.prov.Name(), "pod", key(pod.Namespace, pod.Name), "instanceID", instance) + + log.Info("terminating external instance") if err := h.prov.Terminate(ctx, instance); err != nil { + // A failed Terminate is the leak-risk path: VK will retry DeletePod, but if it + // never succeeds the NodeClaim backstop is the last line of defense. Log loudly. + log.Error(err, "terminate failed; external instance may still be running (NodeClaim backstop will retry)") return err } // Report a terminal status, then forget the pod. VK expects the containers // and pod to reach a terminal state after DeletePod. + log.Info("external instance terminated") h.markStatus(pod, corev1.PodSucceeded, "Terminated", "external instance terminated") pod.DeletionTimestamp = ptrNow(h.nowFn()) h.emit(pod) @@ -238,14 +272,55 @@ func (h *Handler) DeletePod(ctx context.Context, pod *corev1.Pod) error { // GetPod returns the tracked pod, or a NotFound error the pod controller // understands. -func (h *Handler) GetPod(_ context.Context, namespace, name string) (*corev1.Pod, error) { +// +// Tracking is in-memory, so a VK restart (redeploy, crash, OOM, leader handoff) +// starts with an empty map even though the external instances are still running. +// VK's createOrUpdatePod calls GetPod to decide adopt-vs-create: a nil result +// makes it re-issue CreatePod, which resets the Pod's status to Provisioning and +// re-drives provisioning. To avoid that regression we RE-ADOPT here — when the +// Pod is not in the map we ask the provider whether an instance with this Pod's +// claim is still live, and if so rebuild the tracking entry from it. VK then sees +// a non-nil pod and takes the UpdatePod (adopt) branch, and the re-tracked pod is +// advanced to its true state by the next poll tick. +func (h *Handler) GetPod(ctx context.Context, namespace, name string) (*corev1.Pod, error) { h.mu.Lock() - defer h.mu.Unlock() tp, ok := h.tracked[key(namespace, name)] - if !ok { + h.mu.Unlock() + if ok { + return tp.pod.DeepCopy(), nil + } + + // Cold map: re-adopt from the live provider if the instance still exists. + claim := util.ClaimName(namespace, name) + inst, found := h.instanceByClaim(ctx, claim) + if !found { return nil, errdefs.NotFoundf("pod %s/%s not found on virtual node", namespace, name) } - return tp.pod.DeepCopy(), nil + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} + applyState(pod, inst.State, inst.Endpoint, h.nowFn()) + h.store(pod, claim, inst.ID) + logf.FromContext(ctx).WithName("vnode-handler").Info( + "re-adopted live instance after cold tracking map (VK restart)", + "provider", h.prov.Name(), "pod", key(namespace, name), "claim", claim, + "instanceID", inst.ID, "state", inst.State) + return pod.DeepCopy(), nil +} + +// instanceByClaim returns the live provider instance whose ClaimName matches, and +// whether one was found. A List error yields (zero,false): re-adoption then falls +// through to NotFound and VK retries on its next sync rather than acting on a +// half-known fleet. It backs GetPod's post-restart re-adoption. +func (h *Handler) instanceByClaim(ctx context.Context, claim string) (provider.Instance, bool) { + instances, err := h.prov.List(ctx) + if err != nil { + return provider.Instance{}, false + } + for _, inst := range instances { + if inst.ClaimName == claim { + return inst, true + } + } + return provider.Instance{}, false } // GetPodStatus returns the tracked pod's status. @@ -272,9 +347,21 @@ func (h *Handler) GetPods(_ context.Context) ([]*corev1.Pod, error) { // NotifyPods registers the async status callback and starts the poll loop. VK // calls this once at startup; the loop runs until ctx is cancelled. +// +// We WRAP VK's callback rather than store it raw. VK's cb drives its status path +// (enqueuePodStatusUpdate -> UpdateStatus), which writes only the /status +// subresource and silently drops any metadata change on the same object. The +// reachable endpoint must live on the Pod's metadata (PodIP cannot hold a DNS +// name — see applyState), so the wrapper first persists the endpoint annotation +// with its own metadata patch, then hands the same Pod to VK for the status +// write. This folds the two writes onto the one notify signal: every status push +// also reconciles the endpoint annotation (deduped so only a real change patches). func (h *Handler) NotifyPods(ctx context.Context, cb func(*corev1.Pod)) { h.mu.Lock() - h.notify = cb + h.notify = func(pod *corev1.Pod) { + h.persistEndpoint(ctx, pod) + cb(pod) + } h.mu.Unlock() go h.pollLoop(ctx) @@ -320,21 +407,37 @@ func (h *Handler) reconcileOnce(ctx context.Context) { } h.mu.Lock() - changed := make([]*corev1.Pod, 0) + emit := make([]*corev1.Pod, 0, len(h.tracked)) tracked := len(h.tracked) matched := 0 for _, tp := range h.tracked { inst, present := byClaim[tp.claimName] - before := tp.pod.Status.Phase + before := statusSignature(tp.pod) if !present { applyState(tp.pod, provider.InstanceTerminated, "", h.nowFn()) } else { matched++ applyState(tp.pod, inst.State, inst.Endpoint, h.nowFn()) + // Surface the reachable address on the Pod metadata. PodIP can't hold a DNS + // name (see applyState), so the endpoint always rides an annotation. Set it + // on the tracked Pod here; the notify wrapper persists it to the API server + // (deduped against persistedEndpoint so only a real change patches). + if inst.Endpoint != "" && tp.pod.Annotations[nebulav1alpha1.EndpointAnnotation] != inst.Endpoint { + if tp.pod.Annotations == nil { + tp.pod.Annotations = map[string]string{} + } + tp.pod.Annotations[nebulav1alpha1.EndpointAnnotation] = inst.Endpoint + } } - if tp.pod.Status.Phase != before { - changed = append(changed, tp.pod.DeepCopy()) - } + // Log the before -> after status every tick. This is the "how does the system + // look" signal an operator watches: the lifecycle progression (Provisioning -> + // Initializing -> Running, or -> Terminated on a disappearance) shows as the + // before/after differing; a steady state shows them equal, which is itself the + // confirmation the pod is being observed and re-emitted, not stuck. + log.V(1).Info("observed pod status", + "pod", key(tp.pod.Namespace, tp.pod.Name), "before", before, + "after", statusSignature(tp.pod)) + emit = append(emit, tp.pod.DeepCopy()) } notify := h.notify h.mu.Unlock() @@ -343,15 +446,44 @@ func (h *Handler) reconcileOnce(ctx context.Context) { // pods appear stuck: tracked>0 with matched==0 means List returns instances the // claim names don't line up with (the classic "provisioned but never Running"). log.V(1).Info("poll tick", - "listed", len(instances), "tracked", tracked, "matched", matched, "changed", len(changed)) - + "listed", len(instances), "tracked", tracked, "matched", matched) + + // Re-emit EVERY tracked pod's current status each tick. Notification is otherwise + // edge-triggered (notify only on a signature change), and VK dedups each emit + // against the last status IT received from us — never against the API server. So + // if a single UpdateStatus is dropped (a transient conflict, informer-cache lag), + // VK believes it already delivered that status and never retries, and an + // edge-triggered loop never re-sends it: the Pod wedges on a stale status forever + // (classic: instance is Running but the Pod stays Pending/Initializing). Re-handing + // VK the unchanged status is cheap — it dedups the identical object without an API + // write — and, crucially, arms VK's own drift-correction: the dedup sets + // lastPodStatusUpdateSkipped, so the next informer resync notices the API server + // disagrees with what we last sent and re-issues the write. This makes status + // propagation level-triggered and self-healing within one resync period. if notify != nil { - for _, p := range changed { + for _, p := range emit { notify(p) } } } +// statusSignature is a compact rendering of the Pod status fields the poll loop +// surfaces to the API server, logged before/after each tick so a pod's lifecycle +// progression is visible. It intentionally goes beyond Phase: reason (Provisioning +// -> Initializing -> Running), readiness, and the assigned IP all move WITHIN a +// single phase, so a phase-only view would hide those transitions. Keep this in +// sync with what applyState writes. +func statusSignature(pod *corev1.Pod) string { + ready := corev1.ConditionUnknown + for i := range pod.Status.Conditions { + if pod.Status.Conditions[i].Type == corev1.PodReady { + ready = pod.Status.Conditions[i].Status + break + } + } + return string(pod.Status.Phase) + "|" + pod.Status.Reason + "|" + string(ready) + "|" + pod.Status.PodIP +} + // store records/updates the tracked pod under lock. func (h *Handler) store(pod *corev1.Pod, claim, instance string) { h.mu.Lock() @@ -373,6 +505,69 @@ func (h *Handler) emit(pod *corev1.Pod) { } } +// persistEndpoint patches the endpoint annotation onto the Pod metadata. It runs +// inside the notify wrapper (see NotifyPods), just before VK's status callback: +// VK's callback writes only the /status subresource and drops any metadata change +// on the same object, so the reachable address — which is how anything reaches the +// workload, and which PodIP cannot hold when it is a DNS name (see applyState) — +// needs this dedicated metadata write. A merge patch scoped to the single +// annotation is a no-op for every other field and does not collide with the status +// write that follows. +// +// The notify callback fires every poll tick, so this dedups against the tracked +// pod's persistedEndpoint and patches only when the value actually changed (first +// appearance or a re-provision); a steady Running pod is never re-patched. It is +// best-effort: a nil client (tests) or an endpoint-less Pod is a no-op, and a +// failed patch is logged and retried next tick (persistedEndpoint is only advanced +// on success), never fatal to the poll. NotFound is ignored — the Pod is gone. +func (h *Handler) persistEndpoint(ctx context.Context, pod *corev1.Pod) { + if h.client == nil { + return + } + endpoint := pod.Annotations[nebulav1alpha1.EndpointAnnotation] + if endpoint == "" { + return + } + + // Dedup: skip the patch when we have already persisted this exact endpoint. + h.mu.Lock() + tp, tracked := h.tracked[key(pod.Namespace, pod.Name)] + if tracked && tp.persistedEndpoint == endpoint { + h.mu.Unlock() + return + } + h.mu.Unlock() + + patch, err := json.Marshal(map[string]any{ + "metadata": map[string]any{ + "annotations": map[string]string{nebulav1alpha1.EndpointAnnotation: endpoint}, + }, + }) + if err != nil { + // A marshal of a fixed-shape map cannot realistically fail; guard anyway so a + // future change surfaces rather than panics. + logf.FromContext(ctx).WithName("vnode-poll").Error(err, + "marshal endpoint annotation patch", "pod", key(pod.Namespace, pod.Name)) + return + } + if _, err := h.client.CoreV1().Pods(pod.Namespace).Patch( + ctx, pod.Name, types.MergePatchType, patch, metav1.PatchOptions{}); err != nil { + if !apierrors.IsNotFound(err) { + logf.FromContext(ctx).WithName("vnode-poll").Error(err, + "persist endpoint annotation; will retry next tick", + "pod", key(pod.Namespace, pod.Name), "endpoint", endpoint) + } + return // leave persistedEndpoint unchanged so the next tick retries + } + + // Record success so subsequent ticks skip the patch until the endpoint changes. + h.mu.Lock() + if tp, ok := h.tracked[key(pod.Namespace, pod.Name)]; ok { + tp.persistedEndpoint = endpoint + } + h.mu.Unlock() +} + // markStatus sets a coarse pod phase and a single readiness-style condition on // the passed Pod (which VK then reports to the API server). func (h *Handler) markStatus(pod *corev1.Pod, phase corev1.PodPhase, reason, msg string) { @@ -397,21 +592,42 @@ func (h *Handler) markStatus(pod *corev1.Pod, phase corev1.PodPhase, reason, msg // accelerator gets one wasted re-probe before it re-records. That is the right // trade — over-narrow costs a fast retry, over-broad wrongly excludes serviceable // accelerators. DenyAll (auth/quota) ignores the accelerator: it fails for all. -func (h *Handler) recordBlock(pod *corev1.Pod, err error) { +func (h *Handler) recordBlock(ctx context.Context, pod *corev1.Pod, err error) { if h.blocklist == nil { return } - // The accelerator is a property of the REQUEST, not the error; the provider - // cannot see it, so we resolve it off the Pod and hand it over ("" for a - // CPU-only Pod, which the provider treats as "not applicable"). - accel, _, _ := util.AcceleratorRequest(pod) - scope := h.prov.ClassifyProvisionError(err, accel) + // The accelerator id and region are properties of the REQUEST, not the error; the + // provider cannot see them, so we resolve them off the Pod and hand them over. The + // accelerator (type + count from the Pod) is mapped through the provider to its + // RESOLVED id (an EC2 instance type on AWS), so the block is keyed on the concrete + // capacity pool that failed — an L4x8 shortage must not exclude L4x1. "" for either + // means "not applicable" (a CPU-only Pod, or a region-simple provider), which the + // provider treats accordingly. + accel, count, _ := util.AcceleratorRequest(pod) + acceleratorID := "" + if accel != "" { + // ok=false only if the provider stopped offering the (type, count) since + // provisioning; leave the id "" and let the provider scope from the error. + acceleratorID, _ = h.prov.MapAccelerator(accel, count) + } + region := pod.Annotations[nebulav1alpha1.RegionAnnotation] + scope := h.prov.ClassifyProvisionError(err, acceleratorID, region) + if scope == (provider.BlockScope{}) { // An empty scope means the error is not one we know how to blocklist; do not // install a wildcard block that would exclude everything on the provider. return } - h.blocklist.Record(h.prov.Name(), scope, blocklistTTL(pod)) + + ttl := blocklistTTL(pod) + // Use the request-scoped logger from ctx (it carries the virtualNode/provider + // values controller-runtime attached upstream); a logf.FromContext on a fresh + // context.Background() would fall back to the global delegate and could be + // silently dropped before the real sink is installed. + logf.FromContext(ctx).WithName("vnode-blocklist").Info( + "recording blocklist entry for failed placement", + "provider", h.prov.Name(), "scope", scope, "ttl", ttl.String(), "error", err.Error()) + h.blocklist.Record(h.prov.Name(), scope, ttl) } // blocklistTTL reads the pool's FailoverPolicy.BlocklistTTL off the Pod annotation diff --git a/pkg/vnode/handler_test.go b/pkg/vnode/handler_test.go index e8da9f4..1c5afe4 100644 --- a/pkg/vnode/handler_test.go +++ b/pkg/vnode/handler_test.go @@ -26,6 +26,9 @@ import ( "github.com/virtual-kubelet/virtual-kubelet/errdefs" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/provider" @@ -46,11 +49,12 @@ type fakeProvider struct { listErr error capabilities provider.Capabilities // classifyScope is what ClassifyProvisionError returns for a failure; the zero - // value (empty scope) means "not blocklistable". classifyAccel records the - // accelerator the handler passed in, so a test can assert it resolved it off the - // Pod (the provider now owns the whole scope; the handler only supplies this). - classifyScope provider.BlockScope - classifyAccel string + // value (empty scope) means "not blocklistable". classifyAccel/classifyRegion + // record what the handler passed in, so a test can assert it resolved them off the + // Pod (the provider now owns the whole scope; the handler only supplies these). + classifyScope provider.BlockScope + classifyAccel string + classifyRegion string } func (f *fakeProvider) Name() string { return "fake" } @@ -77,9 +81,10 @@ func (f *fakeProvider) List(context.Context) ([]provider.Instance, error) { return f.list, f.listErr } func (f *fakeProvider) Offerings(context.Context) ([]provider.Offering, error) { return nil, nil } -func (f *fakeProvider) MapAccelerator(c string) (string, bool) { return c, true } -func (f *fakeProvider) ClassifyProvisionError(_ error, accel string) provider.BlockScope { +func (f *fakeProvider) MapAccelerator(c string, _ int32) (string, bool) { return c, true } +func (f *fakeProvider) ClassifyProvisionError(_ error, accel, region string) provider.BlockScope { f.classifyAccel = accel + f.classifyRegion = region return f.classifyScope } @@ -108,7 +113,7 @@ func testPod(ns, name string) *corev1.Pod { func TestCreatePod_ProvisionsAndTracks(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") pod.Annotations = map[string]string{nebulav1alpha1.CapacityTypeAnnotation: string(nebulav1alpha1.CapacitySpot)} @@ -136,7 +141,7 @@ func TestCreatePod_ProvisionsAndTracks(t *testing.T) { func TestCreatePod_ProvisionErrorSurfaces(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity")} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") if err := h.CreatePod(context.Background(), pod); err == nil { @@ -155,13 +160,13 @@ func TestCreatePod_ProvisionErrorSurfaces(t *testing.T) { func TestCreatePod_ProvisionFailureRecordsBlock(t *testing.T) { accel, region := "H100", "us-east-1" scope := provider.BlockScope{ - AcceleratorType: &accel, - CapacityType: nebulav1alpha1.CapacitySpot, - Region: ®ion, + AcceleratorID: &accel, + CapacityType: nebulav1alpha1.CapacitySpot, + Region: ®ion, } fp := &fakeProvider{provisionErr: errors.New("no capacity"), classifyScope: scope} bl := &recordingBlocklist{} - h := NewHandler(fp, bl) + h := NewHandler(fp, nil, bl) pod := testPod("default", "p1") pod.Annotations = map[string]string{nebulav1alpha1.BlocklistTTLAnnotation: "3m"} @@ -195,7 +200,7 @@ func TestCreatePod_EmptyScopeDoesNotBlock(t *testing.T) { // (which would exclude everything on the provider). fp := &fakeProvider{provisionErr: errors.New("weird"), classifyScope: provider.BlockScope{}} bl := &recordingBlocklist{} - h := NewHandler(fp, bl) + h := NewHandler(fp, nil, bl) if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { t.Fatal("expected CreatePod to return the provision error") @@ -208,7 +213,7 @@ func TestCreatePod_EmptyScopeDoesNotBlock(t *testing.T) { func TestCreatePod_MissingTTLAnnotationUsesDefault(t *testing.T) { fp := &fakeProvider{provisionErr: errors.New("no capacity"), classifyScope: provider.BlockScope{DenyAll: true}} bl := &recordingBlocklist{} - h := NewHandler(fp, bl) + h := NewHandler(fp, nil, bl) // No BlocklistTTLAnnotation on the Pod => the handler's built-in default. if err := h.CreatePod(context.Background(), testPod("default", "p1")); err == nil { @@ -221,7 +226,7 @@ func TestCreatePod_MissingTTLAnnotationUsesDefault(t *testing.T) { func TestDeletePod_TerminatesAndUntracks(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") if err := h.CreatePod(context.Background(), pod); err != nil { @@ -243,7 +248,7 @@ func TestDeletePod_TerminatesAndUntracks(t *testing.T) { func TestDeletePod_Idempotent(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -257,7 +262,7 @@ func TestDeletePod_Idempotent(t *testing.T) { func TestReconcileOnce_ReportsRunning(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -287,6 +292,10 @@ func TestReconcileOnce_ReportsRunning(t *testing.T) { t.Fatalf("expected endpoint set as PodIP, got %q", got.Status.PodIP) } + if got.Annotations[nebulav1alpha1.EndpointAnnotation] != "5.6.7.8" { + t.Fatalf("expected endpoint annotation set, got %q", got.Annotations[nebulav1alpha1.EndpointAnnotation]) + } + mu.Lock() defer mu.Unlock() if len(notified) == 0 { @@ -294,9 +303,135 @@ func TestReconcileOnce_ReportsRunning(t *testing.T) { } } +func TestReconcileOnce_DNSEndpointNotWrittenToPodIP(t *testing.T) { + // AWS reports a public DNS name as the endpoint. PodIP is validated by the API + // server as a literal IP, so a DNS name there fails the whole status write; it + // must be left empty. The reachable address is surfaced on the annotation + // instead, which accepts any form. + fp := &fakeProvider{provisionID: "inst-1"} + h := NewHandler(fp, nil, nil) + pod := testPod("default", "p1") + _ = h.CreatePod(context.Background(), pod) + + const dns = "ec2-54-161-33-206.compute-1.amazonaws.com" + fp.list = []provider.Instance{{ + ID: "inst-1", ClaimName: "default-p1", State: provider.InstanceRunning, Endpoint: dns, + }} + h.reconcileOnce(context.Background()) + + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("GetPod: %v", err) + } + if got.Status.Phase != corev1.PodRunning { + t.Fatalf("expected Running, got %q", got.Status.Phase) + } + if got.Status.PodIP != "" { + t.Fatalf("a DNS endpoint must not be written to PodIP, got %q", got.Status.PodIP) + } + if got.Annotations[nebulav1alpha1.EndpointAnnotation] != dns { + t.Fatalf("expected DNS endpoint on the annotation, got %q", got.Annotations[nebulav1alpha1.EndpointAnnotation]) + } +} + +func TestNotify_PersistsEndpointAnnotationOnce(t *testing.T) { + // The endpoint must reach the API-server Pod metadata (VK writes only status), + // so the notify wrapper issues a metadata patch. It must fire when the endpoint + // first appears and then dedup: a steady Running pod re-emitted every tick must + // not re-patch. + const dns = "ec2-54-161-33-206.compute-1.amazonaws.com" + pod := testPod("default", "p1") + client := fake.NewSimpleClientset(pod) + + var patches int + client.PrependReactor("patch", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { + patches++ + return false, nil, nil // fall through to the tracker so the object updates + }) + + fp := &fakeProvider{provisionID: "inst-1"} + h := NewHandler(fp, client, nil) + _ = h.CreatePod(context.Background(), pod) + // Register the notify wrapper (this is where persistEndpoint is injected). + h.NotifyPods(context.Background(), func(*corev1.Pod) {}) + + fp.list = []provider.Instance{{ + ID: "inst-1", ClaimName: "default-p1", State: provider.InstanceRunning, Endpoint: dns, + }} + + // First tick: endpoint appears -> one patch. + h.reconcileOnce(context.Background()) + if patches != 1 { + t.Fatalf("expected exactly one patch when the endpoint first appears, got %d", patches) + } + + // The annotation landed on the API-server object. + live, err := client.CoreV1().Pods("default").Get(context.Background(), "p1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("get patched pod: %v", err) + } + if live.Annotations[nebulav1alpha1.EndpointAnnotation] != dns { + t.Fatalf("expected endpoint persisted to API server, got %q", live.Annotations[nebulav1alpha1.EndpointAnnotation]) + } + + // Second tick with the same endpoint: deduped, no additional patch. + h.reconcileOnce(context.Background()) + if patches != 1 { + t.Fatalf("an unchanged endpoint must not re-patch; got %d patches", patches) + } +} + +func TestReconcileOnce_NotifiesOnProvisioningToInitializing(t *testing.T) { + // The Pod starts at "Provisioning" (phase Pending), and the instance comes up + // but has not yet passed its readiness checks => InstancePending, which maps to + // the "Initializing" reason at the SAME phase (Pending). A phase-only change + // check would swallow this and strand the Pod on the stale "Provisioning" + // reason; the reason must move and a notification must fire. + fp := &fakeProvider{provisionID: "inst-1"} + h := NewHandler(fp, nil, nil) + pod := testPod("default", "p1") + _ = h.CreatePod(context.Background(), pod) + + // Sanity: CreatePod left it at the Provisioning reason (still Pending). + if got, _ := h.GetPod(context.Background(), "default", "p1"); got.Status.Reason != reasonProvisioning { + t.Fatalf("precondition: expected %q, got %q", reasonProvisioning, got.Status.Reason) + } + + var mu sync.Mutex + var notified []*corev1.Pod + h.NotifyPods(context.Background(), func(p *corev1.Pod) { + mu.Lock() + notified = append(notified, p) + mu.Unlock() + }) + + // Instance is up but not yet reachable (2/2 checks pending) => InstancePending. + fp.list = []provider.Instance{{ + ID: "inst-1", ClaimName: "default-p1", State: provider.InstancePending, + }} + h.reconcileOnce(context.Background()) + + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("GetPod: %v", err) + } + if got.Status.Phase != corev1.PodPending { + t.Fatalf("expected phase to stay Pending, got %q", got.Status.Phase) + } + if got.Status.Reason != reasonInitializing { + t.Fatalf("expected reason to advance to %q, got %q", reasonInitializing, got.Status.Reason) + } + + mu.Lock() + defer mu.Unlock() + if len(notified) == 0 { + t.Fatal("expected a notification on the Provisioning->Initializing reason change (same phase)") + } +} + func TestReconcileOnce_AbsentInstanceIsTerminated(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) pod := testPod("default", "p1") _ = h.CreatePod(context.Background(), pod) @@ -316,18 +451,61 @@ func TestReconcileOnce_AbsentInstanceIsTerminated(t *testing.T) { func TestNewHandler_PollIntervalFromCapabilities(t *testing.T) { // A provider that declares a cadence overrides the default. custom := &fakeProvider{capabilities: provider.Capabilities{PollInterval: 5 * time.Second}} - if got := NewHandler(custom, nil).pollEvery; got != 5*time.Second { + if got := NewHandler(custom, nil, nil).pollEvery; got != 5*time.Second { t.Fatalf("expected the provider's PollInterval, got %v", got) } // A provider that leaves it zero falls back to the vnode default. - if got := NewHandler(&fakeProvider{}, nil).pollEvery; got != defaultPollInterval { + if got := NewHandler(&fakeProvider{}, nil, nil).pollEvery; got != defaultPollInterval { t.Fatalf("expected the default cadence, got %v", got) } } +func TestGetPod_ReAdoptsLiveInstanceAfterRestart(t *testing.T) { + // Simulate a VK restart: the tracking map is cold (no CreatePod ran this + // process), but the external instance for the Pod's claim is still running. A + // GetPod must re-adopt it from the provider's List and report its true state, + // so VK takes the adopt path instead of re-issuing CreatePod (which would reset + // the Pod to Provisioning). This is the fix for "stuck on Provisioning while the + // instance is actually running". + fp := &fakeProvider{list: []provider.Instance{{ + ID: "inst-9", ClaimName: "default-p1", State: provider.InstanceRunning, Endpoint: "1.2.3.4", + }}} + h := NewHandler(fp, nil, nil) + + got, err := h.GetPod(context.Background(), "default", "p1") + if err != nil { + t.Fatalf("GetPod after restart: %v", err) + } + if got.Status.Phase != corev1.PodRunning { + t.Fatalf("expected re-adopted Pod reported Running, got %q", got.Status.Phase) + } + if got.Status.PodIP != "1.2.3.4" { + t.Fatalf("expected endpoint adopted as PodIP, got %q", got.Status.PodIP) + } + + // It must now be tracked, so the poll loop advances it and DeletePod can find + // the instance id to terminate. + if _, err := h.GetPod(context.Background(), "default", "p1"); err != nil { + t.Fatalf("expected the re-adopted Pod to be tracked: %v", err) + } +} + +func TestGetPod_UnknownClaimStaysNotFound(t *testing.T) { + // No tracking and no live instance for this claim => genuinely absent. GetPod + // must report NotFound so VK creates it, not silently adopt a phantom. + fp := &fakeProvider{list: []provider.Instance{{ + ID: "inst-1", ClaimName: "default-other", State: provider.InstanceRunning, + }}} + h := NewHandler(fp, nil, nil) + + if _, err := h.GetPod(context.Background(), "default", "p1"); !errdefs.IsNotFound(err) { + t.Fatalf("expected NotFound for an unknown, unlisted claim, got %v", err) + } +} + func TestGetPods_ReturnsTracked(t *testing.T) { fp := &fakeProvider{provisionID: "inst-1"} - h := NewHandler(fp, nil) + h := NewHandler(fp, nil, nil) _ = h.CreatePod(context.Background(), testPod("default", "p1")) _ = h.CreatePod(context.Background(), testPod("default", "p2")) diff --git a/pkg/vnode/node.go b/pkg/vnode/node.go index f6f109c..570b343 100644 --- a/pkg/vnode/node.go +++ b/pkg/vnode/node.go @@ -97,7 +97,7 @@ var _ manager.Runnable = (*Runner)(nil) func (r *Runner) Start(ctx context.Context) error { log := logf.FromContext(ctx).WithValues("virtualNode", r.nodeName, "provider", r.prov.Name()) - handler := NewHandler(r.prov, r.blocklist) + handler := NewHandler(r.prov, r.client, r.blocklist) nodeSpec := nodeSpec(r.nodeName, r.prov.Name()) // Pod informer scoped to this node only (spec.nodeName == nodeName), so the diff --git a/pkg/vnode/status.go b/pkg/vnode/status.go index 1572564..c4b0c76 100644 --- a/pkg/vnode/status.go +++ b/pkg/vnode/status.go @@ -17,6 +17,8 @@ limitations under the License. package vnode import ( + "net" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,9 +30,18 @@ import ( // is a passive ledger and does not mirror this), so these live here in the // vnode, not on the API type. const ( - // reasonProvisioning: a provider Provision call has been issued and the - // instance is not yet running. + // reasonProvisioning: a provider Provision call has been issued but the + // instance does not yet exist — we are still allocating it (e.g. EC2 + // RunInstances in flight). Set on CreatePod, before the first poll observes + // the instance. reasonProvisioning = "Provisioning" + // reasonInitializing: the instance EXISTS at the provider but is not yet + // reachable — it is booting (EC2 "pending") or running-but-not-yet-passing its + // reachability checks (running, <2/2, EC2's own "Initializing" status). It + // mirrors that EC2 status-check term. Provisioning is done; the instance is + // coming up. Distinct from Provisioning so a Pod stuck here points at a slow + // boot / failing status checks, not a stuck allocation. + reasonInitializing = "Initializing" // reasonRunning: the provider reports the instance running. reasonRunning = "Running" // reasonProvisionFailed: the provider rejected or failed the Provision call. @@ -63,14 +74,21 @@ func applyState(pod *corev1.Pod, state provider.InstanceState, endpoint string, setPhase(pod, corev1.PodRunning, reasonRunning, "external instance is running", now) setReady(pod, corev1.ConditionTrue, now) setContainerStatuses(pod, corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: now}}, true) - if endpoint != "" { + // PodIP is validated by the API server as a literal IP, so only populate it + // when the endpoint actually is one — an AWS public DNS name (the common + // case) would make the whole status UpdateStatus fail with a 422 and strand + // the Pod on its prior (Initializing) status forever. The endpoint is always + // surfaced on EndpointAnnotation regardless of form (see Handler.applyEndpoint), + // so a DNS-only instance still exposes its reachable address; PodIP just stays + // empty in that case. + if endpoint != "" && net.ParseIP(endpoint) != nil { pod.Status.PodIP = endpoint } case provider.InstancePending: - setPhase(pod, corev1.PodPending, reasonProvisioning, "external instance is starting", now) + setPhase(pod, corev1.PodPending, reasonInitializing, "external instance is initializing", now) setReady(pod, corev1.ConditionFalse, now) setContainerStatuses(pod, corev1.ContainerState{ - Waiting: &corev1.ContainerStateWaiting{Reason: reasonProvisioning, Message: "external instance is starting"}, + Waiting: &corev1.ContainerStateWaiting{Reason: reasonInitializing, Message: "external instance is initializing"}, }, false) case provider.InstanceFailed: setPhase(pod, corev1.PodFailed, reasonFailed, "external instance entered a failed state", now)