Enhance sandbox pause/resume#1149
Conversation
- Added OSEP-0013: Isolated Execution API (implementing, due 2026-06-23) - Added OSEP-0014: Multi-Tenancy Support for Kubernetes Runtime (draft, due 2026-05-07) - Added OSEP-0015: Spec-Driven Pod Snapshot for Pause and Resume (draft, due 2026-06-27)
ryanzhang-oss
requested review from
Generalwin,
Pangjiping,
Spground,
fengcone,
hittyt,
jwx0925,
kevinlynx and
ninan-nn
as code owners
|
Changed directories: oseps. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use |
There was a problem hiding this comment.
Pull request overview
This PR updates the OSEP index and introduces a new enhancement proposal documenting a spec-driven Kubernetes pause/resume design using inline snapshot state on BatchSandbox plus new SnapshotClass/SnapshotClaim CRDs.
Changes:
- Add OSEP-0013/0014/0015 entries to
oseps/README.md. - Add new proposal document
oseps/0015-pod-snapshot.mddescribing pod snapshot-based pause/resume semantics and APIs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| oseps/README.md | Extends the OSEP index table with entries for OSEP-0013 through OSEP-0015. |
| oseps/0015-pod-snapshot.md | Adds the full OSEP-0015 draft describing the proposed pause/resume and snapshot CRD model. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| routes in `server/opensandbox_server/api/lifecycle.py` currently accept only the | ||
| path parameter and `X-Request-ID`. This OSEP updates those definitions to accept | ||
| an optional `PauseSandboxRequest` body. Omitting the body, sending `{}`, or using | ||
| old SDKs remains valid: |
| |---|---|---|---|---| | ||
| | `Stop` | Delete the Pod and release compute. | Lost unless on a retained PVC (OSEP-0003). | Lost | Supported (no snapshot, no `SnapshotClass` required) | | ||
| | `Freeze` | Keep the Pod, freeze container cgroups. | Kept | Kept (in node RAM) | Reserved (Kubernetes freeze is future work) | | ||
| | `Hibernate` | Capture full pod state via the snapshot Job, then delete the Pod. | Persisted as checkpoint artifact | Persisted as checkpoint artifact | **Implemented** (replaces the OSEP-0008 path) | |
| This revision implements one snapshot mechanism: a same-node full-pod-state | ||
| snapshot Job. The `type` enum is the extension point for future backends without | ||
| changing the `BatchSandbox` |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3285b20c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 9. Ctrl status.snapshot.phase: Pending -> Committing -> Ready, | ||
| artifacts[] populated | ||
| 10. Ctrl Delete the Pod, status.phase = Paused. |
There was a problem hiding this comment.
In clusters where the sandbox pod is evicted or recreated while the checkpoint job is running, this flow can record an artifact for one pod and then delete/report Paused for a replacement pod with the same name. Since the proposal records sourcePodUID, require the controller to compare that UID with the current live pod before deletion and fail closed if it changed, otherwise resume can restore stale state and lose writes from the replacement pod.
Useful? React with 👍 / 👎.
| When no `SnapshotClass` is referenced by that claim and none is annotated | ||
| default, the controller synthesizes an implicit default class from the legacy | ||
| startup flags (`--snapshot-registry`, | ||
| `--snapshot-registry-insecure`) so existing clusters keep working with zero |
There was a problem hiding this comment.
Existing OSEP-0008 deployments commonly require --snapshot-push-secret and --resume-pull-secret (the controller flags and pause-resume guide both document them), but this zero-config migration only synthesizes defaults from --snapshot-registry and --snapshot-registry-insecure while the new design forbids Kubernetes Secrets. Authenticated registries will stop working after rollout unless the migration preserves those credentials somehow or the proposal removes the zero-config compatibility claim.
Useful? React with 👍 / 👎.
| metadata: | ||
| name: sandbox-abc123-pause-20260627 | ||
| namespace: default | ||
| labels: | ||
| opensandbox.io/sandbox-id: sandbox-abc123 | ||
| opensandbox.io/generated-from-template: tenant-s3 | ||
| spec: | ||
| snapshotClassName: s3 | ||
| parameters: | ||
| region: us-west-2 | ||
| bucket: opensandbox-snapshots | ||
| prefix: tenants/default/sandbox-abc123/pauses/20260627 |
There was a problem hiding this comment.
When the same sandbox pauses from the same template more than once on the same day, this generated claim name and prefix collide with the previous pause. That conflicts with the design's rule that per-pause values must create a new claim instead of mutating a shared one, and with the e2e expectation that two template pauses produce distinct claims/prefixes; include a generation, timestamp with sufficient granularity, or nonce in both the claim name and default prefix.
Useful? React with 👍 / 👎.
| The implementation updates these source definitions: | ||
|
|
||
| - `specs/sandbox-lifecycle.yml`: add optional `requestBody` for | ||
| `/sandboxes/{sandboxId}/pause` that references `PauseSandboxRequest`; keep | ||
| `/resume` bodyless in this revision. | ||
| - `server/opensandbox_server/api/schema.py`: add Pydantic models for | ||
| `PauseSandboxRequest`, `SnapshotPauseRequest`, and | ||
| inline/generated claim options. | ||
| - `server/opensandbox_server/api/lifecycle.py`: accept | ||
| `body: PauseSandboxRequest | None = Body(None)` on `pause_sandbox`. | ||
| - Kubernetes provider/runtime code: materialize the selected/generated | ||
| `SnapshotClaim`, then patch the `BatchSandbox`. |
There was a problem hiding this comment.
This implementation plan updates the OpenAPI spec, FastAPI schema, route, and Kubernetes provider, but omits SDK interfaces. Current supported SDKs expose PauseSandbox(id) and send no request body, so advanced callers still cannot choose Stop, a claim, or a template through the public clients even though the proposal says new clients can opt in; add generated/handwritten SDK model and method updates to the required source changes.
Useful? React with 👍 / 👎.
ryanzhang-oss
force-pushed
the
copilot/worktree-2026-06-27T20-56-53
branch
from
2e3285b to
496889b
Compare
github-actions
Bot
added
the
documentation
ryanzhang-oss
force-pushed
the
copilot/worktree-2026-06-27T20-56-53
branch
from
496889b to
e91cd5a
Compare
|
This is useful as a future Hibernate/checkpoint + pluggable snapshot backend design, but the motivation should not frame it as fixing a current server-side Please also preserve the current pool-mode pause/resume behavior. Sandboxes created with |
ryanzhang-oss
force-pushed
the
copilot/worktree-2026-06-27T20-56-53
branch
from
e91cd5a to
1233df6
Compare
| // Snapshot references the current SandboxSnapshot result for the latest pause. | ||
| // The full per-container artifact detail lives on that object. | ||
| // +optional | ||
| Snapshot *SnapshotStatusRef `json:"snapshot,omitempty"` |
There was a problem hiding this comment.
The description of SnapshotStatusRef here is inconsistent with the description in the Kubernetes Resource Overview section.
| |---|---|---|---|---| | ||
| | `Stop` | Delete the Pod and release compute. | Lost unless on a retained PVC (OSEP-0003). | Lost | Supported (no snapshot, no `SandboxSnapshotClass` required) | | ||
| | `Freeze` | Keep the Pod, freeze container cgroups. | Kept | Kept (in node RAM) | Reserved (Kubernetes freeze is future work) | | ||
| | `Hibernate` | Capture full pod state via the snapshot Job, then delete the Pod. | Persisted as checkpoint artifact | Persisted as checkpoint artifact | **Implemented** (replaces the OSEP-0008 path) | |
There was a problem hiding this comment.
Maybe I should add a mode that only preserves the filesystem. In current AI Agent scenarios, recovery can be done solely through the filesystem, without needing memory?
|
|
||
| OpenSandbox keeps its existing `pause`/`resume` vocabulary instead of upstream's | ||
| `suspend`/`resume`, but uses an enum rather than a boolean for the CRD field. | ||
| Because OpenSandbox also needs future `Freeze`, it uses `operatingMode` as the |
There was a problem hiding this comment.
Unless it offers significant memory or disk resource benefits, the Freeze operation seems unnecessary. I recommend against introducing this enum until a concrete use case emerges.
| - For `KeepFS` and future `Hibernate`, the snapshot reference recorded in `status.snapshot` must be | ||
| stable and durable enough to survive Pod deletion, because resume relies on it | ||
| after the Pod is gone. | ||
| - Backend authentication is provided by node/workload identity (no Kubernetes |
There was a problem hiding this comment.
The credential model needs a concrete identity section. Saying “node/workload identity” is not enough to implement or operate this.
Which ServiceAccount or node identity does the snapshot Job use? How is tenant isolation enforced across bucket/prefix targets? How do existing --snapshot-push-secret / --resume-pull-secret deployments migrate?
There was a problem hiding this comment.
the snapshotClaim has a parameter section that can pretty much contain arbitury parameters. I would imagine that all the existing parameters will be part of a containerRegisterSnapshotClaim/Template. Does that work?
There was a problem hiding this comment.
Putting backend parameters in SnapshotClaim/Template makes sense for non-sensitive configuration, but I think the proposal still needs to define how node/workload identity actually works.
For example, does the snapshot Job run with a controller-owned ServiceAccount, a tenant namespace ServiceAccount, or node identity? How is that identity bound to cloud IAM / registry permissions, and how do we ensure a tenant cannot point a claim at another tenant’s bucket or prefix?
Without that identity and authorization model, “node/workload identity” is still too abstract to implement or operate safely.
There was a problem hiding this comment.
updated the wording, I wonder if it's clearer now
Signed-off-by: Ryan Zhang <zhangryan@microsoft.com>
Signed-off-by: Ryan Zhang <zhangryan@microsoft.com>
Signed-off-by: Ryan Zhang <zhangryan@microsoft.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee5c07221f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (`make manifests generate`). Because `operatingMode` replaces the legacy | ||
| boolean `spec.pause`, operators need a storage-version/conversion or pre-rollout | ||
| migration that maps `pause=true` to `operatingMode=KeepFS`, | ||
| `pause=false`/unset to `operatingMode=Running`, and then drops the boolean field |
There was a problem hiding this comment.
In clusters with existing automation or an older controller still using the current BatchSandbox.spec.pause field (kubernetes/apis/sandbox/v1alpha1/batchsandbox_types.go:133-139), this rollout plan removes the boolean as part of the same v1alpha1 CRD migration. Kubernetes will then prune/reject spec.pause patches while the OSEP still promises additive CRD schemas and OSEP-0008 fallback, so pause/resume can break during upgrade or rollback; keep spec.pause served/deprecated until a new storage version or dual-read compatibility window is in place.
Useful? React with 👍 / 👎.
| calls behave exactly as before. When the CRDs are **present**, the server switches | ||
| to the inline `spec.operatingMode`/`spec.snapshotStrategy` path. Both paths stay valid |
There was a problem hiding this comment.
In a staged rollout where the new snapshot class/claim CRDs are applied before the BatchSandbox CRD and controller support spec.operatingMode/spec.snapshotStrategy, this detection flips the server to the inline path solely because the claim CRDs exist. The current served BatchSandbox CRD only exposes spec.pause, so empty-body /pause would start patching fields that are rejected or pruned instead of using the promised OSEP-0008 fallback; include the BatchSandbox field/schema/controller capability in the gate, or keep the legacy path until all required capabilities are present.
Useful? React with 👍 / 👎.
| SnapshotClaimTemplate *SnapshotClaimTemplateSelector `json:"snapshotClaimTemplate,omitempty"` | ||
| } | ||
|
|
||
| // +kubebuilder:validation:Enum=Pending;Committing;Ready;Failed |
There was a problem hiding this comment.
The current SandboxSnapshotPhase enum is Pending|Committing|Succeed|Failed, and §3 explicitly says there is no Ready rename, but this type sketch introduces Ready for generated status. Implementing this would make BatchSandbox.status.snapshot.phase diverge from the referenced SandboxSnapshot and from existing controllers/tests that wait for Succeed, so pause completion and GET state mapping can disagree; keep the mirrored phase on Succeed or specify a deliberate conversion.
Useful? React with 👍 / 👎.
| type BatchSandboxStatus struct { | ||
| // ... existing fields ... | ||
| // +optional | ||
| Snapshot *SnapshotStatus `json:"snapshot,omitempty"` |
There was a problem hiding this comment.
Implementations following this sketch would put the full SnapshotStatus inline on BatchSandbox and omit the result object name, but §3 says BatchSandbox.status.snapshot should shrink to a SnapshotStatusRef while per-container artifacts live on a per-pause SandboxSnapshot. Without a status.snapshot.name, resume/history/GC cannot reliably find the latest result object, so the Go sketch should use SnapshotStatusRef here or the earlier result-object contract should be changed consistently.
Useful? React with 👍 / 👎.
| `SandboxSnapshot` pause/resume path can be decommissioned. The CRD and | ||
| reconciler may also be retained for the explicit snapshot API; this OSEP does | ||
| not delete it. |
There was a problem hiding this comment.
If the SandboxSnapshotReconciler is retained for the explicit snapshot API, it currently watches every SandboxSnapshot and dispatches handlePending for empty/Pending phases (kubernetes/internal/controller/sandboxsnapshot_controller.go:136-139). The new inline controller-created result objects still have spec.sandboxName, so without a role discriminator or disabling the old reconciler for those objects, each pause result can be consumed as a new action and start a second snapshot job against the same sandbox.
Useful? React with 👍 / 👎.
| `ResourceClaimTemplate` use. The server copies the template once per sandbox/pod, | ||
| merges any allowed `snapshotClaimTemplate.parameters` into the generated claim's | ||
| `spec.parameters`, creates a distinct concrete `SandboxSnapshotClaim` for that sandbox/pod, then writes that generated |
There was a problem hiding this comment.
When hosted deployments expose template-backed pause to untrusted API callers, merging caller-supplied parameters into the generated claim after copying the template lets the request override storage-placement keys such as bucket or prefix. The proposal recommends exposing template names while gating only inline claims, but the template spec has no allowlist/immutable-key policy, so a caller could redirect or collide snapshot artifacts anywhere the workload identity can write; define the allowed overrides in the template or reject overrides for protected keys.
Useful? React with 👍 / 👎.
| |---|---|---| | ||
| | `POST /pause` with no body | Create `SandboxSnapshot` CR + set `spec.pause` | Resolve defaults, then patch `spec.operatingMode=KeepFS` + `spec.snapshotStrategy` | | ||
| | `POST /pause` with body | Not available | Validate `operatingMode`; resolve an existing shared claim or generate one concrete claim for this sandbox/pod from a template/inline claim; patch `spec.operatingMode` and `spec.snapshotStrategy.snapshotClaimName` | | ||
| | `POST /resume` | Validate `SandboxSnapshot` Ready, set `spec.pause` | Patch `spec.operatingMode=Running` | |
There was a problem hiding this comment.
In the Pending/Committing window, this unconditional /resume mapping can patch operatingMode=Running while the same-node snapshot job is still running, despite the requirement that only one pause/resume transition be in progress. That can leave the job committing a stale pod and the controller racing between cleanup and resume; the server should return 409 for resume until the current pause reaches a terminal phase, just as the proposal already requires for overlapping pause requests.
Useful? React with 👍 / 👎.
| 1. Client POST /sandboxes/{sandboxId}/resume | ||
| 2. Server Patch BatchSandbox: spec.operatingMode = Running | ||
| 3. Ctrl Observe operatingMode=Running; if Pod absent and the current pause result is KeepFS: | ||
| - build Pod from spec.template |
There was a problem hiding this comment.
When spec.template is edited while a sandbox is Paused, this resume flow restores the old filesystem artifact into whatever template happens to be current at resume time. Because the proposal keeps the BatchSandbox alive and does not make the template immutable or record the template generation used for the snapshot, a changed image/container set can receive mismatched writable layers or miss artifacts entirely; record and restore against the snapshotted template or reject template mutations while paused.
Useful? React with 👍 / 👎.
| // it into a concrete SandboxSnapshotClaim for this sandbox/pod before setting | ||
| // SnapshotClaimName. | ||
| // +optional | ||
| SnapshotClaimTemplate *SnapshotClaimTemplateSelector `json:"snapshotClaimTemplate,omitempty"` |
There was a problem hiding this comment.
For direct Kubernetes users or automation that patch BatchSandbox.spec.operatingMode=KeepFS with snapshotStrategy.snapshotClaimTemplate (the examples show this CRD shape), this field is admitted on the CR but the proposal says only the server copies templates and the controller consumes only the final snapshotClaimName. Those callers bypass the server, so no concrete claim is created and the pause either fails or falls back to an unintended default; either have the BatchSandbox controller materialize this CRD field or keep template selection only in the public pause request body.
Useful? React with 👍 / 👎.
Summary
This pull request added
OSEP-0015for "Spec-Driven Pod Snapshot for Pause and Resume" with status "draft" and date "2026-06-27".Testing
Breaking Changes
Checklist