From 6cf40b3f9e607b5b584b3124ed242a13b7d326e7 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 2 Sep 2026 15:14:35 +0100 Subject: [PATCH 01/14] support cost tracking Signed-off-by: kerthcet --- .gitignore | 3 +- CLAUDE.md | 18 + api/v1alpha1/nodeclaim_types.go | 63 +++ api/v1alpha1/zz_generated.deepcopy.go | 13 +- cmd/main.go | 34 ++ .../bases/nebula.inftyai.com_nodeclaims.yaml | 70 +++ docs/architecture.md | 7 +- docs/metrics.md | 254 ++++++++- internal/controller/cost_accrual.go | 297 ++++++++++ internal/controller/cost_accrual_test.go | 523 ++++++++++++++++++ internal/controller/nodeclaim_controller.go | 131 ++++- internal/controller/nodeclaim_price_test.go | 222 ++++++++ pkg/metrics/attribution.go | 161 ++++++ pkg/metrics/attribution_test.go | 179 ++++++ pkg/metrics/cost.go | 97 ++++ pkg/metrics/cost_test.go | 79 +++ pkg/metrics/doc.go | 21 +- pkg/metrics/labels.go | 12 +- pkg/metrics/testing.go | 30 + pkg/provider/catalog/base.go | 68 ++- pkg/provider/catalog/catalog_test.go | 117 ++++ pkg/provider/catalog/data/modal.csv | 4 +- pkg/provider/catalog/data/pricing.go | 57 ++ pkg/provider/modal/modal.go | 30 + pkg/provider/modal/modal_test.go | 65 +++ pkg/provider/pricing.go | 74 +++ pkg/util/accelerator.go | 21 + pkg/util/accelerator_test.go | 44 ++ pkg/util/resources.go | 61 ++ pkg/util/resources_test.go | 109 ++++ 30 files changed, 2819 insertions(+), 45 deletions(-) create mode 100644 CLAUDE.md create mode 100644 internal/controller/cost_accrual.go create mode 100644 internal/controller/cost_accrual_test.go create mode 100644 internal/controller/nodeclaim_price_test.go create mode 100644 pkg/metrics/attribution.go create mode 100644 pkg/metrics/attribution_test.go create mode 100644 pkg/metrics/cost.go create mode 100644 pkg/metrics/cost_test.go create mode 100644 pkg/metrics/testing.go create mode 100644 pkg/provider/catalog/data/pricing.go create mode 100644 pkg/provider/pricing.go create mode 100644 pkg/util/resources.go create mode 100644 pkg/util/resources_test.go diff --git a/.gitignore b/.gitignore index ffa0ea0..d0ee133 100644 --- a/.gitignore +++ b/.gitignore @@ -42,4 +42,5 @@ __pycache__ *.tgz .devcontainer -bin \ No newline at end of file +bin +.claude diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..e097a92 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,18 @@ +# Nebula + +## Comments + +This codebase comments the *why*, not the *what*. Match that — and match its density too, +don't exceed it. + +- Never restate the code. `// increment the counter` above `n++` is noise. +- Don't repeat a rationale a nearby doc comment already gives. Name the symbol that holds + it (`see Offering.GPUCount`) and move on. +- Doc comments: the contract, plus the one constraint a caller would otherwise get wrong. + 2-5 lines. Go longer only for a real invariant — a subtle failure mode, a decision kept + on purpose, a gotcha in an external API. +- Inline comments: only where a reader would otherwise draw the wrong conclusion. Not one + per branch, not one per field assignment. +- One comment on the tricky invariant beats five on the obvious steps. + +If a comment is longer than the code it explains, it is usually the wrong comment. diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index 51d493f..dd10389 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -138,6 +138,67 @@ type NodeClaimStatus struct { // Endpoint is the reachable address (e.g. SSH host:port) once ready. // +optional Endpoint string `json:"endpoint,omitempty"` + + // PriceUSDPerHour is what this instance costs per hour in USD, as a decimal string + // ("7.9000"), resolved from the provider's catalog against the served Pod's shape. + // Status, not spec: it is a derived result nobody can declare up front. A string + // because it is written to be READ — a print column can only echo a field, never + // scale a fixed-point integer back into currency. + // + // Empty means UNPRICED, not free: the provider implements no provider.Pricer, or its + // catalog has no row for this candidate. Consumers must skip such a claim rather than + // count it as $0. + // + // Written once and never refreshed, so a catalog edit cannot retroactively reprice a + // running instance and rewrite the cost history it has already reported. + // +optional + PriceUSDPerHour string `json:"priceUSDPerHour,omitempty"` + + // EstimatedCostUSD is what this instance has cost SO FAR, as a decimal string ("412.9400") + // — PriceUSDPerHour integrated over the time it has held an instance. A string for the same + // reason the rate is one: it exists to be read off a print column. + // + // ESTIMATED, and the name says so on purpose: it is our own arithmetic over a list price + // (see PriceUSDPerHour), not a figure any provider has confirmed. Nothing here has been + // invoiced. Reconcile against the provider's billing export before anyone is charged. + // + // Within those limits it is the AUTHORITATIVE total, not the Prometheus counter: it is + // written exactly once per window and survives a restart. It is also only LIVE cost — it + // dies with the claim, so it is not a history. The metric is what outlives an instance. + // +optional + EstimatedCostUSD string `json:"estimatedCostUSD,omitempty"` + + // LastAccruedAt is how far cost accrual has counted: an ANCHOR for the next measurement, + // not a note about the last one. "Accrued" in the accounting sense — cost incurred but not + // yet invoiced, which is all EstimatedCostUSD ever holds. + // + // It is the reason a restart loses nothing: the next window is rate x (now - LastAccruedAt), + // so time that passed while Nebula was down is still counted on recovery instead of vanishing + // with the in-memory total. + // + // The invariant that makes it safe: this NEVER moves past cost that has been durably + // recorded, because it advances only in the same patch that writes EstimatedCostUSD. A + // failed write therefore loses nothing — the same window is counted next time. + // + // Unset means "not counting yet". Never treat it as the epoch, which would charge decades + // on the first tick. + // +optional + LastAccruedAt *metav1.Time `json:"lastAccruedAt,omitempty"` + + // CostLabels attributes this claim's spend to whoever asked for it: the values the served + // Pod carried for the label names the operator configured (--cost-labels). Keyed by the + // label name, which is the same token on the Pod and on the metric. + // + // Status rather than spec because it is observed from the Pod, and it sits with the rest of + // the billing record for a reason: it is stamped in the SAME patch that opens the accrual + // anchor, so no window can ever be charged before its attribution is known. + // + // Written once, on first observation, and never refreshed — relabeling a Pod must not + // retroactively re-attribute spend already reported under the old values. A name the + // current --cost-labels no longer lists is ignored rather than cleaned up; one it lists but + // this map lacks reports as "none". + // +optional + CostLabels map[string]string `json:"costLabels,omitempty"` } // +kubebuilder:object:root=true @@ -147,6 +208,8 @@ type NodeClaimStatus struct { // +kubebuilder:printcolumn:name="Region",type=string,JSONPath=`.spec.region` // +kubebuilder:printcolumn:name="ACCELERATOR",type=string,JSONPath=`.spec.accelerator` // +kubebuilder:printcolumn:name="CAPACITY_TYPE",type=string,JSONPath=`.spec.capacityType` +// +kubebuilder:printcolumn:name="PRICE/HR",type=string,JSONPath=`.status.priceUSDPerHour` +// +kubebuilder:printcolumn:name="EST_COST",type=string,JSONPath=`.status.estimatedCostUSD` // +kubebuilder:printcolumn:name="Phase",type=string,JSONPath=`.status.phase` // +kubebuilder:printcolumn:name="Instance",type=string,JSONPath=`.status.instanceID` // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 632a0d5..bdd1f95 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -68,7 +68,7 @@ func (in *NodeClaim) DeepCopyInto(out *NodeClaim) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) out.Spec = in.Spec - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodeClaim. @@ -140,6 +140,17 @@ func (in *NodeClaimSpec) DeepCopy() *NodeClaimSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NodeClaimStatus) DeepCopyInto(out *NodeClaimStatus) { *out = *in + if in.LastAccruedAt != nil { + in, out := &in.LastAccruedAt, &out.LastAccruedAt + *out = (*in).DeepCopy() + } + if in.CostLabels != nil { + in, out := &in.CostLabels, &out.CostLabels + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NodeClaimStatus. diff --git a/cmd/main.go b/cmd/main.go index 8cee8f0..368f533 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -49,6 +49,7 @@ import ( webhookv1 "github.com/InftyAI/Nebula/internal/webhook/v1" nebulacert "github.com/InftyAI/Nebula/pkg/cert" "github.com/InftyAI/Nebula/pkg/failover" + nebulametrics "github.com/InftyAI/Nebula/pkg/metrics" "github.com/InftyAI/Nebula/pkg/provider" awsprovider "github.com/InftyAI/Nebula/pkg/provider/aws" "github.com/InftyAI/Nebula/pkg/provider/fake" @@ -93,6 +94,7 @@ func main() { var secureMetrics bool var enableHTTP2 bool var kubeletAddr, kubeletClientCA string + var costLabels string var tlsOpts []func(*tls.Config) flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") @@ -119,6 +121,13 @@ func main() { "serves TLS without client verification, because which CA signs the API server's kubelet "+ "client certificate is not portable across distributions; restrict the port with a "+ "NetworkPolicy, or set this to your API server's kubelet client CA.") + flag.StringVar(&costLabels, "cost-labels", "", + "Comma-separated Pod label names whose values attribute cost, e.g. \"org_id,team_id\". Each name "+ + "is used verbatim as both the Pod label key and the metric label, so it must be legal as "+ + "both: letters, digits and underscores, starting with a letter. Empty (the default) reports "+ + "cost by candidate shape only. Changing this changes the identity of every cost series. "+ + "Values come from Pod labels and are NOT capped: the manager warns once if they push the "+ + "cost metric past 5000 series, but pick names an admission policy constrains.") opts := zap.Options{ Development: true, } @@ -127,6 +136,21 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) + // Before anything can record: the counter's label names are fixed when it is constructed, and + // a bad --cost-labels must stop the process rather than silently report every tenant as "none". + attribution, err := nebulametrics.ParseCostLabels(costLabels) + if err != nil { + setupLog.Error(err, "invalid --cost-labels") + os.Exit(1) + } + if err := nebulametrics.InitCost(attribution); err != nil { + setupLog.Error(err, "configuring the cost metric") + os.Exit(1) + } + if len(attribution) > 0 { + setupLog.Info("cost attribution enabled", "labels", attribution) + } + // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will // prevent from being vulnerable to the HTTP/2 Stream Cancellation and @@ -418,6 +442,16 @@ func setupControllers(mgr ctrl.Manager, blocklist *failover.Blocklist, kubeletSr return fmt.Errorf("unable to create Pod webhook: %w", err) } } + + // Cost accrual is a clock-driven loop, not a reconciler, so it is added directly. It opts into + // leader election by NOT implementing LeaderElectionRunnable, which buys two things: N replicas + // would mean N times the writes, and each would book windows into its OWN cost counter, leaving + // increase() to read a series that holds only the races that replica won. The ledger itself is + // safe either way (see CostAccrual.accrue). + if err := mgr.Add(controller.NewCostAccrual(mgr.GetClient())); err != nil { + return fmt.Errorf("unable to add the cost accrual loop: %w", err) + } + // +kubebuilder:scaffold:builder return nil } diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index dbc8b88..26b4262 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -29,6 +29,12 @@ spec: - jsonPath: .spec.capacityType name: CAPACITY_TYPE type: string + - jsonPath: .status.priceUSDPerHour + name: PRICE/HR + type: string + - jsonPath: .status.estimatedCostUSD + name: EST_COST + type: string - jsonPath: .status.phase name: Phase type: string @@ -141,16 +147,65 @@ spec: NodeClaimStatus is the durable record reconciled against the provider by the poll loop. properties: + costLabels: + additionalProperties: + type: string + description: |- + CostLabels attributes this claim's spend to whoever asked for it: the values the served + Pod carried for the label names the operator configured (--cost-labels). Keyed by the + label name, which is the same token on the Pod and on the metric. + + Status rather than spec because it is observed from the Pod, and it sits with the rest of + the billing record for a reason: it is stamped in the SAME patch that opens the accrual + anchor, so no window can ever be charged before its attribution is known. + + Written once, on first observation, and never refreshed — relabeling a Pod must not + retroactively re-attribute spend already reported under the old values. A name the + current --cost-labels no longer lists is ignored rather than cleaned up; one it lists but + this map lacks reports as "none". + type: object endpoint: description: Endpoint is the reachable address (e.g. SSH host:port) once ready. type: string + estimatedCostUSD: + description: |- + EstimatedCostUSD is what this instance has cost SO FAR, as a decimal string ("412.9400") + — PriceUSDPerHour integrated over the time it has held an instance. A string for the same + reason the rate is one: it exists to be read off a print column. + + ESTIMATED, and the name says so on purpose: it is our own arithmetic over a list price + (see PriceUSDPerHour), not a figure any provider has confirmed. Nothing here has been + invoiced. Reconcile against the provider's billing export before anyone is charged. + + Within those limits it is the AUTHORITATIVE total, not the Prometheus counter: it is + written exactly once per window and survives a restart. It is also only LIVE cost — it + dies with the claim, so it is not a history. The metric is what outlives an instance. + type: string instanceID: description: |- InstanceID is the provider's identifier for the external instance (e.g. a RunPod pod id). This is the field that must not be lost: the terminate finalizer uses it to reclaim the instance. type: string + lastAccruedAt: + description: |- + LastAccruedAt is how far cost accrual has counted: an ANCHOR for the next measurement, + not a note about the last one. "Accrued" in the accounting sense — cost incurred but not + yet invoiced, which is all EstimatedCostUSD ever holds. + + It is the reason a restart loses nothing: the next window is rate x (now - LastAccruedAt), + so time that passed while Nebula was down is still counted on recovery instead of vanishing + with the in-memory total. + + The invariant that makes it safe: this NEVER moves past cost that has been durably + recorded, because it advances only in the same patch that writes EstimatedCostUSD. A + failed write therefore loses nothing — the same window is counted next time. + + Unset means "not counting yet". Never treat it as the epoch, which would charge decades + on the first tick. + format: date-time + type: string nodeName: description: NodeName is the virtual Node created for this instance, once it exists. @@ -158,6 +213,21 @@ spec: phase: description: Phase is the coarse lifecycle state. type: string + priceUSDPerHour: + description: |- + PriceUSDPerHour is what this instance costs per hour in USD, as a decimal string + ("7.9000"), resolved from the provider's catalog against the served Pod's shape. + Status, not spec: it is a derived result nobody can declare up front. A string + because it is written to be READ — a print column can only echo a field, never + scale a fixed-point integer back into currency. + + Empty means UNPRICED, not free: the provider implements no provider.Pricer, or its + catalog has no row for this candidate. Consumers must skip such a claim rather than + count it as $0. + + Written once and never refreshed, so a catalog edit cannot retroactively reprice a + running instance and rewrite the cost history it has already reported. + type: string type: object type: object served: true diff --git a/docs/architecture.md b/docs/architecture.md index 6fd467c..8cabea9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -568,7 +568,12 @@ Two design rules are worth knowing here, because they constrain the code above: `Handler.metricLabels` and `placementLabels` are field-for-field mirrors. - Every label is bounded by **configuration** (NodePools, provider catalogs), never by workload. Nothing derived from a Pod name, UID, namespace or unresolved user-supplied - pool label is ever a label value. + pool label is ever a label value. Cost is no exception, which is why per-instance spend + is a NodeClaim status field rather than a label on the counter. Cost *attribution* is the + single opt-in breach of that rule — `--cost-labels` promotes chosen Pod labels onto the + counter — so it ships off by default, and warns rather than capping when the series count + says an operator chose badly: a billing metric that rewrote its own labels to defend + itself would be wrong where nobody could see it. See [metrics.md](metrics.md) for the full series list, label semantics, example queries and the two known gaps. diff --git a/docs/metrics.md b/docs/metrics.md index dfeac5a..01b343a 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -19,12 +19,13 @@ placed (gate removed) nebula_placement_decisions_total instance accepted | nebula_instance_ready_duration_seconds v -Running +Running nebula_cost_usd_total <- dollars, while it bills ``` Those are the parts whose cost and failure modes are otherwise invisible: placement can -silently leave a Pod gated forever, and provisioning runs against a third party, takes -seconds to minutes, bills money, and fails for reasons the Pod status flattens away. +silently leave a Pod gated forever, provisioning runs against a third party, takes +seconds to minutes, bills money, and fails for reasons the Pod status flattens away, and +the instance it produces keeps charging whether or not anything is using it. Everything else is already covered elsewhere and deliberately not duplicated here — reconcile counts, queue depth and API latency by controller-runtime's own collectors, and Pod-population questions ("how many Pods are gated right now?") by kube-state-metrics. @@ -32,6 +33,8 @@ Pod-population questions ("how many Pods are gated right now?") by kube-state-me - [Where they are served](#where-they-are-served) - [Placement](#placement) - [Provisioning](#provisioning) +- [Cost](#cost) +- [Attribution](#attribution) - [Label semantics](#label-semantics) - [Example queries](#example-queries) - [Known gaps](#known-gaps) @@ -39,7 +42,8 @@ Pod-population questions ("how many Pods are gated right now?") by kube-state-me ## Where they are served Every collector registers into controller-runtime's registry (see `pkg/metrics`; the -`init` in each file is what registers them, so importing the package is the only wiring), +`init` in each file is what registers them, so importing the package is the only wiring — +except the cost counter, whose labels are not known until `--cost-labels` is parsed), which means they are served on the manager's existing `--metrics-bind-address` endpoint alongside the standard controller and workqueue metrics. In the default overlay that is `:8443` with authn/authz, so a scrape needs a bearer token whose subject is bound to the @@ -111,6 +115,179 @@ Fine-grained detail is deliberately *not* here: it stays where it is already ava (the Pod's `Failed` status message and the `vnode-handler` error log). These labels exist to answer "are we losing capacity, or are our credentials broken?" at a glance. +## Cost + +What the fleet has actually spent. + +| Metric | Type | What it answers | +| --- | --- | --- | +| `nebula_cost_usd_total{phase,...}` | counter | Dollars the fleet has run up, added one accrual window at a time. Also carries whatever `--cost-labels` names (see [Attribution](#attribution)). | + +```promql +# Dollars spent yesterday, by provider. +sum by (provider) (increase(nebula_cost_usd_total[1d])) + +# What the last accrual window cost, fleet-wide. +sum(increase(nebula_cost_usd_total[5m])) + +# Burn rate in USD/hour. +sum(rate(nebula_cost_usd_total[30m])) * 3600 +``` + +**Per shape, not per instance.** One series per candidate — the labels in [Label +semantics](#label-semantics), plus `phase` — with no claim identity on it. That is what makes the +metric *billable* rather than merely observable: `increase(...[w])` is a pure function of the +window `w`, so a billing service replaying an old window re-derives the same dollars and can +upsert them idempotently. A per-claim counter would give up exactly that — differencing a +cumulative per-claim series needs a sample at each window boundary, and an instance that lived +and died between two boundaries has neither, so its spend would be unbillable. Per-claim spend +lives on the claim instead (below). + +**Instance-level** cost — infrastructure spend, blind to the workload on top, charging each +claim's whole rate. + +**Every number here is an estimate.** Both the counter and the field are Nebula's own arithmetic +over a hand-maintained list price — cost *incurred*, which is what "accrued" means, not cost any +provider has confirmed. Nothing below has been invoiced. + +**One arithmetic, two views.** A leader-elected loop closes each claim's window every 5 minutes: +it charges `priceUSDPerHour × (now − status.lastAccruedAt)`, adds it to +`NodeClaim.status.estimatedCostUSD` (the `EST_COST` column) and re-anchors, then books the *same* +window on the counter. So the counter is the fleet's stream of charges and the field is the +per-claim rollup of them, and "what did this one instance cost" is a `kubectl get` rather than a +query: + +``` +NAME PROVIDER ACCELERATOR PRICE/HR EST_COST PHASE +nc-train-0 modal H100:8 23.7000 148.1250 Bound +``` + +The counter is advanced only *after* the field's patch lands. A counter has no idempotency key, so +a window booked before its write was durable would be charged twice: the anchor would not have +moved, and the next tick would re-derive the same window. The persisted anchor is also why no time +is lost to a restart — the first tick back charges the whole gap, downtime included — and why the +window is measured from `status`, not from how long the loop actually slept. + +**Teardown closes the last window too.** No checkpoint can close it — the object is going away, so +there is nothing left to re-anchor — but it is still booked on the counter, gated by the same rule: +strictly after the finalizer removal is accepted. That `Update` is a compare-and-swap, so it lands +at most once and only for a claim whose ledger is current, which is the same exactly-once guarantee +the anchor gives the checkpoint path. Without it the metric would miss far more than one window: an +instance that died before its first accrual tick would be billed **nothing at all**, making short +workloads look free while long ones stayed accurate. + +`EST_COST` does *not* get that last window, so it understates a reclaimed instance's lifetime by up +to one interval while the counter has the whole of it. The lifetime figure also goes to the log +(`instance reclaimed`), the only record that outlives both the object and Prometheus retention. + +**Where the number comes from.** `NodeClaim.status.priceUSDPerHour`, resolved from the +provider's catalog (`pkg/provider/catalog/data/*.csv`) against the served Pod's shape and +written once, on the claim's first reconcile — pinned so a catalog edit cannot retroactively +reprice a running instance. These are hand-maintained list prices: no committed-use discounts, +no private pricing, no gap between a Spot quote and the actual charge. Reconcile against the +provider's billing export before anyone gets invoiced. + +**Which claims are charged.** Only `Bound` and `Terminating` hold an instance (see +`NodeClaimPhase`), so only they accrue; a `Terminated` claim's `EST_COST` is its frozen final +total, and nothing more is booked against it. `Provisioning` is excluded, undercounting by about +one poll interval per instance. + +`Terminating` still bills until teardown finishes, which is what makes the `phase` label worth +having: + +```promql +# Dollars burned on instances whose workload was already gone. Rising steadily means the +# teardown backstop is stuck. +sum(increase(nebula_cost_usd_total{phase="Terminating"}[1d])) +``` + +A claim with no usable price (no `Pricer`, or no catalog row) accrues **nothing**, never `0` — a +zero would be summed and averaged as a real "this costs nothing". Fleet totals therefore +under-report by whatever cannot be priced; an empty `PRICE/HR` in `kubectl get nc` finds those +claims. + +## Attribution + +Who to charge. Off by default; `--cost-labels` turns it on: + +``` +--cost-labels=org_id,team_id +``` + +Each name is used **verbatim** as both the Pod label key and the metric label, so there is no +mapping to keep in sync — and no way for the two to disagree. That costs you qualified keys: +`nebula.inftyai.com/org-id` is a legal Pod label and an illegal Prometheus label, so the name has +to satisfy both grammars (letters, digits, underscores; starting with a letter). A name that does +not, or that shadows a label the metric already has, is rejected at startup rather than silently +reporting every tenant as `none`. + +Two names nothing rejects but you should still avoid: **`job` and `instance`**. Prometheus attaches +its own at scrape time, and with the default `honor_labels: false` it renames yours to +`exported_job` / `exported_instance` — so `sum by (job)` would quietly report the scrape job instead +of the tenant, with no error anywhere. `job_id` or `workload` if that is the breakdown you want. + +The values are read off the served Pod once — when the claim first becomes chargeable, in the same +status patch that opens its billing window — and pinned on +`NodeClaim.status.costLabels`: + +```yaml +status: + costLabels: + org_id: acme + team_id: ml +``` + +Three things follow from pinning them there rather than reading the Pod at accrual time: + +- **Spend cannot be re-attributed.** Relabelling a Pod does not move dollars already reported + under another tenant, exactly as a catalog edit cannot reprice a running instance. +- **The last window is attributable.** A `Terminating` claim's Pod is often already gone, and that + window is [booked at teardown](#cost) — from `status`, which is still there. +- **The breakdown is auditable per claim.** `kubectl get nodeclaim -o yaml` shows who a given + instance was charged to, which no aggregate metric can answer. + +A claim serves exactly one Pod, UID-pinned (`spec.podRef`), so its whole rate belongs to that one +workload: nothing is split, and nothing is counted twice. + +```promql +# Yesterday's bill, per tenant. +sum by (org_id) (increase(nebula_cost_usd_total[1d])) + +# One team's burn rate, in USD/hour. +sum(rate(nebula_cost_usd_total{org_id="acme",team_id="ml"}[30m])) * 3600 +``` + +**Values are tenant-controlled, which is a cardinality risk** — and the only one on this endpoint, +since every other label is bounded by configuration. A counter never releases a series, so a +workload generator emitting a fresh `org_id` per Pod leaks one series per Pod for the life of the +process. **Nothing caps this.** Past 5000 cost series the manager logs a warning once: + +``` +WARNING: the cost metric has passed its expected series budget, which usually means an +attribution label is carrying a per-Pod value. Nothing is dropped or merged, so the dollars +stay correct, but memory and every scrape grow until this process restarts. +``` + +Warning rather than enforcing is deliberate. Merging tenants into an `overflow` bucket, or dropping +the window, each corrupts a metric an external billing service reads as truth — and silently, in +the one direction its consumer cannot detect. A metric that is too *big* is an operator's problem +with an obvious fix (constrain the values at admission — a webhook or a policy engine); a metric +that quietly rewrote its own labels is nobody's problem until invoicing. + +Prometheus knows the real number, so alert on it there rather than trusting the log line to be seen: + +```promql +count(nebula_cost_usd_total) > 5000 +``` + +A Pod that carries none of the configured labels reports `none`, the same placeholder every other +absent label uses. + +**Changing `--cost-labels` changes the identity of every cost series.** Adding a name resets each +one to zero and starts a new set, which `increase()` reads as a reset and handles, but no +historical series will carry the new label. Roll it out at a boundary you are happy to see in a +dashboard. + ## Label semantics Every series except the `{pool,reason}` and `{provider,capacity_type,region,reason}` @@ -120,9 +297,10 @@ diagnostics carries the same label set: provider region capacity_type accelerator accelerator_count ``` -That is on purpose: a placement and the provisioning attempt it led to carry **identical -label values**, so the two join in PromQL without label surgery — "placed on Spot but -never provisioned" is one query. For the same reason the two duration histograms measure +That is on purpose: a placement, the provisioning attempt it led to, and the cost of the +instance it produced carry **identical label values**, so they join in PromQL without label +surgery — "placed on Spot but never provisioned" is one query, and so is "what did our +Spot fallbacks cost us". For the same reason the two duration histograms measure adjacent legs of one journey: `placement_wait` ends exactly where `instance_ready` begins, so together they cover `kubectl apply` to `Running`. @@ -154,9 +332,14 @@ Five label values are load-bearing: than in the label. A spike here alongside flat `capacity`/`auth` series is the shape of a network or provider outage. -Cardinality is bounded by *configuration*, not by workload: providers x regions x tiers x -accelerator pools, all of which come from NodePools and provider catalogs. Nothing -derived from a Pod name, UID or namespace is ever a label. +Cardinality is bounded by *configuration*, not by workload, on every label above: providers x +regions x tiers x accelerator pools, all of which come from NodePools and provider catalogs. That +is what makes it safe that they are in-process counters whose series live until the process exits — +including cost, which is why it carries no claim identity. + +The one exception is [attribution](#attribution), whose values come from Pod labels. It is off by +default, and warns rather than caps when on — so if you enable it, the bound is whatever your +admission policy puts on those label values. ## Example queries @@ -185,16 +368,55 @@ sum by (provider) (rate(nebula_provision_failures_total{reason="other"}[6h])) # Requests nobody is retrying their way out of: a human has to fix these. sum by (reason) (rate(nebula_placement_deferrals_total{reason=~"no_pool|invalid_request"}[1h])) + +# What the Spot fallback actually cost over the last week: the same candidate labels, so this +# joins the placement decision to the bill. +sum by (capacity_type) (increase(nebula_cost_usd_total[7d])) + +# Dollars per placement, by candidate. Rising without a price change means instances are +# being held longer per workload. +sum by (provider, accelerator) (increase(nebula_cost_usd_total[1d])) + / sum by (provider, accelerator) (increase(nebula_placement_decisions_total[1d])) + +# Current burn rate in USD/hour, by accelerator. +sum by (accelerator) (rate(nebula_cost_usd_total[30m])) * 3600 ``` ## Known gaps -Both are deliberate, and both bias toward looking *better* than reality — worth knowing -before trusting a dashboard. - -- **Counters reset on restart.** Every collector is in-process. This is ordinary Prometheus - semantics — `rate()` and `increase()` detect resets — but no cumulative history survives - a redeploy; the scrape backend owns durability. +All of these are deliberate, and all bias toward looking *better* than reality — worth +knowing before trusting a dashboard. + +- **Counters reset on restart.** Every counter and histogram here is in-process, cost included: + a redeploy or a leader handoff starts it at zero. This is ordinary Prometheus semantics — + `rate()` and `increase()` detect resets, and no window is lost across one, since the anchor in + `status` survives — but no cumulative history survives a redeploy; the scrape backend owns + durability. Aggregate as `sum(increase(...))`, never `increase(sum(...))`: only the former sees + the per-series reset. +- **`EST_COST` understates a reclaimed instance; the metric does not.** The field is not written on + the deletion path (the object is going away), so it misses the window still open at teardown — + which the counter *does* book (see [Cost](#cost)). The two therefore disagree by up to one + interval on any claim that has been reclaimed, the counter being the complete figure and the log + line agreeing with it. `EST_COST` also lags live spend by up to one interval while a claim is + running — a freshness limit, not an error, since a window not charged now is charged in full next + tick. +- **A crash between the teardown write and the booking loses that window.** The final window is + booked in-process right after the finalizer removal lands, and unlike a checkpoint it has no + anchor to fall back on, so a process that dies in between charges it nowhere. Exactly-once in the + direction that matters — it cannot double-charge — but it can drop one partial window per + unlucky teardown. +- **Cost is a list price, and misses two windows** — `Provisioning` claims, and any claim the + provider cannot price. See [Cost](#cost); reconcile against the billing export before + invoicing anyone. +- **Attribution is only as good as the Pod's labels, and is frozen.** A Pod that was missing its + `org_id` when the claim became chargeable is booked to `none` for its whole life; labelling it + afterwards does not backfill, by design (see [Attribution](#attribution)). Enforce the labels at + admission — a webhook or a policy engine — rather than trusting the metric to notice. +- **Attribution cardinality is unbounded.** Enabling `--cost-labels` promotes tenant-controlled + values onto a counter that never releases a series, and nothing caps it: a bad label choice grows + manager memory and the scrape payload until a restart. The 5000-series warning is a smoke alarm, + not a limit — see [Attribution](#attribution) for why it does not enforce, and alert on + `count(nebula_cost_usd_total)`. - **`instance_ready_duration` under-samples slow boots.** The start timestamp lives only in the virtual node's in-memory tracking map, so a provision still in flight when the manager restarts is re-adopted without one and is never observed. A missing sample beats diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go new file mode 100644 index 0000000..8c78b8f --- /dev/null +++ b/internal/controller/cost_accrual.go @@ -0,0 +1,297 @@ +/* +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 controller + +import ( + "context" + "strconv" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" + "github.com/InftyAI/Nebula/pkg/metrics" + "github.com/InftyAI/Nebula/pkg/util" +) + +// accrualInterval is how often spend is checkpointed. It is a WRITE cadence, not an accounting +// resolution: every window is measured from the persisted anchor, so a longer interval trades +// freshness of the EST_COST column for fewer writes, never accuracy. Five minutes is ~1.7 +// writes/s across a 500-claim fleet. +const accrualInterval = 5 * time.Minute + +// accrualTimeout bounds one whole tick, List plus every write. A tick that cannot finish loses +// nothing: the anchors it did not reach are still where they were, so the next tick charges the +// same windows. +const accrualTimeout = time.Minute + +// CostAccrual advances each claim's durable spend ledger on a ticker. +// +// A Runnable rather than a hook on the reconcile path: spend accrues with the CLOCK, not with +// events, and a Bound claim can sit for hours without a reconcile. Leader election is the +// manager's default for a plain Runnable and is load-bearing here — two replicas accruing the +// same fleet would double every dollar. +// +// It lives beside the reconciler rather than in pkg/metrics because it WRITES: the ledger is a +// status field, and instrumentation that patches API objects is no longer instrumentation. +type CostAccrual struct { + client.Client + interval time.Duration + // now is overridable so tests can drive whole windows without sleeping. + now func() time.Time +} + +var _ manager.Runnable = (*CostAccrual)(nil) + +// NewCostAccrual builds the accrual loop over the manager's client: cached reads, direct writes. +func NewCostAccrual(c client.Client) *CostAccrual { + return &CostAccrual{Client: c, interval: accrualInterval, now: time.Now} +} + +// Start accrues until ctx is cancelled. Unlike a loop that charges the time between its own +// ticks, this one is anchored per claim in status, so a tick that is late, skipped, or the first +// after a restart all charge exactly the time that really passed. The window still open at +// shutdown is not lost either — the next process charges it. +func (a *CostAccrual) Start(ctx context.Context) error { + // An idle fleet writes nothing and emits no series at all, which is indistinguishable from a + // loop that never started. This line is the only evidence either way. + log := logf.Log.WithName("cost-accrual") + log.Info("starting cost accrual", "interval", a.interval) + + ticker := time.NewTicker(a.interval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return nil + case <-ticker.C: + a.accrueAll(ctx) + } + } +} + +// accrueAll checkpoints every billing claim once. Errors are logged per claim and never +// propagated: one unwritable claim must not cost the rest of the fleet its window. +func (a *CostAccrual) accrueAll(ctx context.Context) { + log := logf.Log.WithName("cost-accrual") + + ctx, cancel := context.WithTimeout(ctx, accrualTimeout) + defer cancel() + + var claims nebulav1alpha1.NodeClaimList + if err := a.List(ctx, &claims); err != nil { + log.Error(err, "listing nodeclaims to accrue") + return + } + for i := range claims.Items { + nc := &claims.Items[i] + if err := a.accrue(ctx, nc); err != nil { + // A conflict is the ordinary case — the reconciler patched the same claim from its + // own copy. The anchor did not move, so this window is simply charged next tick. + if apierrors.IsConflict(err) || apierrors.IsNotFound(err) { + log.V(1).Info("skipping accrual this tick", "claim", nc.Name, "reason", err.Error()) + continue + } + log.Error(err, "accruing claim cost", "claim", nc.Name) + } + } +} + +// accrue persists what the claim has cost as of now and re-anchors there, in one patch. +// +// It decides WHETHER to write; costNow decides what the number is. The pair of fields being one +// patch is what makes it crash-safe: the anchor can never +// advance past money that was recorded, so an interrupted tick re-charges the same window rather +// than skipping it. +// +// Charging is also idempotent per unit time — the amount follows from the anchor, not from how +// many ticks fire — and Update carries a resourceVersion, so a writer working from a stale ledger +// is rejected outright. Together that makes a second, unelected accruer wasteful, not harmful. +func (a *CostAccrual) accrue(ctx context.Context, nc *nebulav1alpha1.NodeClaim) error { + if _, ok := billingRate(nc); !ok { + // Not billing. Note this is stricter than costNow, which still reports a Terminated + // claim's frozen total: nothing more can be charged, so writing it again is pure churn. + return nil + } + // Second precision, because that is all metav1.Time serializes. Reading the anchor back + // truncated while having charged from the untruncated instant would re-charge the dropped + // fraction every tick — a small but systematic OVER-count, the one direction this must + // never drift. + now := a.now().Truncate(time.Second) + + // An anchor in the future — a wall-clock jump, or a hand-edited field. Charging the negative + // window would rewind the ledger. Left in place rather than pulled back to now, so billing + // resumes by itself once the clock passes it, having lost only the bogus window. + if at := nc.Status.LastAccruedAt; at != nil && !now.After(at.Time) { + return nil + } + // A claim with no anchor at all — one that became billable before this build, or whose Bound + // patch did not carry the stamp — falls through with nothing added: the window before this + // point has an unknown length, and guessing it would invent spend. Opening one here is the + // whole write. + prev := costSoFar(nc) + total, _ := costNow(nc, now) + nc.Status.EstimatedCostUSD = formatCost(total) + nc.Status.LastAccruedAt = &metav1.Time{Time: now} + if err := a.Status().Update(ctx, nc); err != nil { + return err + } + // Booked only now that the window is durable — see metrics.RecordWindow. Re-reading the + // field instead of using total charges the counter the same ROUNDED dollars the ledger took, + // so the two reconcile exactly rather than drifting a fraction of a cent per tick. + metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, costSoFar(nc)-prev) + return nil +} + +// billingRate returns the claim's hourly rate and whether it should be charged at all. +// +// Two gates, and both are omissions rather than zeros — a zero would be summed and averaged as +// a real "this costs nothing", while absent spend is honestly unknown: +// +// - Phase. Only Bound and Terminating hold an instance that exists (see NodeClaimPhase). +// Terminating still bills until teardown finishes. Provisioning is excluded and undercounts +// by roughly one poll tick, the same lag the phase itself carries. +// - Price. Empty means UNPRICED, not free (see Status.PriceUSDPerHour): no Pricer, or no +// catalog row. An unparseable value is a corrupted claim and is treated the same. +func billingRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { + switch nc.Status.Phase { + case nebulav1alpha1.NodeClaimBound, nebulav1alpha1.NodeClaimTerminating: + default: + return 0, false + } + rate, err := strconv.ParseFloat(nc.Status.PriceUSDPerHour, 64) + if err != nil || rate <= 0 { + return 0, false + } + return rate, true +} + +// costSoFar reads the running total. An unparseable value is treated as zero rather than as a +// reason to stop accruing: losing the history of one corrupted claim beats freezing its ledger +// and under-reporting the fleet forever. +func costSoFar(nc *nebulav1alpha1.NodeClaim) float64 { + total, err := strconv.ParseFloat(nc.Status.EstimatedCostUSD, 64) + if err != nil || total < 0 { + return 0 + } + return total +} + +func formatCost(usd float64) string { + return strconv.FormatFloat(usd, 'f', priceDecimals, 64) +} + +// stampAccrualStart opens the billing window the moment a claim first becomes chargeable, +// returning whether it mutated the claim. +// +// Load-bearing rather than an optimisation: it is what makes a crash BEFORE the first +// checkpoint lossless. The anchor is what recovery measures from, so without it a claim that +// billed for 90 seconds and then lost its manager would be charged from whenever the next tick +// happened to find it. Free, because it rides the status patch markPhase is already making. +func stampAccrualStart(nc *nebulav1alpha1.NodeClaim) bool { + if nc.Status.LastAccruedAt != nil { + return false + } + if _, ok := billingRate(nc); !ok { + return false + } + nc.Status.LastAccruedAt = &metav1.Time{Time: time.Now().Truncate(time.Second)} + return true +} + +// settleFinalCost books the window still open at teardown and logs what the instance cost over its +// whole life. +// +// Booking is safe here despite there being no anchor left to move, because the caller has already +// had its finalizer removal ACCEPTED: that Update is a compare-and-swap, so it lands at most once, +// and only for a copy of the claim whose ledger is current — a stale anchor would have conflicted +// there rather than re-charging time the loop already took. Skipping it would lose far more than +// one window: an instance that died before its first accrual tick would be charged NOTHING at all. +// +// The lifetime figure itself only goes to the log; status is deleted with the object. +func settleFinalCost(ctx context.Context, nc *nebulav1alpha1.NodeClaim) { + // The absent/zero distinction costNow reports is for the ledger; a log line may say $0. + total, _ := costNow(nc, time.Now()) + // Zero once the instance is already gone (Terminated freezes the total), which RecordWindow + // drops rather than minting a series for. + metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, total-costSoFar(nc)) + logf.FromContext(ctx).Info("instance reclaimed", "lifetimeCostUSD", formatCost(total)) +} + +// costNow is what a claim has cost as of now: the persisted ledger plus the window still open +// since its anchor. False means the claim has no cost to report AT ALL — neither billable nor +// ever charged, which is absent spend rather than zero spend (see billingRate). +// +// Both the checkpoint and the final log line read through here, so the ledger and the number a +// reclaimed instance reports cannot drift. +func costNow(nc *nebulav1alpha1.NodeClaim, now time.Time) (float64, bool) { + rate, billing := billingRate(nc) + total := costSoFar(nc) + if !billing && total == 0 { + return 0, false + } + if billing && nc.Status.LastAccruedAt != nil { + if open := now.Sub(nc.Status.LastAccruedAt.Time); open > 0 { + total += rate * open.Hours() + } + } + return total, true +} + +// claimLabels renders the candidate a claim was provisioned against, so its cost carries the same +// label values as the placement and provisioning attempts that produced it and the three join +// without label surgery. +func claimLabels(nc *nebulav1alpha1.NodeClaim) metrics.Labels { + accelerator, count := util.SplitAcceleratorPool(nc.Spec.Accelerator) + return metrics.Labels{ + Provider: nc.Spec.Provider, + Region: nc.Spec.Region, + CapacityType: string(nc.Spec.CapacityType), + Accelerator: accelerator, + AcceleratorCount: count, + } +} + +// stampCostLabels copies the configured attribution labels off the served Pod, returning whether +// it mutated the claim. +// +// Written once and never refreshed, for the same reason the price is: spend already reported +// under one tenant must not be re-attributed to another by an edit to a Pod label. It runs +// alongside stampAccrualStart in one patch, so attribution is durable before the first window it +// has to explain. +func stampCostLabels(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) bool { + names := metrics.CostLabelNames() + if nc.Status.CostLabels != nil || pod == nil || len(names) == 0 { + return false + } + stamped := map[string]string{} + for _, name := range names { + if v := pod.Labels[name]; v != "" { + stamped[name] = v + } + } + // An empty map, not nil: a Pod carrying none of the configured labels is a settled fact, and + // nil would make every later reconcile look it up again. + nc.Status.CostLabels = stamped + return true +} diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go new file mode 100644 index 0000000..3830288 --- /dev/null +++ b/internal/controller/cost_accrual_test.go @@ -0,0 +1,523 @@ +/* +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 controller + +import ( + "context" + "errors" + "math" + "reflect" + "strconv" + "strings" + "testing" + "time" + + "github.com/prometheus/client_golang/prometheus/testutil" + 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/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/event" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" + "github.com/InftyAI/Nebula/pkg/metrics" +) + +// billingClaim is a claim holding a priced instance, anchored ago in the past. A nil ago leaves +// the anchor unset, i.e. never checkpointed. +func billingClaim(name, price string, ago *time.Duration) *nebulav1alpha1.NodeClaim { + nc := &nebulav1alpha1.NodeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: nebulav1alpha1.NodeClaimSpec{Provider: "modal", Accelerator: "H100:1"}, + Status: nebulav1alpha1.NodeClaimStatus{ + Phase: nebulav1alpha1.NodeClaimBound, + PriceUSDPerHour: price, + }, + } + if ago != nil { + nc.Status.LastAccruedAt = &metav1.Time{Time: time.Now().Add(-*ago).Truncate(time.Second)} + } + return nc +} + +// newAccrual wires the loop over a fake client seeded with claims, with the clock pinned so a +// whole window can be charged without sleeping. +func newAccrual(t *testing.T, claims ...*nebulav1alpha1.NodeClaim) (*CostAccrual, client.Client) { + t.Helper() + metrics.CostTotal.Reset() + + objs := make([]client.Object, 0, len(claims)) + for _, nc := range claims { + objs = append(objs, nc) + } + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(objs...). + WithStatusSubresource(&nebulav1alpha1.NodeClaim{}). + Build() + return NewCostAccrual(c), c +} + +// ledger reads back the persisted total and anchor. +func ledger(t *testing.T, c client.Client, name string) (float64, *metav1.Time) { + t.Helper() + var nc nebulav1alpha1.NodeClaim + if err := c.Get(context.Background(), client.ObjectKey{Name: name}, &nc); err != nil { + t.Fatalf("get claim %q: %v", name, err) + } + if nc.Status.EstimatedCostUSD == "" { + return 0, nc.Status.LastAccruedAt + } + total, err := strconv.ParseFloat(nc.Status.EstimatedCostUSD, 64) + if err != nil { + t.Fatalf("status.estimatedCostUSD %q is not a number: %v", nc.Status.EstimatedCostUSD, err) + } + return total, nc.Status.LastAccruedAt +} + +// booked is the dollars the cost counter holds across every series. +func booked(t *testing.T) float64 { + t.Helper() + if testutil.CollectAndCount(metrics.CostTotal) == 0 { + return 0 + } + return testutil.ToFloat64(metrics.CostTotal) +} + +func TestCostAccrual_ChargesFromTheAnchor(t *testing.T) { + halfHour := 30 * time.Minute + a, c := newAccrual(t, billingClaim("bound", "98.3200", &halfHour)) + + a.accrueAll(context.Background()) + + want := 98.32 * 0.5 + total, anchor := ledger(t, c, "bound") + if math.Abs(total-want) > 1e-3 { + t.Fatalf("status.estimatedCostUSD %v, want %v", total, want) + } + // The same window reaches the counter, so a consumer's increase() and the field agree. + if got := booked(t); math.Abs(got-total) > 1e-9 { + t.Fatalf("booked %v, want %v — the counter must take exactly what the ledger did", got, total) + } + if time.Since(anchor.Time) > time.Minute { + t.Fatalf("anchor was not moved forward: %v", anchor) + } +} + +// The whole point of persisting a timestamp: a window that spans a restart is charged in full on +// the first tick back, not clipped to one interval. +func TestCostAccrual_RecoversDowntime(t *testing.T) { + down := 3 * time.Hour + a, c := newAccrual(t, billingClaim("bound", "10.0000", &down)) + + a.accrueAll(context.Background()) + + if total, _ := ledger(t, c, "bound"); math.Abs(total-30) > 1e-2 { + t.Fatalf("status.estimatedCostUSD %v after a 3h gap, want 30 — the downtime was not recovered", total) + } +} + +// Successive checkpoints accumulate on the field rather than replacing it. +func TestCostAccrual_AccumulatesAcrossTicks(t *testing.T) { + hour := time.Hour + a, c := newAccrual(t, billingClaim("bound", "4.0000", &hour)) + + // First tick charges the hour; the second, driven by a clock pushed 30 minutes on, charges + // the half hour since. + a.accrueAll(context.Background()) + a.now = func() time.Time { return time.Now().Add(30 * time.Minute) } + a.accrueAll(context.Background()) + + if total, _ := ledger(t, c, "bound"); math.Abs(total-6) > 1e-2 { + t.Fatalf("status.estimatedCostUSD %v, want 6 (1h + 0.5h at $4/hr)", total) + } +} + +// A claim that became billable before the ledger existed has no anchor. Opening one must not +// invent spend for the window whose length nobody knows. +func TestCostAccrual_StampsMissingAnchorWithoutCharging(t *testing.T) { + a, c := newAccrual(t, billingClaim("bound", "98.3200", nil)) + + a.accrueAll(context.Background()) + + total, anchor := ledger(t, c, "bound") + if total != 0 { + t.Fatalf("status.estimatedCostUSD %v on an unanchored claim, want 0", total) + } + if anchor == nil { + t.Fatal("no anchor was written, so the next tick will charge nothing either") + } + if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { + t.Fatalf("collected %d series, want 0 — nothing was charged", n) + } +} + +// A claim holding no instance, or one nobody can price, accrues NOTHING rather than zero: a zero +// is summed and averaged as a real "this costs nothing". +func TestCostAccrual_SkipsNonBilling(t *testing.T) { + hour := time.Hour + unbilled := func(name, price string, phase nebulav1alpha1.NodeClaimPhase) *nebulav1alpha1.NodeClaim { + nc := billingClaim(name, price, &hour) + nc.Status.Phase = phase + return nc + } + claims := []*nebulav1alpha1.NodeClaim{ + unbilled("provisioning", "3.9500", nebulav1alpha1.NodeClaimProvisioning), + // The instance is gone; the claim lingers only as a ledger. + unbilled("terminated", "3.9500", nebulav1alpha1.NodeClaimTerminated), + unbilled("no-phase", "3.9500", ""), + // Bound but UNPRICED (no Pricer, or no catalog row) — not free. + unbilled("unpriced", "", nebulav1alpha1.NodeClaimBound), + unbilled("malformed", "cheap", nebulav1alpha1.NodeClaimBound), + unbilled("nonpositive", "0.0000", nebulav1alpha1.NodeClaimBound), + } + a, c := newAccrual(t, claims...) + + a.accrueAll(context.Background()) + + for _, nc := range claims { + if total, _ := ledger(t, c, nc.Name); total != 0 { + t.Fatalf("claim %q accrued %v, want 0", nc.Name, total) + } + } + if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { + t.Fatalf("collected %d series, want 0 — no non-billing claim may accrue", n) + } +} + +// An anchor in the future (a wall-clock jump, a hand-edited field) must not rewind the ledger. +func TestCostAccrual_DropsBackwardsWindow(t *testing.T) { + ahead := -time.Hour + a, c := newAccrual(t, billingClaim("bound", "4.0000", &ahead)) + + a.accrueAll(context.Background()) + + if total, _ := ledger(t, c, "bound"); total != 0 { + t.Fatalf("status.estimatedCostUSD %v on a backwards window, want 0", total) + } + if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { + t.Fatalf("collected %d series on a backwards window, want 0", n) + } +} + +// A write that fails must leave the anchor where it was, so the next tick charges the same window +// once — not twice, and not never. +func TestCostAccrual_FailedWriteChargesNothing(t *testing.T) { + hour := time.Hour + nc := billingClaim("bound", "98.3200", &hour) + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(nc). + WithStatusSubresource(&nebulav1alpha1.NodeClaim{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { + return apierrors.NewConflict(schema.GroupResource{Resource: "nodeclaims"}, "bound", errors.New("stale")) + }, + }). + Build() + a := NewCostAccrual(c) + + a.accrueAll(context.Background()) + + if total, anchor := ledger(t, c, "bound"); total != 0 || anchor.Time.Before(time.Now().Add(-90*time.Minute)) { + t.Fatalf("a failed write moved the ledger: estimatedCostUSD=%v anchor=%v", total, anchor) + } + // The ordering that keeps a retry from over-charging: a counter has no idempotency key, so a + // window booked before its patch landed would be charged again next tick. + if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { + t.Fatalf("collected %d series after a failed write, want 0", n) + } +} + +// costNow is what both the checkpoint and the teardown log line read, so the window still open +// since the anchor has to be in it. +func TestCostNow(t *testing.T) { + hour := time.Hour + anchored := billingClaim("bound", "10.0000", &hour) + anchored.Status.EstimatedCostUSD = "5.0000" + + // Terminated: the instance is gone, so the persisted total is final and no window is added. + settled := billingClaim("settled", "10.0000", &hour) + settled.Status.Phase = nebulav1alpha1.NodeClaimTerminated + settled.Status.EstimatedCostUSD = "5.0000" + + cases := map[string]struct { + claim *nebulav1alpha1.NodeClaim + want float64 + report bool + }{ + "ledger plus the open window": {claim: anchored, want: 15, report: true}, + "terminated total is frozen": {claim: settled, want: 5, report: true}, + "unanchored charges nothing": {claim: billingClaim("fresh", "10.0000", nil), want: 0, report: true}, + // Never billable and never charged: absent cost, which must not be reported as zero. + "unpriced is absent, not zero": {claim: billingClaim("unpriced", "", &hour), want: 0, report: false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, ok := costNow(tc.claim, time.Now()) + if ok != tc.report { + t.Fatalf("costNow ok = %v, want %v", ok, tc.report) + } + if math.Abs(got-tc.want) > 1e-2 { + t.Fatalf("costNow = %v, want %v", got, tc.want) + } + }) + } +} + +// status is deliberately untouched on the way out: writing it would race the deletion. +func TestSettleFinalCost_LeavesStatusAlone(t *testing.T) { + hour := time.Hour + nc := billingClaim("bound", "10.0000", &hour) + nc.Status.EstimatedCostUSD = "5.0000" + + settleFinalCost(context.Background(), nc) + + if nc.Status.EstimatedCostUSD != "5.0000" { + t.Fatalf("settleFinalCost wrote status.estimatedCostUSD %q; it must not touch a deleting claim", + nc.Status.EstimatedCostUSD) + } +} + +// The window open at teardown reaches the counter even though no checkpoint can close it — without +// this, an instance that never survived a tick would be billed nothing at all. +func TestSettleFinalCost_BooksTheOpenWindow(t *testing.T) { + metrics.CostTotal.Reset() + + hour := time.Hour + // Never checkpointed: the whole of its life is the open window. + nc := billingClaim("bound", "10.0000", &hour) + nc.Status.Phase = nebulav1alpha1.NodeClaimTerminating + + settleFinalCost(context.Background(), nc) + + if got := booked(t); math.Abs(got-10) > 1e-2 { + t.Fatalf("booked %v at teardown, want 10 (1h at $10/hr charged by no tick)", got) + } +} + +// Once the instance is gone there is no open window, and re-booking the frozen total would charge +// the claim's whole life a second time. +func TestSettleFinalCost_BooksNothingOnceGone(t *testing.T) { + metrics.CostTotal.Reset() + + hour := time.Hour + settled := billingClaim("settled", "10.0000", &hour) + settled.Status.Phase = nebulav1alpha1.NodeClaimTerminated + settled.Status.EstimatedCostUSD = "5.0000" + + settleFinalCost(context.Background(), settled) + + if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { + t.Fatalf("collected %d series for an instance already gone, want 0", n) + } +} + +// The window is booked against the candidate the claim was provisioned on, so cost joins the +// placement and provisioning series without label surgery. +func TestCostAccrual_BooksAgainstTheCandidate(t *testing.T) { + hour := time.Hour + nc := billingClaim("bound", "10.0000", &hour) + nc.Spec.Region = "us-east-1" + nc.Spec.CapacityType = nebulav1alpha1.CapacitySpot + a, _ := newAccrual(t, nc) + + a.accrueAll(context.Background()) + + // The exposition format is line-oriented, so this cannot be wrapped. + //nolint:lll + want := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="Spot",phase="Bound",provider="modal",region="us-east-1"} 10 +` + if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(want)); err != nil { + t.Fatal(err) + } +} + +// Attribution is copied off the Pod once and then frozen: spend already reported under one tenant +// must not move to another because somebody relabelled a Pod. +func TestStampCostLabels(t *testing.T) { + t.Cleanup(func() { metrics.ConfigureCostForTest(nil) }) + metrics.ConfigureCostForTest([]string{"org_id", "team_id"}) + + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"org_id": "acme", "team_id": "ml", "unconfigured": "ignored"}, + }} + + nc := billingClaim("bound", "10.0000", nil) + if !stampCostLabels(nc, pod) { + t.Fatal("stampCostLabels reported no change on an unstamped claim") + } + want := map[string]string{"org_id": "acme", "team_id": "ml"} + if !reflect.DeepEqual(nc.Status.CostLabels, want) { + t.Fatalf("stamped %v, want %v — only configured names are copied", nc.Status.CostLabels, want) + } + + // A relabelled Pod must not re-attribute the claim. + pod.Labels["org_id"] = "someone-else" + if stampCostLabels(nc, pod) { + t.Fatal("stampCostLabels rewrote attribution that was already settled") + } + if nc.Status.CostLabels["org_id"] != "acme" { + t.Fatalf("org_id moved to %q", nc.Status.CostLabels["org_id"]) + } + + // A Pod carrying none of them is a settled fact too, or every later reconcile re-checks it. + bare := billingClaim("bare", "10.0000", nil) + if !stampCostLabels(bare, &corev1.Pod{}) || bare.Status.CostLabels == nil { + t.Fatal("a Pod with no configured labels must still stamp an empty map") + } +} + +// With no --cost-labels there is nothing to copy, and the claim must stay untouched rather than +// carrying an empty map that reads as "we looked and found nothing". +func TestStampCostLabels_NotConfigured(t *testing.T) { + nc := billingClaim("bound", "10.0000", nil) + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"org_id": "acme"}}} + + if stampCostLabels(nc, pod) || nc.Status.CostLabels != nil { + t.Fatalf("stamped %v with no cost labels configured, want nil", nc.Status.CostLabels) + } +} + +// The window booked carries the claim's stamped attribution, which is the whole point of pinning +// it in status: the Pod is long gone by the time a Terminating window is charged. +func TestCostAccrual_BooksAttribution(t *testing.T) { + t.Cleanup(func() { metrics.ConfigureCostForTest(nil) }) + metrics.ConfigureCostForTest([]string{"org_id"}) + + hour := time.Hour + nc := billingClaim("bound", "10.0000", &hour) + nc.Status.CostLabels = map[string]string{"org_id": "acme"} + a, _ := newAccrual(t, nc) + + a.accrueAll(context.Background()) + + //nolint:lll + want := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="acme",phase="Bound",provider="modal",region="none"} 10 +` + if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(want)); err != nil { + t.Fatal(err) + } +} + +// The stamp is what makes a crash before the first checkpoint lossless, so it must land exactly +// when the claim becomes chargeable — and never move once set. +func TestStampAccrualStart(t *testing.T) { + hour := time.Hour + cases := map[string]struct { + claim *nebulav1alpha1.NodeClaim + want bool + }{ + "billable and unanchored": {claim: billingClaim("a", "3.9500", nil), want: true}, + "already anchored": {claim: billingClaim("b", "3.9500", &hour), want: false}, + "unpriced": {claim: billingClaim("c", "", nil), want: false}, + } + provisioning := billingClaim("d", "3.9500", nil) + provisioning.Status.Phase = nebulav1alpha1.NodeClaimProvisioning + cases["not holding an instance"] = struct { + claim *nebulav1alpha1.NodeClaim + want bool + }{claim: provisioning, want: false} + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + before := tc.claim.Status.LastAccruedAt + if got := stampAccrualStart(tc.claim); got != tc.want { + t.Fatalf("stampAccrualStart = %v, want %v", got, tc.want) + } + if !tc.want && tc.claim.Status.LastAccruedAt != before { + t.Fatal("the anchor was rewritten") + } + }) + } +} + +// A cost checkpoint must not re-enqueue the claim, but anything alongside it must. +func TestIgnoreCostAccrual(t *testing.T) { + p := ignoreCostAccrual() + hour := time.Hour + + base := billingClaim("bound", "3.9500", &hour) + costOnly := base.DeepCopy() + costOnly.Status.EstimatedCostUSD = "12.0000" + costOnly.Status.LastAccruedAt = &metav1.Time{Time: time.Now()} + costOnly.ResourceVersion = "999" + if p.Update(event.UpdateEvent{ObjectOld: base, ObjectNew: costOnly}) { + t.Fatal("a cost-only checkpoint was enqueued") + } + + alsoPhase := costOnly.DeepCopy() + alsoPhase.Status.Phase = nebulav1alpha1.NodeClaimTerminating + if !p.Update(event.UpdateEvent{ObjectOld: base, ObjectNew: alsoPhase}) { + t.Fatal("a phase change riding along with a checkpoint was dropped") + } + + withFinalizer := base.DeepCopy() + withFinalizer.Finalizers = append(withFinalizer.Finalizers, nebulav1alpha1.TerminateInstanceFinalizer) + if !p.Update(event.UpdateEvent{ObjectOld: base, ObjectNew: withFinalizer}) { + t.Fatal("a finalizer update was dropped; the reconciler requeues off that event") + } +} + +func TestCostAccrual_StartStopsOnContextCancel(t *testing.T) { + a, _ := newAccrual(t) + a.interval = time.Millisecond + + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { done <- a.Start(ctx) }() + + cancel() + select { + case err := <-done: + if err != nil { + t.Fatalf("Start returned %v, want nil", err) + } + case <-time.After(5 * time.Second): + t.Fatal("Start did not return after its context was cancelled") + } +} + +// Start must actually checkpoint on a tick. Only that something is persisted is asserted: a test +// cannot pin the scheduler to a known window. +func TestCostAccrual_StartAccrues(t *testing.T) { + hour := time.Hour + a, c := newAccrual(t, billingClaim("bound", "3.9500", &hour)) + a.interval = time.Millisecond + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { _ = a.Start(ctx) }() + + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if total, _ := ledger(t, c, "bound"); total > 0 { + return + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("Start persisted nothing after 5s of 1ms ticks") +} diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index 325ca92..05dd59b 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -18,18 +18,24 @@ package controller import ( "context" + "errors" + "strconv" "time" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" @@ -78,9 +84,9 @@ type NodeClaimReconciler struct { client.Client Scheme *runtime.Scheme - // Providers resolves a provider name (NodeClaim.spec.provider) to its - // backend, used ONLY on the teardown path. Defaults to the process-wide - // registry; overridable in tests. + // Providers resolves a provider name (NodeClaim.spec.provider) to its backend, for + // the teardown path (Terminate) and for pricing (the optional provider.Pricer). + // Defaults to the process-wide registry; overridable in tests. Providers func(name string) (provider.Provider, bool) } @@ -246,12 +252,16 @@ func (r *NodeClaimReconciler) findInstanceID(ctx context.Context, prov provider. } // releaseFinalizer removes the terminate finalizer, allowing the API server to -// delete the NodeClaim. +// delete the NodeClaim — and settles what the instance cost over its whole life on the +// way out, since status.estimatedCostUSD goes with the object. func (r *NodeClaimReconciler) releaseFinalizer(ctx context.Context, nc *nebulav1alpha1.NodeClaim) (ctrl.Result, error) { if controllerutil.RemoveFinalizer(nc, nebulav1alpha1.TerminateInstanceFinalizer) { if err := r.Update(ctx, nc); err != nil { return ctrl.Result{}, err } + // Strictly after the update, which is what makes the cost booking inside exactly-once: + // this branch is unreachable on a retry, since the finalizer is already gone. + settleFinalCost(ctx, nc) } return ctrl.Result{}, nil } @@ -302,12 +312,12 @@ func (r *NodeClaimReconciler) desiredPhase(nc *nebulav1alpha1.NodeClaim, pod *co } // markPhase reflects the served Pod into the claim's status: the coarse phase -// desiredPhase maps it to, plus best-effort status.InstanceID, in one idempotent -// write. An empty desiredPhase means "leave the phase as-is" (the Bound-flap hold); -// 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 the Pod carries it, -// regardless of phase. A no-op (phase unchanged and id already set) writes nothing. +// desiredPhase maps it to, plus best-effort status.InstanceID and status.PriceUSDPerHour, +// in one idempotent write. An empty desiredPhase means "leave the phase as-is" (the +// Bound-flap hold); 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 the Pod +// carries it, regardless of phase. A no-op (nothing to change) writes nothing. func (r *NodeClaimReconciler) markPhase(ctx context.Context, nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) error { changed := false if phase := r.desiredPhase(nc, pod); phase != "" && nc.Status.Phase != phase { @@ -317,6 +327,18 @@ func (r *NodeClaimReconciler) markPhase(ctx context.Context, nc *nebulav1alpha1. if recordInstanceID(nc, pod) { changed = true } + if r.recordPrice(ctx, nc, pod) { + changed = true + } + if stampCostLabels(nc, pod) { + changed = true + } + // Last, so it sees the phase and price the two calls above just set: the window opens the + // moment those make the claim chargeable — and in the same patch as the attribution above, + // so no window is ever charged before we know who to charge it to. + if stampAccrualStart(nc) { + changed = true + } if !changed { return nil } @@ -346,6 +368,67 @@ func recordInstanceID(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) bool { return true } +// priceDecimals is how many fractional digits status.PriceUSDPerHour keeps. Four, not two: +// the cheapest catalog rows are sub-cent per hour (a T4 fraction, Modal's memory +// component), and rounding those to cents would price them at 0.00 — indistinguishable +// from the UNPRICED empty string. +const priceDecimals = 4 + +// recordPrice resolves the hourly cost of what this claim runs and records it, returning +// whether it mutated the claim. Set ONCE: a claim already carrying a price keeps it, which +// is what pins the rate against a later catalog edit (see Status.PriceUSDPerHour). +// +// Here rather than at claim creation because the status subresource strips status on Create, +// and here rather than in the optimizer because the price must describe what was actually +// launched — the Pod's own CPU/memory reservation included, which only a provider metering +// them apart from the accelerator charges for. +// +// Unpriceable is not a failure: a provider with no Pricer, or a candidate with no catalog +// row, leaves the field empty and every consumer skips the claim. Pricing NEVER fails a +// reconcile — a wrong-looking cost column must not stop a teardown ledger from doing its +// actual job. +func (r *NodeClaimReconciler) recordPrice(ctx context.Context, nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) bool { + if nc.Status.PriceUSDPerHour != "" || pod == nil { + return false + } + prov, ok := r.provider(nc.Spec.Provider) + if !ok { + return false + } + pricer, ok := prov.(provider.Pricer) + if !ok { + return false // this provider cannot price its instances + } + + log := logf.FromContext(ctx) + accelerator, count, err := util.AcceleratorRequest(pod) + if err != nil { + // A contradictory request (a GPU count with no type). Provision rejects it too, so + // there is nothing here worth more than a trace. + log.V(1).Info("skipping price: unreadable accelerator request", "error", err) + return false + } + cpuCores, memoryMiB := util.PodReservation(pod) + rate, err := pricer.PricePerHour(provider.PriceRequest{ + AcceleratorType: accelerator, + Count: count, + CapacityType: nc.Spec.CapacityType, + CPUCores: cpuCores, + MemoryMiB: memoryMiB, + }) + if err != nil { + if errors.Is(err, provider.ErrNoPrice) { + log.V(1).Info("no price for claim", "provider", nc.Spec.Provider, "accelerator", nc.Spec.Accelerator) + } else { + // A malformed request, i.e. our bug: loud, but still not fatal to the reconcile. + log.Error(err, "pricing claim", "provider", nc.Spec.Provider) + } + return false + } + nc.Status.PriceUSDPerHour = strconv.FormatFloat(rate, 'f', priceDecimals, 64) + return true +} + // wasBound reports whether an external instance has ever been confirmed to exist // for this claim — the fact the teardown backstop keys off. A Terminated claim was // necessarily Bound first, so it also counts. @@ -400,7 +483,7 @@ func (r *NodeClaimReconciler) patchStatus(ctx context.Context, nc *nebulav1alpha // requeue. func (r *NodeClaimReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). - For(&nebulav1alpha1.NodeClaim{}). + For(&nebulav1alpha1.NodeClaim{}, builder.WithPredicates(ignoreCostAccrual())). Watches(&corev1.Pod{}, handler.EnqueueRequestsFromMapFunc(r.claimsForPod)). Named("nodeclaim"). // One claim per Pod, and the teardown path is the expensive one: per claim a @@ -411,6 +494,32 @@ func (r *NodeClaimReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } +// ignoreCostAccrual drops the update events whose ONLY change is a cost checkpoint (see +// CostAccrual). Without it every claim in the fleet is re-enqueued on the accrual cadence for a +// reconcile that has nothing to do — at 500 claims, a few hundred wasted reconciles every +// interval into the workqueue that is already this controller's bottleneck at scale. +// +// Deliberately narrow: it compares the whole object with the cost fields neutralized, rather +// than filtering on generation, so the finalizer update this reconciler relies on for its own +// requeue still comes through. Anything else that changed alongside the cost is not dropped. +func ignoreCostAccrual() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + before, okOld := e.ObjectOld.(*nebulav1alpha1.NodeClaim) + after, okNew := e.ObjectNew.(*nebulav1alpha1.NodeClaim) + if !okOld || !okNew { + return true + } + rest := after.DeepCopy() + rest.Status.EstimatedCostUSD = before.Status.EstimatedCostUSD + rest.Status.LastAccruedAt = before.Status.LastAccruedAt + rest.ResourceVersion = before.ResourceVersion + rest.ManagedFields = before.ManagedFields + return !equality.Semantic.DeepEqual(before, rest) + }, + } +} + // claimsForPod maps a Pod event to the claim that serves it. The name is DERIVED rather // than searched for: util.ClaimName is what ensureClaim named the claim, so the same // namespace/name yields the same token, truncate-and-hash case included. diff --git a/internal/controller/nodeclaim_price_test.go b/internal/controller/nodeclaim_price_test.go new file mode 100644 index 0000000..a918f32 --- /dev/null +++ b/internal/controller/nodeclaim_price_test.go @@ -0,0 +1,222 @@ +/* +Copyright 2026. + +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 controller + +import ( + "fmt" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "sigs.k8s.io/controller-runtime/pkg/client" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" + "github.com/InftyAI/Nebula/pkg/provider" +) + +// pricedProvider is a fakeProvider that also implements provider.Pricer. A separate type +// on purpose: adding PricePerHour to fakeProvider itself would silently start pricing every +// other test's claims, and the no-Pricer path is one this must keep exercising. +type pricedProvider struct { + *fakeProvider + rate float64 + err error + got []provider.PriceRequest // every request, in order +} + +func (p *pricedProvider) PricePerHour(req provider.PriceRequest) (float64, error) { + p.got = append(p.got, req) + return p.rate, p.err +} + +// newPricedReconciler wires a reconciler whose only provider is a Pricer named "fake". +func newPricedReconciler(t *testing.T, objs []client.Object, pp *pricedProvider) (*NodeClaimReconciler, client.Client) { + t.Helper() + r, c := newClaimReconciler(t, objs) + r.Providers = func(name string) (provider.Provider, bool) { + if name == pp.name { + return pp, true + } + return nil, false + } + return r, c +} + +// gpuPod is a served Pod shaped like a real GPU workload: the accelerator label, a +// nvidia.com/gpu count, and a CPU/memory reservation. +func gpuPod(accelerator string, gpus int64, cpu, memory string) *corev1.Pod { + pod := newPod("p1", "default", "uid-1", corev1.PodRunning) + pod.Labels = map[string]string{nebulav1alpha1.AcceleratorTypeLabel: accelerator} + pod.Spec.Containers[0].Resources = corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(cpu), + corev1.ResourceMemory: resource.MustParse(memory), + "nvidia.com/gpu": *resource.NewQuantity(gpus, resource.DecimalSI), + }, + } + return pod +} + +func TestRecordPrice_WritesRateAndRequest(t *testing.T) { + pod := gpuPod("H100", 2, "4", "8Gi") + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + claim.Spec.CapacityType = nebulav1alpha1.CapacityOnDemand + pp := &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, rate: 7.9} + r, c := newPricedReconciler(t, []client.Object{pod, claim}, pp) + + reconcileClaim(t, r, "c1") + + // Four decimals, so a sub-cent rate is not rounded into the UNPRICED-looking 0. + if got := getClaim(t, c, "c1").Status.PriceUSDPerHour; got != "7.9000" { + t.Fatalf("PriceUSDPerHour = %q, want %q", got, "7.9000") + } + if len(pp.got) == 0 { + t.Fatal("PricePerHour was never called") + } + want := provider.PriceRequest{ + AcceleratorType: "H100", Count: 2, + CapacityType: nebulav1alpha1.CapacityOnDemand, + // The Pod's own reservation, which only a provider metering CPU/memory apart from + // the accelerator charges for — but the request carries it either way. + CPUCores: 4, MemoryMiB: 8192, + } + if pp.got[0] != want { + t.Fatalf("PriceRequest = %+v, want %+v", pp.got[0], want) + } +} + +// The rate is pinned at the first write: re-pricing on every reconcile would let a catalog +// edit retroactively reprice a running instance and rewrite cost history. +func TestRecordPrice_PinnedOnce(t *testing.T) { + pod := gpuPod("H100", 1, "1", "1Gi") + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + pp := &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, rate: 3.95} + r, c := newPricedReconciler(t, []client.Object{pod, claim}, pp) + + reconcileClaim(t, r, "c1") + if got := getClaim(t, c, "c1").Status.PriceUSDPerHour; got != "3.9500" { + t.Fatalf("first pass: PriceUSDPerHour = %q, want 3.9500", got) + } + + pp.rate = 99.0 // the catalog changes under a running instance + reconcileClaim(t, r, "c1") + + if got := getClaim(t, c, "c1").Status.PriceUSDPerHour; got != "3.9500" { + t.Fatalf("second pass: PriceUSDPerHour = %q, want the pinned 3.9500", got) + } + if len(pp.got) != 1 { + t.Fatalf("PricePerHour called %d times, want 1 — a priced claim must not be re-priced", len(pp.got)) + } +} + +// Unpriceable is not a failure. Every one of these leaves the field EMPTY (which readers +// treat as UNPRICED, never as free) and must not fail the reconcile — the claim's real job +// is teardown, and a missing cost column may never block it. +func TestRecordPrice_UnpriceableLeavesEmpty(t *testing.T) { + cases := map[string]struct { + prov func() provider.Provider + pod *corev1.Pod + }{ + "provider implements no Pricer": { + prov: func() provider.Provider { return &fakeProvider{name: "fake"} }, + pod: gpuPod("H100", 1, "1", "1Gi"), + }, + "no catalog row": { + prov: func() provider.Provider { + return &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, err: provider.ErrNoPrice} + }, + pod: gpuPod("H100", 1, "1", "1Gi"), + }, + "malformed request is loud but not fatal": { + prov: func() provider.Provider { + return &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, err: fmt.Errorf("bad request")} + }, + pod: gpuPod("H100", 1, "1", "1Gi"), + }, + "contradictory accelerator request": { + prov: func() provider.Provider { + return &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, rate: 1} + }, + // nvidia.com/gpu with no accelerator label: util.AcceleratorRequest rejects it. + pod: func() *corev1.Pod { + p := gpuPod("H100", 1, "1", "1Gi") + p.Labels = nil + return p + }(), + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + r, c := newClaimReconciler(t, []client.Object{tc.pod, claim}) + prov := tc.prov() + r.Providers = func(string) (provider.Provider, bool) { return prov, true } + + reconcileClaim(t, r, "c1") // fails the test on any error + + got := getClaim(t, c, "c1") + if got.Status.PriceUSDPerHour != "" { + t.Fatalf("PriceUSDPerHour = %q, want empty (UNPRICED)", got.Status.PriceUSDPerHour) + } + // The claim's own job must still have happened. + if got.Status.Phase != nebulav1alpha1.NodeClaimBound { + t.Fatalf("phase = %q, want Bound — pricing must not disturb the ledger", got.Status.Phase) + } + }) + } +} + +// An unregistered provider cannot be priced and must not panic on the nil returned +// alongside ok=false. +func TestRecordPrice_UnknownProvider(t *testing.T) { + pod := gpuPod("H100", 1, "1", "1Gi") + claim := newClaim("c1", "p1", "default", "uid-1", "gone") + r, c := newClaimReconciler(t, []client.Object{pod, claim}) + r.Providers = func(string) (provider.Provider, bool) { return nil, false } + + reconcileClaim(t, r, "c1") + + if got := getClaim(t, c, "c1").Status.PriceUSDPerHour; got != "" { + t.Fatalf("PriceUSDPerHour = %q, want empty", got) + } +} + +// A CPU-only claim still reaches the Pricer: a provider that can run CPU-only work +// (Modal) prices it, and one that cannot answers ErrNoPrice, which is its call to make +// rather than ours to pre-empt. +func TestRecordPrice_CPUOnlyReachesPricer(t *testing.T) { + pod := newPod("p1", "default", "uid-1", corev1.PodRunning) + pod.Spec.Containers[0].Resources = corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("512Mi"), + }, + } + claim := newClaim("c1", "p1", "default", "uid-1", "fake") + pp := &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, rate: 0.0276} + r, c := newPricedReconciler(t, []client.Object{pod, claim}, pp) + + reconcileClaim(t, r, "c1") + + if got := getClaim(t, c, "c1").Status.PriceUSDPerHour; got != "0.0276" { + t.Fatalf("PriceUSDPerHour = %q, want 0.0276", got) + } + want := provider.PriceRequest{CPUCores: 0.5, MemoryMiB: 512} + if pp.got[0] != want { + t.Fatalf("PriceRequest = %+v, want %+v", pp.got[0], want) + } +} diff --git a/pkg/metrics/attribution.go b/pkg/metrics/attribution.go new file mode 100644 index 0000000..3a92a67 --- /dev/null +++ b/pkg/metrics/attribution.go @@ -0,0 +1,161 @@ +/* +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 metrics + +import ( + "fmt" + "regexp" + "strings" + "sync" + + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +// costSeriesWarnThreshold is where cost cardinality stops looking like a fleet and starts looking +// like a leak. Attribution values come from Pod labels, which are TENANT-controlled, and a counter +// never releases a series, so a workload minting a fresh value per Pod grows process memory and +// every scrape payload until a restart. +// +// A budget rather than a guess: ~1KB per series in client_golang, so 5000 is a few MB of memory and +// roughly a megabyte of exposition text — far more than any fleet reaches by candidate shape alone. +const costSeriesWarnThreshold = 5000 + +// costLabelName is the intersection of two grammars, because one token is used as both a +// Kubernetes label key and a Prometheus label name: Prometheus forbids '-' and '.' and a +// leading digit, Kubernetes forbids a leading or trailing '_'. Anything outside the overlap is +// rejected at startup rather than mangled, since a silently normalised name that collides with +// another is a mis-attribution nobody would notice. +var costLabelName = regexp.MustCompile(`^[a-zA-Z]([a-zA-Z0-9_]*[a-zA-Z0-9])?$`) + +// ParseCostLabels turns the --cost-labels value ("org_id,team_id") into the attribution +// dimension, in the order given. Empty input means no attribution, which is the default. +// +// Each entry names BOTH the Pod label to read and the metric label to emit — one token, so what +// the manifest configures is exactly what appears in PromQL. It fails rather than skipping a bad +// entry: silently dropping one would be discovered at invoicing time, when every series has +// already been recorded as "none". +// +// Only the NAME is checked here. An entry that shadows a dimension the counter already carries +// ("provider", "phase") parses, then fails at InitCost, where the registry rejects a duplicate +// label name — still at startup, just with a less pointed message. +// +// Setting this at all is what makes cost the only metric here whose CARDINALITY is not bounded by +// configuration: the values come from Pod labels. Nothing caps them — noteSeries only warns — so +// pick names whose value set the cluster's admission policy actually constrains. +func ParseCostLabels(spec string) ([]string, error) { + fields := strings.Split(spec, ",") + names := make([]string, 0, len(fields)) + seen := map[string]struct{}{} + for _, raw := range fields { + name := strings.TrimSpace(raw) + if name == "" { + continue + } + if len(name) > 63 { + return nil, fmt.Errorf("cost label %q is longer than the 63-character Kubernetes label limit", name) + } + if !costLabelName.MatchString(name) { + return nil, fmt.Errorf("cost label %q must match %s — it is used as both a Pod label key "+ + "and a Prometheus label name, so '-', '.' and a leading digit are all out", name, costLabelName) + } + if _, dup := seen[name]; dup { + return nil, fmt.Errorf("cost label %q is listed twice", name) + } + seen[name] = struct{}{} + names = append(names, name) + } + if len(names) == 0 { + // nil rather than an empty slice: "unconfigured" is a state callers compare against. + return nil, nil + } + return names, nil +} + +// costLabelNames is the configured attribution dimension, in emit order. Read on every +// recording and written only by InitCost, before the manager starts. +// +// The ORDER lives here rather than in the stamped NodeClaim, which is what makes attribution +// safe against a flag change: values are looked up BY NAME at emit time, so adding, removing or +// reordering --cost-labels can never slide a team_id into the org_id column. +var costLabelNames []string + +// CostLabelNames is the configured attribution dimension. Exported so the controller that stamps +// the values onto a claim reads the SAME list the metric emits, rather than keeping a second copy +// that could disagree with it. +func CostLabelNames() []string { + return costLabelNames +} + +// attributionValues renders the configured dimension from a claim's stamped labels, in +// costLabelNames order. A name the claim never carried reports "none" rather than being +// omitted, which would move the sample to a different series. +func attributionValues(stamped map[string]string) []string { + if len(costLabelNames) == 0 { + return nil + } + values := make([]string, 0, len(costLabelNames)) + for _, name := range costLabelNames { + values = append(values, orNone(stamped[name])) + } + return values +} + +// costSeries holds the label combinations emitted so far, and only until the warning fires: it is a +// leak DETECTOR, not a guard, so it is dropped once it has done its one job rather than growing +// alongside the leak it reports. Guarded because there are two recorders — the accrual loop and the +// reconciler workers that settle a teardown. +var ( + costSeriesMu sync.Mutex + costSeries map[string]struct{} + costSeriesWarned bool +) + +// noteSeries warns once when the cost counter passes costSeriesWarnThreshold distinct series. +// +// It deliberately caps NOTHING. Merging tenants into an "overflow" bucket, or dropping the window, +// would each corrupt a metric an external billing service reads as truth; a metric that is too big +// is an operator's problem to fix at admission, while one that quietly rewrote its own labels is +// nobody's problem until invoicing. Skipped entirely without attribution, where every dimension is +// bounded by NodePools and provider catalogs. +func noteSeries(values []string) { + if len(costLabelNames) == 0 { + return + } + + costSeriesMu.Lock() + defer costSeriesMu.Unlock() + + if costSeriesWarned { + return + } + if costSeries == nil { + costSeries = map[string]struct{}{} + } + // NUL, because it cannot appear in a Kubernetes label value: joining on any legal character + // would let two different combinations collide into one key. + costSeries[strings.Join(values, "\x00")] = struct{}{} + if len(costSeries) <= costSeriesWarnThreshold { + return + } + costSeriesWarned = true + logf.Log.WithName("metrics").Info("WARNING: the cost metric has passed its expected series "+ + "budget, which usually means an attribution label is carrying a per-Pod value. Nothing is "+ + "dropped or merged, so the dollars stay correct, but memory and every scrape grow until this "+ + "process restarts. Constrain these label values at admission.", + "series", len(costSeries), "threshold", costSeriesWarnThreshold, "costLabels", costLabelNames) + costSeries = nil +} diff --git a/pkg/metrics/attribution_test.go b/pkg/metrics/attribution_test.go new file mode 100644 index 0000000..b5bcf9c --- /dev/null +++ b/pkg/metrics/attribution_test.go @@ -0,0 +1,179 @@ +/* +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 metrics + +import ( + "reflect" + "strconv" + "strings" + "testing" + + "github.com/prometheus/client_golang/prometheus/testutil" +) + +// withAttribution swaps in a label set for one test and restores the default afterwards. It goes +// through configureCost rather than InitCost on purpose: a registry refuses the second set of label +// dimensions it ever sees for a name, and testutil collects straight from the collector anyway. +func withAttribution(t *testing.T, names ...string) { + t.Helper() + + configureCost(names) + t.Cleanup(func() { configureCost(nil) }) +} + +func TestParseCostLabels(t *testing.T) { + cases := map[string]struct { + spec string + want []string + err bool + }{ + "empty is no attribution": {spec: "", want: nil}, + "names in order": {spec: "org_id,team_id", want: []string{"org_id", "team_id"}}, + "whitespace and blanks": {spec: " org_id , , team_id ", want: []string{"org_id", "team_id"}}, + // One token has to be legal as a Pod label key AND a Prometheus label name. + "hyphen is not a prometheus name": {spec: "org-id", err: true}, + "dot is not a prometheus name": {spec: "org.id", err: true}, + "qualified key": {spec: "nebula.inftyai.com/org-id", err: true}, + "leading digit": {spec: "2team", err: true}, + "trailing underscore": {spec: "org_", err: true}, + "duplicate": {spec: "org_id,org_id", err: true}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, err := ParseCostLabels(tc.spec) + if tc.err { + if err == nil { + t.Fatalf("ParseCostLabels(%q) = %v, want an error", tc.spec, got) + } + return + } + if err != nil { + t.Fatalf("ParseCostLabels(%q): %v", tc.spec, err) + } + if !reflect.DeepEqual(got, tc.want) { + t.Fatalf("ParseCostLabels(%q) = %v, want %v", tc.spec, got, tc.want) + } + }) + } +} + +// A name that shadows one of the counter's own dimensions is not rejected by the parser — it is +// caught one step later, when InitCost registers a descriptor with a duplicate label name. This +// pins that remaining net, because it is the only thing standing between --cost-labels=provider and +// a manager that starts with a broken cost metric. +func TestInitCost_RejectsAShadowingName(t *testing.T) { + t.Cleanup(func() { configureCost(nil) }) + + if err := InitCost([]string{"provider"}); err == nil { + t.Fatal("InitCost accepted a label name the counter already carries") + } +} + +// The configured names become metric labels, and a Pod that carried none of them is reported as +// "none" rather than dropping the dimension — which would silently split the series. +func TestRecordWindow_Attribution(t *testing.T) { + withAttribution(t, "org_id", "team_id") + + l := Labels{Provider: "modal", Accelerator: "H100", AcceleratorCount: 1} + RecordWindow(l, "Bound", map[string]string{"org_id": "acme", "team_id": "ml"}, 3.95) + RecordWindow(l, "Bound", map[string]string{"org_id": "acme"}, 1.05) + RecordWindow(l, "Bound", nil, 0.5) + + // The exposition format is line-oriented, so these cannot be wrapped. + //nolint:lll + want := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="acme",phase="Bound",provider="modal",region="none",team_id="ml"} 3.95 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="acme",phase="Bound",provider="modal",region="none",team_id="none"} 1.05 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="none",phase="Bound",provider="modal",region="none",team_id="none"} 0.5 +` + if err := testutil.CollectAndCompare(CostTotal, strings.NewReader(want)); err != nil { + t.Fatal(err) + } +} + +// Values are looked up BY NAME, so the order they were stamped in cannot slide a team_id into the +// org_id column — the property that makes a --cost-labels change safe. +func TestRecordWindow_AttributionIsKeyedNotPositional(t *testing.T) { + withAttribution(t, "org_id", "team_id") + + l := Labels{Provider: "modal"} + RecordWindow(l, "Bound", map[string]string{"team_id": "ml", "org_id": "acme"}, 1) + RecordWindow(l, "Bound", map[string]string{"org_id": "acme", "team_id": "ml"}, 1) + + if n := testutil.CollectAndCount(CostTotal); n != 1 { + t.Fatalf("collected %d series, want 1 — the same pair rendered two ways", n) + } +} + +// Cardinality is WATCHED, never enforced: past the threshold the warning fires once and every +// window is still booked under its own real attribution. A billing metric that merged or dropped +// samples to defend itself would be wrong in a way its consumer could not detect. +func TestNoteSeries_WarnsWithoutCapping(t *testing.T) { + withAttribution(t, "org_id") + + l := Labels{Provider: "modal"} + for i := range costSeriesWarnThreshold + 10 { + RecordWindow(l, "Bound", map[string]string{"org_id": strconv.Itoa(i)}, 1) + } + + if n := testutil.CollectAndCount(CostTotal); n != costSeriesWarnThreshold+10 { + t.Fatalf("collected %d series, want %d — nothing may be merged away", n, costSeriesWarnThreshold+10) + } + if got := testutil.ToFloat64(CostTotal.WithLabelValues( + "modal", "none", "none", "none", "none", "Bound", "0")); got != 1 { + t.Fatalf("the first tenant's series holds %v, want 1 — its attribution was rewritten", got) + } + + // Having warned, the detector drops its bookkeeping rather than tracking a leak it has already + // reported, and stays quiet from then on. + costSeriesMu.Lock() + defer costSeriesMu.Unlock() + if !costSeriesWarned || costSeries != nil { + t.Fatalf("warned = %v, tracking %d keys; want warned with the tracking released", + costSeriesWarned, len(costSeries)) + } +} + +// The detector must not run at all without attribution, where every dimension is bounded by +// configuration — otherwise a long-lived fleet would eventually warn about its own shapes. +func TestNoteSeries_QuietWithoutAttribution(t *testing.T) { + configureCost(nil) + + RecordWindow(Labels{Provider: "modal"}, "Bound", map[string]string{"org_id": "acme"}, 1) + + costSeriesMu.Lock() + defer costSeriesMu.Unlock() + if costSeries != nil { + t.Fatalf("tracking %d keys with attribution off, want none", len(costSeries)) + } +} + +// With attribution off, a stamped claim's labels are ignored rather than joined onto the series: +// the counter's shape follows --cost-labels alone. +func TestRecordWindow_IgnoresStampsWhenUnconfigured(t *testing.T) { + CostTotal.Reset() + + for _, p := range []string{"modal", "aws", "runpod"} { + RecordWindow(Labels{Provider: p}, "Bound", map[string]string{"org_id": "ignored"}, 1) + } + + if n := testutil.CollectAndCount(CostTotal); n != 3 { + t.Fatalf("collected %d series, want 3 — one per provider, with no attribution dimension", n) + } +} diff --git a/pkg/metrics/cost.go b/pkg/metrics/cost.go new file mode 100644 index 0000000..4550f4e --- /dev/null +++ b/pkg/metrics/cost.go @@ -0,0 +1,97 @@ +/* +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 metrics + +import ( + "fmt" + + "github.com/prometheus/client_golang/prometheus" + ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +// CostTotal accumulates spend one CLOSED WINDOW at a time, which is what makes it billable: +// increase(...[w]) over any window is a pure function of that window, so a consumer replaying +// an old window re-derives the same dollars and can upsert them idempotently. +// +// Deliberately carries no claim identity. A per-claim series would churn — one per instance ever +// created, retained until the process exits — and worse, a claim that lived and died between two +// window boundaries could not be billed from it at all: differencing a cumulative per-claim series +// needs a sample at each boundary, and a short-lived instance has neither. Aggregating first does +// not help, since a sum over a changing claim set is not monotonic. Per-claim spend lives in +// NodeClaim.status.estimatedCostUSD (the EST_COST column) instead. +var CostTotal = newCostTotal(nil) + +// Deliberately no init here: CostTotal cannot self-register the way every other metric in this +// package does, because its label names are not known until --cost-labels is parsed. See InitCost. + +func newCostTotal(attribution []string) *prometheus.CounterVec { + return prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "nebula_cost_usd_total", + Help: "Cumulative USD billed by external instances, added window by window as it accrues.", + }, + withExtra(append([]string{"phase"}, attribution...)...), + ) +} + +// InitCost fixes the attribution dimension the cost counter carries, from --cost-labels, and +// registers it. MUST be called once from main after flags are parsed and before the manager +// starts — cost is the one metric here that is not scraped until it is. +// +// Two facts force that. A CounterVec's label names are set at CONSTRUCTION, while the flag is not +// known until main runs; and a Prometheus registry remembers a metric NAME's label dimensions for +// the life of the process even across Unregister, so registering a placeholder first would +// permanently forbid the real shape. Nothing is at risk in the gap: no window can be recorded +// before the manager starts. +// +// Not safe against concurrent recording. +func InitCost(attribution []string) error { + configureCost(attribution) + if err := ctrlmetrics.Registry.Register(CostTotal); err != nil { + return fmt.Errorf("registering cost counter with labels %v: %w", attribution, err) + } + return nil +} + +// configureCost rebuilds the counter for a label set. Split out so tests can swap the dimension +// without touching the process-wide registry, which would refuse the second shape it ever saw. +func configureCost(attribution []string) { + CostTotal = newCostTotal(attribution) + costLabelNames = attribution + + costSeriesMu.Lock() + defer costSeriesMu.Unlock() + costSeries = nil + costSeriesWarned = false +} + +// RecordWindow books the dollars one claim ran up over a single accrual window, attributed with +// the labels stamped on the claim (NodeClaimStatus.CostLabels). +// +// Call it only AFTER the window has been persisted: a counter has no idempotency key, so a +// window added twice is charged twice. Nothing is lost by waiting, because a failed write leaves +// the anchor in place and the next tick re-derives the same window. +func RecordWindow(l Labels, phase string, attribution map[string]string, usd float64) { + // A zero window is the ordinary case for a claim whose anchor was just opened, and a + // negative one would panic. Neither is worth a series. + if usd <= 0 { + return + } + values := l.values(append([]string{phase}, attributionValues(attribution)...)...) + noteSeries(values) + CostTotal.WithLabelValues(values...).Add(usd) +} diff --git a/pkg/metrics/cost_test.go b/pkg/metrics/cost_test.go new file mode 100644 index 0000000..c2da3d1 --- /dev/null +++ b/pkg/metrics/cost_test.go @@ -0,0 +1,79 @@ +/* +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 metrics + +import ( + "math" + "strings" + "testing" + + "github.com/prometheus/client_golang/prometheus/testutil" +) + +// Windows accumulate on one series, which is what makes increase() over any range the dollars +// charged in that range. +func TestRecordWindow_Accumulates(t *testing.T) { + CostTotal.Reset() + + l := Labels{Provider: "aws", Region: "us-east-1", CapacityType: "Spot", Accelerator: "H100", AcceleratorCount: 8} + RecordWindow(l, "Bound", nil, 8.1933) + RecordWindow(l, "Bound", nil, 8.1933) + + got := testutil.ToFloat64(CostTotal.WithLabelValues(l.values("Bound")...)) + if want := 2 * 8.1933; math.Abs(got-want) > 1e-9 { + t.Fatalf("booked %v, want %v", got, want) + } +} + +// Claims on the same candidate share a series — this is spend by shape, not per instance, so their +// windows must sum. A CPU-only claim renders both accelerator labels "none", and Terminating spend +// splits off so a stuck teardown is visible. +func TestRecordWindow_Series(t *testing.T) { + CostTotal.Reset() + + gpu := Labels{Provider: "modal", Accelerator: "H100", AcceleratorCount: 1} + RecordWindow(gpu, "Bound", nil, 3.95) + RecordWindow(gpu, "Bound", nil, 3.95) + RecordWindow(gpu, "Terminating", nil, 0.33) + RecordWindow(Labels{Provider: "modal"}, "Bound", nil, 0.0946) + + // The exposition format is line-oriented, so these cannot be wrapped. + //nolint:lll + want := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Bound",provider="modal",region="none"} 7.9 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Terminating",provider="modal",region="none"} 0.33 +nebula_cost_usd_total{accelerator="none",accelerator_count="none",capacity_type="none",phase="Bound",provider="modal",region="none"} 0.0946 +` + if err := testutil.CollectAndCompare(CostTotal, strings.NewReader(want)); err != nil { + t.Fatal(err) + } +} + +// A counter panics on a negative delta, and a zero window — the ordinary case for a claim whose +// anchor was just opened — would mint an empty series that reads as a real "this costs nothing". +func TestRecordWindow_DropsNonPositive(t *testing.T) { + CostTotal.Reset() + + for _, usd := range []float64{0, -1} { + RecordWindow(Labels{Provider: "modal"}, "Bound", nil, usd) + } + if n := testutil.CollectAndCount(CostTotal); n != 0 { + t.Fatalf("collected %d series on a non-positive window, want 0", n) + } +} diff --git a/pkg/metrics/doc.go b/pkg/metrics/doc.go index 29f942e..def30f1 100644 --- a/pkg/metrics/doc.go +++ b/pkg/metrics/doc.go @@ -18,18 +18,29 @@ limitations under the License. // // Everything here registers into controller-runtime's registry, so it is served on // the manager's existing --metrics-bind-address endpoint alongside the standard -// controller/workqueue metrics. Importing this package is what registers the -// collectors (see the init in each file); no wiring is needed in main. +// controller/workqueue metrics. Importing this package is enough wiring: every metric +// self-registers (see the init in each file) and every entry point is a helper the callers +// already on those paths invoke. +// +// The cost counter is the one exception: its label names depend on --cost-labels, so main must +// call InitCost once after flags are parsed. See InitCost for why it cannot self-register. +// +// Nothing here reads or writes the API. Cost also accrues onto a NodeClaim status field, so +// the loop that advances it stays in internal/controller and calls in here with each closed +// window. // // The instrumented surface is the path a Pod takes from admission to a running -// external instance, one file per leg: +// external instance, one file per leg, plus what that instance costs while it runs: // // placement.go the Pod is gated -> a candidate is chosen -> the gate is removed // provision.go the provider is called -> the instance reports Running +// cost.go dollars accrued, one billing window at a time +// attribution.go whose dollars those are, from the operator's chosen Pod labels // // Those are the legs whose cost and failure modes are otherwise invisible: placement -// can silently leave a Pod gated forever, and provisioning runs against a third party, -// takes minutes, bills money, and fails for reasons Pod status flattens away. +// can silently leave a Pod gated forever, provisioning runs against a third party, +// takes minutes, bills money, and fails for reasons Pod status flattens away, and the +// instance it produces keeps charging whether or not anything is using it. // Everything else is covered elsewhere and not duplicated — reconcile counts, queue // depth and API latency by controller-runtime's collectors, "how many Pods are gated // right now?" by kube-state-metrics. diff --git a/pkg/metrics/labels.go b/pkg/metrics/labels.go index 0b70658..7578605 100644 --- a/pkg/metrics/labels.go +++ b/pkg/metrics/labels.go @@ -43,11 +43,13 @@ const none = "none" // recoverable as accelerator + ":" + accelerator_count. var candidateLabels = []string{"provider", "region", "capacity_type", "accelerator", "accelerator_count"} -// withExtra returns candidateLabels plus one trailing dimension (result, reason), for -// the collectors that carry an outcome. It copies, because appending to a package-level -// slice would let two collectors share and overwrite one backing array. -func withExtra(name string) []string { - return append(append([]string{}, candidateLabels...), name) +// withExtra returns candidateLabels plus the trailing dimensions a collector carries of +// its own (result, reason, claim, phase). Variadic to pair with Labels.values, which takes +// the matching values the same way — the two are positional, so they must be given in the +// same order. It copies, because appending to a package-level slice would let two +// collectors share and overwrite one backing array. +func withExtra(names ...string) []string { + return append(append([]string{}, candidateLabels...), names...) } // Labels identifies the candidate one placement or provisioning attempt was made diff --git a/pkg/metrics/testing.go b/pkg/metrics/testing.go new file mode 100644 index 0000000..ca9573f --- /dev/null +++ b/pkg/metrics/testing.go @@ -0,0 +1,30 @@ +/* +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 metrics + +// ConfigureCostForTest points the cost counter at a label set, for tests in other packages that +// exercise attribution. Not for production use: InitCost is the real entry point. +// +// It exists because InitCost can only ever succeed ONCE per process — a registry remembers a +// metric name's label dimensions for the life of the process — so a test binary cannot try two +// label sets through it. This skips registration; testutil collects from the collector directly. +// +// Pass nil to restore the unconfigured default, and do so in a Cleanup: the counter is a package +// variable, so a test that leaves it swapped changes what every later test measures. +func ConfigureCostForTest(attribution []string) { + configureCost(attribution) +} diff --git a/pkg/provider/catalog/base.go b/pkg/provider/catalog/base.go index 1b2322b..84ec8b9 100644 --- a/pkg/provider/catalog/base.go +++ b/pkg/provider/catalog/base.go @@ -18,8 +18,10 @@ package catalog import ( "context" + "fmt" "strings" + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/provider" ) @@ -36,9 +38,9 @@ type Lookup interface { } // Base supplies the parts of provider.Provider that are identical for every -// adapter whose price/availability comes from a catalog: Name, Offerings, and a -// default identity MapAccelerator. Adapters embed it so those three methods are -// not re-implemented per provider: +// adapter whose price/availability comes from a catalog: Name, Offerings, a +// default identity MapAccelerator, and the optional provider.Pricer. Adapters +// embed it so none of those are re-implemented per provider: // // type Provider struct { // catalog.Base @@ -47,7 +49,9 @@ type Lookup interface { // // Name and Offerings are fully generic. MapAccelerator is generic only while a provider // names its accelerators like Nebula's canonical names (Modal does); one whose identifiers -// diverge overrides just that method and still reuses the rest. +// diverge overrides just that method and still reuses the rest. PricePerHour is generic +// only while a provider's catalog price is all-in; one that meters CPU/memory separately +// overrides it and adds those components. // // Lifecycle, Capabilities and ClassifyProvisionError are genuinely provider-specific and // are not provided here. @@ -133,3 +137,59 @@ func (b Base) MapAccelerator(canonical string, count int32) (providerAccelerator } return ids, len(ids) > 0 } + +// PricePerHour implements the optional provider.Pricer from the matched catalog row. That +// row is the WHOLE rate only where a provider's price is all-in (AWS sells an instance); one +// metering CPU and memory apart (Modal, whose rows quote a GPU alone) overrides this and +// adds them. +// +// Rows match as MapAccelerator matches them, plus capacity type: the one dimension +// MapAccelerator can ignore and pricing cannot (AWS p5.48xlarge is $34.412 Spot against +// $98.320 OnDemand). Among interchangeable alternates the FIRST row wins, so the price +// describes the id a launch actually tries first. +func (b Base) PricePerHour(req provider.PriceRequest) (float64, error) { + // A CPU-only request. No row can match an empty type, so this is the same ErrNoPrice the + // loop below would reach; it is stated here for a clearer message, and because it MUST + // land before the Count check, which would otherwise flag count 0 as malformed. A + // provider that really can run CPU-only work (Modal) overrides this and prices it. + if req.AcceleratorType == "" { + return 0, fmt.Errorf("catalog: %s prices accelerators only: %w", b.ProviderName, provider.ErrNoPrice) + } + // Contradictory input, not a missing price — hence a plain error, which callers do not + // swallow the way they swallow ErrNoPrice. + if req.Count <= 0 { + return 0, fmt.Errorf("catalog: %s: accelerator %q with count %d", b.ProviderName, req.AcceleratorType, req.Count) + } + // An empty tier is the candidate a pool declaring no capacityTypes emits. + // servesCapacityTier already reads that as non-Spot, so resolve it here rather than let + // row order decide — on AWS that is a threefold difference. + tier := req.CapacityType + if tier == "" { + tier = nebulav1alpha1.CapacityOnDemand + } + + for _, o := range b.Catalog.Offerings(b.ProviderName) { + if !strings.EqualFold(o.AcceleratorType, req.AcceleratorType) || !o.Available { + continue + } + if o.CapacityType != tier { + continue + } + if o.GPUCount != 0 && o.GPUCount != req.Count { + continue + } + // An unpriced placeholder row (a hand-edited ConfigMap), not a free accelerator. + if o.PricePerHour <= 0 { + continue + } + rate := o.PricePerHour + // GPUCount 0 => the row prices ONE accelerator and the count is a runtime knob, so + // the rate scales with it. A row carrying a count prices the whole offering already. + if o.GPUCount == 0 { + rate *= float64(req.Count) + } + return rate, nil + } + return 0, fmt.Errorf("catalog: %s has no available %s x%d row on %s: %w", + b.ProviderName, req.AcceleratorType, req.Count, tier, provider.ErrNoPrice) +} diff --git a/pkg/provider/catalog/catalog_test.go b/pkg/provider/catalog/catalog_test.go index 35b9c4e..4c150b9 100644 --- a/pkg/provider/catalog/catalog_test.go +++ b/pkg/provider/catalog/catalog_test.go @@ -17,6 +17,7 @@ limitations under the License. package catalog import ( + "errors" "os" "path/filepath" "reflect" @@ -271,6 +272,122 @@ func TestBaseMapAccelerator_SkipsUnavailable(t *testing.T) { } } +// The two catalog shapes price differently: a GPUCount 0 row (Modal) quotes ONE +// accelerator and scales with the count, while a row carrying a count (AWS) quotes the +// whole instance and must not be scaled. +func TestBasePricePerHour_Shapes(t *testing.T) { + od, sp := nebulav1alpha1.CapacityOnDemand, nebulav1alpha1.CapacitySpot + type pr = provider.PriceRequest + rows := []provider.Offering{ + {AcceleratorType: "H100", CapacityType: od, PricePerHour: 3.95, Available: true}, + { + AcceleratorType: "L4", AcceleratorID: "g6.48xlarge", GPUCount: 8, + CapacityType: od, PricePerHour: 13.350, Available: true, + }, + { + AcceleratorType: "L4", AcceleratorID: "g6.48xlarge", GPUCount: 8, + CapacityType: sp, PricePerHour: 4.672, Available: true, + }, + { + AcceleratorType: "L4", AcceleratorID: "g6.xlarge", GPUCount: 1, + CapacityType: od, PricePerHour: 0.805, Available: true, + }, + } + base := Base{ProviderName: "p", Catalog: fakeLookup{rows: rows}} + + cases := map[string]struct { + req provider.PriceRequest + want float64 + }{ + "per-accelerator row scales with count": {pr{AcceleratorType: "H100", Count: 2, CapacityType: od}, 7.90}, + "per-instance row does not scale": {pr{AcceleratorType: "L4", Count: 8, CapacityType: od}, 13.350}, + "count selects the row": {pr{AcceleratorType: "L4", Count: 1, CapacityType: od}, 0.805}, + "capacity type selects the row": {pr{AcceleratorType: "L4", Count: 8, CapacityType: sp}, 4.672}, + // An empty tier is the pool-declares-nothing candidate, priced as OnDemand rather + // than as whichever row comes first. + "empty tier is OnDemand": {pr{AcceleratorType: "L4", Count: 8}, 13.350}, + "case insensitive": {pr{AcceleratorType: "h100", Count: 1, CapacityType: od}, 3.95}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, err := base.PricePerHour(tc.req) + if err != nil { + t.Fatalf("PricePerHour(%+v): %v", tc.req, err) + } + if got != tc.want { + t.Fatalf("PricePerHour(%+v) = %v, want %v", tc.req, got, tc.want) + } + }) + } +} + +// Every unpriceable request must come back as ErrNoPrice so a caller can swallow it the +// way it swallows a provider with no Pricer at all — except a malformed one, which is a +// bug and must stay loud. +func TestBasePricePerHour_NoPrice(t *testing.T) { + od := nebulav1alpha1.CapacityOnDemand + base := Base{ProviderName: "p", Catalog: fakeLookup{rows: []provider.Offering{ + {AcceleratorType: "H100", CapacityType: od, PricePerHour: 3.95, Available: true}, + {AcceleratorType: "A100-80GB", CapacityType: od, PricePerHour: 2.50, Available: false}, + {AcceleratorType: "B200", CapacityType: od, PricePerHour: 0, Available: true}, + }}} + + noPrice := map[string]provider.PriceRequest{ + "cpu-only workload": {Count: 0}, + "unknown type": {AcceleratorType: "TPU-v4", Count: 1, CapacityType: od}, + "unavailable row": {AcceleratorType: "A100-80GB", Count: 1, CapacityType: od}, + "unpriced row": {AcceleratorType: "B200", Count: 1, CapacityType: od}, + "tier has no row": {AcceleratorType: "H100", Count: 1, CapacityType: nebulav1alpha1.CapacitySpot}, + "unknown tier": {AcceleratorType: "H100", Count: 1, CapacityType: "Reserved"}, + } + for name, req := range noPrice { + t.Run(name, func(t *testing.T) { + got, err := base.PricePerHour(req) + if !errors.Is(err, provider.ErrNoPrice) { + t.Fatalf("PricePerHour(%+v) err = %v, want ErrNoPrice", req, err) + } + if got != 0 { + t.Fatalf("PricePerHour(%+v) = %v, want 0 alongside the error", req, got) + } + }) + } + + // A counted accelerator with no count is contradictory input, not a missing price: + // swallowing it as ErrNoPrice would report a GPU workload as costing nothing. + _, err := base.PricePerHour(provider.PriceRequest{AcceleratorType: "H100", Count: 0, CapacityType: od}) + if err == nil || errors.Is(err, provider.ErrNoPrice) { + t.Fatalf("PricePerHour(H100 x0) err = %v, want a loud non-ErrNoPrice error", err) + } +} + +// The embedded CSVs must price through unchanged, which is what pins the per-GPU (Modal) +// against per-instance (AWS) reading of price_per_hour to the real data. +func TestBasePricePerHour_EmbeddedCatalog(t *testing.T) { + c, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + od := nebulav1alpha1.CapacityOnDemand + + modal := Base{ProviderName: "modal", Catalog: c} + got, err := modal.PricePerHour(provider.PriceRequest{AcceleratorType: "H100", Count: 2, CapacityType: od}) + if err != nil { + t.Fatalf("modal H100 x2: %v", err) + } + if got != 2*3.95 { + t.Fatalf("modal H100 x2 = %v, want %v (per-GPU rate x count)", got, 2*3.95) + } + + aws := Base{ProviderName: "aws", Catalog: c} + got, err = aws.PricePerHour(provider.PriceRequest{AcceleratorType: "H100", Count: 8, CapacityType: od}) + if err != nil { + t.Fatalf("aws H100 x8: %v", err) + } + if got != 98.320 { + t.Fatalf("aws H100 x8 = %v, want 98.320 (whole-instance rate, unscaled)", got) + } +} + func TestOfferings_CopyIsolation(t *testing.T) { c, err := Load() if err != nil { diff --git a/pkg/provider/catalog/data/modal.csv b/pkg/provider/catalog/data/modal.csv index d4aee2d..efd5024 100644 --- a/pkg/provider/catalog/data/modal.csv +++ b/pkg/provider/catalog/data/modal.csv @@ -38,4 +38,6 @@ A100-80GB,,,OnDemand,2.50,true,,2026-07-25 # "H100!", not "H100": Modal silently upgrades a bare H100 request onto an H200, and # the "!" suffix is its documented opt-out. H100,H100!,,OnDemand,3.95,true,,2026-07-25 -H200,,,OnDemand,4.55,true,,2026-07-25 +H200,,,OnDemand,4.54,true,,2026-08-31 +B200,,,OnDemand,6.25,true,,2026-08-31 +B300,,,OnDemand,7.10,true,,2026-08-31 diff --git a/pkg/provider/catalog/data/pricing.go b/pkg/provider/catalog/data/pricing.go new file mode 100644 index 0000000..25c3fe7 --- /dev/null +++ b/pkg/provider/catalog/data/pricing.go @@ -0,0 +1,57 @@ +/* +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 data holds the price catalog. The per-accelerator rows live in the CSVs +// (embedded by the parent package, so a price change is a reviewable data diff); this +// file holds the rates that are NOT per-accelerator, and so have no CSV row to sit on. +package data + +// Modal meters CPU and memory SEPARATELY from the accelerator, so a sandbox's hourly +// cost is the GPU price PLUS these. Not universal: AWS bundles both into the instance +// price (p5.48xlarge's $98.320/hr already covers its vCPU and RAM), so a provider with +// no rates here is one whose CSV price is already all-in. +// +// Hourly, not per-second: Modal publishes $0.0000131/core/s and $0.00000222/GiB/s, but +// those are rounded renderings of the rates below (0.0473/3600 = 1.314e-5), and every +// price downstream of provider.Offering.PricePerHour is hourly. +// +// The GPU price is deliberately absent: it is a modal.csv row (H100 at $3.95 per GPU +// per hour), so each number keeps a single source of truth. +const ( + ModalCPUPricePerCoreHour = 0.0473 + ModalMemoryPricePerGiBHour = 0.0080 +) + +// mibPerGiB converts a Pod's MiB request to the GiB the memory rate is quoted in. +const mibPerGiB = 1024 + +// ModalCPUCostPerHour and ModalMemoryCostPerHour are what Modal charges for a sandbox's +// CPU and memory, to be ADDED to its accelerator price. One function per published Modal +// rate, so each stays checkable against Modal's price page on its own. +// +// Each takes the unit the adapter already carries — fractional physical cores, and MiB +// (see modal.SandboxSpec) — so no conversion happens at the call site, which is where a +// factor-of-1024 slip would hide. +// +// Reservation, not usage: a sandbox bursting above its request toward CPULimit may bill +// above these. +func ModalCPUCostPerHour(cpuCores float64) float64 { + return cpuCores * ModalCPUPricePerCoreHour +} + +func ModalMemoryCostPerHour(memoryMiB int) float64 { + return float64(memoryMiB) / mibPerGiB * ModalMemoryPricePerGiBHour +} diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 914d1a7..31919ad 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -56,6 +56,7 @@ import ( nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/provider" "github.com/InftyAI/Nebula/pkg/provider/catalog" + "github.com/InftyAI/Nebula/pkg/provider/catalog/data" "github.com/InftyAI/Nebula/pkg/util" ) @@ -330,6 +331,35 @@ func (p *Provider) ExpandRegions(declared []string) []string { return []string{strings.Join(regions, regionSeparator)} } +// PricePerHour overrides catalog.Base's all-in reading of the catalog, because Modal +// meters CPU and memory SEPARATELY from the accelerator: a modal.csv row prices ONE GPU +// and nothing else, so the sandbox's real rate is that plus what its reservation costs. +// (AWS needs no override — a p5.48xlarge's price already covers its vCPU and RAM.) +// +// It also prices the CPU-ONLY sandbox that Base refuses. Base can only reject an empty +// accelerator type, having nothing but per-accelerator rows to match; Modal genuinely runs +// CPU-only work, and here the two non-accelerator rates are the whole price. +// +// ErrNoPrice for a CPU-only sandbox that reserves neither CPU nor memory: Modal then +// applies its own defaults, and we do not know them. Unpriced is the honest answer — a 0 +// would be read as free. A GPU sandbox in that state still prices, understating by those +// same defaults, which is immaterial beside the accelerator. +func (p *Provider) PricePerHour(req provider.PriceRequest) (float64, error) { + metered := data.ModalCPUCostPerHour(req.CPUCores) + data.ModalMemoryCostPerHour(req.MemoryMiB) + + if req.AcceleratorType == "" { + if metered <= 0 { + return 0, fmt.Errorf("modal: cpu-only sandbox reserves no cpu or memory: %w", provider.ErrNoPrice) + } + return metered, nil + } + gpu, err := p.Base.PricePerHour(req) + if err != nil { + return 0, err // ErrNoPrice or a malformed request, both already worded by Base + } + return gpu + metered, 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 { diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 8fb3d3b..bc1671d 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "io" + "math" "slices" "strings" "sync" @@ -1751,3 +1752,67 @@ func TestSandboxSpecStringRedactsRegistryAuth(t *testing.T) { t.Errorf("String() = %q, must not contain the password", got) } } + +// Modal's rate is the GPU row PLUS the separately metered CPU and memory, and a CPU-only +// sandbox is priced from those two alone — the case catalog.Base refuses outright. +func TestPricePerHour_AddsCPUAndMemory(t *testing.T) { + p := newTestProvider(&fakeClient{}) + od := nebulav1alpha1.CapacityOnDemand + + // 4 cores at $0.0473 = $0.1892; 8 GiB at $0.0080 = $0.0640. + const cpuAndMem = 4*0.0473 + 8*0.0080 + + cases := map[string]struct { + req provider.PriceRequest + want float64 + }{ + "gpu plus metered resources": { + provider.PriceRequest{AcceleratorType: "H100", Count: 2, CapacityType: od, CPUCores: 4, MemoryMiB: 8192}, + 2*3.95 + cpuAndMem, + }, + "gpu with no reservation is the row alone": { + provider.PriceRequest{AcceleratorType: "H100", Count: 1, CapacityType: od}, + 3.95, + }, + "cpu-only is the metered resources alone": { + provider.PriceRequest{CPUCores: 4, MemoryMiB: 8192}, + cpuAndMem, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got, err := p.PricePerHour(tc.req) + if err != nil { + t.Fatalf("PricePerHour(%+v): %v", tc.req, err) + } + if math.Abs(got-tc.want) > 1e-9 { + t.Fatalf("PricePerHour(%+v) = %v, want %v", tc.req, got, tc.want) + } + }) + } +} + +// A CPU-only sandbox reserving nothing runs on Modal's own defaults, which we do not know: +// unpriced is honest, 0 would read as free. An unknown accelerator still comes back as +// Base's ErrNoPrice rather than silently costing only its CPU and memory. +func TestPricePerHour_NoPrice(t *testing.T) { + p := newTestProvider(&fakeClient{}) + + for name, req := range map[string]provider.PriceRequest{ + "cpu-only reserving nothing": {}, + "unknown accelerator": { + AcceleratorType: "TPU-v4", Count: 1, + CapacityType: nebulav1alpha1.CapacityOnDemand, CPUCores: 4, MemoryMiB: 8192, + }, + } { + t.Run(name, func(t *testing.T) { + got, err := p.PricePerHour(req) + if !errors.Is(err, provider.ErrNoPrice) { + t.Fatalf("PricePerHour(%+v) err = %v, want ErrNoPrice", req, err) + } + if got != 0 { + t.Fatalf("PricePerHour(%+v) = %v, want 0 alongside the error", req, got) + } + }) + } +} diff --git a/pkg/provider/pricing.go b/pkg/provider/pricing.go new file mode 100644 index 0000000..2509689 --- /dev/null +++ b/pkg/provider/pricing.go @@ -0,0 +1,74 @@ +/* +Copyright 2026 The InftyAI Team. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package provider + +import ( + "errors" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" +) + +// ErrNoPrice: this Pricer has no price for this request — no matching catalog row, or one +// that carries no usable price. Kept a distinct sentinel so a caller can treat it exactly +// as it treats a provider implementing no Pricer at all (report no cost and carry on), +// while a genuinely malformed request still surfaces as a loud error. +// +// It is NOT a provision failure and must never reach ClassifyError: a candidate nobody can +// price is still perfectly launchable. +var ErrNoPrice = errors.New("provider: no price for request") + +// PriceRequest is what to price. It describes a CANDIDATE, not an instance, and carries +// no Pod: the optimizer compares candidates before anything is provisioned, and a +// Pod-shaped signature would put pricing out of reach there. +// +// No region axis, because no catalog row is region-partitioned today (every CSV leaves +// the column empty), so a price is uniform across a provider's regions. Add the axis back +// alongside the first region-varying row. +type PriceRequest struct { + // AcceleratorType is the canonical type ("H100"), empty for a CPU-only workload. + AcceleratorType string + // Count is how many accelerators. It does two jobs, and both providers need it: it + // SELECTS the row where a provider bakes the count into the offering (AWS's + // g4dn.xlarge is T4x1, g4dn.metal is T4x8), and it MULTIPLIES the rate where the + // provider prices per accelerator instead (Modal). Which one applies is read off the + // matched row's GPUCount rather than hardcoded per provider — see Offering.GPUCount. + Count int32 + // CapacityType selects between a row's Spot and OnDemand prices, which differ + // sharply (AWS p5.48xlarge: $34.412 Spot vs $98.320 OnDemand). + CapacityType nebulav1alpha1.CapacityType + // CPUCores and MemoryMiB are the workload's RESERVATION, priced only by providers + // that meter them separately from the accelerator. Ignored by a provider whose + // instance price is all-in. + CPUCores float64 + MemoryMiB int +} + +// Pricer reports the hourly USD cost of what a Provision would create: ONE rate, with every +// resource the provider bills for folded in. +// +// No per-resource breakdown, because providers disagree on what is even billed separately — +// AWS sells an instance whose price already covers its vCPU and RAM, while Modal meters GPU, +// CPU and memory apart. Splitting the all-in case means inventing an allocation no invoice +// line matches. Add a breakdown when a provider's own billing hands us one. +// +// It is OPTIONAL, resolved by type assertion like LogStreamer and Executor: a provider that +// cannot price its instances implements nothing and its cost goes unreported, which is +// better than a confidently wrong number. Adapters embedding catalog.Base get an +// implementation for free. +type Pricer interface { + PricePerHour(PriceRequest) (float64, error) +} diff --git a/pkg/util/accelerator.go b/pkg/util/accelerator.go index c56798c..f1ebea9 100644 --- a/pkg/util/accelerator.go +++ b/pkg/util/accelerator.go @@ -18,6 +18,8 @@ package util import ( "fmt" + "strconv" + "strings" corev1 "k8s.io/api/core/v1" @@ -79,6 +81,25 @@ func AcceleratorPool(accelerator string, count int32) string { return fmt.Sprintf("%s%s%d", accelerator, acceleratorSep, count) } +// SplitAcceleratorPool is the inverse of AcceleratorPool: it recovers the type and count +// from a "type:count" pool identity. For readers that hold only the joined form — +// NodeClaimSpec.Accelerator — and need the two apart, as metrics labels do so both +// "every H100 size" and "every 8-GPU request" stay aggregatable. +// +// ("", 0) for an empty pool (a CPU-only claim) and for anything not in the grammar, so a +// hand-edited claim degrades to "no accelerator" rather than minting a garbage label value. +func SplitAcceleratorPool(pool string) (accelerator string, count int32) { + typ, n, found := strings.Cut(pool, acceleratorSep) + if !found || typ == "" { + return "", 0 + } + parsed, err := strconv.ParseInt(n, 10, 32) + if err != nil || parsed <= 0 { + return "", 0 + } + return typ, int32(parsed) +} + // gpuCount returns the largest nvidia.com/gpu quantity requested across the // Pod's containers, preferring limits and falling back to requests (Kubernetes // treats an extended resource's request and limit as equal, but a Pod may set diff --git a/pkg/util/accelerator_test.go b/pkg/util/accelerator_test.go index f7096c2..574f3f9 100644 --- a/pkg/util/accelerator_test.go +++ b/pkg/util/accelerator_test.go @@ -99,3 +99,47 @@ func TestAcceleratorRequest_CountFromRequestsWhenNoLimit(t *testing.T) { t.Fatalf("AcceleratorRequest = (%q, %d), want (h100, 3)", accel, count) } } + +// SplitAcceleratorPool must round-trip AcceleratorPool for every real request, and +// degrade to "no accelerator" for anything outside the grammar rather than minting a +// garbage metrics label. +func TestSplitAcceleratorPool(t *testing.T) { + for _, tc := range []struct { + pool string + wantType string + wantCount int32 + }{ + {"H100:8", "H100", 8}, + {"A100-80GB:1", "A100-80GB", 1}, + {"", "", 0}, // CPU-only claim + {"H100", "", 0}, // no separator + {":8", "", 0}, // no type + {"H100:", "", 0}, // no count + {"H100:x", "", 0}, // unparseable count + {"H100:0", "", 0}, // a count of zero is not a request + {"H100:-2", "", 0}, // negative + } { + t.Run(tc.pool, func(t *testing.T) { + typ, count := SplitAcceleratorPool(tc.pool) + if typ != tc.wantType || count != tc.wantCount { + t.Fatalf("SplitAcceleratorPool(%q) = (%q, %d), want (%q, %d)", + tc.pool, typ, count, tc.wantType, tc.wantCount) + } + }) + } +} + +// The two are inverses: whatever AcceleratorPool joins, SplitAcceleratorPool recovers. +func TestAcceleratorPool_RoundTrip(t *testing.T) { + for _, tc := range []struct { + typ string + count int32 + }{{"H100", 1}, {"H100", 8}, {"A100-80GB", 4}, {"T4", 16}} { + pool := AcceleratorPool(tc.typ, tc.count) + gotType, gotCount := SplitAcceleratorPool(pool) + if gotType != tc.typ || gotCount != tc.count { + t.Fatalf("round trip of (%q, %d) through %q = (%q, %d)", + tc.typ, tc.count, pool, gotType, gotCount) + } + } +} diff --git a/pkg/util/resources.go b/pkg/util/resources.go new file mode 100644 index 0000000..2aba96e --- /dev/null +++ b/pkg/util/resources.go @@ -0,0 +1,61 @@ +/* +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 ( + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// mibBytes is one MiB, the unit provider.PriceRequest quotes memory in. +const mibBytes = 1024 * 1024 + +// PodReservation reads the workload's CPU and memory RESERVATION in the units +// provider.PriceRequest quotes: fractional physical cores and MiB. Requests, falling +// back to limits, and 0 for either when neither is set — which a provider reads as +// "your default", so a priced 0 is a floor, not a claim that nothing was reserved. +// +// Reservation and not the limit, because a provider metering CPU/memory apart from the +// accelerator (Modal) bills what was held for the workload; a burstable Pod's ceiling is +// not what shows up on the invoice. +// +// The FIRST container only, matching the single-workload-container shape the whole +// provisioning path assumes (see modal.sandboxSpecFromPod). Returns (0, 0) for a Pod with +// no containers. +func PodReservation(pod *corev1.Pod) (cpuCores float64, memoryMiB int) { + if pod == nil || len(pod.Spec.Containers) == 0 { + return 0, 0 + } + c := &pod.Spec.Containers[0] + cpu := reservedQty(c, corev1.ResourceCPU) + mem := reservedQty(c, corev1.ResourceMemory) + // MilliValue is cores*1000; Value is bytes. + return float64(cpu.MilliValue()) / 1000.0, int(mem.Value() / mibBytes) +} + +// reservedQty returns the container's request for name, falling back to its limit, and a +// zero quantity when it declares neither. By value, so the caller never holds a pointer +// into the Pod it was read from. +func reservedQty(c *corev1.Container, name corev1.ResourceName) resource.Quantity { + if q, ok := c.Resources.Requests[name]; ok { + return q + } + if q, ok := c.Resources.Limits[name]; ok { + return q + } + return resource.Quantity{} +} diff --git a/pkg/util/resources_test.go b/pkg/util/resources_test.go new file mode 100644 index 0000000..e7909bb --- /dev/null +++ b/pkg/util/resources_test.go @@ -0,0 +1,109 @@ +/* +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 ( + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +func podWith(reqs, limits corev1.ResourceList) *corev1.Pod { + return &corev1.Pod{Spec: corev1.PodSpec{Containers: []corev1.Container{{ + Name: "main", + Resources: corev1.ResourceRequirements{Requests: reqs, Limits: limits}, + }}}} +} + +func TestPodReservation(t *testing.T) { + cases := map[string]struct { + pod *corev1.Pod + wantCPU float64 + wantMiB int + whatFor string + }{ + "requests win over limits": { + pod: podWith( + corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("2"), corev1.ResourceMemory: resource.MustParse("4Gi")}, + corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("8"), corev1.ResourceMemory: resource.MustParse("16Gi")}, + ), + wantCPU: 2, wantMiB: 4096, + whatFor: "a burstable Pod is billed for what it reserved, not its ceiling", + }, + "falls back to limits": { + pod: podWith(nil, + corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("8"), corev1.ResourceMemory: resource.MustParse("16Gi")}, + ), + wantCPU: 8, wantMiB: 16384, + whatFor: "Kubernetes defaults the request to the limit", + }, + "fractional cores": { + pod: podWith(corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("250m"), + corev1.ResourceMemory: resource.MustParse("512Mi"), + }, nil), + wantCPU: 0.25, wantMiB: 512, + whatFor: "millicores are the common way to express a fraction", + }, + "decimal memory units convert to MiB": { + pod: podWith(corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1G")}, nil), + wantCPU: 0, wantMiB: 953, // 1e9 / 1048576, truncated + whatFor: "1G is not 1Gi, and the price is quoted per GiB", + }, + "nothing declared": { + pod: podWith(nil, nil), + wantCPU: 0, wantMiB: 0, + whatFor: "0 reaches the provider as its own default", + }, + "no containers": { + pod: &corev1.Pod{}, + wantCPU: 0, wantMiB: 0, + whatFor: "must not panic on a Pod nothing has filled in yet", + }, + "nil pod": { + pod: nil, + wantCPU: 0, wantMiB: 0, + whatFor: "callers hold a Pod that may be absent", + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cpu, mib := PodReservation(tc.pod) + if cpu != tc.wantCPU || mib != tc.wantMiB { + t.Fatalf("PodReservation = (%v cores, %v MiB), want (%v, %v): %s", + cpu, mib, tc.wantCPU, tc.wantMiB, tc.whatFor) + } + }) + } +} + +// Only the first container counts, matching the single-workload-container shape the +// provisioning path assumes; a sidecar must not inflate the priced reservation. +func TestPodReservation_FirstContainerOnly(t *testing.T) { + pod := podWith(corev1.ResourceList{corev1.ResourceCPU: resource.MustParse("1")}, nil) + pod.Spec.Containers = append(pod.Spec.Containers, corev1.Container{ + Name: "sidecar", + Resources: corev1.ResourceRequirements{Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("32"), + }}, + }) + + if cpu, _ := PodReservation(pod); cpu != 1 { + t.Fatalf("PodReservation cpu = %v, want 1 (the first container's)", cpu) + } +} From 3b0bacc4f90fe36d5f4d9192ee6a99eaedf055be Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 2 Sep 2026 15:52:58 +0100 Subject: [PATCH 02/14] reduce the interval to 1min Signed-off-by: kerthcet --- docs/metrics.md | 6 ++++-- internal/controller/cost_accrual.go | 11 ++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/metrics.md b/docs/metrics.md index 01b343a..e7aea81 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -127,7 +127,9 @@ What the fleet has actually spent. # Dollars spent yesterday, by provider. sum by (provider) (increase(nebula_cost_usd_total[1d])) -# What the last accrual window cost, fleet-wide. +# Recent spend, fleet-wide. Keep the range well above BOTH the accrual interval (1m) and the +# scrape interval: increase() needs two samples inside the range, so [1m] against a 60s scrape +# usually returns nothing at all. sum(increase(nebula_cost_usd_total[5m])) # Burn rate in USD/hour. @@ -150,7 +152,7 @@ claim's whole rate. over a hand-maintained list price — cost *incurred*, which is what "accrued" means, not cost any provider has confirmed. Nothing below has been invoiced. -**One arithmetic, two views.** A leader-elected loop closes each claim's window every 5 minutes: +**One arithmetic, two views.** A leader-elected loop closes each claim's window every minute: it charges `priceUSDPerHour × (now − status.lastAccruedAt)`, adds it to `NodeClaim.status.estimatedCostUSD` (the `EST_COST` column) and re-anchors, then books the *same* window on the counter. So the counter is the fleet's stream of charges and the field is the diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index 8c78b8f..18263dc 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -35,13 +35,18 @@ import ( // accrualInterval is how often spend is checkpointed. It is a WRITE cadence, not an accounting // resolution: every window is measured from the persisted anchor, so a longer interval trades -// freshness of the EST_COST column for fewer writes, never accuracy. Five minutes is ~1.7 -// writes/s across a 500-claim fleet. -const accrualInterval = 5 * time.Minute +// freshness of the EST_COST column for fewer writes, never accuracy. One minute is ~8.3 writes/s +// across a 500-claim fleet, about a sixth of the client's rate budget (see the QPS in cmd/main.go) +// — the point where this stops being free and starts competing with the reconcilers for it. +const accrualInterval = time.Minute // accrualTimeout bounds one whole tick, List plus every write. A tick that cannot finish loses // nothing: the anchors it did not reach are still where they were, so the next tick charges the // same windows. +// +// Now equal to accrualInterval, which is safe rather than tidy: ticks run sequentially and a +// time.Ticker drops the ticks a slow receiver missed instead of queueing them, so the worst case is +// back-to-back ticks, and re-deriving a window from its anchor cannot double-charge it. const accrualTimeout = time.Minute // CostAccrual advances each claim's durable spend ledger on a ticker. From 6cc0bbec8e4330e23b2ad87a15e933a2c3303b99 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 2 Sep 2026 16:55:02 +0100 Subject: [PATCH 03/14] update the cost Signed-off-by: kerthcet --- pkg/provider/catalog/data/modal.csv | 20 ++++++++++---------- pkg/provider/catalog/data/pricing.go | 17 ++++++++++++----- pkg/provider/modal/modal_test.go | 6 ++++-- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/pkg/provider/catalog/data/modal.csv b/pkg/provider/catalog/data/modal.csv index efd5024..d8d36d8 100644 --- a/pkg/provider/catalog/data/modal.csv +++ b/pkg/provider/catalog/data/modal.csv @@ -29,15 +29,15 @@ # region BLANK — Modal is region-simple # updated YYYY-MM-DD the row was last verified accelerator_type,accelerator_id,gpu_count,capacity_type,price_per_hour,available,region,updated -T4,,,OnDemand,0.59,true,,2026-07-25 -L4,,,OnDemand,0.80,true,,2026-07-25 -A10G,,,OnDemand,1.10,true,,2026-07-25 -L40S,,,OnDemand,1.95,true,,2026-07-25 -A100-40GB,,,OnDemand,2.10,true,,2026-07-25 -A100-80GB,,,OnDemand,2.50,true,,2026-07-25 +T4,,,OnDemand,0.59,true,,2026-09-02 +L4,,,OnDemand,0.80,true,,2026-09-02 +A10G,,,OnDemand,1.10,true,,2026-09-02 +L40S,,,OnDemand,1.95,true,,2026-09-02 +A100-40GB,,,OnDemand,2.10,true,,2026-09-02 +A100-80GB,,,OnDemand,2.50,true,,2026-09-02 # "H100!", not "H100": Modal silently upgrades a bare H100 request onto an H200, and # the "!" suffix is its documented opt-out. -H100,H100!,,OnDemand,3.95,true,,2026-07-25 -H200,,,OnDemand,4.54,true,,2026-08-31 -B200,,,OnDemand,6.25,true,,2026-08-31 -B300,,,OnDemand,7.10,true,,2026-08-31 +H100,H100!,,OnDemand,3.95,true,,2026-09-02 +H200,,,OnDemand,4.54,true,,2026-09-02 +B200,,,OnDemand,6.25,true,,2026-09-02 +B300,,,OnDemand,7.10,true,,2026-09-02 diff --git a/pkg/provider/catalog/data/pricing.go b/pkg/provider/catalog/data/pricing.go index 25c3fe7..ddf4459 100644 --- a/pkg/provider/catalog/data/pricing.go +++ b/pkg/provider/catalog/data/pricing.go @@ -24,15 +24,22 @@ package data // price (p5.48xlarge's $98.320/hr already covers its vCPU and RAM), so a provider with // no rates here is one whose CSV price is already all-in. // -// Hourly, not per-second: Modal publishes $0.0000131/core/s and $0.00000222/GiB/s, but -// those are rounded renderings of the rates below (0.0473/3600 = 1.314e-5), and every -// price downstream of provider.Offering.PricePerHour is hourly. +// Modal publishes these PER SECOND, so the literal stays exactly as printed on the price +// page and the scaling to an hour is left in the expression: the number a reviewer compares +// is the number Modal wrote, and the conversion is a compile-time constant fold. Hourly at +// all because every price downstream of provider.Offering.PricePerHour is. +// +// These are the SANDBOX/NOTEBOOK rates, ~3x Modal's standard Function rates ($0.0000131 +// and $0.00000222 per second). The tier follows what we create, not what is cheapest: a +// NodeClaim becomes one Modal Sandbox (see modal.Client.CreateSandbox). Getting it wrong is +// nearly invisible on a GPU sandbox, where the accelerator dominates, and a 3x undercount +// on a CPU-only one, where these two rates are the whole bill. // // The GPU price is deliberately absent: it is a modal.csv row (H100 at $3.95 per GPU // per hour), so each number keeps a single source of truth. const ( - ModalCPUPricePerCoreHour = 0.0473 - ModalMemoryPricePerGiBHour = 0.0080 + ModalCPUPricePerCoreHour = 0.00003942 * 60 * 60 + ModalMemoryPricePerGiBHour = 0.00000667 * 60 * 60 ) // mibPerGiB converts a Pod's MiB request to the GiB the memory rate is quoted in. diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index bc1671d..67f5c80 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -1759,8 +1759,10 @@ func TestPricePerHour_AddsCPUAndMemory(t *testing.T) { p := newTestProvider(&fakeClient{}) od := nebulav1alpha1.CapacityOnDemand - // 4 cores at $0.0473 = $0.1892; 8 GiB at $0.0080 = $0.0640. - const cpuAndMem = 4*0.0473 + 8*0.0080 + // Modal's published per-second sandbox rates: 4 cores = $0.567648/hr, 8 GiB = $0.192096/hr. + // Transcribed again here rather than imported from data, so a slip in those constants fails + // this test instead of being multiplied through it. + const cpuAndMem = 4*0.00003942*60*60 + 8*0.00000667*60*60 cases := map[string]struct { req provider.PriceRequest From 930d7b77ca4e01e7664fc751a96f780770e4c106 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 2 Sep 2026 20:18:32 +0100 Subject: [PATCH 04/14] support metrics Signed-off-by: kerthcet --- api/v1alpha1/nodeclaim_types.go | 6 +- cmd/main.go | 13 +- .../bases/nebula.inftyai.com_nodeclaims.yaml | 6 +- docs/metrics.md | 34 ++-- internal/controller/cost_accrual.go | 10 +- internal/controller/cost_accrual_test.go | 32 ++-- pkg/metrics/attribution.go | 146 ++++++++++++------ pkg/metrics/attribution_test.go | 112 ++++++++++---- pkg/metrics/cost.go | 21 ++- pkg/metrics/testing.go | 17 +- 10 files changed, 268 insertions(+), 129 deletions(-) diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index dd10389..7f49d2f 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -186,8 +186,10 @@ type NodeClaimStatus struct { LastAccruedAt *metav1.Time `json:"lastAccruedAt,omitempty"` // CostLabels attributes this claim's spend to whoever asked for it: the values the served - // Pod carried for the label names the operator configured (--cost-labels). Keyed by the - // label name, which is the same token on the Pod and on the metric. + // Pod carried for the label keys the operator configured (--cost-labels). Keyed by the POD + // label key, verbatim — so this map reads like the Pod it came from, including any domain + // prefix. The metric emits under a derived name Prometheus accepts + // ("example.com/org-id" here is org_id there), so do not expect the two to match by string. // // Status rather than spec because it is observed from the Pod, and it sits with the rest of // the billing record for a reason: it is stamped in the SAME patch that opens the accrual diff --git a/cmd/main.go b/cmd/main.go index 368f533..8e15741 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -122,12 +122,13 @@ func main() { "client certificate is not portable across distributions; restrict the port with a "+ "NetworkPolicy, or set this to your API server's kubelet client CA.") flag.StringVar(&costLabels, "cost-labels", "", - "Comma-separated Pod label names whose values attribute cost, e.g. \"org_id,team_id\". Each name "+ - "is used verbatim as both the Pod label key and the metric label, so it must be legal as "+ - "both: letters, digits and underscores, starting with a letter. Empty (the default) reports "+ - "cost by candidate shape only. Changing this changes the identity of every cost series. "+ - "Values come from Pod labels and are NOT capped: the manager warns once if they push the "+ - "cost metric past 5000 series, but pick names an admission policy constrains.") + "Comma-separated Pod label keys whose values attribute cost, e.g. "+ + "\"example.com/org-id,team_id\". Qualified keys are fine: the metric label is derived from the "+ + "key's name part with '-' and '.' folded to '_' (example.com/org-id is queried as org_id), so "+ + "two keys that would collide there are rejected at startup. Empty (the default) reports cost "+ + "by candidate shape only. Changing this changes the identity of every cost series. Values "+ + "come from Pod labels and are NOT capped: the manager warns once if they push the cost "+ + "metric past 5000 series, but pick keys an admission policy constrains.") opts := zap.Options{ Development: true, } diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index 26b4262..71d45ff 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -152,8 +152,10 @@ spec: type: string description: |- CostLabels attributes this claim's spend to whoever asked for it: the values the served - Pod carried for the label names the operator configured (--cost-labels). Keyed by the - label name, which is the same token on the Pod and on the metric. + Pod carried for the label keys the operator configured (--cost-labels). Keyed by the POD + label key, verbatim — so this map reads like the Pod it came from, including any domain + prefix. The metric emits under a derived name Prometheus accepts + ("example.com/org-id" here is org_id there), so do not expect the two to match by string. Status rather than spec because it is observed from the Pod, and it sits with the rest of the billing record for a reason: it is stamped in the SAME patch that opens the accrual diff --git a/docs/metrics.md b/docs/metrics.md index e7aea81..1767a69 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -213,15 +213,27 @@ claims. Who to charge. Off by default; `--cost-labels` turns it on: ``` ---cost-labels=org_id,team_id +--cost-labels=example.com/org-id,example.com/team-id ``` -Each name is used **verbatim** as both the Pod label key and the metric label, so there is no -mapping to keep in sync — and no way for the two to disagree. That costs you qualified keys: -`nebula.inftyai.com/org-id` is a legal Pod label and an illegal Prometheus label, so the name has -to satisfy both grammars (letters, digits, underscores; starting with a letter). A name that does -not, or that shadows a label the metric already has, is rejected at startup rather than silently -reporting every tenant as `none`. +Each entry is a **Pod label key**, written exactly as the Pod carries it. The metric label is +*derived* from it: the key's name part, with `-` and `.` folded to `_`, and the domain prefix +dropped. + +| `--cost-labels` entry | Pod label read | PromQL label | +| --- | --- | --- | +| `example.com/org-id` | `example.com/org-id` | `org_id` | +| `team.id` | `team.id` | `team_id` | +| `org_id` | `org_id` | `org_id` | + +So a key is configured the way Kubernetes spells it and queried the way PromQL can express it — +nobody has to write `sum by (example_com_org_id)`. What dropping the prefix costs is that two keys +can want the same name; `a.com/org-id,b.com/org-id` is **rejected at startup** rather than merging +two tenants' spend into one series. Also rejected there: a key Kubernetes would not accept, a name +part no folding can rescue (`2team` — Prometheus label names cannot start with a digit), the same +key twice, and one whose derived name shadows a label the metric already carries (`provider`, +`region`, `phase`, …). Every one of those fails the process at boot instead of silently reporting +every tenant as `none`. Two names nothing rejects but you should still avoid: **`job` and `instance`**. Prometheus attaches its own at scrape time, and with the default `honor_labels: false` it renames yours to @@ -229,14 +241,14 @@ its own at scrape time, and with the default `honor_labels: false` it renames yo of the tenant, with no error anywhere. `job_id` or `workload` if that is the breakdown you want. The values are read off the served Pod once — when the claim first becomes chargeable, in the same -status patch that opens its billing window — and pinned on -`NodeClaim.status.costLabels`: +status patch that opens its billing window — and pinned on `NodeClaim.status.costLabels`, keyed by +the **Pod** key rather than the derived one, so the record reads like the Pod it came from: ```yaml status: costLabels: - org_id: acme - team_id: ml + example.com/org-id: acme + example.com/team-id: ml ``` Three things follow from pinning them there rather than reading the Pod at accrual time: diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index 18263dc..8cd7771 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -285,14 +285,14 @@ func claimLabels(nc *nebulav1alpha1.NodeClaim) metrics.Labels { // alongside stampAccrualStart in one patch, so attribution is durable before the first window it // has to explain. func stampCostLabels(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) bool { - names := metrics.CostLabelNames() - if nc.Status.CostLabels != nil || pod == nil || len(names) == 0 { + keys := metrics.CostLabelKeys() + if nc.Status.CostLabels != nil || pod == nil || len(keys) == 0 { return false } stamped := map[string]string{} - for _, name := range names { - if v := pod.Labels[name]; v != "" { - stamped[name] = v + for _, key := range keys { + if v := pod.Labels[key]; v != "" { + stamped[key] = v } } // An empty map, not nil: a Pod carrying none of the configured labels is a settled fact, and diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index 3830288..15b95fd 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -355,30 +355,37 @@ nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="Sp // Attribution is copied off the Pod once and then frozen: spend already reported under one tenant // must not move to another because somebody relabelled a Pod. +// +// Configured with QUALIFIED keys, which is what a real deployment uses: the stamp must be keyed by +// the key the Pod carries, prefix and all, not by the name the metric emits under. func TestStampCostLabels(t *testing.T) { t.Cleanup(func() { metrics.ConfigureCostForTest(nil) }) - metrics.ConfigureCostForTest([]string{"org_id", "team_id"}) + metrics.ConfigureCostForTest([]string{"example.com/org-id", "example.com/team-id"}) - pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{"org_id": "acme", "team_id": "ml", "unconfigured": "ignored"}, - }} + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "example.com/org-id": "acme", + "example.com/team-id": "ml", + // The derived metric name is not a Pod key, so it must not be read as one. + "org_id": "ignored", + "unconfigured": "ignored", + }}} nc := billingClaim("bound", "10.0000", nil) if !stampCostLabels(nc, pod) { t.Fatal("stampCostLabels reported no change on an unstamped claim") } - want := map[string]string{"org_id": "acme", "team_id": "ml"} + want := map[string]string{"example.com/org-id": "acme", "example.com/team-id": "ml"} if !reflect.DeepEqual(nc.Status.CostLabels, want) { - t.Fatalf("stamped %v, want %v — only configured names are copied", nc.Status.CostLabels, want) + t.Fatalf("stamped %v, want %v — only configured keys are copied", nc.Status.CostLabels, want) } // A relabelled Pod must not re-attribute the claim. - pod.Labels["org_id"] = "someone-else" + pod.Labels["example.com/org-id"] = "someone-else" if stampCostLabels(nc, pod) { t.Fatal("stampCostLabels rewrote attribution that was already settled") } - if nc.Status.CostLabels["org_id"] != "acme" { - t.Fatalf("org_id moved to %q", nc.Status.CostLabels["org_id"]) + if nc.Status.CostLabels["example.com/org-id"] != "acme" { + t.Fatalf("org-id moved to %q", nc.Status.CostLabels["example.com/org-id"]) } // A Pod carrying none of them is a settled fact too, or every later reconcile re-checks it. @@ -401,13 +408,16 @@ func TestStampCostLabels_NotConfigured(t *testing.T) { // The window booked carries the claim's stamped attribution, which is the whole point of pinning // it in status: the Pod is long gone by the time a Terminating window is charged. +// +// End to end on the key/name split: the claim is keyed "example.com/org-id" and the series comes out +// as org_id. func TestCostAccrual_BooksAttribution(t *testing.T) { t.Cleanup(func() { metrics.ConfigureCostForTest(nil) }) - metrics.ConfigureCostForTest([]string{"org_id"}) + metrics.ConfigureCostForTest([]string{"example.com/org-id"}) hour := time.Hour nc := billingClaim("bound", "10.0000", &hour) - nc.Status.CostLabels = map[string]string{"org_id": "acme"} + nc.Status.CostLabels = map[string]string{"example.com/org-id": "acme"} a, _ := newAccrual(t, nc) a.accrueAll(context.Background()) diff --git a/pkg/metrics/attribution.go b/pkg/metrics/attribution.go index 3a92a67..b7e92f2 100644 --- a/pkg/metrics/attribution.go +++ b/pkg/metrics/attribution.go @@ -22,6 +22,7 @@ import ( "strings" "sync" + "k8s.io/apimachinery/pkg/util/validation" logf "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -34,82 +35,131 @@ import ( // roughly a megabyte of exposition text — far more than any fleet reaches by candidate shape alone. const costSeriesWarnThreshold = 5000 -// costLabelName is the intersection of two grammars, because one token is used as both a -// Kubernetes label key and a Prometheus label name: Prometheus forbids '-' and '.' and a -// leading digit, Kubernetes forbids a leading or trailing '_'. Anything outside the overlap is -// rejected at startup rather than mangled, since a silently normalised name that collides with -// another is a mis-attribution nobody would notice. -var costLabelName = regexp.MustCompile(`^[a-zA-Z]([a-zA-Z0-9_]*[a-zA-Z0-9])?$`) +// promLabelName is Prometheus's own label-name grammar, applied to the DERIVED name rather than to +// the Pod key. The one legal Kubernetes key it still rejects is a name part starting with a digit +// ("2team"), which nothing here can repair without inventing a prefix. +var promLabelName = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`) -// ParseCostLabels turns the --cost-labels value ("org_id,team_id") into the attribution +// metricNameFor derives the Prometheus label name a Pod label key is emitted under: the key's name +// part, with '-' and '.' folded to '_'. +// +// The domain prefix is dropped because it answers no question a dashboard asks — "example.com/org-id" +// and "org-id" mean the same thing, and `sum by (example_com_org_id)` is nobody's idea of a query. +// What that costs is two prefixes sharing a name part, which ParseCostLabels rejects outright rather +// than letting one silently absorb the other's spend. +func metricNameFor(podKey string) (string, error) { + name := podKey + if slash := strings.IndexByte(name, '/'); slash >= 0 { + name = name[slash+1:] + } + name = strings.NewReplacer("-", "_", ".", "_").Replace(name) + if !promLabelName.MatchString(name) { + return "", fmt.Errorf("cost label %q emits as %q, which Prometheus will not accept as a label "+ + "name (it must start with a letter or underscore)", podKey, name) + } + return name, nil +} + +// ParseCostLabels turns the --cost-labels value ("example.com/org-id,team_id") into the attribution // dimension, in the order given. Empty input means no attribution, which is the default. // -// Each entry names BOTH the Pod label to read and the metric label to emit — one token, so what -// the manifest configures is exactly what appears in PromQL. It fails rather than skipping a bad -// entry: silently dropping one would be discovered at invoicing time, when every series has -// already been recorded as "none". +// Each entry is a POD LABEL KEY, qualified or not; the metric label it emits under is derived from +// it, so a key is configured as the Pod carries it and queried as PromQL can express it. It fails +// rather than skipping a bad entry: silently dropping one would be discovered at invoicing time, +// when every series has already been recorded as "none". // -// Only the NAME is checked here. An entry that shadows a dimension the counter already carries -// ("provider", "phase") parses, then fails at InitCost, where the registry rejects a duplicate -// label name — still at startup, just with a less pointed message. +// Four rejections, all at startup: a key Kubernetes would not accept, one whose derived name +// Prometheus would not, the same key twice, and — the price of dropping the domain prefix — two +// keys deriving the SAME name. A derived name that shadows a dimension the counter already carries +// ("provider", "phase") gets through here and fails at InitCost, where the registry rejects a +// duplicate label name; still at startup, just with a less pointed message. // // Setting this at all is what makes cost the only metric here whose CARDINALITY is not bounded by // configuration: the values come from Pod labels. Nothing caps them — noteSeries only warns — so -// pick names whose value set the cluster's admission policy actually constrains. +// pick keys whose value set the cluster's admission policy actually constrains. func ParseCostLabels(spec string) ([]string, error) { fields := strings.Split(spec, ",") - names := make([]string, 0, len(fields)) - seen := map[string]struct{}{} + keys := make([]string, 0, len(fields)) + seenKey := map[string]struct{}{} + claimedBy := map[string]string{} // derived name -> the key that got there first for _, raw := range fields { - name := strings.TrimSpace(raw) - if name == "" { + key := strings.TrimSpace(raw) + if key == "" { continue } - if len(name) > 63 { - return nil, fmt.Errorf("cost label %q is longer than the 63-character Kubernetes label limit", name) + // IsQualifiedName carries the whole Kubernetes key grammar — the 253-character prefix, the + // 63-character name, and the alphanumeric-at-both-ends rule — so none of it is restated here. + if errs := validation.IsQualifiedName(key); len(errs) > 0 { + return nil, fmt.Errorf("cost label %q is not a valid Pod label key: %s", key, strings.Join(errs, "; ")) } - if !costLabelName.MatchString(name) { - return nil, fmt.Errorf("cost label %q must match %s — it is used as both a Pod label key "+ - "and a Prometheus label name, so '-', '.' and a leading digit are all out", name, costLabelName) + name, err := metricNameFor(key) + if err != nil { + return nil, err } - if _, dup := seen[name]; dup { - return nil, fmt.Errorf("cost label %q is listed twice", name) + if _, dup := seenKey[key]; dup { + return nil, fmt.Errorf("cost label %q is listed twice", key) } - seen[name] = struct{}{} - names = append(names, name) + if first, clash := claimedBy[name]; clash { + return nil, fmt.Errorf("cost labels %q and %q both emit as %q, which would merge two "+ + "tenants' spend into one series; drop one or rename its name part", first, key, name) + } + seenKey[key] = struct{}{} + claimedBy[name] = key + keys = append(keys, key) } - if len(names) == 0 { + if len(keys) == 0 { // nil rather than an empty slice: "unconfigured" is a state callers compare against. return nil, nil } - return names, nil + return keys, nil } -// costLabelNames is the configured attribution dimension, in emit order. Read on every -// recording and written only by InitCost, before the manager starts. +// costLabelKeys is the configured attribution dimension, as POD LABEL KEYS in emit order. Read on +// every recording and written only by configureCost, before the manager starts. // -// The ORDER lives here rather than in the stamped NodeClaim, which is what makes attribution -// safe against a flag change: values are looked up BY NAME at emit time, so adding, removing or +// Only the keys are kept. The Prometheus names are a pure function of them (metricNameFor) and are +// needed in exactly one place, when the counter is constructed, so storing them here would be a +// second copy of a derivable fact — one that could go stale against this one. +// +// The ORDER lives here rather than in the stamped NodeClaim, which is what makes attribution safe +// against a flag change: values are looked up BY KEY at emit time, so adding, removing or // reordering --cost-labels can never slide a team_id into the org_id column. -var costLabelNames []string +var costLabelKeys []string + +// CostLabelKeys is the Pod label keys attribution reads, in emit order. Exported so the controller +// that stamps the values onto a claim reads the SAME list the metric emits, rather than keeping a +// second copy that could disagree with it. +// +// POD KEYS, not the metric's label names — the two differ whenever a key is qualified. Stamping a +// claim needs the former; building the counter needs metricNames. +func CostLabelKeys() []string { + return costLabelKeys +} -// CostLabelNames is the configured attribution dimension. Exported so the controller that stamps -// the values onto a claim reads the SAME list the metric emits, rather than keeping a second copy -// that could disagree with it. -func CostLabelNames() []string { - return costLabelNames +// metricNames is the Prometheus label names a set of Pod keys is emitted under, in the same order. +// +// The error metricNameFor can return is dropped: ParseCostLabels has already rejected any key that +// fails, and if a caller ever bypassed it, prometheus.Register refuses an illegal label name — so +// the invariant is enforced twice over without an unreachable error path here. +func metricNames(podKeys []string) []string { + names := make([]string, 0, len(podKeys)) + for _, key := range podKeys { + name, _ := metricNameFor(key) + names = append(names, name) + } + return names } // attributionValues renders the configured dimension from a claim's stamped labels, in -// costLabelNames order. A name the claim never carried reports "none" rather than being -// omitted, which would move the sample to a different series. +// costLabelKeys order. A key the claim never carried reports "none" rather than being omitted, +// which would move the sample to a different series. func attributionValues(stamped map[string]string) []string { - if len(costLabelNames) == 0 { + if len(costLabelKeys) == 0 { return nil } - values := make([]string, 0, len(costLabelNames)) - for _, name := range costLabelNames { - values = append(values, orNone(stamped[name])) + values := make([]string, 0, len(costLabelKeys)) + for _, key := range costLabelKeys { + values = append(values, orNone(stamped[key])) } return values } @@ -132,7 +182,7 @@ var ( // nobody's problem until invoicing. Skipped entirely without attribution, where every dimension is // bounded by NodePools and provider catalogs. func noteSeries(values []string) { - if len(costLabelNames) == 0 { + if len(costLabelKeys) == 0 { return } @@ -156,6 +206,6 @@ func noteSeries(values []string) { "budget, which usually means an attribution label is carrying a per-Pod value. Nothing is "+ "dropped or merged, so the dollars stay correct, but memory and every scrape grow until this "+ "process restarts. Constrain these label values at admission.", - "series", len(costSeries), "threshold", costSeriesWarnThreshold, "costLabels", costLabelNames) + "series", len(costSeries), "threshold", costSeriesWarnThreshold, "costLabels", costLabelKeys) costSeries = nil } diff --git a/pkg/metrics/attribution_test.go b/pkg/metrics/attribution_test.go index b5bcf9c..4199a42 100644 --- a/pkg/metrics/attribution_test.go +++ b/pkg/metrics/attribution_test.go @@ -25,13 +25,18 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil" ) -// withAttribution swaps in a label set for one test and restores the default afterwards. It goes -// through configureCost rather than InitCost on purpose: a registry refuses the second set of label -// dimensions it ever sees for a name, and testutil collects straight from the collector anyway. -func withAttribution(t *testing.T, names ...string) { +// withAttribution swaps in a label set for one test and restores the default afterwards. Keys go +// through the real ParseCostLabels so a test cannot configure a shape the flag could not, then +// through configureCost rather than InitCost: a registry refuses the second set of label dimensions +// it ever sees for a name, and testutil collects straight from the collector anyway. +func withAttribution(t *testing.T, podKeys ...string) { t.Helper() - configureCost(names) + labels, err := ParseCostLabels(strings.Join(podKeys, ",")) + if err != nil { + t.Fatalf("ParseCostLabels(%v): %v", podKeys, err) + } + configureCost(labels) t.Cleanup(func() { configureCost(nil) }) } @@ -39,18 +44,48 @@ func TestParseCostLabels(t *testing.T) { cases := map[string]struct { spec string want []string - err bool + // names is the Prometheus dimension those keys emit under, checked wherever it differs from + // the keys themselves — the parse and the derivation are one contract for a caller. + names []string + err bool }{ - "empty is no attribution": {spec: "", want: nil}, - "names in order": {spec: "org_id,team_id", want: []string{"org_id", "team_id"}}, - "whitespace and blanks": {spec: " org_id , , team_id ", want: []string{"org_id", "team_id"}}, - // One token has to be legal as a Pod label key AND a Prometheus label name. - "hyphen is not a prometheus name": {spec: "org-id", err: true}, - "dot is not a prometheus name": {spec: "org.id", err: true}, - "qualified key": {spec: "nebula.inftyai.com/org-id", err: true}, - "leading digit": {spec: "2team", err: true}, - "trailing underscore": {spec: "org_", err: true}, - "duplicate": {spec: "org_id,org_id", err: true}, + "empty is no attribution": {spec: ""}, + "bare keys pass through": { + spec: "org_id,team_id", + want: []string{"org_id", "team_id"}, + }, + "whitespace and blanks": { + spec: " org_id , , team_id ", + want: []string{"org_id", "team_id"}, + }, + // The point of the whole exercise: a qualified key is configured as the Pod carries it and + // emitted as PromQL can express it. + "qualified key drops its prefix": { + spec: "example.com/org-id", + want: []string{"example.com/org-id"}, + names: []string{"org_id"}, + }, + "a set of qualified keys sharing one prefix": { + spec: "example.com/experiment-id,example.com/org-id,example.com/team-id,example.com/user-id", + want: []string{ + "example.com/experiment-id", "example.com/org-id", "example.com/team-id", "example.com/user-id", + }, + names: []string{"experiment_id", "org_id", "team_id", "user_id"}, + }, + "hyphen and dot are folded": { + spec: "org-id,team.id", + want: []string{"org-id", "team.id"}, + names: []string{"org_id", "team_id"}, + }, + // A legal Pod key whose name part no folding can rescue. + "leading digit": {spec: "2team", err: true}, + "trailing underscore": {spec: "org_", err: true}, + "spaces inside a key": {spec: "org id", err: true}, + "empty name part": {spec: "example.com/", err: true}, + "duplicate key": {spec: "org_id,org_id", err: true}, + // The cost of dropping the prefix, and the reason it is safe to drop. + "distinct keys colliding on the derived name": {spec: "a.com/org-id,b.com/org-id", err: true}, + "bare key colliding with a qualified one": {spec: "org_id,example.com/org-id", err: true}, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -67,30 +102,43 @@ func TestParseCostLabels(t *testing.T) { if !reflect.DeepEqual(got, tc.want) { t.Fatalf("ParseCostLabels(%q) = %v, want %v", tc.spec, got, tc.want) } + // Unset means the keys are already legal names and emit unchanged. + wantNames := tc.names + if wantNames == nil { + wantNames = tc.want + } + if names := metricNames(got); len(wantNames) > 0 && !reflect.DeepEqual(names, wantNames) { + t.Fatalf("metricNames(%q) = %v, want %v", tc.spec, names, wantNames) + } }) } } -// A name that shadows one of the counter's own dimensions is not rejected by the parser — it is -// caught one step later, when InitCost registers a descriptor with a duplicate label name. This -// pins that remaining net, because it is the only thing standing between --cost-labels=provider and -// a manager that starts with a broken cost metric. +// A DERIVED name that shadows one of the counter's own dimensions is not rejected by the parser — +// it is caught one step later, when InitCost registers a descriptor with a duplicate label name. +// This pins that remaining net, and does it through a qualified key because the prefix is exactly +// what hides the collision from a reader of the flag. func TestInitCost_RejectsAShadowingName(t *testing.T) { t.Cleanup(func() { configureCost(nil) }) - if err := InitCost([]string{"provider"}); err == nil { - t.Fatal("InitCost accepted a label name the counter already carries") + labels, err := ParseCostLabels("example.com/provider") + if err != nil { + t.Fatalf("ParseCostLabels: %v", err) + } + if err := InitCost(labels); err == nil { + t.Fatal("InitCost accepted a key emitting a label name the counter already carries") } } -// The configured names become metric labels, and a Pod that carried none of them is reported as -// "none" rather than dropping the dimension — which would silently split the series. +// Values are read by POD KEY and emitted under the DERIVED name — the whole point of splitting the +// two. A Pod that carried none of the keys is reported as "none" rather than dropping the +// dimension, which would silently split the series. func TestRecordWindow_Attribution(t *testing.T) { - withAttribution(t, "org_id", "team_id") + withAttribution(t, "example.com/org-id", "example.com/team-id") l := Labels{Provider: "modal", Accelerator: "H100", AcceleratorCount: 1} - RecordWindow(l, "Bound", map[string]string{"org_id": "acme", "team_id": "ml"}, 3.95) - RecordWindow(l, "Bound", map[string]string{"org_id": "acme"}, 1.05) + RecordWindow(l, "Bound", map[string]string{"example.com/org-id": "acme", "example.com/team-id": "ml"}, 3.95) + RecordWindow(l, "Bound", map[string]string{"example.com/org-id": "acme"}, 1.05) RecordWindow(l, "Bound", nil, 0.5) // The exposition format is line-oriented, so these cannot be wrapped. @@ -107,14 +155,14 @@ nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="no } } -// Values are looked up BY NAME, so the order they were stamped in cannot slide a team_id into the -// org_id column — the property that makes a --cost-labels change safe. +// Values are looked up BY POD KEY, so the order they were stamped in cannot slide a team_id into +// the org_id column — the property that makes a --cost-labels change safe. func TestRecordWindow_AttributionIsKeyedNotPositional(t *testing.T) { - withAttribution(t, "org_id", "team_id") + withAttribution(t, "example.com/org-id", "example.com/team-id") l := Labels{Provider: "modal"} - RecordWindow(l, "Bound", map[string]string{"team_id": "ml", "org_id": "acme"}, 1) - RecordWindow(l, "Bound", map[string]string{"org_id": "acme", "team_id": "ml"}, 1) + RecordWindow(l, "Bound", map[string]string{"example.com/team-id": "ml", "example.com/org-id": "acme"}, 1) + RecordWindow(l, "Bound", map[string]string{"example.com/org-id": "acme", "example.com/team-id": "ml"}, 1) if n := testutil.CollectAndCount(CostTotal); n != 1 { t.Fatalf("collected %d series, want 1 — the same pair rendered two ways", n) diff --git a/pkg/metrics/cost.go b/pkg/metrics/cost.go index 4550f4e..85edd3b 100644 --- a/pkg/metrics/cost.go +++ b/pkg/metrics/cost.go @@ -38,13 +38,16 @@ var CostTotal = newCostTotal(nil) // Deliberately no init here: CostTotal cannot self-register the way every other metric in this // package does, because its label names are not known until --cost-labels is parsed. See InitCost. -func newCostTotal(attribution []string) *prometheus.CounterVec { +func newCostTotal(podKeys []string) *prometheus.CounterVec { + // The DERIVED names, not the Pod keys the operator configured: a qualified key is not a legal + // Prometheus label name. Derived here rather than stored, so the counter's shape and what + // CostLabelKeys hands the stamper cannot disagree. return prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "nebula_cost_usd_total", Help: "Cumulative USD billed by external instances, added window by window as it accrues.", }, - withExtra(append([]string{"phase"}, attribution...)...), + withExtra(append([]string{"phase"}, metricNames(podKeys)...)...), ) } @@ -59,19 +62,21 @@ func newCostTotal(attribution []string) *prometheus.CounterVec { // before the manager starts. // // Not safe against concurrent recording. -func InitCost(attribution []string) error { - configureCost(attribution) +func InitCost(podKeys []string) error { + configureCost(podKeys) if err := ctrlmetrics.Registry.Register(CostTotal); err != nil { - return fmt.Errorf("registering cost counter with labels %v: %w", attribution, err) + // The derived names, since those are what the registry objected to — a shadowing key looks + // innocent until you see what it emits as. + return fmt.Errorf("registering cost counter with labels %v: %w", metricNames(podKeys), err) } return nil } // configureCost rebuilds the counter for a label set. Split out so tests can swap the dimension // without touching the process-wide registry, which would refuse the second shape it ever saw. -func configureCost(attribution []string) { - CostTotal = newCostTotal(attribution) - costLabelNames = attribution +func configureCost(podKeys []string) { + CostTotal = newCostTotal(podKeys) + costLabelKeys = podKeys costSeriesMu.Lock() defer costSeriesMu.Unlock() diff --git a/pkg/metrics/testing.go b/pkg/metrics/testing.go index ca9573f..fb11c06 100644 --- a/pkg/metrics/testing.go +++ b/pkg/metrics/testing.go @@ -16,15 +16,24 @@ limitations under the License. package metrics -// ConfigureCostForTest points the cost counter at a label set, for tests in other packages that -// exercise attribution. Not for production use: InitCost is the real entry point. +import "strings" + +// ConfigureCostForTest points the cost counter at a set of Pod label keys, for tests in other +// packages that exercise attribution. Not for production use: InitCost is the real entry point. // // It exists because InitCost can only ever succeed ONCE per process — a registry remembers a // metric name's label dimensions for the life of the process — so a test binary cannot try two // label sets through it. This skips registration; testutil collects from the collector directly. // +// Keys go through the real ParseCostLabels, so a test cannot configure a shape the flag could not, +// and an unparseable key panics rather than silently configuring nothing. +// // Pass nil to restore the unconfigured default, and do so in a Cleanup: the counter is a package // variable, so a test that leaves it swapped changes what every later test measures. -func ConfigureCostForTest(attribution []string) { - configureCost(attribution) +func ConfigureCostForTest(podKeys []string) { + labels, err := ParseCostLabels(strings.Join(podKeys, ",")) + if err != nil { + panic("metrics: ConfigureCostForTest: " + err.Error()) + } + configureCost(labels) } From 05c8a8cf9e9c06e61b73d6e9f5c62e0c8fa060aa Mon Sep 17 00:00:00 2001 From: kerthcet Date: Wed, 2 Sep 2026 23:57:06 +0100 Subject: [PATCH 05/14] add debug info Signed-off-by: kerthcet --- internal/controller/cost_accrual.go | 2 +- pkg/provider/modal/client.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index 8cd7771..4cfa7f5 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -240,7 +240,7 @@ func settleFinalCost(ctx context.Context, nc *nebulav1alpha1.NodeClaim) { // Zero once the instance is already gone (Terminated freezes the total), which RecordWindow // drops rather than minting a series for. metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, total-costSoFar(nc)) - logf.FromContext(ctx).Info("instance reclaimed", "lifetimeCostUSD", formatCost(total)) + logf.FromContext(ctx).Info("instance reclaimed", "TotalTimeCostUSD", formatCost(total)) } // costNow is what a claim has cost as of now: the persisted ledger plus the window still open diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index b6d4557..c40f820 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -31,6 +31,7 @@ import ( "google.golang.org/grpc/status" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/intstr" + logf "sigs.k8s.io/controller-runtime/pkg/log" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/provider" @@ -139,6 +140,9 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if spec.Image == "" { return "", Credential{}, fmt.Errorf("modal: empty image in sandbox spec") } + log := logf.FromContext(ctx) + + buildStart := time.Now() image, err := c.imageFor(ctx, spec) if err != nil { return "", Credential{}, err @@ -149,6 +153,7 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if err != nil { return "", Credential{}, err } + imageBuild := time.Since(buildStart) probe, err := modalProbe(spec.ReadinessProbe) if err != nil { @@ -160,6 +165,7 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string return "", Credential{}, err } + createStart := time.Now() sb, err := c.mc.Sandboxes.Create(ctx, app, image, &modal.SandboxCreateParams{ Command: spec.Command, // Env is the whole environment, including values resolved from this cluster's @@ -190,10 +196,18 @@ func (c *sdkClient) CreateSandbox(ctx context.Context, spec SandboxSpec) (string if err != nil { return "", Credential{}, err } + // Emitted here rather than folded into one line at the end, so a call killed by + // ProvisionTimeout still records how far it got and how long the earlier legs took. + log.V(1).Info("modal sandbox created", "sandboxID", sb.SandboxID, + "imageBuild", imageBuild.String(), "create", time.Since(createStart).String()) + + mintStart := time.Now() cred, err := c.mintCredential(ctx, sb, firstPort(spec.Ports)) if err != nil { return "", Credential{}, err } + log.V(1).Info("modal connect credential minted", "sandboxID", sb.SandboxID, + "mint", time.Since(mintStart).String()) return sb.SandboxID, cred, nil } From b56099c7bbb6f63011bfb48f210001e2d1dac805 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 00:42:23 +0100 Subject: [PATCH 06/14] update comments Signed-off-by: kerthcet --- pkg/metrics/provision.go | 7 +++--- pkg/provider/modal/client.go | 5 ++++ pkg/provider/modal/modal.go | 40 +++++++++++++++++++------------- pkg/provider/modal/modal_test.go | 9 ++++--- pkg/provider/provider.go | 8 +++---- pkg/vnode/handler.go | 6 ++--- 6 files changed, 44 insertions(+), 31 deletions(-) diff --git a/pkg/metrics/provision.go b/pkg/metrics/provision.go index b2c6f32..72fa5c0 100644 --- a/pkg/metrics/provision.go +++ b/pkg/metrics/provision.go @@ -80,9 +80,10 @@ var ( // inside the call (so a capacity shortage shows up as latency HERE), Modal // builds the image inside it (so a cache miss does). // - // The top bucket tracks the largest Capabilities.ProvisionTimeout — Modal's 10 - // minutes. A lower ceiling would bury every slow build in +Inf; at 600s, +Inf - // means one thing only: a call outran its own deadline. + // The buckets run past the largest Capabilities.ProvisionTimeout (Modal's 5 + // minutes), rather than stopping at it: a ceiling AT the deadline would bury every + // slow build in +Inf, while this way the 300s bucket is where a call killed by its + // own deadline lands and anything above it is overshoot. ProvisionDuration = prometheus.NewHistogramVec(prometheus.HistogramOpts{ Name: "nebula_provision_duration_seconds", Help: "Latency of the provider's Provision call, by provider, region, capacity type, " + diff --git a/pkg/provider/modal/client.go b/pkg/provider/modal/client.go index c40f820..fd20836 100644 --- a/pkg/provider/modal/client.go +++ b/pkg/provider/modal/client.go @@ -277,6 +277,11 @@ func (c *sdkClient) registrySecret(ctx context.Context, kv map[string]string) (* // credential belongs in an access-controlled Secret, and this layer has no cluster // access, so it hands the pair up to the virtual kubelet, which writes it. // +// CreateConnectToken BLOCKS until Modal has assigned the sandbox a worker, and that wait is +// load-bearing: it is what makes a returning CreateSandbox proof of a placed GPU rather than +// of an accepted request (see Provision). Modal caps it itself, at ~3m40s, so a queue longer +// than the cap fails a Pod whose sandbox would have been placed. +// // A failure is REPORTED, never swallowed into a zero credential. Minting is one-shot with // no read-back, so a dropped error loses the credential of a sandbox that exists and is // billing — silently, since a caller handed an empty pair has nothing to log. The Pod fails diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 31919ad..e054169 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -67,11 +67,16 @@ import ( // different ceiling sets spec.activeDeadlineSeconds, which maps straight through. const defaultSandboxTimeout = 24 * time.Hour -// provisionTimeout raises the vnode handler's generic Provision deadline, because Provision -// here BLOCKS on the image build (see sdkClient.buildImage): a cold image is pulled into -// Modal's cache on this call, and the create and credential legs only get what the build -// leaves them. -const provisionTimeout = 10 * time.Minute +// provisionTimeout raises the vnode handler's generic Provision deadline, because Provision here +// BLOCKS twice: a cold image is pulled into Modal's cache on this call (see sdkClient.buildImage), +// and the credential mint then waits for a worker (see sdkClient.mintCredential). +// +// The two legs SHARE this budget, and the mint is the greedy one: Modal caps it at ~3m43s of its +// own accord, so it always fails before this deadline does and the build is left roughly the +// remaining minute. That is the cost of a tight ceiling here — a cold pull of a large image can +// outrun it — and it is deliberate: a call that will die at 3m43s anyway should not hold a +// pod-controller worker for ten minutes first. +const provisionTimeout = 5 * time.Minute // compile-time assertions that Provider satisfies the interfaces. LogStreamer and // Executor are the optional halves: they are what make `kubectl logs` and `kubectl exec` @@ -88,7 +93,11 @@ var ( // real implementation (Modal SDK/HTTP) and a fake (tests) are interchangeable. type Client interface { // CreateSandbox launches one sandbox from spec and returns its Modal id plus the - // connect credential minted for it. Minting is one-shot and there is no read-back, so + // connect credential minted for it. It returns only once Modal has PLACED the sandbox on + // a worker, because the mint blocks on that (see sdkClient.mintCredential) — so a queued + // GPU is a call that blocks for minutes, and a successful return means real capacity. + // + // Minting is one-shot and there is no read-back, so // a caller that drops the credential can only get another from MintConnectCredential. // A sandbox that could not be given one is unreachable, so a failed mint is an ERROR // with no id, not a zero credential — the sandbox may exist, and the claim tag is what @@ -370,18 +379,17 @@ func (p *Provider) Capabilities() provider.Capabilities { NativeTags: true, // sandbox tags carry identity PreemptionNotice: 0, // no push; poll-based detection PollInterval: 0, // OnDemand-only (never preempts) → the default cadence is fine - ProvisionTimeout: provisionTimeout, // Provision blocks on the image build + ProvisionTimeout: provisionTimeout, // Provision blocks on the build, then on placement } } // Provision implements provider.Provider. The Pod is the source of truth for // the workload; req carries only the claim identity and capacity tier. // -// A fresh sandbox is NEVER reserved. Create returns as soon as Modal accepts it: the id is -// real (listable, terminable, fully on the hook for teardown) but the GPU may be queued for -// minutes. So id and capacity are two separate facts here, unlike AWS, and reserved=false -// is what keeps the Pod at the provisioning reason instead of claiming to be initializing. -// The queued→running transition is then observed through the poll loop's List. +// A fresh sandbox comes back RESERVED, but not because Modal's create says so — it accepts a +// sandbox whose GPU is still queued. The mint is what waits (see mintCredential), so id and +// capacity arrive together, at the price of a call that blocks for as long as the queue does — +// bounded by Modal's own ~3m43s cap on the mint, not by provisionTimeout. // // The connect credential comes back on this call and only this call, since Modal mints it // once with no read-back. The caller must persist it or it is lost; see mintCredential. @@ -398,9 +406,9 @@ func (p *Provider) Provision( // Idempotency: if a sandbox already carries this claim tag, return it rather // than creating a second (guards against a retry after a partial create). // - // Unlike a fresh create, this sandbox has been OBSERVED, so its state is known: one - // the poll loop reports Running has capacity, one still queued does not. That is more - // information than a create can return, so report it rather than a flat false. + // Unlike a fresh create, nothing here waits for placement, so the OBSERVED state is the only + // evidence of capacity: one the poll loop reports Running has it, one still queued does not. + // This is therefore the one branch that can report reserved=false. // // No credential comes back, per the Provider contract: minting is one-shot with no // read-back, and a fresh token revokes nothing, so re-minting for a sandbox whose token is @@ -424,7 +432,7 @@ func (p *Provider) Provision( } return provider.ProvisionResult{ InstanceID: id, - Reserved: false, + Reserved: true, // mintCredential is a blocking call, so the GPU is reserved on return ConnectURL: cred.URL, ConnectToken: cred.Token, }, nil diff --git a/pkg/provider/modal/modal_test.go b/pkg/provider/modal/modal_test.go index 67f5c80..2034f2a 100644 --- a/pkg/provider/modal/modal_test.go +++ b/pkg/provider/modal/modal_test.go @@ -194,11 +194,10 @@ func TestProvision_GPUPod(t *testing.T) { if id != "sb-1" { t.Fatalf("id = %q, want sb-1", id) } - // Create only means the control plane ACCEPTED the sandbox — the GPU may still be - // queued — so a fresh sandbox is never reserved. Claiming otherwise would let the - // Pod report Initializing while nothing has been allocated. - if reserved { - t.Fatal("reserved = true; a freshly created sandbox may still be queued for capacity") + // A returning CreateSandbox means the sandbox is PLACED, not merely accepted: the + // credential mint inside it blocks until Modal assigns a worker (see mintCredential). + if !reserved { + t.Fatal("reserved = false for a created sandbox; the mint only returns once it is placed") } if f.lastSpec.GPU != "H100" || f.lastSpec.GPUCount != 2 { t.Fatalf("spec GPU=%q count=%d, want H100/2", f.lastSpec.GPU, f.lastSpec.GPUCount) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index ae6e52c..1a44466 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -54,10 +54,10 @@ type Provider interface { // credential. Reserved means the provider committed real capacity, not merely accepted // the request — two different guarantees an id alone cannot express: // - // - AWS reserves: an *instant* CreateFleet is synchronous, so an id means EC2 found - // capacity and the instance is booting; a shortfall is an error. - // - Modal does not: Create returns once the control plane accepts the sandbox, and - // the GPU may stay queued for minutes on a large shape. + // - AWS reserves in its API: an *instant* CreateFleet is synchronous, so an id means EC2 + // found capacity and the instance is booting; a shortfall is an error. + // - Modal's create does not — it accepts a sandbox whose GPU may stay queued for minutes + // — so its adapter WAITS on a later call that Modal holds until a worker is assigned. // // Callers use it for honest status: an unreserved instance is not yet initializing, so // its Pod stays at the provisioning reason. It says nothing about readiness, which is diff --git a/pkg/vnode/handler.go b/pkg/vnode/handler.go index 1dd57c4..f231cb9 100644 --- a/pkg/vnode/handler.go +++ b/pkg/vnode/handler.go @@ -383,9 +383,9 @@ func (h *Handler) CreatePod(ctx context.Context, pod *corev1.Pod) error { } // Only a RESERVED instance advances: capacity is committed and it is booting. An - // unreserved id (a Modal sandbox accepted but still queued for a GPU) stays at the - // Provisioning stamped above, which is exactly true — the id is real and must be - // reclaimed, but nothing is allocated yet. + // unreserved id (a Modal sandbox re-attached by its idempotency branch while still queued + // for a GPU) stays at the Provisioning stamped above, which is exactly true — the id is + // real and must be reclaimed, but nothing is allocated yet. // // store runs either way: an id means the instance exists. markStatus first, because // store deep-copies and would otherwise track a copy without the new status. From f949266eb5828fc2bdc36889f1bf3d565b891646 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 00:48:08 +0100 Subject: [PATCH 07/14] optimize log Signed-off-by: kerthcet --- internal/controller/cost_accrual.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index 4cfa7f5..023e273 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -240,7 +240,7 @@ func settleFinalCost(ctx context.Context, nc *nebulav1alpha1.NodeClaim) { // Zero once the instance is already gone (Terminated freezes the total), which RecordWindow // drops rather than minting a series for. metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, total-costSoFar(nc)) - logf.FromContext(ctx).Info("instance reclaimed", "TotalTimeCostUSD", formatCost(total)) + logf.FromContext(ctx).Info("claim finalized", "phase", nc.Status.Phase, "TotalTimeCostUSD", formatCost(total)) } // costNow is what a claim has cost as of now: the persisted ledger plus the window still open From b3357ed5e0088104e9114cbd7366a30109dae3df Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 09:03:53 +0100 Subject: [PATCH 08/14] change the prometheus label name Signed-off-by: kerthcet --- api/v1alpha1/nodeclaim_types.go | 6 +- cmd/main.go | 4 +- .../bases/nebula.inftyai.com_nodeclaims.yaml | 6 +- docs/metrics.md | 28 +++--- internal/controller/cost_accrual.go | 2 +- internal/controller/cost_accrual_test.go | 87 ++++++++++++++++++- pkg/metrics/attribution.go | 35 ++++---- pkg/metrics/attribution_test.go | 49 +++++++---- 8 files changed, 158 insertions(+), 59 deletions(-) diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index 7f49d2f..d904769 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -187,9 +187,9 @@ type NodeClaimStatus struct { // CostLabels attributes this claim's spend to whoever asked for it: the values the served // Pod carried for the label keys the operator configured (--cost-labels). Keyed by the POD - // label key, verbatim — so this map reads like the Pod it came from, including any domain - // prefix. The metric emits under a derived name Prometheus accepts - // ("example.com/org-id" here is org_id there), so do not expect the two to match by string. + // label key, verbatim — so this map reads like the Pod it came from. The metric emits under a + // derived name Prometheus accepts ("example.com/org-id" here is example_com_org_id there), so + // do not expect the two to match by string. // // Status rather than spec because it is observed from the Pod, and it sits with the rest of // the billing record for a reason: it is stamped in the SAME patch that opens the accrual diff --git a/cmd/main.go b/cmd/main.go index 8e15741..5e4b9bc 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -123,8 +123,8 @@ func main() { "NetworkPolicy, or set this to your API server's kubelet client CA.") flag.StringVar(&costLabels, "cost-labels", "", "Comma-separated Pod label keys whose values attribute cost, e.g. "+ - "\"example.com/org-id,team_id\". Qualified keys are fine: the metric label is derived from the "+ - "key's name part with '-' and '.' folded to '_' (example.com/org-id is queried as org_id), so "+ + "\"example.com/org-id,team_id\". Qualified keys are fine: the metric label is the whole key "+ + "with '/', '-' and '.' folded to '_' (example.com/org-id is queried as example_com_org_id), so "+ "two keys that would collide there are rejected at startup. Empty (the default) reports cost "+ "by candidate shape only. Changing this changes the identity of every cost series. Values "+ "come from Pod labels and are NOT capped: the manager warns once if they push the cost "+ diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index 71d45ff..a357f43 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -153,9 +153,9 @@ spec: description: |- CostLabels attributes this claim's spend to whoever asked for it: the values the served Pod carried for the label keys the operator configured (--cost-labels). Keyed by the POD - label key, verbatim — so this map reads like the Pod it came from, including any domain - prefix. The metric emits under a derived name Prometheus accepts - ("example.com/org-id" here is org_id there), so do not expect the two to match by string. + label key, verbatim — so this map reads like the Pod it came from. The metric emits under a + derived name Prometheus accepts ("example.com/org-id" here is example_com_org_id there), so + do not expect the two to match by string. Status rather than spec because it is observed from the Pod, and it sits with the rest of the billing record for a reason: it is stamped in the SAME patch that opens the accrual diff --git a/docs/metrics.md b/docs/metrics.md index 1767a69..a814007 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -180,7 +180,7 @@ workloads look free while long ones stayed accurate. `EST_COST` does *not* get that last window, so it understates a reclaimed instance's lifetime by up to one interval while the counter has the whole of it. The lifetime figure also goes to the log -(`instance reclaimed`), the only record that outlives both the object and Prometheus retention. +(`claim finalized`), the only record that outlives both the object and Prometheus retention. **Where the number comes from.** `NodeClaim.status.priceUSDPerHour`, resolved from the provider's catalog (`pkg/provider/catalog/data/*.csv`) against the served Pod's shape and @@ -217,21 +217,25 @@ Who to charge. Off by default; `--cost-labels` turns it on: ``` Each entry is a **Pod label key**, written exactly as the Pod carries it. The metric label is -*derived* from it: the key's name part, with `-` and `.` folded to `_`, and the domain prefix -dropped. +*derived* from it: the whole key, with `/`, `-` and `.` folded to `_`. | `--cost-labels` entry | Pod label read | PromQL label | | --- | --- | --- | -| `example.com/org-id` | `example.com/org-id` | `org_id` | +| `example.com/org-id` | `example.com/org-id` | `example_com_org_id` | | `team.id` | `team.id` | `team_id` | | `org_id` | `org_id` | `org_id` | -So a key is configured the way Kubernetes spells it and queried the way PromQL can express it — -nobody has to write `sum by (example_com_org_id)`. What dropping the prefix costs is that two keys -can want the same name; `a.com/org-id,b.com/org-id` is **rejected at startup** rather than merging -two tenants' spend into one series. Also rejected there: a key Kubernetes would not accept, a name -part no folding can rescue (`2team` — Prometheus label names cannot start with a digit), the same -key twice, and one whose derived name shadows a label the metric already carries (`provider`, +So a key is configured the way Kubernetes spells it and emitted under a name Prometheus accepts, +with nothing thrown away in between — the same convention kube-state-metrics +(`label_app_kubernetes_io_name`) and Prometheus service discovery +(`__meta_kubernetes_pod_label_…`) follow. Keeping the prefix is what makes `a.com/org-id` and +`b.com/org-id` two distinct series rather than a collision, and what keeps a qualified key from +quietly shadowing a label the metric already carries. + +Rejected at startup: a key Kubernetes would not accept, one whose derived name Prometheus would not +(anything starting with a digit — a bare `2team`, or a domain like `4paradigm.com/org-id`; use an +unqualified Pod label there), the same key twice, two keys folding to the same name (`org-id` and +`org.id` still meet), and a bare key that shadows a label the metric already carries (`provider`, `region`, `phase`, …). Every one of those fails the process at boot instead of silently reporting every tenant as `none`. @@ -265,10 +269,10 @@ workload: nothing is split, and nothing is counted twice. ```promql # Yesterday's bill, per tenant. -sum by (org_id) (increase(nebula_cost_usd_total[1d])) +sum by (example_com_org_id) (increase(nebula_cost_usd_total[1d])) # One team's burn rate, in USD/hour. -sum(rate(nebula_cost_usd_total{org_id="acme",team_id="ml"}[30m])) * 3600 +sum(rate(nebula_cost_usd_total{example_com_org_id="acme",example_com_team_id="ml"}[30m])) * 3600 ``` **Values are tenant-controlled, which is a cardinality risk** — and the only one on this endpoint, diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index 023e273..f395182 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -240,7 +240,7 @@ func settleFinalCost(ctx context.Context, nc *nebulav1alpha1.NodeClaim) { // Zero once the instance is already gone (Terminated freezes the total), which RecordWindow // drops rather than minting a series for. metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, total-costSoFar(nc)) - logf.FromContext(ctx).Info("claim finalized", "phase", nc.Status.Phase, "TotalTimeCostUSD", formatCost(total)) + logf.FromContext(ctx).Info("claim finalized", "phase", nc.Status.Phase, "totalTimeCostUSD", formatCost(total)) } // costNow is what a claim has cost as of now: the persisted ledger plus the window still open diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index 15b95fd..b6e9b0a 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -38,6 +38,7 @@ import ( nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" "github.com/InftyAI/Nebula/pkg/metrics" + "github.com/InftyAI/Nebula/pkg/provider" ) // billingClaim is a claim holding a priced instance, anchored ago in the past. A nil ago leaves @@ -426,13 +427,97 @@ func TestCostAccrual_BooksAttribution(t *testing.T) { want := ` # HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. # TYPE nebula_cost_usd_total counter -nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="acme",phase="Bound",provider="modal",region="none"} 10 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",example_com_org_id="acme",phase="Bound",provider="modal",region="none"} 10 ` if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(want)); err != nil { t.Fatal(err) } } +// The whole chain --cost-labels exists for, through a real reconcile: a label on the served Pod +// reaches the claim's status, and the window charged later comes out under it. Nothing is stamped +// by hand here, so the two halves are only connected if markPhase picks the attribution up and +// opens the accrual anchor in the SAME write it records the phase and price with. +func TestMarkPhase_StampsAttributionThatIsThenBooked(t *testing.T) { + t.Cleanup(func() { metrics.ConfigureCostForTest(nil) }) + metrics.ConfigureCostForTest([]string{"example.com/org-id"}) + metrics.CostTotal.Reset() + + pod := gpuPod("H100", 1, "1", "1Gi") + pod.Labels["example.com/org-id"] = "acme" + withInstanceID(pod, "sb-1") + + nc := newClaim("c1", "p1", "default", "uid-1", "fake") + nc.Spec.Accelerator = "H100:1" + nc.Spec.Region = "us-east-1" + nc.Spec.CapacityType = nebulav1alpha1.CapacityOnDemand + + // Status writes are counted, not merely observed: a window charged before the attribution + // explaining it is durable is spend nobody can bill, so the four fields must land together. + writes := 0 + s := testScheme(t) + c := fake.NewClientBuilder(). + WithScheme(s). + WithObjects(pod, nc). + WithStatusSubresource(&nebulav1alpha1.NodeClaim{}). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, cl client.Client, sub string, + obj client.Object, opts ...client.SubResourceUpdateOption) error { + writes++ + return cl.SubResource(sub).Update(ctx, obj, opts...) + }, + }). + Build() + pp := &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, rate: 10} + r := &NodeClaimReconciler{Client: c, Scheme: s, Providers: func(name string) (provider.Provider, bool) { + return pp, name == pp.name + }} + + reconcileClaim(t, r, "c1") + + if writes != 1 { + t.Fatalf("%d status writes, want 1 — the attribution and the anchor must ride one patch", writes) + } + got := getClaim(t, c, "c1") + if got.Status.Phase != nebulav1alpha1.NodeClaimBound || got.Status.PriceUSDPerHour != "10.0000" { + t.Fatalf("phase %q price %q, want Bound / 10.0000", got.Status.Phase, got.Status.PriceUSDPerHour) + } + want := map[string]string{"example.com/org-id": "acme"} + if !reflect.DeepEqual(got.Status.CostLabels, want) { + t.Fatalf("status.costLabels = %v, want %v", got.Status.CostLabels, want) + } + // The anchor opens only once the claim is Bound AND priced, so its presence is also the + // evidence that stampAccrualStart ran after the two calls it reads — not before them. + if got.Status.LastAccruedAt == nil { + t.Fatal("no accrual anchor was opened; the window before the first checkpoint is lost") + } + + // Everything is settled, so a second pass over the same Pod must write nothing. + reconcileClaim(t, r, "c1") + if writes != 1 { + t.Fatalf("%d status writes after a second reconcile, want 1", writes) + } + + // Rewind the anchor an hour (truncated, as the accrual's own clock is) so a whole window is + // chargeable without sleeping, then let the loop charge it. + got.Status.LastAccruedAt = &metav1.Time{Time: time.Now().Add(-time.Hour).Truncate(time.Second)} + if err := c.Status().Update(context.Background(), got); err != nil { + t.Fatalf("rewind the anchor: %v", err) + } + NewCostAccrual(c).accrueAll(context.Background()) + + // The exposition format is line-oriented, so this cannot be wrapped. + //nolint:lll + wantSeries := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Bound",provider="fake",region="us-east-1"} 10 +` + if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(wantSeries)); err != nil { + t.Fatal(err) + } +} + // The stamp is what makes a crash before the first checkpoint lossless, so it must land exactly // when the claim becomes chargeable — and never move once set. func TestStampAccrualStart(t *testing.T) { diff --git a/pkg/metrics/attribution.go b/pkg/metrics/attribution.go index b7e92f2..62e8fc3 100644 --- a/pkg/metrics/attribution.go +++ b/pkg/metrics/attribution.go @@ -36,23 +36,20 @@ import ( const costSeriesWarnThreshold = 5000 // promLabelName is Prometheus's own label-name grammar, applied to the DERIVED name rather than to -// the Pod key. The one legal Kubernetes key it still rejects is a name part starting with a digit -// ("2team"), which nothing here can repair without inventing a prefix. +// the Pod key. The legal Kubernetes keys it still rejects are the ones STARTING with a digit — a +// bare "2team", or a digit-leading domain like "4paradigm.com/org-id" — which nothing here can +// repair without inventing a prefix of its own. var promLabelName = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`) -// metricNameFor derives the Prometheus label name a Pod label key is emitted under: the key's name -// part, with '-' and '.' folded to '_'. +// metricNameFor derives the Prometheus label name a Pod label key is emitted under: the WHOLE key, +// with '/', '-' and '.' folded to '_' — so "example.com/org-id" is queried as example_com_org_id. // -// The domain prefix is dropped because it answers no question a dashboard asks — "example.com/org-id" -// and "org-id" mean the same thing, and `sum by (example_com_org_id)` is nobody's idea of a query. -// What that costs is two prefixes sharing a name part, which ParseCostLabels rejects outright rather -// than letting one silently absorb the other's spend. +// Keeping the domain prefix is what every Kubernetes exporter does (kube-state-metrics emits +// label_app_kubernetes_io_name, Prometheus service discovery __meta_kubernetes_pod_label_…), and +// for the reason that the prefix is part of the key's identity: two tenants' keys stay distinct, +// and the name in a dashboard is one that appears verbatim in `kubectl get pod --show-labels`. func metricNameFor(podKey string) (string, error) { - name := podKey - if slash := strings.IndexByte(name, '/'); slash >= 0 { - name = name[slash+1:] - } - name = strings.NewReplacer("-", "_", ".", "_").Replace(name) + name := strings.NewReplacer("/", "_", "-", "_", ".", "_").Replace(podKey) if !promLabelName.MatchString(name) { return "", fmt.Errorf("cost label %q emits as %q, which Prometheus will not accept as a label "+ "name (it must start with a letter or underscore)", podKey, name) @@ -69,10 +66,12 @@ func metricNameFor(podKey string) (string, error) { // when every series has already been recorded as "none". // // Four rejections, all at startup: a key Kubernetes would not accept, one whose derived name -// Prometheus would not, the same key twice, and — the price of dropping the domain prefix — two -// keys deriving the SAME name. A derived name that shadows a dimension the counter already carries -// ("provider", "phase") gets through here and fails at InitCost, where the registry rejects a -// duplicate label name; still at startup, just with a less pointed message. +// Prometheus would not, the same key twice, and two keys folding to the SAME name. That last one is +// a corner case now that the prefix is kept ("org-id" and "org.id" still meet), but it would merge +// two tenants' spend into one series, so it fails rather than being tolerated. A derived name that +// shadows a dimension the counter already carries ("provider", "phase") gets through here and fails +// at InitCost, where the registry rejects a duplicate label name; still at startup, just with a +// less pointed message. // // Setting this at all is what makes cost the only metric here whose CARDINALITY is not bounded by // configuration: the values come from Pod labels. Nothing caps them — noteSeries only warns — so @@ -101,7 +100,7 @@ func ParseCostLabels(spec string) ([]string, error) { } if first, clash := claimedBy[name]; clash { return nil, fmt.Errorf("cost labels %q and %q both emit as %q, which would merge two "+ - "tenants' spend into one series; drop one or rename its name part", first, key, name) + "tenants' spend into one series; drop one or rename it", first, key, name) } seenKey[key] = struct{}{} claimedBy[name] = key diff --git a/pkg/metrics/attribution_test.go b/pkg/metrics/attribution_test.go index 4199a42..028cf54 100644 --- a/pkg/metrics/attribution_test.go +++ b/pkg/metrics/attribution_test.go @@ -59,33 +59,44 @@ func TestParseCostLabels(t *testing.T) { want: []string{"org_id", "team_id"}, }, // The point of the whole exercise: a qualified key is configured as the Pod carries it and - // emitted as PromQL can express it. - "qualified key drops its prefix": { + // emitted as PromQL can express it — prefix and all, as kube-state-metrics does it. + "qualified key keeps its prefix": { spec: "example.com/org-id", want: []string{"example.com/org-id"}, - names: []string{"org_id"}, + names: []string{"example_com_org_id"}, }, "a set of qualified keys sharing one prefix": { spec: "example.com/experiment-id,example.com/org-id,example.com/team-id,example.com/user-id", want: []string{ "example.com/experiment-id", "example.com/org-id", "example.com/team-id", "example.com/user-id", }, - names: []string{"experiment_id", "org_id", "team_id", "user_id"}, + names: []string{ + "example_com_experiment_id", "example_com_org_id", "example_com_team_id", "example_com_user_id", + }, + }, + // Same name part under two prefixes: distinct series, which is what keeping the prefix buys. + "same name part under two domains": { + spec: "a.com/org-id,b.com/org-id", + want: []string{"a.com/org-id", "b.com/org-id"}, + names: []string{"a_com_org_id", "b_com_org_id"}, }, "hyphen and dot are folded": { spec: "org-id,team.id", want: []string{"org-id", "team.id"}, names: []string{"org_id", "team_id"}, }, - // A legal Pod key whose name part no folding can rescue. - "leading digit": {spec: "2team", err: true}, - "trailing underscore": {spec: "org_", err: true}, - "spaces inside a key": {spec: "org id", err: true}, - "empty name part": {spec: "example.com/", err: true}, - "duplicate key": {spec: "org_id,org_id", err: true}, - // The cost of dropping the prefix, and the reason it is safe to drop. - "distinct keys colliding on the derived name": {spec: "a.com/org-id,b.com/org-id", err: true}, - "bare key colliding with a qualified one": {spec: "org_id,example.com/org-id", err: true}, + // Legal Pod keys no folding can rescue: a Prometheus label name cannot start with a digit, + // and the prefix being part of the name puts a digit-leading DOMAIN in the same boat. + "leading digit": {spec: "2team", err: true}, + "digit-leading domain": {spec: "4paradigm.com/org-id", err: true}, + "trailing underscore": {spec: "org_", err: true}, + "spaces inside a key": {spec: "org id", err: true}, + "empty name part": {spec: "example.com/", err: true}, + "duplicate key": {spec: "org_id,org_id", err: true}, + // Folding still lets two distinct keys meet, rarely — and a merge of two tenants' spend is + // worth refusing even so. + "distinct keys colliding on the derived name": {spec: "org-id,org.id", err: true}, + "bare key colliding with a qualified one": {spec: "com_org_id,com/org-id", err: true}, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -116,12 +127,12 @@ func TestParseCostLabels(t *testing.T) { // A DERIVED name that shadows one of the counter's own dimensions is not rejected by the parser — // it is caught one step later, when InitCost registers a descriptor with a duplicate label name. -// This pins that remaining net, and does it through a qualified key because the prefix is exactly -// what hides the collision from a reader of the flag. +// Only a BARE key can reach it now that the prefix is kept: "example.com/provider" emits as +// example_com_provider and collides with nothing. func TestInitCost_RejectsAShadowingName(t *testing.T) { t.Cleanup(func() { configureCost(nil) }) - labels, err := ParseCostLabels("example.com/provider") + labels, err := ParseCostLabels("provider") if err != nil { t.Fatalf("ParseCostLabels: %v", err) } @@ -146,9 +157,9 @@ func TestRecordWindow_Attribution(t *testing.T) { want := ` # HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. # TYPE nebula_cost_usd_total counter -nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="acme",phase="Bound",provider="modal",region="none",team_id="ml"} 3.95 -nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="acme",phase="Bound",provider="modal",region="none",team_id="none"} 1.05 -nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",org_id="none",phase="Bound",provider="modal",region="none",team_id="none"} 0.5 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",example_com_org_id="acme",example_com_team_id="ml",phase="Bound",provider="modal",region="none"} 3.95 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",example_com_org_id="acme",example_com_team_id="none",phase="Bound",provider="modal",region="none"} 1.05 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",example_com_org_id="none",example_com_team_id="none",phase="Bound",provider="modal",region="none"} 0.5 ` if err := testutil.CollectAndCompare(CostTotal, strings.NewReader(want)); err != nil { t.Fatal(err) From cea2ce32b93bf13f64cfbca4db7719a8efe18c5d Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 17:05:56 +0100 Subject: [PATCH 09/14] address comment Signed-off-by: kerthcet --- api/v1alpha1/nodeclaim_types.go | 15 +++- .../bases/nebula.inftyai.com_nodeclaims.yaml | 13 ++- docs/metrics.md | 42 +++++++--- internal/controller/cost_accrual.go | 49 ++++++++--- internal/controller/cost_accrual_test.go | 84 +++++++++++++++++-- .../cost_labels_persistence_test.go | 74 ++++++++++++++++ pkg/provider/pricing.go | 1 + 7 files changed, 245 insertions(+), 33 deletions(-) create mode 100644 internal/controller/cost_labels_persistence_test.go diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index d904769..b8bbe70 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -154,10 +154,15 @@ type NodeClaimStatus struct { // +optional PriceUSDPerHour string `json:"priceUSDPerHour,omitempty"` - // EstimatedCostUSD is what this instance has cost SO FAR, as a decimal string ("412.9400") + // EstimatedCostUSD is what this instance has cost SO FAR, as a decimal string ("412.94000000") // — PriceUSDPerHour integrated over the time it has held an instance. A string for the same // reason the rate is one: it exists to be read off a print column. // + // More decimals than the rate, and they are not decoration: each checkpoint re-reads this field + // to measure the next window, so digits dropped here are money dropped, and a cheap enough claim + // would round back to where it started every minute and never accrue at all. Round it for + // display; never round it back into this field. + // // ESTIMATED, and the name says so on purpose: it is our own arithmetic over a list price // (see PriceUSDPerHour), not a figure any provider has confirmed. Nothing here has been // invoiced. Reconcile against the provider's billing export before anyone is charged. @@ -199,8 +204,14 @@ type NodeClaimStatus struct { // retroactively re-attribute spend already reported under the old values. A name the // current --cost-labels no longer lists is ignored rather than cleaned up; one it lists but // this map lacks reports as "none". + // + // Nil and EMPTY differ, which is why this field has no omitempty: nil means nothing has been + // observed yet (or --cost-labels is unset), while an empty map is the settled fact that the + // Pod carried none of the configured keys. With omitempty an empty map would not survive the + // write, so every reconcile would re-read the Pod and a label added later could still move + // the claim's attribution. // +optional - CostLabels map[string]string `json:"costLabels,omitempty"` + CostLabels map[string]string `json:"costLabels"` } // +kubebuilder:object:root=true diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index a357f43..e039efe 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -165,6 +165,12 @@ spec: retroactively re-attribute spend already reported under the old values. A name the current --cost-labels no longer lists is ignored rather than cleaned up; one it lists but this map lacks reports as "none". + + Nil and EMPTY differ, which is why this field has no omitempty: nil means nothing has been + observed yet (or --cost-labels is unset), while an empty map is the settled fact that the + Pod carried none of the configured keys. With omitempty an empty map would not survive the + write, so every reconcile would re-read the Pod and a label added later could still move + the claim's attribution. type: object endpoint: description: Endpoint is the reachable address (e.g. SSH host:port) @@ -172,10 +178,15 @@ spec: type: string estimatedCostUSD: description: |- - EstimatedCostUSD is what this instance has cost SO FAR, as a decimal string ("412.9400") + EstimatedCostUSD is what this instance has cost SO FAR, as a decimal string ("412.94000000") — PriceUSDPerHour integrated over the time it has held an instance. A string for the same reason the rate is one: it exists to be read off a print column. + More decimals than the rate, and they are not decoration: each checkpoint re-reads this field + to measure the next window, so digits dropped here are money dropped, and a cheap enough claim + would round back to where it started every minute and never accrue at all. Round it for + display; never round it back into this field. + ESTIMATED, and the name says so on purpose: it is our own arithmetic over a list price (see PriceUSDPerHour), not a figure any provider has confirmed. Nothing here has been invoiced. Reconcile against the provider's billing export before anyone is charged. diff --git a/docs/metrics.md b/docs/metrics.md index a814007..651e487 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -160,10 +160,15 @@ per-claim rollup of them, and "what did this one instance cost" is a `kubectl ge query: ``` -NAME PROVIDER ACCELERATOR PRICE/HR EST_COST PHASE -nc-train-0 modal H100:8 23.7000 148.1250 Bound +NAME PROVIDER ACCELERATOR PRICE/HR EST_COST PHASE +nc-train-0 modal H100:8 23.7000 148.12500000 Bound ``` +`EST_COST` carries eight decimals to the rate's four because each checkpoint measures its window +from the field it wrote last time, so digits dropped there are money dropped, not display noise: at +four decimals a claim under $0.003/hour rounded back to its previous value every minute and never +accrued at all. Round it when you show it. + The counter is advanced only *after* the field's patch lands. A counter has no idempotency key, so a window booked before its write was durable would be charged twice: the anchor would not have moved, and the next tick would re-derive the same window. The persisted anchor is also why no time @@ -190,9 +195,15 @@ no private pricing, no gap between a Spot quote and the actual charge. Reconcile provider's billing export before anyone gets invoiced. **Which claims are charged.** Only `Bound` and `Terminating` hold an instance (see -`NodeClaimPhase`), so only they accrue; a `Terminated` claim's `EST_COST` is its frozen final -total, and nothing more is booked against it. `Provisioning` is excluded, undercounting by about -one poll interval per instance. +`NodeClaimPhase`), so only they accrue; a `Terminated` claim's `EST_COST` is its frozen final total. +`Provisioning` is excluded, undercounting by about one poll interval per instance. + +An instance that ends on its own — a preemption, a crashed sandbox — reaches `Terminated` without +passing through `Terminating`, and the loop will not touch it again. Teardown books the time since +its last checkpoint anyway, under `phase="Terminated"`, capped at one accrual interval: the claim +then waits in that phase until its Pod is deleted, and charging the wait would bill idle hours at GPU +rates. So that series is a real one, accurate to ±1 interval, and it separates spend that ended by +itself from spend we ended. `Terminating` still bills until teardown finishes, which is what makes the `phase` label worth having: @@ -405,12 +416,15 @@ sum by (accelerator) (rate(nebula_cost_usd_total[30m])) * 3600 All of these are deliberate, and all bias toward looking *better* than reality — worth knowing before trusting a dashboard. -- **Counters reset on restart.** Every counter and histogram here is in-process, cost included: - a redeploy or a leader handoff starts it at zero. This is ordinary Prometheus semantics — - `rate()` and `increase()` detect resets, and no window is lost across one, since the anchor in - `status` survives — but no cumulative history survives a redeploy; the scrape backend owns - durability. Aggregate as `sum(increase(...))`, never `increase(sum(...))`: only the former sees - the per-series reset. +- **Counters reset on restart, and lose their unscraped tail.** Every counter and histogram here is + in-process, cost included: a redeploy or a leader handoff starts it at zero. `rate()` and + `increase()` detect the reset, but the increments booked *since the last scrape* were never + observed by anyone, and for cost they cannot be re-booked — the next leader measures from + `status.lastAccruedAt`, which has already advanced past them. So an unclean exit drops up to one + scrape interval of spend from the counter, and a crash landing between a checkpoint's write and its + booking drops that window too. **`status.estimatedCostUSD` is what survives this** — cross-check + against it after a redeploy rather than treating the counter as durable. Aggregate as + `sum(increase(...))`, never `increase(sum(...))`: only the former sees the per-series reset. - **`EST_COST` understates a reclaimed instance; the metric does not.** The field is not written on the deletion path (the object is going away), so it misses the window still open at teardown — which the counter *does* book (see [Cost](#cost)). The two therefore disagree by up to one @@ -418,6 +432,12 @@ knowing before trusting a dashboard. line agreeing with it. `EST_COST` also lags live spend by up to one interval while a claim is running — a freshness limit, not an error, since a window not charged now is charged in full next tick. +- **The last window of a self-terminated instance is capped, not measured.** A `Terminated` claim's + anchor froze when the instance died, and nothing since distinguishes "died a minute ago" from + "died while the manager was down three hours ago", so teardown charges one interval either way + (see [Cost](#cost)). The cap is the safe direction — it cannot bill idle hours — but a preemption + during an outage is undercharged by the whole outage. Nothing is booked at all if the Pod object + never goes away, since teardown is what triggers it. - **A crash between the teardown write and the booking loses that window.** The final window is booked in-process right after the finalizer removal lands, and unlike a checkpoint it has no anchor to fall back on, so a process that dies in between charges it nowhere. Exactly-once in the diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index f395182..16ca4e3 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -184,6 +184,14 @@ func billingRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { default: return 0, false } + return finalRate(nc) +} + +// finalRate is billingRate without the phase gate. Terminated is excluded there because a claim +// sits in that phase until its Pod is deleted and charging those idle hours would be wrong — but the +// window between the last checkpoint and the instance's death is real, and settleFinalCost is the +// only place left that can book it. Nothing else may use this. +func finalRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { rate, err := strconv.ParseFloat(nc.Status.PriceUSDPerHour, 64) if err != nil || rate <= 0 { return 0, false @@ -202,8 +210,17 @@ func costSoFar(nc *nebulav1alpha1.NodeClaim) float64 { return total } +// costDecimals is how many fractional digits the LEDGER keeps — more than priceDecimals, because a +// rate is an input that is written once while a total is an accumulator that is re-read and +// re-written every minute. Rounding it at each checkpoint would quantize the effective rate onto the +// grid: the residue is discarded rather than carried, so a claim cheaper than half a grid step per +// window freezes forever (at four digits, anything under $0.003/hr — a Modal CPU-only sandbox), and +// one just above it is charged the rounded-UP step every tick. Eight digits puts that error below +// 0.01% at any rate a catalog carries, at the cost of a wider EST_COST column. +const costDecimals = 8 + func formatCost(usd float64) string { - return strconv.FormatFloat(usd, 'f', priceDecimals, 64) + return strconv.FormatFloat(usd, 'f', costDecimals, 64) } // stampAccrualStart opens the billing window the moment a claim first becomes chargeable, @@ -235,20 +252,29 @@ func stampAccrualStart(nc *nebulav1alpha1.NodeClaim) bool { // // The lifetime figure itself only goes to the log; status is deleted with the object. func settleFinalCost(ctx context.Context, nc *nebulav1alpha1.NodeClaim) { - // The absent/zero distinction costNow reports is for the ledger; a log line may say $0. - total, _ := costNow(nc, time.Now()) - // Zero once the instance is already gone (Terminated freezes the total), which RecordWindow - // drops rather than minting a series for. - metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, total-costSoFar(nc)) - logf.FromContext(ctx).Info("claim finalized", "phase", nc.Status.Phase, "totalTimeCostUSD", formatCost(total)) + var final float64 + if rate, ok := finalRate(nc); ok && nc.Status.LastAccruedAt != nil { + open := time.Since(nc.Status.LastAccruedAt.Time) + // Clamped in Terminated alone: its anchor froze when the instance died and the claim may + // have sat for days since. A Terminating claim is still running, so its whole open window is + // real money — including hours recovered after a manager outage. + if nc.Status.Phase == nebulav1alpha1.NodeClaimTerminated && open > accrualInterval { + open = accrualInterval + } + if open > 0 { + final = rate * open.Hours() + } + } + // Zero for a claim that was never priced or never anchored, which RecordWindow drops rather + // than minting a series for. + metrics.RecordWindow(claimLabels(nc), string(nc.Status.Phase), nc.Status.CostLabels, final) + logf.FromContext(ctx).Info("claim finalized", "phase", nc.Status.Phase, + "totalTimeCostUSD", formatCost(costSoFar(nc)+final)) } // costNow is what a claim has cost as of now: the persisted ledger plus the window still open // since its anchor. False means the claim has no cost to report AT ALL — neither billable nor // ever charged, which is absent spend rather than zero spend (see billingRate). -// -// Both the checkpoint and the final log line read through here, so the ledger and the number a -// reclaimed instance reports cannot drift. func costNow(nc *nebulav1alpha1.NodeClaim, now time.Time) (float64, bool) { rate, billing := billingRate(nc) total := costSoFar(nc) @@ -296,7 +322,8 @@ func stampCostLabels(nc *nebulav1alpha1.NodeClaim, pod *corev1.Pod) bool { } } // An empty map, not nil: a Pod carrying none of the configured labels is a settled fact, and - // nil would make every later reconcile look it up again. + // nil would make every later reconcile look it up again. Durable only because the field has no + // omitempty — see NodeClaimStatus.CostLabels. nc.Status.CostLabels = stamped return true } diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index b6e9b0a..4d903ee 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -151,6 +151,42 @@ func TestCostAccrual_AccumulatesAcrossTicks(t *testing.T) { } } +// Every checkpoint re-reads the ledger to measure the next window, so the digits it keeps ARE the +// accounting resolution — see costDecimals. Driven at the real cadence, a cheap claim is where that +// shows: at four decimals $0.0028/hr froze at $0.0001 and never moved again, while $0.01/hr was +// charged a rounded-up step every minute and ran 20% over. +func TestCostAccrual_ChargesCheapClaimsAtTheirRealRate(t *testing.T) { + // Modal's CPU-only components: a 20-millicore reservation, a 64MiB one, and one whole core. + for _, rate := range []string{"0.0028", "0.0100", "0.1419"} { + t.Run(rate, func(t *testing.T) { + none := time.Duration(0) + nc := billingClaim("cheap", rate, &none) + base := nc.Status.LastAccruedAt.Time + a, c := newAccrual(t, nc) + + const ticks = 240 // four hours at the one-minute write cadence + for i := 1; i <= ticks; i++ { + at := base.Add(time.Duration(i) * accrualInterval) + a.now = func() time.Time { return at } + a.accrueAll(context.Background()) + } + + hourly, err := strconv.ParseFloat(rate, 64) + if err != nil { + t.Fatalf("bad rate %q: %v", rate, err) + } + want := hourly * float64(ticks) * accrualInterval.Hours() + total, _ := ledger(t, c, "cheap") + if math.Abs(total-want)/want > 1e-4 { + t.Fatalf("status.estimatedCostUSD %v after %d ticks at $%s/hr, want %v", total, ticks, rate, want) + } + if got := booked(t); math.Abs(got-total) > 1e-9 { + t.Fatalf("booked %v, want %v — the counter must take exactly what the ledger did", got, total) + } + }) + } +} + // A claim that became billable before the ledger existed has no anchor. Opening one must not // invent spend for the window whose length nobody knows. func TestCostAccrual_StampsMissingAnchorWithoutCharging(t *testing.T) { @@ -314,20 +350,52 @@ func TestSettleFinalCost_BooksTheOpenWindow(t *testing.T) { } } -// Once the instance is gone there is no open window, and re-booking the frozen total would charge -// the claim's whole life a second time. -func TestSettleFinalCost_BooksNothingOnceGone(t *testing.T) { +// An instance that ended on its own reaches Terminated, which the accrual loop refuses to touch, so +// the time since its last checkpoint is booked here or nowhere. The clamp is what keeps that safe: +// the claim then sits in Terminated until its Pod is deleted, and charging that wait would bill idle +// hours at GPU rates. +func TestSettleFinalCost_BooksTheLastWindowOfADeadInstance(t *testing.T) { + // Half a window, so the first case measures the real gap rather than tripping the clamp. + halfWindow, day := accrualInterval/2, 24*time.Hour + cases := map[string]struct { + ago time.Duration + want float64 + }{ + "the window since the last checkpoint": {ago: halfWindow, want: 10 * halfWindow.Hours()}, + "never longer than one interval": {ago: day, want: 10 * accrualInterval.Hours()}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + metrics.CostTotal.Reset() + + nc := billingClaim("settled", "10.0000", &tc.ago) + nc.Status.Phase = nebulav1alpha1.NodeClaimTerminated + nc.Status.EstimatedCostUSD = "5.0000" + + settleFinalCost(context.Background(), nc) + + // billingClaim truncates the anchor to the second, so the real window is up to a + // second longer than ago. Two seconds of spend covers that and the test's own time. + if got := booked(t); math.Abs(got-tc.want) > 10*(2*time.Second).Hours() { + t.Fatalf("booked %v, want %v", got, tc.want) + } + }) + } +} + +// Unpriced is absent spend, not zero — a claim that was never chargeable must not mint a series on +// its way out either. +func TestSettleFinalCost_BooksNothingUnpriced(t *testing.T) { metrics.CostTotal.Reset() hour := time.Hour - settled := billingClaim("settled", "10.0000", &hour) - settled.Status.Phase = nebulav1alpha1.NodeClaimTerminated - settled.Status.EstimatedCostUSD = "5.0000" + nc := billingClaim("unpriced", "", &hour) + nc.Status.Phase = nebulav1alpha1.NodeClaimTerminated - settleFinalCost(context.Background(), settled) + settleFinalCost(context.Background(), nc) if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { - t.Fatalf("collected %d series for an instance already gone, want 0", n) + t.Fatalf("collected %d series for a claim that was never priced, want 0", n) } } diff --git a/internal/controller/cost_labels_persistence_test.go b/internal/controller/cost_labels_persistence_test.go new file mode 100644 index 0000000..c719ce0 --- /dev/null +++ b/internal/controller/cost_labels_persistence_test.go @@ -0,0 +1,74 @@ +/* +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 controller + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "sigs.k8s.io/controller-runtime/pkg/client" + + nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" +) + +// status.costLabels distinguishes nil (nothing observed yet) from empty (observed, the Pod carried +// none of the configured keys), and stampCostLabels relies on that to write attribution exactly +// once. Only a real apiserver can prove it: the fake client the unit tests use stores deep copies +// rather than marshalling, so an empty map survives there whatever the json tag says — while an +// omitempty would drop it on the way to etcd and leave the claim looking unstamped forever. +// Requires envtest binaries; the whole suite is skipped in BeforeSuite when they are absent. +var _ = Describe("NodeClaim status.costLabels", func() { + claim := func(name string) *nebulav1alpha1.NodeClaim { + return newClaimNoFinalizer(name, "p1", "default", "uid-1", "modal") + } + readBack := func(name string) map[string]string { + var nc nebulav1alpha1.NodeClaim + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: name}, &nc)).To(Succeed()) + return nc.Status.CostLabels + } + + It("keeps an empty map distinct from an unset one", func() { + nc := claim("cost-labels-empty") + Expect(k8sClient.Create(ctx, nc)).To(Succeed()) + DeferCleanup(func() { Expect(k8sClient.Delete(ctx, nc)).To(Succeed()) }) + + // Unset: nothing observed yet. + Expect(readBack(nc.Name)).To(BeNil()) + + nc.Status.Phase = nebulav1alpha1.NodeClaimBound + nc.Status.CostLabels = map[string]string{} + Expect(k8sClient.Status().Update(ctx, nc)).To(Succeed()) + + // Stamped and empty. A nil here is the bug this spec exists for: stampCostLabels would + // re-read the Pod on every reconcile, and a label added later would move the attribution of + // a claim whose earlier windows were already booked to "none". + Expect(readBack(nc.Name)).NotTo(BeNil()) + Expect(readBack(nc.Name)).To(BeEmpty()) + }) + + It("keeps a stamped value verbatim, qualified key and all", func() { + nc := claim("cost-labels-stamped") + Expect(k8sClient.Create(ctx, nc)).To(Succeed()) + DeferCleanup(func() { Expect(k8sClient.Delete(ctx, nc)).To(Succeed()) }) + + nc.Status.Phase = nebulav1alpha1.NodeClaimBound + nc.Status.CostLabels = map[string]string{"example.com/org-id": "acme"} + Expect(k8sClient.Status().Update(ctx, nc)).To(Succeed()) + + Expect(readBack(nc.Name)).To(Equal(map[string]string{"example.com/org-id": "acme"})) + }) +}) diff --git a/pkg/provider/pricing.go b/pkg/provider/pricing.go index 2509689..1ccdec0 100644 --- a/pkg/provider/pricing.go +++ b/pkg/provider/pricing.go @@ -38,6 +38,7 @@ var ErrNoPrice = errors.New("provider: no price for request") // No region axis, because no catalog row is region-partitioned today (every CSV leaves // the column empty), so a price is uniform across a provider's regions. Add the axis back // alongside the first region-varying row. +// TODO: maybe need to support region in the future. type PriceRequest struct { // AcceleratorType is the canonical type ("H100"), empty for a CPU-only workload. AcceleratorType string From b91cd6d211f2cf4f3fb0c1f5d080acad4ce2a0eb Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 17:26:48 +0100 Subject: [PATCH 10/14] fix comments Signed-off-by: kerthcet --- internal/controller/cost_accrual.go | 11 ++++++++-- internal/controller/cost_accrual_test.go | 27 ++++++++++++++++++++++++ pkg/metrics/cost.go | 8 ++++++- pkg/metrics/cost_test.go | 15 +++++++++++++ pkg/provider/catalog/catalog.go | 7 ++++++ pkg/provider/catalog/catalog_test.go | 20 ++++++++++++++++++ pkg/provider/modal/modal.go | 4 ++-- 7 files changed, 87 insertions(+), 5 deletions(-) diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index 16ca4e3..c0d4a00 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -18,6 +18,7 @@ package controller import ( "context" + "math" "strconv" "time" @@ -193,7 +194,10 @@ func billingRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { // only place left that can book it. Nothing else may use this. func finalRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { rate, err := strconv.ParseFloat(nc.Status.PriceUSDPerHour, 64) - if err != nil || rate <= 0 { + // ParseFloat accepts "NaN" and "Inf" without an error, and both slip past rate <= 0. Left + // unchecked, one such value reaches the ledger, and from there it is unrecoverable: the total + // is re-read every tick, so NaN + x stays NaN even after the price is fixed. + if err != nil || math.IsNaN(rate) || math.IsInf(rate, 0) || rate <= 0 { return 0, false } return rate, true @@ -204,7 +208,10 @@ func finalRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { // and under-reporting the fleet forever. func costSoFar(nc *nebulav1alpha1.NodeClaim) float64 { total, err := strconv.ParseFloat(nc.Status.EstimatedCostUSD, 64) - if err != nil || total < 0 { + // Non-finite is what makes the zero here load-bearing rather than tidy: a ledger already + // holding "NaN" (see finalRate) would otherwise stay poisoned for the claim's whole life, + // since every later total is derived from this read. + if err != nil || math.IsNaN(total) || math.IsInf(total, 0) || total < 0 { return 0 } return total diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index 4d903ee..a78f261 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -224,6 +224,10 @@ func TestCostAccrual_SkipsNonBilling(t *testing.T) { unbilled("unpriced", "", nebulav1alpha1.NodeClaimBound), unbilled("malformed", "cheap", nebulav1alpha1.NodeClaimBound), unbilled("nonpositive", "0.0000", nebulav1alpha1.NodeClaimBound), + // ParseFloat reads these as valid floats and they outrank every ordering check, so + // without an explicit test the guard against them is one refactor from vanishing. + unbilled("nan", "NaN", nebulav1alpha1.NodeClaimBound), + unbilled("inf", "+Inf", nebulav1alpha1.NodeClaimBound), } a, c := newAccrual(t, claims...) @@ -239,6 +243,29 @@ func TestCostAccrual_SkipsNonBilling(t *testing.T) { } } +// A ledger that already holds a non-finite total recovers on the next tick. Without this the claim +// would stay poisoned for the rest of its life even after the bad price was fixed, because every +// total is derived from reading the previous one back. +func TestCostAccrual_RecoversFromPoisonedLedger(t *testing.T) { + halfHour := 30 * time.Minute + nc := billingClaim("poisoned", "10.0000", &halfHour) + nc.Status.EstimatedCostUSD = "NaN" + a, c := newAccrual(t, nc) + + a.accrueAll(context.Background()) + + want := 10 * 0.5 + total, _ := ledger(t, c, "poisoned") + // Tested for finiteness FIRST: every comparison against NaN is false, so an ordinary + // tolerance check would pass on the very value this test exists to catch. + if math.IsNaN(total) || math.IsInf(total, 0) || math.Abs(total-want) > 1e-3 { + t.Fatalf("status.estimatedCostUSD %v, want %v — the poisoned total must be dropped, not carried", total, want) + } + if got := booked(t); math.IsNaN(got) || math.Abs(got-total) > 1e-9 { + t.Fatalf("booked %v, want %v", got, total) + } +} + // An anchor in the future (a wall-clock jump, a hand-edited field) must not rewind the ledger. func TestCostAccrual_DropsBackwardsWindow(t *testing.T) { ahead := -time.Hour diff --git a/pkg/metrics/cost.go b/pkg/metrics/cost.go index 85edd3b..483b172 100644 --- a/pkg/metrics/cost.go +++ b/pkg/metrics/cost.go @@ -18,6 +18,7 @@ package metrics import ( "fmt" + "math" "github.com/prometheus/client_golang/prometheus" ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -93,7 +94,12 @@ func configureCost(podKeys []string) { func RecordWindow(l Labels, phase string, attribution map[string]string, usd float64) { // A zero window is the ordinary case for a claim whose anchor was just opened, and a // negative one would panic. Neither is worth a series. - if usd <= 0 { + // + // Non-finite is checked separately because it passes every comparison above: a counter cannot + // be decremented, so one NaN added here is permanent, and it spreads — any sum() spanning that + // series is NaN too, taking the whole fleet's cost query with it. Cheaper to refuse here than + // to trust every caller's own parsing (see controller.finalRate). + if usd <= 0 || math.IsNaN(usd) || math.IsInf(usd, 0) { return } values := l.values(append([]string{phase}, attributionValues(attribution)...)...) diff --git a/pkg/metrics/cost_test.go b/pkg/metrics/cost_test.go index c2da3d1..d2d200c 100644 --- a/pkg/metrics/cost_test.go +++ b/pkg/metrics/cost_test.go @@ -77,3 +77,18 @@ func TestRecordWindow_DropsNonPositive(t *testing.T) { t.Fatalf("collected %d series on a non-positive window, want 0", n) } } + +// NaN and Inf pass every ordering comparison, so the non-positive guard does not stop them. A +// counter cannot be decremented: one of these lands permanently, and every sum() over the fleet +// that spans the series reads NaN with it. +func TestRecordWindow_DropsNonFinite(t *testing.T) { + CostTotal.Reset() + + RecordWindow(Labels{Provider: "modal"}, "Bound", nil, 1) + for _, usd := range []float64{math.NaN(), math.Inf(1), math.Inf(-1)} { + RecordWindow(Labels{Provider: "modal"}, "Bound", nil, usd) + } + if got := testutil.ToFloat64(CostTotal); got != 1 { + t.Fatalf("counter reads %v after non-finite windows, want the 1 it legitimately took", got) + } +} diff --git a/pkg/provider/catalog/catalog.go b/pkg/provider/catalog/catalog.go index 382a116..9e709fa 100644 --- a/pkg/provider/catalog/catalog.go +++ b/pkg/provider/catalog/catalog.go @@ -39,6 +39,7 @@ import ( "fmt" "io" "io/fs" + "math" "os" "path/filepath" "strconv" @@ -188,6 +189,12 @@ func parseCSV(r io.Reader) ([]provider.Offering, error) { if err != nil { return nil, fmt.Errorf("row %d: price_per_hour %q: %w", i, field(rec, colPrice), err) } + // ParseFloat takes "NaN" and "Inf" as valid floats. Refused at the boundary because a rate + // is pinned onto the claim and integrated from there — the damage is downstream, in a + // durable ledger and a counter that cannot be corrected, not in this row. + if math.IsNaN(price) || math.IsInf(price, 0) { + return nil, fmt.Errorf("row %d: price_per_hour %q is not a finite number", i, field(rec, colPrice)) + } available, err := strconv.ParseBool(field(rec, colAvailable)) if err != nil { return nil, fmt.Errorf("row %d: available %q: %w", i, field(rec, colAvailable), err) diff --git a/pkg/provider/catalog/catalog_test.go b/pkg/provider/catalog/catalog_test.go index 4c150b9..269a5d4 100644 --- a/pkg/provider/catalog/catalog_test.go +++ b/pkg/provider/catalog/catalog_test.go @@ -145,6 +145,26 @@ func TestLoadFrom_OptionalColumnsAndComments(t *testing.T) { } } +// ParseFloat accepts these as numbers, so only an explicit check keeps them out. Refused at load +// rather than downstream: the rate is pinned onto a claim and integrated from there, into a durable +// ledger and a counter that cannot be corrected afterwards. +func TestLoadFrom_RejectsNonFinitePrice(t *testing.T) { + for _, price := range []string{"NaN", "+Inf", "Infinity", "-Inf"} { + t.Run(price, func(t *testing.T) { + dir := t.TempDir() + csv := "accelerator_type,capacity_type,price_per_hour,available,updated\n" + + "H100,OnDemand," + price + ",true,2026-07-25\n" + if err := os.WriteFile(filepath.Join(dir, "modal.csv"), []byte(csv), 0o644); err != nil { + t.Fatal(err) + } + + if _, err := LoadFrom(dir); err == nil { + t.Fatalf("LoadFrom accepted price_per_hour %q, want an error", price) + } + }) + } +} + func TestLoadFrom_OverrideDir(t *testing.T) { dir := t.TempDir() csv := "accelerator_type,capacity_type,price_per_hour,available,updated\n" + diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index e054169..9832a74 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -357,8 +357,8 @@ func (p *Provider) PricePerHour(req provider.PriceRequest) (float64, error) { metered := data.ModalCPUCostPerHour(req.CPUCores) + data.ModalMemoryCostPerHour(req.MemoryMiB) if req.AcceleratorType == "" { - if metered <= 0 { - return 0, fmt.Errorf("modal: cpu-only sandbox reserves no cpu or memory: %w", provider.ErrNoPrice) + if req.CPUCores <= 0 || req.MemoryMiB <= 0 { + return 0, fmt.Errorf("modal: cpu-only sandbox has an unpriced default cpu or memory reservation: %w", provider.ErrNoPrice) } return metered, nil } From e8eaeff0726f5e151800d1da7b474f7c535fc285 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 17:29:13 +0100 Subject: [PATCH 11/14] fix lint Signed-off-by: kerthcet --- pkg/provider/modal/modal.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/provider/modal/modal.go b/pkg/provider/modal/modal.go index 9832a74..af4bf17 100644 --- a/pkg/provider/modal/modal.go +++ b/pkg/provider/modal/modal.go @@ -358,7 +358,8 @@ func (p *Provider) PricePerHour(req provider.PriceRequest) (float64, error) { if req.AcceleratorType == "" { if req.CPUCores <= 0 || req.MemoryMiB <= 0 { - return 0, fmt.Errorf("modal: cpu-only sandbox has an unpriced default cpu or memory reservation: %w", provider.ErrNoPrice) + return 0, fmt.Errorf("modal: cpu-only sandbox has an unpriced default cpu or memory reservation: %w", + provider.ErrNoPrice) } return metered, nil } From 1366b7875b9cebeddd19208552e6007035567ac6 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 18:14:22 +0100 Subject: [PATCH 12/14] fix increase error for the first metric Signed-off-by: kerthcet --- docs/metrics.md | 32 +++++++++++-- internal/controller/cost_accrual.go | 10 ++++ internal/controller/cost_accrual_test.go | 17 ++++++- internal/controller/nodeclaim_controller.go | 14 +++++- pkg/metrics/cost.go | 27 ++++++++++- pkg/metrics/cost_test.go | 51 +++++++++++++++++++++ 6 files changed, 144 insertions(+), 7 deletions(-) diff --git a/docs/metrics.md b/docs/metrics.md index 651e487..930e9ca 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -145,6 +145,13 @@ cumulative per-claim series needs a sample at each window boundary, and an insta and died between two boundaries has neither, so its spend would be unbillable. Per-claim spend lives on the claim instead (below). +Sharing series across claims narrows that problem rather than removing it, because `increase()` +still needs a sample from *before* the charge and a series' first sample has none. So the moment a +claim becomes chargeable, Nebula publishes its label set at `0` under all three billing phases — +a baseline for the first window each will ever book. What is left is [one scrape +interval](#known-gaps): an instance born and gone inside a single scrape is invisible to +`increase()` however the series are shaped. + **Instance-level** cost — infrastructure spend, blind to the workload on top, charging each claim's whole rate. @@ -310,7 +317,8 @@ count(nebula_cost_usd_total) > 5000 ``` A Pod that carries none of the configured labels reports `none`, the same placeholder every other -absent label uses. +absent label uses. A Pod that carries the label with an *empty* value reports `none` too: an empty +tenant id names no payer, so it is folded into the unattributed pool rather than splitting it. **Changing `--cost-labels` changes the identity of every cost series.** Adding a name resets each one to zero and starts a new set, which `increase()` reads as a reset and handles, but no @@ -339,7 +347,8 @@ Five label values are load-bearing: (unconstrained), no capacity tier (the provider's default), no accelerator (a CPU-only Pod). An explicit token beats an empty string, which in PromQL is indistinguishable from a label that was never set and silently matches `{region=""}` selectors nobody meant to - write. + write. It is a plain word rather than an unforgeable token, which a real value can therefore + collide with — see [Known gaps](#known-gaps). - **`pool`** on the deferral counter is only ever the name of a NodePool that *exists*, or `none`. The pool a Pod asks for is a Pod label — user-controlled and unbounded — so the `no_pool` deferral files `none` rather than the unresolved string; a mislabeled workload @@ -425,6 +434,15 @@ knowing before trusting a dashboard. booking drops that window too. **`status.estimatedCostUSD` is what survives this** — cross-check against it after a redeploy rather than treating the counter as durable. Aggregate as `sum(increase(...))`, never `increase(sum(...))`: only the former sees the per-series reset. +- **An instance shorter than one scrape interval is invisible to `increase()`.** A counter's first + sample carries no information — `increase()` recovers a *rise* between two samples — so dollars + that arrive on a series' first sample are in `nebula_cost_usd_total` but in no `increase()` or + `rate()` query over it. Nebula heads this off by publishing every chargeable claim's label set at + `0` when the window opens (see [Cost](#cost)), which covers any claim that survives a scrape; a + claim whose baseline and whole spend land between two scrapes is not covered, and with + `--cost-labels` on — where the series is one tenant's — that tenant's entire bill can read as zero. + Cross-check against `status.estimatedCostUSD` and the `claim finalized` log, which are the durable + records. Closing it properly means a durable per-window event stream, not a counter. - **`EST_COST` understates a reclaimed instance; the metric does not.** The field is not written on the deletion path (the object is going away), so it misses the window still open at teardown — which the counter *does* book (see [Cost](#cost)). The two therefore disagree by up to one @@ -450,11 +468,19 @@ knowing before trusting a dashboard. `org_id` when the claim became chargeable is booked to `none` for its whole life; labelling it afterwards does not backfill, by design (see [Attribution](#attribution)). Enforce the labels at admission — a webhook or a policy engine — rather than trusting the metric to notice. +- **`none` is a placeholder a real value can forge.** A Pod whose attribution label reads literally + `none` shares a series with every Pod that carries no such label, merging that tenant's spend into + the unattributed pool. The alternatives were each worse in a way that mattered more: an unforgeable + token (``) is unreadable and vanishes wherever a label value reaches HTML, and the empty + string is what PromQL uses for "label absent", so `{org_id=""}` could no longer select the + unattributed bucket on its own. Pick attribution keys whose values your admission policy + constrains. - **Attribution cardinality is unbounded.** Enabling `--cost-labels` promotes tenant-controlled values onto a counter that never releases a series, and nothing caps it: a bad label choice grows manager memory and the scrape payload until a restart. The 5000-series warning is a smoke alarm, not a limit — see [Attribution](#attribution) for why it does not enforce, and alert on - `count(nebula_cost_usd_total)`. + `count(nebula_cost_usd_total)`. Budget three series per distinct label set, not one: the baselines + above are published for all three billing phases, and a claim only ever spends under two of them. - **`instance_ready_duration` under-samples slow boots.** The start timestamp lives only in the virtual node's in-memory tracking map, so a provision still in flight when the manager restarts is re-adopted without one and is never observed. A missing sample beats diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index c0d4a00..f94b4b9 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -188,6 +188,16 @@ func billingRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { return finalRate(nc) } +// billingPhases is every phase a window can be booked under: the two billingRate admits, plus +// Terminated for the last window settleFinalCost closes. Named here so metrics.TouchSeries can +// publish all three baselines up front — one of them stays empty for the whole claim, since a +// reclaimed instance passes through Terminating and a self-terminated one does not. +var billingPhases = []string{ + string(nebulav1alpha1.NodeClaimBound), + string(nebulav1alpha1.NodeClaimTerminating), + string(nebulav1alpha1.NodeClaimTerminated), +} + // finalRate is billingRate without the phase gate. Terminated is excluded there because a claim // sits in that phase until its Pod is deleted and charging those idle hours would be wrong — but the // window between the last checkpoint and the instance's death is real, and settleFinalCost is the diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index a78f261..c848050 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -586,6 +586,15 @@ func TestMarkPhase_StampsAttributionThatIsThenBooked(t *testing.T) { if got.Status.LastAccruedAt == nil { t.Fatal("no accrual anchor was opened; the window before the first checkpoint is lost") } + // Published with the anchor and BEFORE any charge, which is the only ordering that makes the + // first window differenceable — one series per billing phase, all still at zero. + if n := testutil.CollectAndCount(metrics.CostTotal); n != len(billingPhases) { + t.Fatalf("collected %d series after the window opened, want %d baselines", n, len(billingPhases)) + } + bound := metrics.CostTotal.WithLabelValues("fake", "us-east-1", "OnDemand", "H100", "1", "Bound", "acme") + if got := testutil.ToFloat64(bound); got != 0 { + t.Fatalf("the Bound baseline holds %v, want 0 — a baseline may not invent spend", got) + } // Everything is settled, so a second pass over the same Pod must write nothing. reconcileClaim(t, r, "c1") @@ -601,12 +610,18 @@ func TestMarkPhase_StampsAttributionThatIsThenBooked(t *testing.T) { } NewCostAccrual(c).accrueAll(context.Background()) - // The exposition format is line-oriented, so this cannot be wrapped. + // The two zeros are the baselines the reconcile published for the phases this claim has not + // reached yet (see metrics.TouchSeries): without them, the first window booked under either would + // be a series' first sample and invisible to increase(). + // + // The exposition format is line-oriented, so these cannot be wrapped. //nolint:lll wantSeries := ` # HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. # TYPE nebula_cost_usd_total counter nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Bound",provider="fake",region="us-east-1"} 10 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Terminated",provider="fake",region="us-east-1"} 0 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Terminating",provider="fake",region="us-east-1"} 0 ` if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(wantSeries)); err != nil { t.Fatal(err) diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index 05dd59b..1757fe9 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" + "github.com/InftyAI/Nebula/pkg/metrics" "github.com/InftyAI/Nebula/pkg/provider" "github.com/InftyAI/Nebula/pkg/util" ) @@ -336,13 +337,22 @@ func (r *NodeClaimReconciler) markPhase(ctx context.Context, nc *nebulav1alpha1. // Last, so it sees the phase and price the two calls above just set: the window opens the // moment those make the claim chargeable — and in the same patch as the attribution above, // so no window is ever charged before we know who to charge it to. - if stampAccrualStart(nc) { + opened := stampAccrualStart(nc) + if opened { changed = true } if !changed { return nil } - return r.patchStatus(ctx, nc) + if err := r.patchStatus(ctx, nc); err != nil { + return err + } + if opened { + // Only once the attribution this reads is durable, and only on the transition, so a requeue + // cannot re-touch: the zero is harmless to repeat, but the cardinality warning counts it. + metrics.TouchSeries(claimLabels(nc), nc.Status.CostLabels, billingPhases...) + } + return nil } // recordInstanceID copies the instance id off the served Pod into status.InstanceID when it diff --git a/pkg/metrics/cost.go b/pkg/metrics/cost.go index 483b172..656f82f 100644 --- a/pkg/metrics/cost.go +++ b/pkg/metrics/cost.go @@ -26,7 +26,8 @@ import ( // CostTotal accumulates spend one CLOSED WINDOW at a time, which is what makes it billable: // increase(...[w]) over any window is a pure function of that window, so a consumer replaying -// an old window re-derives the same dollars and can upsert them idempotently. +// an old window re-derives the same dollars and can upsert them idempotently. That holds only for a +// series scraped before its first charge, which is what TouchSeries is for. // // Deliberately carries no claim identity. A per-claim series would churn — one per instance ever // created, retained until the process exits — and worse, a claim that lived and died between two @@ -106,3 +107,27 @@ func RecordWindow(l Labels, phase string, attribution map[string]string, usd flo noteSeries(values) CostTotal.WithLabelValues(values...).Add(usd) } + +// TouchSeries publishes one claim's label set as a zero-valued series under each of phases, so the +// first charge booked there has an earlier sample to be differenced against. +// +// increase() recovers a RISE between two samples, so a series whose very first sample already holds +// money reads as no rise at all: those dollars are in the counter's absolute value but not in any +// increase()/rate() query, which is what a billing consumer runs. Sharing series across claims +// usually hides that — but attribution makes them tenant-scoped, and a tenant whose whole usage is +// one short job would be billed nothing. +// +// This does not contradict RecordWindow's refusal of zeros. A zero WINDOW is a measurement claiming +// something cost nothing; a zero COUNTER only says nothing has been charged here yet, and publishing +// it is the ordinary way to make rate() work over label values not known until runtime. +// +// Call it when the claim becomes chargeable, one window ahead of its first charge. That still needs +// a scrape to land in between, so an instance born and gone inside one scrape interval is beyond +// help — see docs/metrics.md. +func TouchSeries(l Labels, attribution map[string]string, phases ...string) { + for _, phase := range phases { + values := l.values(append([]string{phase}, attributionValues(attribution)...)...) + noteSeries(values) + CostTotal.WithLabelValues(values...).Add(0) + } +} diff --git a/pkg/metrics/cost_test.go b/pkg/metrics/cost_test.go index d2d200c..722e4dc 100644 --- a/pkg/metrics/cost_test.go +++ b/pkg/metrics/cost_test.go @@ -78,6 +78,57 @@ func TestRecordWindow_DropsNonPositive(t *testing.T) { } } +// The baseline exists so increase() has an earlier sample to difference against, which means it must +// be a series that COLLECTS at zero — not merely a child object the counter knows about. +func TestTouchSeries_PublishesAZeroBaseline(t *testing.T) { + CostTotal.Reset() + + l := Labels{Provider: "modal", Accelerator: "H100", AcceleratorCount: 1} + TouchSeries(l, nil, "Bound", "Terminating", "Terminated") + + // The exposition format is line-oriented, so these cannot be wrapped. + //nolint:lll + want := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Bound",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Terminated",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Terminating",provider="modal",region="none"} 0 +` + if err := testutil.CollectAndCompare(CostTotal, strings.NewReader(want)); err != nil { + t.Fatal(err) + } +} + +// A baseline must not displace the charge that follows it, on the series it opened or on a repeat +// touch — the whole point is that the first real window still reads as a rise from zero. +func TestTouchSeries_DoesNotDisplaceTheFirstCharge(t *testing.T) { + CostTotal.Reset() + + l := Labels{Provider: "modal"} + TouchSeries(l, nil, "Bound") + RecordWindow(l, "Bound", nil, 3.95) + TouchSeries(l, nil, "Bound") + + if got := testutil.ToFloat64(CostTotal.WithLabelValues(l.values("Bound")...)); got != 3.95 { + t.Fatalf("series reads %v, want 3.95", got) + } +} + +// The baseline is what makes a tenant-scoped series billable, so it has to land on the SAME series +// the charge will — attribution and all, or it buys nothing. +func TestTouchSeries_Attribution(t *testing.T) { + withAttribution(t, "org_id") + + l := Labels{Provider: "modal"} + TouchSeries(l, map[string]string{"org_id": "acme"}, "Bound") + RecordWindow(l, "Bound", map[string]string{"org_id": "acme"}, 1) + + if n := testutil.CollectAndCount(CostTotal); n != 1 { + t.Fatalf("collected %d series, want 1 — the baseline and the charge must share one", n) + } +} + // NaN and Inf pass every ordering comparison, so the non-positive guard does not stop them. A // counter cannot be decremented: one of these lands permanently, and every sum() over the fleet // that spans the series reads NaN with it. From ec9b44b7f53e4e84a397401eef0a0f6b21c282ba Mon Sep 17 00:00:00 2001 From: Kante Yin Date: Thu, 3 Sep 2026 18:49:03 +0100 Subject: [PATCH 13/14] Add nullable validation for CostLabels field Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- api/v1alpha1/nodeclaim_types.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index b8bbe70..0f36a84 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -211,6 +211,7 @@ type NodeClaimStatus struct { // write, so every reconcile would re-read the Pod and a label added later could still move // the claim's attribution. // +optional + // +kubebuilder:validation:Nullable CostLabels map[string]string `json:"costLabels"` } From 8e98269940ef5e2ea95ddfe3c62ed8328f1f147b Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 3 Sep 2026 19:08:45 +0100 Subject: [PATCH 14/14] fix test Signed-off-by: kerthcet --- docs/metrics.md | 39 +++++---- internal/controller/cost_accrual.go | 51 ++++++++++- internal/controller/cost_accrual_test.go | 93 +++++++++++++++++---- internal/controller/nodeclaim_controller.go | 14 +--- pkg/metrics/cost.go | 10 ++- 5 files changed, 157 insertions(+), 50 deletions(-) diff --git a/docs/metrics.md b/docs/metrics.md index 930e9ca..8b74572 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -146,11 +146,16 @@ and died between two boundaries has neither, so its spend would be unbillable. P lives on the claim instead (below). Sharing series across claims narrows that problem rather than removing it, because `increase()` -still needs a sample from *before* the charge and a series' first sample has none. So the moment a -claim becomes chargeable, Nebula publishes its label set at `0` under all three billing phases — -a baseline for the first window each will ever book. What is left is [one scrape -interval](#known-gaps): an instance born and gone inside a single scrape is invisible to -`increase()` however the series are shaped. +still needs a sample from *before* the charge and a series' first sample has none. Counter series are +also **process-local**: a redeploy or a leader handoff starts the new process with none of them. So +when the accrual loop starts — which under leader election is the moment this process starts charging +— it publishes every already-anchored, priced claim's label set at `0` under all three billing +phases, a baseline for the first window any of them will book. The seeding pass is one interval ahead +of the first tick, so a scrape lands in between. + +That covers the fleet across a restart, which is where the money is. It does **not** cover a label +set appearing *after* the pass — a shape or a tenant seen for the first time mid-process — whose +first window is still booked onto a newborn series. See [Known gaps](#known-gaps). **Instance-level** cost — infrastructure spend, blind to the workload on top, charging each claim's whole rate. @@ -434,15 +439,21 @@ knowing before trusting a dashboard. booking drops that window too. **`status.estimatedCostUSD` is what survives this** — cross-check against it after a redeploy rather than treating the counter as durable. Aggregate as `sum(increase(...))`, never `increase(sum(...))`: only the former sees the per-series reset. -- **An instance shorter than one scrape interval is invisible to `increase()`.** A counter's first - sample carries no information — `increase()` recovers a *rise* between two samples — so dollars - that arrive on a series' first sample are in `nebula_cost_usd_total` but in no `increase()` or - `rate()` query over it. Nebula heads this off by publishing every chargeable claim's label set at - `0` when the window opens (see [Cost](#cost)), which covers any claim that survives a scrape; a - claim whose baseline and whole spend land between two scrapes is not covered, and with - `--cost-labels` on — where the series is one tenant's — that tenant's entire bill can read as zero. +- **A series' first charge is invisible to `increase()`.** A counter's first sample carries no + information — `increase()` recovers a *rise* between two samples — so dollars that arrive on a + series' first sample are in `nebula_cost_usd_total` but in no `increase()` or `rate()` query over + it. The baselines seeded when the accrual loop starts (see [Cost](#cost)) cover every claim already + billing, so a restart does not cost the fleet a window. Two cases remain, both of them worse with + `--cost-labels` on, where a series belongs to one tenant rather than a whole shape: + - a **label set first seen mid-process** — a new tenant, or a shape nothing was running on at + startup — gets no baseline at all, so its first window is lost to `increase()` for the life of + that process. Nothing re-seeds; the accrual loop seeds once. + - an **instance born and gone inside one scrape interval**, which no baseline can help: the + baseline and the charge land in the same scrape either way. + + Both bias toward *undercounting*, and a tenant whose only job is short can read as zero spend. Cross-check against `status.estimatedCostUSD` and the `claim finalized` log, which are the durable - records. Closing it properly means a durable per-window event stream, not a counter. + records. Closing this properly means a durable per-window event stream, not a counter. - **`EST_COST` understates a reclaimed instance; the metric does not.** The field is not written on the deletion path (the object is going away), so it misses the window still open at teardown — which the counter *does* book (see [Cost](#cost)). The two therefore disagree by up to one @@ -480,7 +491,7 @@ knowing before trusting a dashboard. manager memory and the scrape payload until a restart. The 5000-series warning is a smoke alarm, not a limit — see [Attribution](#attribution) for why it does not enforce, and alert on `count(nebula_cost_usd_total)`. Budget three series per distinct label set, not one: the baselines - above are published for all three billing phases, and a claim only ever spends under two of them. + above are seeded for all three billing phases, and a claim only ever spends under two of them. - **`instance_ready_duration` under-samples slow boots.** The start timestamp lives only in the virtual node's in-memory tracking map, so a provision still in flight when the manager restarts is re-adopted without one and is never observed. A missing sample beats diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index f94b4b9..87f6c1f 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -83,6 +83,8 @@ func (a *CostAccrual) Start(ctx context.Context) error { log := logf.Log.WithName("cost-accrual") log.Info("starting cost accrual", "interval", a.interval) + a.seedBaselines(ctx) + ticker := time.NewTicker(a.interval) defer ticker.Stop() @@ -96,6 +98,46 @@ func (a *CostAccrual) Start(ctx context.Context) error { } } +// seedBaselines publishes a zero-valued series for every claim that was already chargeable when +// this process took over, before the first tick can charge one. +// +// Counter series are process-local, so without this the first post-restart window of the WHOLE FLEET +// lands on a newborn series, which increase() cannot difference (see metrics.TouchSeries). Prometheus +// recovers some of that by reset detection, but only for an unchanged scrape target: a redeploy's new +// Pod carries a new instance label, and a handoff to another replica is a different target entirely. +// +// Its position in Start is both the once-per-process guard and the ordering that matters — leader +// election makes it the moment this process starts charging, and the first tick is an interval later, +// so a scrape lands between a baseline and the window it explains. A claim that becomes chargeable +// after this pass gets no baseline; see docs/metrics.md, Known gaps. +// +// Best-effort: a failed List costs one window's visibility, never the window itself, which the anchor +// still carries. +func (a *CostAccrual) seedBaselines(ctx context.Context) { + log := logf.Log.WithName("cost-accrual") + + ctx, cancel := context.WithTimeout(ctx, accrualTimeout) + defer cancel() + + var claims nebulav1alpha1.NodeClaimList + if err := a.List(ctx, &claims); err != nil { + log.Error(err, "listing nodeclaims to seed cost baselines") + return + } + seeded := 0 + for i := range claims.Items { + nc := &claims.Items[i] + // finalRate rather than billingRate: a Terminated claim still books a window when its Pod + // is finally deleted (see settleFinalCost), so it needs a baseline as much as a live one. + if _, ok := finalRate(nc); !ok || nc.Status.LastAccruedAt == nil { + continue + } + metrics.TouchSeries(claimLabels(nc), nc.Status.CostLabels, billingPhases...) + seeded++ + } + log.Info("seeded cost baselines", "claims", seeded) +} + // accrueAll checkpoints every billing claim once. Errors are logged per claim and never // propagated: one unwritable claim must not cost the rest of the fleet its window. func (a *CostAccrual) accrueAll(ctx context.Context) { @@ -189,9 +231,9 @@ func billingRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { } // billingPhases is every phase a window can be booked under: the two billingRate admits, plus -// Terminated for the last window settleFinalCost closes. Named here so metrics.TouchSeries can -// publish all three baselines up front — one of them stays empty for the whole claim, since a -// reclaimed instance passes through Terminating and a self-terminated one does not. +// Terminated for the last window settleFinalCost closes. seedBaselines publishes all three up front +// rather than guessing: one always stays empty, since a reclaimed instance passes through Terminating +// and a self-terminated one does not, and which of the two it will be is not knowable in advance. var billingPhases = []string{ string(nebulav1alpha1.NodeClaimBound), string(nebulav1alpha1.NodeClaimTerminating), @@ -201,7 +243,8 @@ var billingPhases = []string{ // finalRate is billingRate without the phase gate. Terminated is excluded there because a claim // sits in that phase until its Pod is deleted and charging those idle hours would be wrong — but the // window between the last checkpoint and the instance's death is real, and settleFinalCost is the -// only place left that can book it. Nothing else may use this. +// only place left that can book it. Nothing may use this to decide that a claim ACCRUES; seedBaselines +// is the one other caller, and it only asks whether a window could ever be booked. func finalRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { rate, err := strconv.ParseFloat(nc.Status.PriceUSDPerHour, 64) // ParseFloat accepts "NaN" and "Inf" without an error, and both slip past rate <= 0. Left diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index c848050..6a73f30 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -586,16 +586,6 @@ func TestMarkPhase_StampsAttributionThatIsThenBooked(t *testing.T) { if got.Status.LastAccruedAt == nil { t.Fatal("no accrual anchor was opened; the window before the first checkpoint is lost") } - // Published with the anchor and BEFORE any charge, which is the only ordering that makes the - // first window differenceable — one series per billing phase, all still at zero. - if n := testutil.CollectAndCount(metrics.CostTotal); n != len(billingPhases) { - t.Fatalf("collected %d series after the window opened, want %d baselines", n, len(billingPhases)) - } - bound := metrics.CostTotal.WithLabelValues("fake", "us-east-1", "OnDemand", "H100", "1", "Bound", "acme") - if got := testutil.ToFloat64(bound); got != 0 { - t.Fatalf("the Bound baseline holds %v, want 0 — a baseline may not invent spend", got) - } - // Everything is settled, so a second pass over the same Pod must write nothing. reconcileClaim(t, r, "c1") if writes != 1 { @@ -610,24 +600,95 @@ func TestMarkPhase_StampsAttributionThatIsThenBooked(t *testing.T) { } NewCostAccrual(c).accrueAll(context.Background()) - // The two zeros are the baselines the reconcile published for the phases this claim has not - // reached yet (see metrics.TouchSeries): without them, the first window booked under either would - // be a series' first sample and invisible to increase(). + // No baseline anywhere: accrueAll is driven directly, so nothing here has seeded one. That is the + // gap this claim would fall into if it had been born mid-process — see seedBaselines. // - // The exposition format is line-oriented, so these cannot be wrapped. + // The exposition format is line-oriented, so this cannot be wrapped. //nolint:lll wantSeries := ` # HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. # TYPE nebula_cost_usd_total counter nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Bound",provider="fake",region="us-east-1"} 10 -nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Terminated",provider="fake",region="us-east-1"} 0 -nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="OnDemand",example_com_org_id="acme",phase="Terminating",provider="fake",region="us-east-1"} 0 ` if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(wantSeries)); err != nil { t.Fatal(err) } } +// A restart throws away every series markPhase baselined when these claims were born, and it will +// not run again for a claim that is already anchored. Seeding is what stops the whole fleet's first +// post-restart window from landing on newborn series that increase() cannot difference. +func TestSeedBaselines(t *testing.T) { + hour := time.Hour + + // Every claim gets a shape of its own, or a claim wrongly skipped would hide behind the series + // another one seeded — only "same-shape" shares deliberately. + shape := billingClaim("same-shape", "10.0000", &hour) + // Anchored and priced, so its last window is still booked at teardown: it needs a baseline as + // much as a running claim does, which is why seeding asks finalRate rather than billingRate. + terminated := billingClaim("terminated", "10.0000", &hour) + terminated.Status.Phase = nebulav1alpha1.NodeClaimTerminated + terminated.Spec.Accelerator = "A100:2" + unanchored := billingClaim("unanchored", "10.0000", nil) + unanchored.Spec.Accelerator = "L4:1" + unpriced := billingClaim("unpriced", "", &hour) + unpriced.Spec.Accelerator = "T4:1" + cpu := billingClaim("cpu", "0.1000", &hour) + cpu.Spec.Accelerator = "" + + a, _ := newAccrual(t, billingClaim("bound", "10.0000", &hour), shape, terminated, unanchored, unpriced, cpu) + a.seedBaselines(context.Background()) + + // Three shapes, three phases each. The two claims sharing H100:1 collapse onto one set of series + // — a baseline is per SERIES, not per claim — while L4 and T4 are absent entirely: no window can + // ever be booked for a claim with no anchor or no price, so a baseline would be an empty promise. + // + // The exposition format is line-oriented, so these cannot be wrapped. + //nolint:lll + want := ` +# HELP nebula_cost_usd_total Cumulative USD billed by external instances, added window by window as it accrues. +# TYPE nebula_cost_usd_total counter +nebula_cost_usd_total{accelerator="A100",accelerator_count="2",capacity_type="none",phase="Bound",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="A100",accelerator_count="2",capacity_type="none",phase="Terminated",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="A100",accelerator_count="2",capacity_type="none",phase="Terminating",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Bound",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Terminated",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="H100",accelerator_count="1",capacity_type="none",phase="Terminating",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="none",accelerator_count="none",capacity_type="none",phase="Bound",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="none",accelerator_count="none",capacity_type="none",phase="Terminated",provider="modal",region="none"} 0 +nebula_cost_usd_total{accelerator="none",accelerator_count="none",capacity_type="none",phase="Terminating",provider="modal",region="none"} 0 +` + if err := testutil.CollectAndCompare(metrics.CostTotal, strings.NewReader(want)); err != nil { + t.Fatal(err) + } +} + +// Seeding must land BEFORE the first tick, or it baselines a series the same tick already charged +// and buys nothing. Only Start guarantees that ordering, so it is asserted through Start — with a +// claim whose whole hour is chargeable, so a tick that beat the seed would show up as a value. +func TestStart_SeedsBeforeCharging(t *testing.T) { + hour := time.Hour + a, _ := newAccrual(t, billingClaim("bound", "10.0000", &hour)) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { _ = a.Start(ctx) }() + + // a.interval is a minute, so anything observed within seconds is the seed, not a tick. + deadline := time.Now().Add(5 * time.Second) + for testutil.CollectAndCount(metrics.CostTotal) < len(billingPhases) { + if time.Now().After(deadline) { + t.Fatalf("collected %d series, want %d baselines seeded on the way to the first tick", + testutil.CollectAndCount(metrics.CostTotal), len(billingPhases)) + } + time.Sleep(10 * time.Millisecond) + } + if got := testutil.ToFloat64(metrics.CostTotal.WithLabelValues( + "modal", "none", "none", "H100", "1", "Bound")); got != 0 { + t.Fatalf("the Bound baseline holds %v, want 0 — seeding may not charge anything", got) + } +} + // The stamp is what makes a crash before the first checkpoint lossless, so it must land exactly // when the claim becomes chargeable — and never move once set. func TestStampAccrualStart(t *testing.T) { diff --git a/internal/controller/nodeclaim_controller.go b/internal/controller/nodeclaim_controller.go index 1757fe9..05dd59b 100644 --- a/internal/controller/nodeclaim_controller.go +++ b/internal/controller/nodeclaim_controller.go @@ -39,7 +39,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" nebulav1alpha1 "github.com/InftyAI/Nebula/api/v1alpha1" - "github.com/InftyAI/Nebula/pkg/metrics" "github.com/InftyAI/Nebula/pkg/provider" "github.com/InftyAI/Nebula/pkg/util" ) @@ -337,22 +336,13 @@ func (r *NodeClaimReconciler) markPhase(ctx context.Context, nc *nebulav1alpha1. // Last, so it sees the phase and price the two calls above just set: the window opens the // moment those make the claim chargeable — and in the same patch as the attribution above, // so no window is ever charged before we know who to charge it to. - opened := stampAccrualStart(nc) - if opened { + if stampAccrualStart(nc) { changed = true } if !changed { return nil } - if err := r.patchStatus(ctx, nc); err != nil { - return err - } - if opened { - // Only once the attribution this reads is durable, and only on the transition, so a requeue - // cannot re-touch: the zero is harmless to repeat, but the cardinality warning counts it. - metrics.TouchSeries(claimLabels(nc), nc.Status.CostLabels, billingPhases...) - } - return nil + return r.patchStatus(ctx, nc) } // recordInstanceID copies the instance id off the served Pod into status.InstanceID when it diff --git a/pkg/metrics/cost.go b/pkg/metrics/cost.go index 656f82f..c1b68d8 100644 --- a/pkg/metrics/cost.go +++ b/pkg/metrics/cost.go @@ -27,7 +27,8 @@ import ( // CostTotal accumulates spend one CLOSED WINDOW at a time, which is what makes it billable: // increase(...[w]) over any window is a pure function of that window, so a consumer replaying // an old window re-derives the same dollars and can upsert them idempotently. That holds only for a -// series scraped before its first charge, which is what TouchSeries is for. +// series scraped before its first charge, which is what TouchSeries is for — and not at all for one +// born after that pass has run. // // Deliberately carries no claim identity. A per-claim series would churn — one per instance ever // created, retained until the process exits — and worse, a claim that lived and died between two @@ -121,9 +122,10 @@ func RecordWindow(l Labels, phase string, attribution map[string]string, usd flo // something cost nothing; a zero COUNTER only says nothing has been charged here yet, and publishing // it is the ordinary way to make rate() work over label values not known until runtime. // -// Call it when the claim becomes chargeable, one window ahead of its first charge. That still needs -// a scrape to land in between, so an instance born and gone inside one scrape interval is beyond -// help — see docs/metrics.md. +// Series are PROCESS-local, so the baseline has to be republished by whatever process is doing the +// charging — see controller.CostAccrual.seedBaselines, which is the one caller. A scrape still has to +// land between the baseline and the charge, so a claim that becomes chargeable after the seeding pass +// is not covered at all; see docs/metrics.md. func TouchSeries(l Labels, attribution map[string]string, phases ...string) { for _, phase := range phases { values := l.values(append([]string{phase}, attributionValues(attribution)...)...)