[gjkim42]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Replaces the TaskSpawner-owned budgetPolicy with a Task-native design that enforces spending controls on any Task regardless of how it was created (CLI, YAML, webhook, or TaskSpawner).

The original approach tied cost governance to the TaskSpawner, which left direct Tasks outside the model. This redesign makes budget enforcement a first-class Task concern via three new API surfaces:

New CRDs:

• TaskBudget — namespace-scoped policy that gates Task admission using label selectors. Supports daily period accounting (timezone-aware) with cost and token limits. Blocked Tasks stay in Waiting phase with a BudgetBlocked condition until the period resets.
• TaskRecord — immutable terminal record created when a Task completes. Survives Task TTL deletion (no ownerReference) and serves as the durable accounting source for budget enforcement.

New Task.status fields:

• status.usage — structured TaskUsage (costUSD as resource.Quantity, inputTokens, outputTokens) populated from status.results on completion.
• status.conditions — typed conditions including BudgetBlocked.

Admission semantics:

• TaskBudget.spec.taskSelector matches Tasks by label (uses metav1.LabelSelector)
• Zero matching budgets → no gate; multiple matches → all must admit
• Budget enforcement sums TaskRecord usage within the current period
• Fail-closed: a misconfigured matching budget blocks admission and sets a Degraded condition on the TaskBudget
• Server-side label filtering on TaskRecord listing for performance

TaskSpawner integration:

• TaskSpawner remains a Task producer; attaches budget labels via taskTemplate.metadata.labels
• All old budget code removed: BudgetPolicy, CostBudget, PeriodBudget, BudgetStatus, HighCostTaskRecord types; spawner-side enforcement; accumulateSpawnerCost; kelos_spawner_budget_* Prometheus gauges; TaskCountExhausted/TaskBudgetExhausted/CostBudgetExceeded conditions

Design decisions:

• resource.Quantity for cost fields (type-safe, integrates with k8s tooling)
• MVP period: Daily only (Weekly/Monthly deferred until boundary semantics specified)
• BudgetAction enum with Block as sole value (future-proofs for Warn etc.)
• NowFunc on reconciler for deterministic testing
• TaskRecord name = Task UID (guaranteed unique, survives recreation)
• Operators responsible for TaskRecord retention/pruning (documented)

Follow-up fixes in this update:

• Adds a generated validating admission webhook for TaskBudget.spec.taskSelector validation.
• Serves the validating webhook from the existing kelos-controller binary through the existing kelos-webhook Service; no separate binary is required.
• Extends make update/make verify so the chart includes the generated ValidatingWebhookConfiguration.
• Keeps controller-side fail-closed handling for any already-stored invalid TaskBudget selector.
• Stages newly added Kelos CRDs during upgrades before waiting for webhook readiness, while limiting conversion CA bundle checks to CRDs that actually use conversion webhooks.
• Fails closed when a matching stored TaskBudget has an invalid period configuration.
• Retries TaskRecord creation for unchanged terminal Job-backed Tasks.
• Ensures worker-pool pod assignment cleanup still happens when TaskRecord creation fails, while returning the error so reconciliation retries the record.
• Snapshots Task labels at budget admission time and uses the snapshot for TaskRecord labels, so later Task label mutation cannot move recorded usage out of the matching budget.
• Populates worker-pool TaskRecord type/model from the referenced WorkerPool.spec.worker, without writing deprecated Task fields.
• Requires TaskRecord.spec and TaskRecord.spec.completionTime in the CRD schema.
• Filters worker-pool budget wakeups to Tasks that are actually BudgetBlocked=True.
• Emits the TaskBudgetExhausted TaskSpawner condition alias alongside MaxTotalTasksReached for compatibility.
• Updates Helm CRD ownership docs and TaskRecord reference docs.
• Removes an unused TaskSpawner RBAC marker from the Task controller.

Which issue(s) this PR is related to:

Fixes #977

Special notes for your reviewer:

• CRD manifests are regenerated via make update and included.
• The TaskRecordSpec is immutable after creation (XValidation: self == oldSelf).
• sumPeriodUsage pushes label filtering server-side via client.MatchingLabelsSelector; only time filtering is client-side.
• Invalid timezone on a matching budget fails closed (blocks task, sets Degraded condition) rather than silently passing.
• Invalid TaskBudget.spec.taskSelector values are rejected by the validating admission webhook on create/update.
• Tests cover: usageFromResults, computePeriodBoundaries, checkLimitsExceeded, createTaskRecord (including idempotency, label snapshot behavior, and worker-pool metadata), validating webhook rejection, e2e validating webhook rejection coverage, TaskRecord API validation, terminal TaskRecord retry, worker-pool assignment cleanup after record failures, worker-pool budget wakeup filtering, TaskSpawner condition aliases, install CRD staging lists, and invalid stored period fail-closed behavior.
• Example manifest at examples/14-task-budget/taskbudget.yaml.

Validation:

• git diff --check
• make test
• make test-integration
• make verify
• go test ./test/e2e -run TestDoesNotExist

Does this PR introduce a user-facing change?

Add Task-native budget enforcement via new TaskBudget and TaskRecord CRDs. TaskBudget uses label selectors to match Tasks and enforces daily cost/token limits based on durable TaskRecord accounting. Blocked Tasks wait in Waiting phase with a BudgetBlocked condition. Task.status.usage provides structured cost and token data. Removes TaskSpawner-specific budgetPolicy in favor of the namespace-scoped TaskBudget design.

[gjkim42]

cc @knechtionscoding updated a bit based on your PR to introduce a new validation webhook to prevent invalid resources from being accepted.

Can you review this?

[gjkim42]

/kelos review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-b0f9b0674a92 has succeeded. ✅

[kelos-bot]

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: REQUEST CHANGES (sticky comment only)
Overall correctness: patch is incorrect
Scope: Adds TaskBudget/TaskRecord CRDs plus controller, webhook, docs, and tests for Task-native budget admission and durable usage accounting.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	1	internal/controller/task_budget_admission.go:72	Invalid period config fails open instead of blocking matching Tasks
P2	1	cmd/kelos-spawner/main.go:469	Existing TaskSpawner maxTotalTasks condition was renamed without compatibility
P3	0	—	none

Findings

Correctness

• [P1] internal/controller/task_budget_admission.go:72 — A matching TaskBudget whose stored spec.period cannot be evaluated is skipped with continue, so an invalid timezone or unsupported period type makes that budget stop gating matching Tasks. That is fail-open behavior for invalid budget configuration: any cluster with an already-stored malformed TaskBudget, for example from before the webhook/schema was tightened or from a restore, will admit Tasks that should remain blocked. Return an error after setting Degraded, mirroring the invalid-selector/list-error paths, and have the status reconciler surface the same degraded state instead of silently skipping it.

API Compatibility

• [P2] cmd/kelos-spawner/main.go:469 — This PR renames the existing maxTotalTasks status condition from TaskBudgetExhausted to MaxTotalTasksReached, so clients that already watch TaskSpawner.status.conditions[type=TaskBudgetExhausted] stop seeing the limit when it is reached. The old name is awkward with the new TaskBudget CRD, but it is already part of the TaskSpawner status surface; keep emitting it for compatibility, or emit both and mark the old one deprecated, while adding the clearer condition.

Key takeaways

• Budget admission/accounting paths are otherwise covered across Job-backed and worker-pool Tasks, including TaskRecord retry paths.

[cubic-dev-ai]

8 issues found across 45 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="api/v1alpha2/task_types.go">

<violation number="1" location="api/v1alpha2/task_types.go:421">
P2: v1alpha1 clients can accidentally erase the new Task budget/accounting status fields on status updates, because the conversion path does not preserve v1alpha2-only `status.usage` or `status.conditions`. Consider extending conversion to preserve these hub-only status fields (for example via annotations or explicit carry-forward) while v1alpha1 remains served.</violation>
</file>

<sub>Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: v1alpha1 clients can accidentally erase the new Task budget/accounting status fields on status updates, because the conversion path does not preserve v1alpha2-only status.usage or status.conditions. Consider extending conversion to preserve these hub-only status fields (for example via annotations or explicit carry-forward) while v1alpha1 remains served.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At api/v1alpha2/task_types.go, line 421:

<comment>v1alpha1 clients can accidentally erase the new Task budget/accounting status fields on status updates, because the conversion path does not preserve v1alpha2-only `status.usage` or `status.conditions`. Consider extending conversion to preserve these hub-only status fields (for example via annotations or explicit carry-forward) while v1alpha1 remains served.</comment>

<file context>
@@ -412,6 +414,37 @@ type TaskStatus struct {
+	// Usage contains structured cost and token usage populated from results
+	// when the Task reaches a terminal phase.
+	// +optional
+	Usage *TaskUsage `json:"usage,omitempty"`
+
+	// Conditions provides detailed status information.
</file context>

[gjkim42]

Valid, but intentionally not addressed in this update. This batch excludes conversion-related changes, so the v1alpha1 carry-forward/preservation work should stay open for a separate follow-up.

[cubic-dev-ai]

That’s fair — the conversion carry-forward gap is still the right issue, just out of scope for this batch. The parent comment still applies for the v1alpha1↔v1alpha2 status conversion path, and it should stay open as a separate follow-up.

[gjkim42]

/kelos glm-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-glm-reviewer-issue-comment-b74960b02530 has succeeded. ✅

[gjkim42]

/kelos glm-review

[kelos-bot]

🤖 Kelos Task Status

Task kelos-glm-reviewer-issue-comment-a7675bedd7ff has succeeded. ✅

[kelos-bot]

🤖 Kelos GLM Reviewer Agent @gjkim42

Review Summary

Verdict: APPROVE
Overall correctness: patch is correct
Scope: Adds TaskBudget/TaskRecord CRDs with a fail-closed admission enforcer, durable usage accounting, a TaskBudget validating webhook, and spawner/controller integration for Task-native budget governance.

Findings Overview

Priority	Count	File:Line	Summary
P0	0	—	none
P1	0	—	none
P2	0	—	none
P3	1	internal/controller/taskbudget_controller.go:72	Invalid-period branch skips Degraded condition, inconsistent with sibling selector branch

Findings

Correctness / Conventions

• [P3] internal/controller/taskbudget_controller.go:72 — TaskBudgetReconciler.Reconcile sets a Degraded condition and returns an error for an invalid taskSelector (line 65), but for an invalid period it only logs and returns nil with no condition and no requeue. Both branches handle the same "stored invalid config" case and are commented as unreachable, yet the period path diverges from both the selector path here and the admission path's InvalidPeriod Degraded in task_budget_admission.go:76. A stored invalid-period budget (e.g. from a pre-webhook restore) would not be surfaced in this reconciler's status until a matching Task triggers admission. Align the two branches — at minimum set the InvalidPeriod Degraded condition here so the degraded state is observable without relying on a Task admission to surface it.

Suggestions

• [P2] internal/conversion/task.go:16 — The Task hub↔v1alpha1 conversion uses convertViaJSON on status, which drops hub-only fields on the v1alpha1→hub direction. This PR adds status.usage and status.conditions as hub-only fields, so a v1alpha1 status update by any client would erase them — transiently dropping the BudgetBlocked condition (re-set on the next reconcile) or, in the narrow window between usage being set and createTaskRecord running, dropping usage so no TaskRecord is written and the budget undercounts. The Kelos controllers write status via v1alpha2 so the primary control plane is unaffected, and the impact depends on a v1alpha1 status-updating client existing. This is a deeper API-compatibility check — recommend running /kelos glm-api-review to confirm whether hub-only status fields need explicit carry-forward (e.g. via annotations) while v1alpha1 remains served.

Key takeaways

• Budget admission is fail-closed for both Job-backed and worker-pool Tasks: invalid selectors, list errors, and invalid periods all block matching Tasks and set a Degraded condition on the budget; TaskRecord creation is idempotent and retried from the terminal-state path so transient failures do not undercount usage.
• Test coverage is strong across admission, limits, period boundaries, label snapshots, the validating webhook, CRD schema, terminal retries, and worker-pool cleanup-after-record-failure; e2e Eventually helpers use the return-zero-on-error form rather than in-poller Expect.
• No P0/P1 issues found; the one P3 is an unreachable-branch observability inconsistency, and the conversion concern is deferred to the API review.

[knechtionscoding]

lgtm