feat(controller): apply Task resource limits to the ActorTemplate by mvanhorn · Pull Request #405 · google/ax

mvanhorn · 2026-09-25T06:20:16Z

TaskSpec.resources is documented in docs/manifests.md, accepted by ax-server, and shown by ax describe, but the controller never read it, so setting limits had no effect on the sandbox. This makes spec.resources.limits real by copying it onto the per-task Substrate ActorTemplate.

Fixes #369

What changed

internal/substrate: new ResourceLimits() translates spec.resources.limits.{cpu,memory} into Substrate's ActorTemplate.resources.limits ({name, quantity} list). BuildActorTemplate / EnsureActorTemplateWithImage take the resources block; nil keeps today's behaviour (sandbox sized by the worker defaults).
internal/controller: the reconciler passes the task's limits when provisioning the per-task template.
docs/manifests.md: short "Sizing the sandbox" section with Substrate's constraints (cpu/memory only, quantities > 0, cpu < 1000).

Notes for review:

Substrate's Resources only has limits, so requests is documented as stored-but-not-applied rather than silently mapped onto something else. Happy to turn that into a server-side rejection or a reconcile warning instead if you prefer.
No change to the template digest was needed: the launch spec already rides along in AX_TASK_YAML, so a limits change yields a new template. TestTaskReconciler_ResourceLimits pins that so it can't regress if the digest inputs change.

How tested

go test ./... (green), go vet ./..., go mod tidy leaves go.mod / go.sum unchanged.
New tests:
- internal/substrate: TestResourceLimits (nil / empty / requests-only produce no block; cpu, memory, both) and TestBuildActorTemplate_Resources.
- internal/controller: TestTaskReconciler_ResourceLimits reconciles against the mock Control API and asserts the CreateActorTemplate request carries cpu=2, memory=4Gi, that raising a limit provisions a new template with the new value, and that a task without limits has no resources block.
Manually reconciled the docs/manifests.md Task example through the controller against the mock Substrate Control API and inspected the template it created (first screenshot).

Evidence

Demo

Full-resolution MP4

google-cla · 2026-09-25T06:20:33Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

ashishsinghbora · 2026-09-25T06:24:53Z

-		tmpl, err := r.client.EnsureActorTemplateWithImage(ctx, templateAtespace, templateName, atespace, customTemplateName, task.Spec.Image, extraEnv)
+		// spec.resources rides along in AX_TASK_YAML, so a limits change is already
+		// part of the template digest and re-provisions the template.
+		resources := substrate.ResourceLimits(task.Spec.Resources)


Could we make invalid resource limits fail reconciliation instead of silently falling back to the worker's default template?

ResourceLimits currently forwards the raw strings to the ActorTemplate. If Substrate rejects an invalid quantity or an out-of-range CPU value, EnsureActorTemplateWithImage returns an error, and the surrounding reconciler treats that as a reason to fall back to the default template.

That means a Task can request a resource limit, fail to provision the requested template, and still run successfully with different resource limits.

Since the documentation now specifies constraints such as quantities being > 0 and CPU < 1000, I'd suggest validating these values before provisioning (or propagating the template creation error) rather than silently falling back to a template that does not honor the Task's requested resources.

Agreed, that fallback was a real hole. Fixed in 079ca2d:

spec.resources is now validated before anything is provisioned (v1alpha1.ValidateResources, also called from ValidateTask so ax apply rejects bad values up front). Quantities follow the Kubernetes quantity grammar, must be > 0, and cpu must be < 1000 cores, matching what Substrate enforces. Invalid values fail the reconcile with Ready=False / InvalidResources.

If a task has limits and Substrate still rejects the template, the reconcile now fails with Ready=False / TemplateCreationFailed instead of falling back. The fallback only remains for tasks that set no limits, so the existing contract there is unchanged.

I kept the quantity parser dependency-free rather than pulling in k8s.io/apimachinery; happy to switch to resource.ParseQuantity if you'd rather have the exact upstream parser.

ashishsinghbora · 2026-09-25T06:27:28Z

+	// Raising a limit is a launch configuration change and must provision a new
+	// template carrying the new value.
+	task.Spec.Resources.Limits.Memory = "8Gi"
+	if _, err := reconciler.Reconcile(ctx, task); err != nil {


Could we add a failure-path test for invalid resource limits?

The current test verifies valid CPU/memory limits and the no-limits case, but not what happens when the requested limits cannot be accepted by Substrate.

Given that the reconciler currently falls back to the default ActorTemplate when custom template creation fails, a regression here could cause the Task to run without the requested limits while the reconciliation still succeeds.

A test covering an invalid/rejected resource configuration would make this behavior explicit and protect the resource-isolation guarantee.

Added TestTaskReconciler_ResourceLimitsFailurePaths in 079ca2d with three cases:

invalid limits (two, 0, 1000, -4Gi, and a requests block) fail before provisioning: phase Failed, InvalidResources, and nothing reaches Substrate;

the mock rejects CreateActorTemplate for a task with valid limits: phase Failed, TemplateCreationFailed, and no actor is created or resumed on a fallback template;

the mock rejects the template for a task without limits: it still falls back and runs, so that contract is pinned too.

TestValidateResources covers the parser edge cases (suffixes, exponents, bounds, bad strings).

ashishsinghbora · 2026-09-25T06:29:08Z

+	if _, err := reconciler.Reconcile(ctx, task); err != nil {
+		t.Fatalf("Reconcile with changed limits failed: %v", err)
+	}
+	if len(mockSrv.createdTemplates) != 2 {


Could we also assert that the two generated ActorTemplate names differ when the resource limits change?

The implementation relies on AX_TASK_YAML being part of taskTemplateName's digest input. The current test proves that a second template is created, but explicitly comparing the template names would make the digest/re-provisioning contract clearer and protect against accidentally reusing the old template.

Done in 079ca2d: TestTaskReconciler_ResourceLimits now compares the two CreateActorTemplate names and fails if the second reconcile reused the first template's name.

ashishsinghbora

@mvanhorn The core implementation looks good and directly addresses #369: TaskSpec.resources.limits is now translated into the per-task ActorTemplate, and the tests cover CPU-only, memory-only, combined limits, no limits, and a resource change causing a new template.

I have a couple of correctness concerns before this is ready to merge.

The main one is the error path. The reconciler currently falls back to the default ActorTemplate when custom template creation fails. Since resource quantities are passed through to Substrate, an invalid or unsupported resource value could therefore result in the Task running without the requested limits instead of failing clearly. I think resource validation or explicit propagation of this failure is important so a requested limit is never silently dropped.

The other concern is spec.resources.requests: the API accepts the field but the controller intentionally ignores it. Since users can reasonably expect a populated requests field to have an effect, I'd prefer either explicit validation/rejection or a visible warning/status condition rather than silently accepting it.

I'd also add a small failure-path test and explicitly verify that changing resource limits changes the generated ActorTemplate identity.

The overall direction is sound; tightening these failure semantics would make the resource-limits feature considerably safer and easier to reason about.

Address review on google#405: - Validate spec.resources before provisioning, at apply time via ValidateTask and again in the reconciler. Quantities follow the Kubernetes quantity grammar, must be greater than zero, and the cpu limit must be below 1000 cores, matching what Substrate enforces on the ActorTemplate. Invalid values fail the reconcile with Ready=False, reason InvalidResources. - When a task has limits and Substrate rejects the per-task template, fail the reconcile (reason TemplateCreationFailed) instead of falling back to the default template, which would run the task without the limits it asked for. Tasks without limits keep the existing fallback. - Reject spec.resources.requests with a clear message: Substrate sizes sandboxes by limits alone, so accepting and ignoring it was misleading. The docs and examples/task.yaml drop the requests block. - Tests: failure paths for invalid limits and for a rejected template with and without limits, plus an explicit assertion that a limits change yields a differently named template. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>

mvanhorn · 2026-09-25T06:47:17Z

Thanks for the careful review, @ashishsinghbora. All four points are addressed in 079ca2d (plus a one-line test fixture tweak in 52b1bfb so it composes with #406):

Error path: limits are validated before provisioning (Kubernetes quantity grammar, > 0, cpu < 1000 cores) both at ax apply via ValidateTask and again in the reconciler, failing with InvalidResources. When a task has limits and Substrate rejects the template, the reconcile now fails with TemplateCreationFailed instead of falling back to the default template. Tasks without limits keep the existing fallback.
requests: rejected at validation with spec.resources.requests: not supported, only spec.resources.limits is applied to the sandbox. docs/manifests.md, docs/concepts.md and examples/task.yaml no longer show a requests block.
Failure-path tests: TestTaskReconciler_ResourceLimitsFailurePaths covers invalid limits (nothing reaches Substrate), a rejected template with limits (no fallback, no actor), and a rejected template without limits (fallback still works). TestValidateResources covers the parser.
Template identity: the resource-change test now asserts the two generated template names differ.

go vet, go test ./... and the go mod tidy check are clean, and the branch merges cleanly with #406 with all tests passing on the combined tree.

TaskSpec.resources is documented and accepted by ax-server, but the controller never read it, so spec.resources.limits had no effect on the sandbox. Translate limits.cpu and limits.memory into the Substrate ActorTemplate resources block (Substrate's {name, quantity} list) when provisioning the per-task template. Substrate sizes sandboxes by limits only, so requests are documented as not applied. A limits change already yields a new template because the launch spec is part of the template digest via AX_TASK_YAML; a test now pins that behaviour. Fixes google#369 Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>

Address review on google#405: - Validate spec.resources before provisioning, at apply time via ValidateTask and again in the reconciler. Quantities follow the Kubernetes quantity grammar, must be greater than zero, and the cpu limit must be below 1000 cores, matching what Substrate enforces on the ActorTemplate. Invalid values fail the reconcile with Ready=False, reason InvalidResources. - When a task has limits and Substrate rejects the per-task template, fail the reconcile (reason TemplateCreationFailed) instead of falling back to the default template, which would run the task without the limits it asked for. Tasks without limits keep the existing fallback. - Reject spec.resources.requests with a clear message: Substrate sizes sandboxes by limits alone, so accepting and ignoring it was misleading. The docs and examples/task.yaml drop the requests block. - Tests: failure paths for invalid limits and for a rejected template with and without limits, plus an explicit assertion that a limits change yields a differently named template. Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>

Keeps the test valid alongside metadata.name validation (google#406). Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>

ashishsinghbora reviewed Sep 25, 2026

View reviewed changes

mvanhorn added 3 commits September 25, 2026 07:52

test: name the ValidateTask fixtures in TestValidateResources

c1194df

Keeps the test valid alongside metadata.name validation (google#406). Co-authored-by: Matt Van Horn <mvanhorn@users.noreply.github.com>

mvanhorn force-pushed the cursor/task-resource-limits-dfaf branch from 52b1bfb to c1194df Compare September 25, 2026 14:52

mvanhorn mentioned this pull request Sep 25, 2026

feat(server): reject resource names that are not RFC 1123 labels #406

Merged

Navigation Menu

feat(controller): apply Task resource limits to the ActorTemplate - #405

feat(controller): apply Task resource limits to the ActorTemplate#405
mvanhorn wants to merge 3 commits into
google:mainfrom
mvanhorn:cursor/task-resource-limits-dfaf

mvanhorn commented Sep 25, 2026 •

edited

Loading

Uh oh!

google-cla Bot commented Sep 25, 2026

Uh oh!

ashishsinghbora Sep 25, 2026

Uh oh!

mvanhorn Sep 25, 2026

Uh oh!

ashishsinghbora Sep 25, 2026

Uh oh!

mvanhorn Sep 25, 2026

Uh oh!

ashishsinghbora Sep 25, 2026

Uh oh!

mvanhorn Sep 25, 2026

Uh oh!

ashishsinghbora left a comment

Uh oh!

mvanhorn commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mvanhorn commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

How tested

Evidence

Demo

Uh oh!

google-cla Bot commented Sep 25, 2026

Uh oh!

ashishsinghbora Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

mvanhorn Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

ashishsinghbora Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

mvanhorn Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

ashishsinghbora Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

mvanhorn Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

ashishsinghbora left a comment

Choose a reason for hiding this comment

Uh oh!

mvanhorn commented Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mvanhorn commented Sep 25, 2026 •

edited

Loading