Repository navigation
Add Loop Runtime on GKE with isolated Sandbox Service workers - #98
Alex R (alexr17) wants to merge 28 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3b7cd9e53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e8a133ecc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback". |
Context The Helm chart had no Loop Runtime workload, so external EKS deployments could not run the Kubernetes-hosted runtime. Description Add an opt-in AWS EKS Loop Runtime Deployment, Service, ServiceAccount, and ConfigMap. Wire the runtime to the existing data-plane Secret, API URL, AWS MicroVM sandbox settings, and optional Istio route. Squashed from #98 and rebased onto main.
1bc6147 to
b97b95f
Compare
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e9322fa75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e459430706
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 922cc8a6ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d40b5ed21b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Erik Weathers (erikdw)
left a comment
There was a problem hiding this comment.
Yes, this looks like the right direction. Loop Runtime in the Braintrust chart, the shared Sandbox Service chart as a separate release, and infrastructure outside Helm are sensible ownership boundaries.
Three integration fixes before it's ready (inline comments below):
- Shutdown time: the Deployment gets the default 30s grace period, but Loop takes up to 90s to drain (CFN uses
StopTimeout: 120). Rollouts can cut off turns still draining after 30 seconds. FUNCTION_SECRET_KEY: it's only mapped intoSERVICE_TOKEN_SECRET_KEY. The runtime also needsFUNCTION_SECRET_KEYitself, or MCP servers that use stored credentials break.- Empty
orgName: the chart allows it for multi-org installs, but with noallowedOrgIdsthe runtime refuses to start.
Resource/version contract:
- Give the runtime's disk cache an explicit budget:
BRAINSTORE_OBJECT_STORE_CACHE_FILE_SIZE, anemptyDirsizeLimit, and an ephemeral-storage request so scheduling accounts for it on the shared system nodes. - Pin a tested runtime/worker/sandbox image combination. The chart's runtime default (
latest-2.x) and the sandbox guest image currently float.
Then a real EKS lifecycle test: create a sandbox, use Git through the proxy, run a Loop turn, replace a runtime pod, and upgrade/drain/uninstall workers. That's the main evidence still missing. The GKE instructions also depend on unmerged Terraform work in terraform-google-braintrust-data-plane (draft braintrustdata/terraform-google-braintrust-data-plane#28, stacked on jeff/gke-standard), so make that dependency explicit.
All of this is integration and rollout work within the proposed design. The overall split makes sense.
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0baeda9668
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
1 similar comment
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a858ef5645
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61788e2e30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review |
Drop the AWS Lambda MicroVM backend. Loop Runtime on EKS and GKE now sends every sandbox to Braintrust Sandbox Service workers. With a Sandbox Service endpoint and no EXO_SANDBOX_PROVIDER, the runtime uses the Sandbox Service for all sandboxes instead of the MicroVM default plus a LaunchDarkly rollout, as the CloudFormation mixed mode does. Flatten loopRuntime.sandbox.sandboxService.* into loopRuntime.sandbox.* and expose the sandbox egress mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop checks the runtime already enforces (urls type, egress mode, egress port collision, TLS key names), inline the one-use egress Service name helper, and shorten the values comments and README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rkers Loop Runtime on AWS stays on ECS through the Terraform module, so the chart now rejects every cloud except google. Workers run in the isolated GKE cluster from the GCP Terraform module; the runtime reaches them through the discovery endpoint and exposes its Git forward proxy on an internal load balancer bound to an address Terraform reserves. - Drop AWS (IRSA, s3:// URIs), sandbox.urls, and the EKS example - Always mount the client TLS Secret and run the egress gateway - Match the ECS Sandbox Service environment and pinned images - Default runtime pods to 1 vCPU / 2 GiB with two replicas - Add a GKE example with a TLS Secret script and worker chart values Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The worker chart expects node-local storage at /var/lib/braintrust-sandboxes/1, /run/netns, and /dev/kvm. EKS node bootstrap provides these; GKE node pools have no bootstrap hook, so the example adds a node-init DaemonSet that formats and mounts the raw local SSDs (striping them when there are several) in the host mount namespace. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Give pods 120s to stop and set the runtime's 90s drain explicitly - Pass FUNCTION_SECRET_KEY so stored MCP credentials decrypt - Render ORG_NAME as * when orgName is empty, as the API does - Bound the Brainstore file cache, size-limit the state and cache emptyDirs, and request matching ephemeral storage - Reject blank, null, padded, and non-CIDR egress source ranges - Node init: mount the SSD at the parent and create the worker directory on it, so workers cannot start before the mount - TLS script: check both clusters first and write the worker server Secret last, so a failed run can be retried - Note the dependency on terraform-google-braintrust-data-plane#28 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An empty, null, or missing repository or tag rendered an invalid image reference that only failed when pods started. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Model loopRuntime on the AI Gateway: the same value shapes, probes in values, and fixed defaults for settings with one correct value (state and cache paths and limits, security contexts, the 120s grace period, the TLS Secret name, the Git proxy port, in-cluster service URLs). Drop knobs no install needs and the tests that only covered them, and shorten the docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c089dfa to
0d3f9ab
Compare
Erik Weathers (erikdw)
left a comment
There was a problem hiding this comment.
Re-reviewed at 0d3f9ab now that #101 is in. The earlier asks are done: the CIDR parser is gone with the non-empty sourceRanges check kept, and sandbox.image is required.
Before merge (2):
- A quoted service port renders invalid probes (
loop-runtime-deployment.yaml). - Document
BRAINTRUST_APP_URLfor EU / custom control planes (example README).
Optional (3): the port-range check, the whitespace checks, and consolidating the nested-default tests. These are optional simplifications. Keep them if the earlier errors or coverage justify the code.
| {{- with index $.Values.loopRuntime $name }} | ||
| {{- $probe := deepCopy . -}} | ||
| {{- if and (kindIs "map" $probe.httpGet) (not (hasKey $probe.httpGet "port")) -}} | ||
| {{- $_ := set $probe.httpGet "port" $.Values.loopRuntime.service.port -}} |
There was a problem hiding this comment.
Before merge: a quoted port breaks both probes.
With loopRuntime.service.port: "4001" (or --set-string loopRuntime.service.port=4001), the port check passes and containerPort renders 4001. This line copies the string, so both probes render port: "4001". Kubernetes reads a string probe port as a port name, which must contain a letter, so the API server rejects the Deployment.
| {{- $_ := set $probe.httpGet "port" $.Values.loopRuntime.service.port -}} | |
| {{- $_ := set $probe.httpGet "port" (int $.Values.loopRuntime.service.port) -}} |
Plus one test: a quoted port, asserting livenessProbe.httpGet.port is 4001. Explicit probe ports aren't affected.
There was a problem hiding this comment.
Fixed in 622d883. Automatically populated HTTP probe ports are now converted to integers, including quoted Service ports. The existing custom-port test now uses a quoted value and checks both liveness and readiness; explicit numeric and named probe ports remain unchanged.
| --namespace braintrust-sandbox --values braintrust/examples/gke-loop-runtime/sandbox-service.yaml --wait | ||
| ``` | ||
|
|
||
| 3. Install this chart with your GKE values plus `values.yaml` from this directory. |
There was a problem hiding this comment.
Before merge (docs only): EU / custom control planes.
Loop Runtime doesn't set BRAINTRUST_APP_URL, so it uses the runtime default https://www.braintrust.dev (brainstore/agent_runtime/src/cli.rs) to authenticate with the control plane. Setting aiGateway.braintrustAppUrl doesn't carry over. loopRuntime.extraEnvVars already handles it, so a note here is enough:
loopRuntime:
extraEnvVars:
- name: BRAINTRUST_APP_URL
value: https://<your control plane>Wiring it from aiGateway.braintrustAppUrl instead would also work. Your call.
There was a problem hiding this comment.
Fixed in 622d883. Loop Runtime now inherits aiGateway.braintrustAppUrl, and the example documents that shared setting for EU/custom control planes. loopRuntime.extraEnvVars can still override BRAINTRUST_APP_URL. Tests cover the default and custom URL, an unset URL falling back to the runtime default, and the explicit environment override.
| {{- end -}} | ||
| {{- end -}} | ||
| {{- $port := toString $runtime.service.port -}} | ||
| {{- if or (not (regexMatch "^[1-9][0-9]{0,4}$" $port)) (gt (int $port) 65535) -}} |
There was a problem hiding this comment.
Optional: drop the port-range check. Kubernetes admission already rejects a containerPort or Service port of 0 or >65535, and nothing else in the chart range-checks ports. Dropping it would also remove should reject a zero runtime port / above the maximum. Either way, the int fix above is still needed.
There was a problem hiding this comment.
Removed in 622d883, along with the zero/out-of-range rejection cases. Kubernetes validates Service and container ports. The quoted-port probe fix remains covered.
| {{- fail (printf "%s is required" $path) -}} | ||
| {{- end -}} | ||
| {{- /* Padding would otherwise be rendered into identities and endpoints unchanged. */ -}} | ||
| {{- if ne $value (trim $value) -}} |
There was a problem hiding this comment.
Optional: drop the whitespace checks (here, and the ne ... trim half of line 172). No other value in the chart is checked for padding, and a padded service account or URL would fail visibly at Workload Identity or DNS anyway. Keep the empty-string checks.
There was a problem hiding this comment.
Removed the leading/trailing padding checks in 622d883, including the nested-default check. Required-value and empty/whitespace-only checks remain; padded strings are passed through unchanged.
| path: data.LOOP_RUNTIME_ENABLE_SANDBOX_ADMIN | ||
| value: "false" | ||
|
|
||
| - it: should explain reused values without an image pull policy |
There was a problem hiding this comment.
Optional: consolidate the nested-default tests without losing partial-map coverage. The four null-value tests for pullPolicy, service.type, serviceAccount.name and portName (lines 96–113 and 120–125) all hit the same loop at _helpers.tpl:171. Keep without resource defaults (114): it's the outer-map check.
Keep the guard itself. It catches --reuse-values from a pre-Loop chart plus a partial --set, e.g. --set loopRuntime.enabled=true --set loopRuntime.service.port=4001, which leaves a service map without type/portName. The outer-map check misses that.
There was a problem hiding this comment.
Consolidated in 622d883 into one incomplete-nested-maps case. The nested-default guard remains, along with the separate missing outer-resource-map case and the pre-Loop retained-values coverage.
|
Codex (@codex) review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback". |
Adds an opt-in Loop Runtime for GKE. Sandboxes run on Sandbox Service workers in a separate isolated GKE cluster.
On AWS, Loop Runtime stays on ECS through the Terraform module, so the chart rejects other clouds.
The GCP Terraform module supplies the worker cluster, discovery zone, Loop Runtime service account, and reserved egress address.
The worker chart and the worker, runtime, and guest images use the same published build, available for amd64 and arm64.
Loop requires API feature version 82+ and AI Gateway routing; the GKE example enables the gateway.
It uses the GKE Standard selectors from #101 and pairs with Terraform #28. Merge Terraform #28 first; the docs use its outputs.
discovery.endpointand mounts thesandbox-client-tlsSecret.<service>-egress: an internal load balancer on the reserved address that admits only worker ranges.LOOP_RUNTIME_URL; the VirtualService routes/loop/runtime.examples/gke-loop-runtimesupplies worker values and the mTLS Secret setup script.Input matrix
loopRuntime.enabledfalse, orloopRuntimemissing (--reuse-values)--reuse-values --set loopRuntime.enabled=truewith missing default maps or required nested fields--reset-then-reuse-valuescloud: aws,azure, or emptyreplicasunset / null / empty /0loopRuntime.service.port, including quoted numeric valuesaiGateway.braintrustAppUrl: default/custom URL; missing/null/emptyloopRuntime.extraEnvVarscan overrideBRAINTRUST_APP_URLloopRuntime.enableSandboxAdminmissing / null / false / trueglobal.orgNameemptyORG_NAME: "*"locksBackend: objectStorage/redis1.234567e+06), like every image in the chart; quote all-digit tagsLarge histories and higher concurrency still need load testing; worker uninstall has not been exercised.
./test.shpasses (491 Helm cases, 33 Python tests, lint, cloud renders).The TLS script retains the prior partial-failure recovery flow and uses server-side apply to remove legacy key-bearing annotations.
🤖 Generated with Claude Code