feat: support cost tracking

[kerthcet]

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?

[Copilot]

🟡 Changes recommended

There are confirmed correctness bugs around NaN handling in cost parsing/metrics updates that could poison ledgers/counters and should be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds end-to-end cost tracking for NodeClaims by introducing a provider pricing interface, persisting per-claim estimated cost in status, and exporting cumulative spend via Prometheus with consistent candidate labels.

Changes:

• Introduces provider.Pricer/PriceRequest and catalog-based pricing semantics, including Modal-specific CPU/memory metering.
• Records hourly price on NodeClaim status once, checkpoints estimated cost periodically, and emits nebula_nodeclaim_cost_usd_total metrics.
• Updates CRD schema/printcolumns and metrics documentation to reflect the new cost fields and queries.

File summaries

File	Description
pkg/util/resources.go	Computes Pod CPU/memory reservation for pricing inputs.
pkg/util/resources_test.go	Tests reservation extraction and unit conversions.
pkg/util/accelerator.go	Adds parsing for joined accelerator pool identity.
pkg/util/accelerator_test.go	Tests pool split and round-trip behavior.
pkg/provider/pricing.go	Defines pricing request model, pricer interface, and ErrNoPrice.
pkg/provider/modal/modal.go	Overrides pricing to add Modal CPU/memory metering and CPU-only pricing.
pkg/provider/modal/modal_test.go	Tests Modal pricing composition and no-price cases.
pkg/provider/catalog/data/pricing.go	Adds non-CSV metered rates (Modal CPU/memory).
pkg/provider/catalog/data/modal.csv	Updates Modal accelerator catalog rows/prices.
pkg/provider/catalog/catalog_test.go	Adds tests for per-GPU vs per-instance catalog pricing behavior.
pkg/provider/catalog/base.go	Implements default PricePerHour for all-in catalog providers.
pkg/metrics/labels.go	Extends label helper to support variadic “extra” dimensions.
pkg/metrics/doc.go	Documents cost metrics as part of the metrics surface.
pkg/metrics/cost.go	Adds CostTotal counter and RecordSpend helper.
pkg/metrics/cost_test.go	Tests cost metric accumulation/labels/phase splitting behavior.
internal/controller/nodeclaim_price_test.go	Tests stamping NodeClaim price and pricing request construction.
internal/controller/nodeclaim_controller.go	Records price in status, stamps accrual anchor, and filters cost-only updates.
internal/controller/cost_accrual.go	Adds leader-elected loop to checkpoint estimated cost and advance metrics.
internal/controller/cost_accrual_test.go	Tests accrual correctness, durability, and enqueue-filter predicate behavior.
docs/metrics.md	Adds cost metric docs and example PromQL queries.
config/crd/bases/nebula.inftyai.com_nodeclaims.yaml	Adds CRD schema fields + print columns for price and estimated cost.
cmd/main.go	Registers the cost accrual runnable with the manager.
CLAUDE.md	Adds repo guidance on comment style/density.
api/v1alpha1/zz_generated.deepcopy.go	Updates deep-copy logic for new pointer status field(s).
api/v1alpha1/nodeclaim_types.go	Adds NodeClaim status fields for pricing and cost ledger.
.gitignore	Ignores .claude directory.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

• Files reviewed: 24/26 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There are correctness/operability issues in the new cost-related event filtering and scrape-path listing behavior that can cause unnecessary reconciles or blocked metric scrapes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/provider/modal/modal.go:349

• PricePerHour doesn’t validate CPUCores/MemoryMiB. Negative values would make metered < 0 and could reduce the returned hourly rate for GPU workloads, or turn a malformed CPU-only request into ErrNoPrice, which hides a bug.

internal/controller/nodeclaim_controller.go:512

• ignoreCostAccrual compares the full NodeClaim object, but cost-only Status().Update calls can also change metadata.managedFields. Since ManagedFields isn’t neutralized, cost-only checkpoints may still look like “real” updates and re-enqueue claims on every accrual tick in a real cluster.

			rest := after.DeepCopy()
			rest.Status.EstimatedCostUSD = before.Status.EstimatedCostUSD
			rest.Status.LastAccruedAt = before.Status.LastAccruedAt
			rest.ResourceVersion = before.ResourceVersion
			return !equality.Semantic.DeepEqual(before, rest)

• Files reviewed: 25/27 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The cost-accrual update predicate is likely ineffective as written (metadata like managedFields can still differ), so the intended “don’t re-enqueue on checkpoint” optimization won’t reliably work.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

internal/controller/nodeclaim_controller.go:512

• ignoreCostAccrual is intended to drop cost-only status updates, but it only normalizes EstimatedCostUSD/LastAccruedAt and ResourceVersion. Kubernetes Status().Update also mutates metadata like managedFields, so equality.Semantic.DeepEqual(before, rest) will still see a difference and the event will be enqueued, defeating the optimization.

			rest := after.DeepCopy()
			rest.Status.EstimatedCostUSD = before.Status.EstimatedCostUSD
			rest.Status.LastAccruedAt = before.Status.LastAccruedAt
			rest.ResourceVersion = before.ResourceVersion
			return !equality.Semantic.DeepEqual(before, rest)

• Files reviewed: 25/27 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new ignoreCostAccrual predicate likely won’t suppress cost-only updates if metadata like managedFields changes on status updates, causing unnecessary reconcile churn at scale.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

internal/controller/nodeclaim_controller.go:513

• ignoreCostAccrual normalizes Status.{EstimatedCostUSD,LastAccruedAt} and ResourceVersion, but Kubernetes updates typically also mutate ObjectMeta.ManagedFields on status updates. If ManagedFields changes, equality.Semantic.DeepEqual(before, rest) will still differ and the controller will be re-enqueued every accrual tick, defeating the purpose of this predicate.

			rest := after.DeepCopy()
			rest.Status.EstimatedCostUSD = before.Status.EstimatedCostUSD
			rest.Status.LastAccruedAt = before.Status.LastAccruedAt
			rest.ResourceVersion = before.ResourceVersion
			return !equality.Semantic.DeepEqual(before, rest)

• Files reviewed: 25/27 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

PodReservation can truncate sub‑MiB memory requests to 0 MiB, conflating “some reservation” with the documented “unset/default” sentinel and potentially underpricing.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/util/resources.go:47

• PodReservation converts bytes to MiB with integer division, which truncates any non-zero request smaller than 1Mi (e.g. 512Ki) down to 0. That makes "a small reservation" indistinguishable from "no reservation set" (0 is documented as the sentinel for "use provider defaults"), and would under-price such pods.

• Files reviewed: 28/30 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The implementation is cohesive with thorough test coverage; only a minor documentation punctuation nit was found.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

• Files reviewed: 28/30 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are two confirmed billing correctness gaps (Modal pricing can underprice malformed requests, and accrual can continue/double-book after finalizer removal during deletion) that should be addressed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/provider/modal/modal.go:355

• Modal.PricePerHour treats an empty AcceleratorType as CPU-only and ignores req.Count; a malformed request with Count>0 but no type would be underpriced as CPU+memory only. Add a guard to fail loudly on nonzero Count when AcceleratorType is empty.

• Files reviewed: 28/30 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces a large, cross-cutting billing/metrics feature spanning provider pricing, controller reconciliation, CRD/status schema, and a new leader-elected write loop that warrants careful human validation.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/metrics/testing.go:39

• ConfigureCostForTest is compiled into production code (non _test.go) and can silently break metrics if it’s ever called after InitCost: it swaps the global CostTotal without re-registering, so future RecordWindow calls won’t update the registered collector. Consider adding a runtime guard to ensure this helper is only callable from go test binaries (or moving it into a test-only package).

• Files reviewed: 33/35 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

recordPrice currently persists non-positive/non-finite rates without validation, which can pin unusable values into NodeClaim status and undermine downstream cost logic.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

internal/controller/nodeclaim_controller.go:428

• recordPrice persists whatever the provider returns into status.priceUSDPerHour without validating it. If a Pricer accidentally returns 0/negative or a non-finite float (NaN/±Inf), the claim gets pinned to an unusable value (and could later poison accrual if the guards change). Treat non-positive and non-finite rates as unpriceable and leave the field empty.

	nc.Status.PriceUSDPerHour = strconv.FormatFloat(rate, 'f', priceDecimals, 64)

• Files reviewed: 34/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces new billing/pricing semantics, persistent ledger fields, and a clock-driven controller loop that warrant careful human validation in a real cluster and against provider billing exports.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/provider/modal/modal.go:355

• The doc comment says ErrNoPrice is returned only when a CPU-only sandbox reserves neither CPU nor memory, but the implementation returns ErrNoPrice when either CPU or memory is defaulted (req.CPUCores <= 0 || req.MemoryMiB <= 0). Update the comment to match the actual contract so callers/readers aren’t misled about when CPU-only pricing is possible.

• Files reviewed: 34/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

status.costLabels can serialize as JSON null while the CRD schema currently disallows null for that object field, which can cause status update validation failures in the default (no --cost-labels) configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

• Files reviewed: 34/36 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

recordPrice should validate and refuse non-positive rates before pinning them, otherwise claims can be marked “priced” with 0/negative values and never be eligible for re-pricing.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

Suppressed comments (1)

internal/controller/nodeclaim_controller.go:439

• recordPrice persists whatever float a provider returns. If a Pricer ever returns a non-positive rate with a nil error, this will store "0.0000" (or negative), which looks priced but will never be billed (finalRate drops <=0) and will also prevent later re-pricing because the field is no longer empty. Consider validating rate > 0 before pinning it and otherwise leaving the field empty (and logging).

	nc.Status.PriceUSDPerHour = strconv.FormatFloat(rate, 'f', priceDecimals, 64)
	return true

• Files reviewed: 34/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core controller behavior and introduces new cost-accounting/metrics paths that warrant final human review despite strong test coverage.

Review details

Files not reviewed (1)

• api/v1alpha1/zz_generated.deepcopy.go: Generated file

• Files reviewed: 34/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[kerthcet]

/lgtm
/kind feature

[InftyAI-Agent]

Approved: PR has both lgtm and approved labels