Skip to content

Give executor pods an ownerReference to the Deployment - #73

Merged
justin-tahara merged 1 commit into
mainfrom
jtahara/code-exec-owner-references
Aug 17, 2026
Merged

Give executor pods an ownerReference to the Deployment#73
justin-tahara merged 1 commit into
mainfrom
jtahara/code-exec-owner-references

Conversation

@justin-tahara

Copy link
Copy Markdown
Contributor

Summary

Executor and session pods carried no ownerReferences. The service creates them
directly through the Kubernetes client rather than through a controller, and
_create_pod_manifest never set the field. Two consequences:

  • Monitoring cannot identify them. The standard "pod has an owner" test that
    distinguishes ephemeral pods from long-lived workloads does not apply, so
    code-exec-* pods raise alerts that ephemeral pods should not raise. This was
    reported by a self-hosted deployment whose alerting excludes owned pods.
  • Nothing garbage-collects leaked pods. One-shot pods run sleep 3600 with
    restartPolicy: Never and no deadline, and cleanup only happens in the
    service's finally block. A crash mid-execution leaves the pod for up to an
    hour, then a Completed pod until the cluster prunes terminated pods.

This resolves the owning Deployment once at startup and sets it as the owner of
every pod the service creates. Both code-exec-* and code-session-* pods go
through _create_pod_manifest, so both are covered.

Behavioral changes

  • Executor and session pods gain an ownerReference to the chart's Deployment
    (controller: true, blockOwnerDeletion: false).
  • The chart grants get on apps/deployments and passes the service's own
    namespace and Deployment name via the downward API. Both are gated on
    codeInterpreter.kubernetesExecutor.setOwnerReferences, default true.
  • Deleting the Deployment now cascades to in-flight executor pods. This is the
    intended cleanup.

Design notes

Why the Deployment, not the service pod. Owning by the service pod would be
cheaper — the downward API exposes metadata.uid with no extra RBAC and no API
call. But a rollout would then garbage-collect live session pods. The Deployment
is stable across rollouts.

Why the lookup never raises. It runs in __init__, and /health builds the
executor through get_executor() without a guard. A raising constructor would
turn a graceful health error into a 500 and let the liveness probe restart the
pod, so the lookup catches every exception, not only ApiException. It returns
None and pods are created exactly as before when the feature is unconfigured,
when the read is not permitted, or when the executor namespace differs from the
service namespace — Kubernetes does not honour ownerReferences across
namespaces and would read the owner as deleted, collecting the pod immediately.

Why blockOwnerDeletion: false. Setting it true requires update on the
owner's finalizers subresource, which this service does not have and should
not need.

Known corner case. The UID is resolved once. Deleting the Deployment with
--cascade=orphan and recreating it would leave a stale UID, and new executor
pods would be collected immediately. Normal deletion takes the service pod with
it and forces re-resolution on restart.

Versions

Bumps code-interpreter and the Helm chart 0.4.5 -> 0.4.6, matching the release
convention from #63/#64. executor/ is unchanged and stays at 0.4.5.

Note that docker-build-push.yml triggers on v* tags while this repo's
release tags are code-interpreter-X.Y.Z, so the image does not build
automatically. The image needs a workflow_dispatch run after merge to publish
:0.4.6 and :latest. The chart publishes automatically on merge; if it lands
first, the older image simply ignores the new environment variables.

Testing

  • New tests/integration_tests/test_kubernetes_owner_references.py: owner
    resolves correctly; returns None when unconfigured, across namespaces (and
    does not call the API), when the read is forbidden, and when the API is
    unreachable; the manifest carries the reference and omits it when unresolved.
  • Full suite goes 128 -> 135 passing. The 22 failures are pre-existing on a host
    without a Docker daemon and are identical before and after this change.
  • mypy ., ruff check ., ruff format --check . clean. helm lint clean, and
    the chart renders correctly with the flag on, off, and with a separate
    executor namespace.
  • A server-side dry-run against a live cluster confirms the API accepts this
    ownerReference shape. kubectl auth can-i get deployments.apps for the
    service account returns no today, confirming the new rule is required and
    that the fallback path is reachable during a partial upgrade.

Not yet verified end to end against a live cluster with a built image, which
needs the image published first.

Follow-ups (not in this PR)

  • The Onyx chart pins this subchart at 0.4.3 and needs a separate bump. That
    also pulls in 0.4.4/0.4.5, including the always-on deny-all-egress executor
    NetworkPolicy, so it deserves its own review.
  • The Go rewrite in Rewrite code-interpreter service in Go #65 has the same omission at
    internal/executor/kubernetes.go.
  • The v* tag filter in docker-build-push.yml does not match the repo's
    actual release tags.

Executor and session pods were created with no ownerReferences, because
the service creates them directly through the Kubernetes client rather
than through a controller. Two consequences:

- Monitoring cannot tell these short-lived pods apart from long-lived
  workloads. The usual "pod has an owner" test does not apply to them,
  so they raise alerts that ephemeral pods should not raise.
- Nothing garbage-collects pods the service fails to clean up. One-shot
  pods run "sleep 3600" with restartPolicy Never and no deadline, so a
  service crash mid-execution leaves a pod for up to an hour, then a
  Completed pod until the cluster prunes terminated pods.

Resolve the owning Deployment once at startup and set it as the owner of
every pod the service creates.

The lookup returns None, and pods are created exactly as before, when
the feature is unconfigured, when the read is not permitted, or when the
executor namespace differs from the service namespace. Kubernetes does
not honour ownerReferences across namespaces: it would read the owner as
deleted and collect the pod immediately.

Catch every exception around the lookup, not only ApiException. This
runs in __init__, and /health builds the executor through get_executor(),
so a raising constructor would turn a graceful health error into a 500
and let the liveness probe restart the pod.

blockOwnerDeletion stays false; setting it needs "update" on the owner's
finalizers subresource, which the service does not have.

The chart gates the env plumbing and the new apps/deployments rule on
codeInterpreter.kubernetesExecutor.setOwnerReferences, default true.
@justin-tahara
justin-tahara merged commit 11714f4 into main Aug 17, 2026
3 checks passed
@justin-tahara
justin-tahara deleted the jtahara/code-exec-owner-references branch August 17, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants