Repository navigation
Conversation
Add image-only UpdateSandbox with conditional versions, durable replay, fenced pending intent, and Kubernetes storage retention. Enable non-destructive image repair after completed timeout cleanup and exercise both Kubernetes controller API lanes. Closes NVIDIA#4623 Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
OpenClaw-Publication: b2036b09-b697-4d65-aba9-5c96a3198f9c Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
Move image updates to a separate authenticated service and retain the baseline public sandbox status. Persist image intent and revisions independently with atomic baseline-known lifecycle fencing and explicit post-upgrade activation. Preserve concurrent provisioning clocks, fence replaced attempts and delayed callbacks, and verify private-state transactions plus both Kubernetes controller API lanes. Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
Move image updates to a separate optional compute-driver service, preserving the baseline generated trait and streaming associated type. Add source and TCP/UDS transport regressions, capability negotiation coverage, and a default-unsupported in-process extension. Keep Go compatibility tests outside generated output and update production reflection expectations for the separate sandbox update service. Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
Restore the original in-process driver trait object and existing instance enum. Register optional image updates through a default factory hook without requiring changes to existing drivers or factories, and require registration plus capability before mutation. Accept OpenShell-only descriptors while retaining strict validation of present update services. Cover baseline registration, missing-updater no-write behavior, descriptor validation, and both live Kubernetes retention lanes. Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
Drop the task-level fallback superseded by main's runner defaults and explicit e2e-conformance-only selector. Preserve current upstream task definitions and recovery waiver unchanged. Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
Adapt non-launch image updates to the upstream remote admission API while retaining resource-policy verification before forwarding. Cover both negotiated extensions and preservation of backend pins and persisted launch identity. Signed-off-by: sjenning <493891+sjenning@users.noreply.github.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
|
/ok to test 42ae543 |
|
Label |
|
Label |
mrunalp
left a comment
There was a problem hiding this comment.
Review summary
This adds a new SandboxUpdates.UpdateSandbox RPC. It changes a stopped sandbox's image, command, environment, labels and annotations, and keeps its ID, custom resource and PVC. The sandbox stays stopped. Only the Kubernetes driver implements it, and it is off by default behind enable_sandbox_updates.
The core mechanics look sound:
- Accept and complete are each a single transaction, with resource-version checks.
- The stopped state is re-checked inside the transaction.
request_idpayloads are fingerprinted.- Private state is keyed by sandbox UUID and removed on delete.
validate_onlywrites nothing.- PVC and volume templates are untouched.
- The supervisor command and
OPENSHELL_*env are protected. - Older drivers and gateways still negotiate correctly.
The concerns below are mostly about recovery and operability.
Concerns
1. A sandbox can become impossible to start, stop or delete.
Before this PR, an operator could always delete a sandbox once its driver work had settled. With this PR, an accepted update blocks Start, Stop, Delete and the GC/TTL sweeps (compute/mod.rs:1900, 2103, 3097, 3253). The only way out is retrying UpdateSandbox with exactly the same fields, and nothing clears the intent in the background.
That retry is refused before the pending path is reached (compute/image_update.rs:58-87) in any of these cases:
- the operator turns the flag back off;
- the Postgres check in (3) fails;
- the driver stops advertising the capability.
A driver error that repeats on every retry has the same effect, for example a precondition failure after an out-of-band change to the custom resource. The only fix left is editing the database by hand.
Suggestion: let completion of an existing intent skip the activation gates, and add an abort or force-delete path.
2. Recovery needs state the client can't see.
The retry must match the stored intent's field selection, including whether each optional field was present or absent (image_update.rs:124-133). That intent is private, and the accept step has already written the target values into the public spec, so a different client, or one that lost the original request, can't rebuild it.
For example, sending image + command with identical values when the original sent only image fails with "another sandbox update is pending". Reusing the original request_id returns REQUEST_OUTCOME_UNCERTAIN. At minimum, the error should include the pending selection.
3. The Postgres activation check blocks updates in common deployments.
persistence/image_activation.rs:41: any client backend on the same database without the openshell-image-update-v2 marker refuses every update, including pending retries. That covers psql sessions, pg_dump, postgres_exporter and managed-DB agents. Two more ways the check misfires:
- Rows with
backend_type IS NULLcount against it. Those are probably the sessions the gateway role can't fully see, which is common on managed Postgres withoutpg_read_all_stats. - The marker is set with
set_configafter connecting (postgres.rs:33), so each fresh gateway connection briefly looks unmarked.
The marker also unconditionally overwrites any application_name the operator set in the DSN, even with updates disabled, which affects their database monitoring.
Combined with (1), a stray admin session can freeze a pending sandbox. Suggestion: set application_name through PgConnectOptions, and make the check advisory or run it only at activation.
4. Updates are refused (HTTP 403) in the documented split gateway/workspace Helm setup.
The driver now lists every Pod in the namespace (driver-kubernetes/src/image_update.rs:235-245). The openshell-workspace Role grants pods: [get] only (deploy/helm/openshell-workspace/templates/role.yaml:39-45). In shared mode with workspaceResources.enabled=false, every UpdateSandbox call fails as an internal error, the validate_only preflight included. CI uses the default chart, so it doesn't catch this.
Listing by label plus get_opt on recorded pod names would avoid needing list. It would also cut the full-namespace scan, which can hit the 30s API timeout in large shared namespaces.
5. Annotation updates can turn an ephemeral sandbox persistent, and some user keys get locked.
Before this PR, user metadata was only set at create. Now:
- An
annotationsupdate that leaves outopenshell.nvidia.com/retention=ephemeraldrops it, because that key isn't treated as system metadata (core/src/sandbox_update.rs:10, read atcompute/mod.rs:5698). The sandbox silently stops being auto-deleted. - Create only reserves
internal.openshell.ai/(grpc/validation.rs:485), but update protects all of*.openshell.ai/and*.agents.x-k8s.io/. User keys created under those domains can never be changed or removed.
6. Label-only or annotation-only updates go through the driver and the lock.
These are gateway-only metadata and don't touch pod metadata, yet they still require the configuration capability and a no-op driver RPC, and they set the pending flag (image_update.rs:46-54). So a metadata-only edit returns UNIMPLEMENTED on an image-only driver, and it's exposed to (1). It could be a plain conditional write on the sandbox record.
7. Docs and code describe a "protocol 2" that replaces something never released.
main has no image-update code at all, yet the docs, skills and server README talk about "protocol 2", "gateways that previously supported image-only updates" and "legacy image-only intents". Operators will look for a v1 that doesn't exist, and the compatibility branches are dead code. Collapsing to a single protocol and a single driver RPC (the configuration request already embeds the image request) would simplify both.
8. Bundled lifecycle change.
Stop on a timed-out Error sandbox whose cleanup has finished now writes Stopped directly, without a driver stop. Driver Deleted events no longer prune timed-out Error/Stopped records (compute/mod.rs:1916-1969, 5734-5741). This may be intended for image repair, but it's a separate behavior change and deserves its own description or PR. The heading rewrite in configuration.mdx:1507 is unrelated too.
9. E2E cost.
Adding openshell-sdk and openshell-bootstrap to the e2e-kubernetes feature pulls about 900 lockfile lines (proto codegen, OIDC, tunnel deps) into every Kubernetes e2e binary. The test also re-implements gateway discovery and mTLS loading that tests/testkit already provides, so it would fit better as a #[testkit::test] suite. It pulls images from docker.io, so Docker Hub rate limits apply.
Smaller items
- Two HA replicas retrying at the same time: the one that loses the row lock reports a conflict even though the update succeeded (
sandbox_image.rs:228). - The
UNIMPLEMENTEDmessage says "image updates" even when the missing capability is the configuration one (image_update.rs:79). UpdateSandboxConfigurationskips the RPC tracer (driver-kubernetes/src/grpc.rs:333).- Label maps are checked against the annotation entry limit (
grpc/sandbox.rs:405). metadata.uidin the Kubernetes merge patch isn't a real precondition;resourceVersiondoes the fencing, so the README's "UID-fenced" claim overstates it.- The protocol
2is hard-coded in two places (gateway_members.rs,image_activation.rs:98).
Summary
Add support for updating a stopped sandbox’s image and configuration while preserving its identity and persistent storage. A new image may require different environment variables or startup commands, so these fields can be updated together before the next start. Labels and annotations are included for completeness and consistency with the sandbox resource.
Related Issue
Closes #4623
Changes
Release Notes
Kubernetes sandboxes can now update their image, command, environment, and metadata without recreating the sandbox or losing persistent storage. Updates require operator activation and a stopped sandbox.
Testing
Checklist