From 6864e0ac7a12e1a8c59a55a05f086cc4683e922a Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 10 Sep 2026 18:10:29 +0100 Subject: [PATCH 1/2] fix cost leak issue Signed-off-by: kerthcet --- api/v1alpha1/nodeclaim_types.go | 6 + api/v1alpha1/zz_generated.deepcopy.go | 4 + .../bases/nebula.inftyai.com_nodeclaims.yaml | 7 + docs/metrics.md | 5 + internal/controller/cost_accrual.go | 52 ++++-- internal/controller/cost_accrual_test.go | 167 +++++++++++++++++- 6 files changed, 218 insertions(+), 23 deletions(-) diff --git a/api/v1alpha1/nodeclaim_types.go b/api/v1alpha1/nodeclaim_types.go index 0f36a84..3e2acb9 100644 --- a/api/v1alpha1/nodeclaim_types.go +++ b/api/v1alpha1/nodeclaim_types.go @@ -173,6 +173,12 @@ type NodeClaimStatus struct { // +optional EstimatedCostUSD string `json:"estimatedCostUSD,omitempty"` + // ProvisionedAt is when this claim was first observed holding a chargeable instance — the + // instant billing began. Written once, in the same patch that opens LastAccruedAt, and never + // refreshed afterwards. + // +optional + ProvisionedAt *metav1.Time `json:"provisionedAt,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. diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index bdd1f95..19a3ad7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -140,6 +140,10 @@ 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.ProvisionedAt != nil { + in, out := &in.ProvisionedAt, &out.ProvisionedAt + *out = (*in).DeepCopy() + } if in.LastAccruedAt != nil { in, out := &in.LastAccruedAt, &out.LastAccruedAt *out = (*in).DeepCopy() diff --git a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml index e039efe..d2bcef2 100644 --- a/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml +++ b/config/crd/bases/nebula.inftyai.com_nodeclaims.yaml @@ -241,6 +241,13 @@ spec: 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 + provisionedAt: + description: |- + ProvisionedAt is when this claim was first observed holding a chargeable instance — the + instant billing began. Written once, in the same patch that opens LastAccruedAt, and never + refreshed afterwards. + format: date-time + type: string type: object type: object served: true diff --git a/docs/metrics.md b/docs/metrics.md index d11f6b7..4e33cda 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -212,6 +212,11 @@ provider's billing export before anyone gets invoiced. `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. +`Terminating` bills only to *continue* a window that was already open, which `status.provisionedAt` +records — the instant a claim was first seen holding a chargeable instance. Deleting a Pod promotes +its claim straight from `Provisioning` to `Terminating`, so without that field the teardown of an +instance that never existed would be charged at the full GPU rate. + 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 diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index e266fa6..becfd2e 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -149,16 +149,22 @@ func (a *CostAccrual) accrue(ctx context.Context, nc *nebulav1alpha1.NodeClaim) // never drift. now := a.now().Truncate(time.Second) + // No anchor: nothing is counting yet, and this loop is deliberately not what starts it. + // stampAccrualStart is the only opener, and it rides the patch that makes a claim Bound — + // markPhase being the only writer of status.phase, and one that requeues if that patch fails, + // so a billable claim reaching this line without an anchor has no path left to produce it. + // Opening one here would also have to invent ProvisionedAt, which is what refuses to bill a + // teardown that never billed (see billingRate). + at := nc.Status.LastAccruedAt + if at == nil { + return nil + } // 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) { + if !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) @@ -179,13 +185,19 @@ func (a *CostAccrual) accrue(ctx context.Context, nc *nebulav1alpha1.NodeClaim) // 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. +// Terminating bills only to CONTINUE what was already being charged, which is what +// ProvisionedAt records — without it, deleting a Provisioning claim opens its FIRST window at +// teardown (see desiredPhase) and charges the reclaim of an instance that never existed. +// 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. +// +// Bound cannot require ProvisionedAt: it is the phase that creates it (see stampAccrualStart). func billingRate(nc *nebulav1alpha1.NodeClaim) (float64, bool) { - switch nc.Status.Phase { - case nebulav1alpha1.NodeClaimBound, nebulav1alpha1.NodeClaimTerminating: + switch { + case nc.Status.Phase == nebulav1alpha1.NodeClaimBound, + nc.Status.Phase == nebulav1alpha1.NodeClaimTerminating && nc.Status.ProvisionedAt != nil: default: return 0, false } @@ -253,10 +265,20 @@ func formatCost(usd float64) string { // 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. +// The ONLY opener: accrue moves an anchor but never creates one, so a claim this misses bills +// nothing for its whole life, not merely late. Safe to concentrate here because markPhase is the +// only writer of status.phase and requeues on a failed patch, so every claim that reaches a +// billing phase has had this run. Free, too — it rides the status patch markPhase is already +// making, which is also what makes a crash before the first checkpoint lossless: the anchor is +// what recovery measures the window from. +// +// Both stamps or neither, one patch: ProvisionedAt is what later refuses to bill a teardown that +// was never billing (see billingRate), so a claim must never carry an anchor without it. +// +// With no anchor yet, billingRate admits Bound alone, which is the gate this needs and does not +// restate. What that gives up is a claim whose Pod is deleted between two reconciles, only ever +// observed Terminating, and then charged nothing rather than for its brief real life — bounded by +// one reconcile gap, and absent spend is the honest direction. func stampAccrualStart(nc *nebulav1alpha1.NodeClaim) bool { if nc.Status.LastAccruedAt != nil { return false @@ -264,7 +286,9 @@ func stampAccrualStart(nc *nebulav1alpha1.NodeClaim) bool { if _, ok := billingRate(nc); !ok { return false } - nc.Status.LastAccruedAt = &metav1.Time{Time: time.Now().Truncate(time.Second)} + at := time.Now().Truncate(time.Second) + nc.Status.ProvisionedAt = &metav1.Time{Time: at} + nc.Status.LastAccruedAt = &metav1.Time{Time: at} return true } diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index b955091..f70dcea 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -44,7 +44,8 @@ import ( ) // billingClaim is a claim holding a priced instance, anchored ago in the past. A nil ago leaves -// the anchor unset, i.e. never checkpointed. +// the anchor unset, i.e. never checkpointed — and with it ProvisionedAt, which is written in the +// same patch and so never exists without one. func billingClaim(name, price string, ago *time.Duration) *nebulav1alpha1.NodeClaim { nc := &nebulav1alpha1.NodeClaim{ ObjectMeta: metav1.ObjectMeta{Name: name}, @@ -55,7 +56,9 @@ func billingClaim(name, price string, ago *time.Duration) *nebulav1alpha1.NodeCl }, } if ago != nil { - nc.Status.LastAccruedAt = &metav1.Time{Time: time.Now().Add(-*ago).Truncate(time.Second)} + at := metav1.Time{Time: time.Now().Add(-*ago).Truncate(time.Second)} + nc.Status.ProvisionedAt = &at + nc.Status.LastAccruedAt = at.DeepCopy() } return nc } @@ -95,6 +98,16 @@ func ledger(t *testing.T, c client.Client, name string) (float64, *metav1.Time) return total, nc.Status.LastAccruedAt } +// provisioned reads back the durable record that this claim ever billed. +func provisioned(t *testing.T, c client.Client, name string) *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) + } + return nc.Status.ProvisionedAt +} + // booked is the dollars the cost counter holds across every series. func booked(t *testing.T) float64 { t.Helper() @@ -193,19 +206,26 @@ func TestCostAccrual_ChargesCheapClaimsAtTheirRealRate(t *testing.T) { } } -// 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 billable claim with no anchor is left entirely alone: the window before this tick has an +// unknown length, so charging it would invent spend, and opening one would have to invent +// ProvisionedAt too — the field that decides whether teardown is billable. stampAccrualStart is +// the only opener; nothing here may quietly become a second one. +func TestCostAccrual_LeavesAnUnanchoredClaimAlone(t *testing.T) { a, c := newAccrual(t, billingClaim("bound", "98.3200", nil)) + a.accrueAll(context.Background()) + a.now = func() time.Time { return time.Now().Add(accrualInterval) } 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 anchor != nil { + t.Fatalf("an anchor was opened outside markPhase: %v", anchor) + } + if at := provisioned(t, c, "bound"); at != nil { + t.Fatalf("status.provisionedAt %v was invented from a tick", at) } if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { t.Fatalf("collected %d series, want 0 — nothing was charged", n) @@ -249,6 +269,57 @@ func TestCostAccrual_SkipsNonBilling(t *testing.T) { } } +// The loop opens an anchor for anything it considers billing, so Terminating has to be refused +// unless status.provisionedAt says something was already being charged — otherwise the tick that +// finds a never-billed claim on its way out opens its FIRST window there and every tick after that +// charges a full interval, at the GPU rate, for an instance that never ran. Two ticks, because the +// first only stamps: the charge this guards against appears on the second. +func TestCostAccrual_TerminatingWithoutAWindowNeverStarts(t *testing.T) { + nc := billingClaim("terminating", "98.3200", nil) + nc.Status.Phase = nebulav1alpha1.NodeClaimTerminating + a, c := newAccrual(t, nc) + + a.accrueAll(context.Background()) + a.now = func() time.Time { return time.Now().Add(accrualInterval) } + a.accrueAll(context.Background()) + + total, anchor := ledger(t, c, "terminating") + if anchor != nil { + t.Fatalf("an accrual window was opened in Terminating (anchor %v); nothing here ever held "+ + "an instance", anchor) + } + if at := provisioned(t, c, "terminating"); at != nil { + t.Fatalf("status.provisionedAt %v on a claim that never billed", at) + } + if total != 0 { + t.Fatalf("status.estimatedCostUSD %v, want 0", total) + } + if n := testutil.CollectAndCount(metrics.CostTotal); n != 0 { + t.Fatalf("collected %d series, want 0 — no window may be booked in Terminating", n) + } +} + +// The other half of the same rule: a claim that WAS billing keeps billing through teardown, which +// is the only reason Terminating is a billing phase at all. A gate that looked at the phase alone +// would stop the meter while the instance is still alive and still costing money. +func TestCostAccrual_TerminatingKeepsAnOpenWindowRunning(t *testing.T) { + halfHour := 30 * time.Minute + nc := billingClaim("terminating", "10.0000", &halfHour) + nc.Status.Phase = nebulav1alpha1.NodeClaimTerminating + a, c := newAccrual(t, nc) + + a.accrueAll(context.Background()) + + total, anchor := ledger(t, c, "terminating") + if math.Abs(total-5) > 1e-2 { + t.Fatalf("status.estimatedCostUSD %v, want 5 (0.5h at $10/hr) — teardown of a live instance "+ + "is still billed", total) + } + if anchor == nil || time.Since(anchor.Time) > time.Minute { + t.Fatalf("anchor was not moved forward: %v", anchor) + } +} + // 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. @@ -328,6 +399,9 @@ func TestCostNow(t *testing.T) { settled.Status.Phase = nebulav1alpha1.NodeClaimTerminated settled.Status.EstimatedCostUSD = "5.0000" + terminatingUnanchored := billingClaim("torn-down", "10.0000", nil) + terminatingUnanchored.Status.Phase = nebulav1alpha1.NodeClaimTerminating + cases := map[string]struct { claim *nebulav1alpha1.NodeClaim want float64 @@ -338,6 +412,9 @@ func TestCostNow(t *testing.T) { "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}, + // Terminating with no window ever opened is the same kind of absence: reporting $0 would + // assert that the teardown was free rather than that nothing was ever billable here. + "terminating with no window is absent": {claim: terminatingUnanchored, want: 0, report: false}, } for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -779,6 +856,14 @@ func TestStampAccrualStart(t *testing.T) { claim *nebulav1alpha1.NodeClaim want bool }{claim: provisioning, want: false} + // Terminating BILLS but must not OPEN: a claim reaching it with nothing stamped never held an + // instance, so a window opened here would charge the teardown of something that never ran. + terminating := billingClaim("e", "3.9500", nil) + terminating.Status.Phase = nebulav1alpha1.NodeClaimTerminating + cases["terminating with no window already open"] = struct { + claim *nebulav1alpha1.NodeClaim + want bool + }{claim: terminating, want: false} for name, tc := range cases { t.Run(name, func(t *testing.T) { @@ -786,13 +871,77 @@ func TestStampAccrualStart(t *testing.T) { 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") + if !tc.want { + if tc.claim.Status.LastAccruedAt != before { + t.Fatal("the anchor was rewritten") + } + // Nothing may record that this claim started billing when it did not: billingRate + // reads ProvisionedAt to decide whether teardown is chargeable. + if before == nil && tc.claim.Status.ProvisionedAt != nil { + t.Fatalf("status.provisionedAt %v on a claim that opened no window", + tc.claim.Status.ProvisionedAt) + } + return + } + // The pair, at one instant: an anchor without ProvisionedAt has its teardown refused as + // never-billed, and the two are only comparable because they come from one patch. + at, start := tc.claim.Status.LastAccruedAt, tc.claim.Status.ProvisionedAt + if start == nil || !start.Equal(at) { + t.Fatalf("status.provisionedAt %v, want the anchor's %v", start, at) } }) } } +// Deleting a claim that never got capacity must cost nothing, through the real reconcile. +// +// The path that made this a leak is not obvious from either half: a Provisioning claim is not +// billable, but deleting its Pod promotes it straight past Bound to Terminating (desiredPhase +// checks the deletion before the Bound hold), and Terminating IS billable — so the same patch +// opened the claim's first accrual window, and teardown was then charged at the full GPU rate for +// an instance that never existed. +func TestMarkPhase_TerminatingAProvisioningClaimOpensNoWindow(t *testing.T) { + metrics.CostTotal.Reset() + + // Pending with no Initializing reason: nothing was ever placed. The finalizer is what lets the + // fake client hold a Pod with a deletionTimestamp instead of dropping it. + pod := gpuPod("L4", 1, "64", "128Gi") + pod.Status.Phase = corev1.PodPending + pod.Finalizers = []string{"test.nebula.inftyai.com/hold"} + now := metav1.Now() + pod.DeletionTimestamp = &now + + nc := newClaim("c1", "p1", "default", "uid-1", "fake") + nc.Spec.Accelerator = "L4:1" + nc.Spec.CapacityType = nebulav1alpha1.CapacityOnDemand + nc.Status.Phase = nebulav1alpha1.NodeClaimProvisioning + nc.Status.PriceUSDPerHour = "12.9559" + + pp := &pricedProvider{fakeProvider: &fakeProvider{name: "fake"}, rate: 12.9559} + r, c := newPricedReconciler(t, []client.Object{pod, nc}, pp) + + reconcileClaim(t, r, "c1") + + got := getClaim(t, c, "c1") + if got.Status.Phase != nebulav1alpha1.NodeClaimTerminating { + t.Fatalf("phase %q, want Terminating — the rest of this test asserts nothing otherwise", + got.Status.Phase) + } + if got.Status.LastAccruedAt != nil { + t.Fatalf("an accrual window was opened at teardown (anchor %v); a claim that never held an "+ + "instance must not start billing on its way out", got.Status.LastAccruedAt) + } + if total, _ := ledger(t, c, "c1"); total != 0 { + t.Fatalf("status.estimatedCostUSD %v, want 0", total) + } + + // And the window settleFinalCost would close on the way out is nothing, not one tick's worth. + settleFinalCost(context.Background(), got) + if charged := booked(t); charged != 0 { + t.Fatalf("settleFinalCost booked $%v for an instance that never ran, want 0", charged) + } +} + // A cost checkpoint must not re-enqueue the claim, but anything alongside it must. func TestIgnoreCostAccrual(t *testing.T) { p := ignoreCostAccrual() From e39b89e5773efc369fc14c33581f54aab3cf8ecf Mon Sep 17 00:00:00 2001 From: kerthcet Date: Thu, 10 Sep 2026 18:19:47 +0100 Subject: [PATCH 2/2] fix comments Signed-off-by: kerthcet --- internal/controller/cost_accrual.go | 19 ++++--------------- internal/controller/cost_accrual_test.go | 7 ++----- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/internal/controller/cost_accrual.go b/internal/controller/cost_accrual.go index becfd2e..cc4aff8 100644 --- a/internal/controller/cost_accrual.go +++ b/internal/controller/cost_accrual.go @@ -263,22 +263,11 @@ func formatCost(usd float64) string { } // stampAccrualStart opens the billing window the moment a claim first becomes chargeable, -// returning whether it mutated the claim. +// returning whether it mutated the claim. The only opener — accrue moves an anchor but never +// creates one — so a claim this misses bills nothing for its whole life, not merely late. // -// The ONLY opener: accrue moves an anchor but never creates one, so a claim this misses bills -// nothing for its whole life, not merely late. Safe to concentrate here because markPhase is the -// only writer of status.phase and requeues on a failed patch, so every claim that reaches a -// billing phase has had this run. Free, too — it rides the status patch markPhase is already -// making, which is also what makes a crash before the first checkpoint lossless: the anchor is -// what recovery measures the window from. -// -// Both stamps or neither, one patch: ProvisionedAt is what later refuses to bill a teardown that -// was never billing (see billingRate), so a claim must never carry an anchor without it. -// -// With no anchor yet, billingRate admits Bound alone, which is the gate this needs and does not -// restate. What that gives up is a claim whose Pod is deleted between two reconciles, only ever -// observed Terminating, and then charged nothing rather than for its brief real life — bounded by -// one reconcile gap, and absent spend is the honest direction. +// Both stamps at one instant or neither: ProvisionedAt is what later refuses to bill a teardown +// that was never billing (see billingRate), so an anchor must never exist without it. func stampAccrualStart(nc *nebulav1alpha1.NodeClaim) bool { if nc.Status.LastAccruedAt != nil { return false diff --git a/internal/controller/cost_accrual_test.go b/internal/controller/cost_accrual_test.go index f70dcea..bd15971 100644 --- a/internal/controller/cost_accrual_test.go +++ b/internal/controller/cost_accrual_test.go @@ -269,11 +269,8 @@ func TestCostAccrual_SkipsNonBilling(t *testing.T) { } } -// The loop opens an anchor for anything it considers billing, so Terminating has to be refused -// unless status.provisionedAt says something was already being charged — otherwise the tick that -// finds a never-billed claim on its way out opens its FIRST window there and every tick after that -// charges a full interval, at the GPU rate, for an instance that never ran. Two ticks, because the -// first only stamps: the charge this guards against appears on the second. +// A Terminating claim with no previously opened window must remain unbillable, because it may +// never have held an instance. func TestCostAccrual_TerminatingWithoutAWindowNeverStarts(t *testing.T) { nc := billingClaim("terminating", "98.3200", nil) nc.Status.Phase = nebulav1alpha1.NodeClaimTerminating