feat(server): add sandbox templates - #2833
Conversation
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
drew
left a comment
There was a problem hiding this comment.
Inline review findings from the sandbox workload template review.
| metadata.workspace.as_str() | ||
| } else { | ||
| req.workspace.as_str() | ||
| }; |
There was a problem hiding this comment.
[P2] Honor the documented default workspace
When request.workspace is empty, the proto contract and resolve_workspace() say it defaults to default, but this code instead uses template.metadata.workspace. Reusing a template returned from another workspace through a default-workspace SDK method can therefore create the resource in the wrong workspace. Resolve solely from req.workspace, then reject or ignore mismatched embedded metadata.
| policy: spec.policy, | ||
| }, | ||
| workloadTemplateName: spec.templateName, | ||
| }); |
There was a problem hiding this comment.
[P2] Allow template-backed creates in non-default workspaces
SandboxTemplateClient can create templates in an explicit workspace, but SandboxClient.createFromTemplate has no workspace field and omits CreateSandboxRequest.workspace. Consequently, the curated TypeScript SDK can create a template in team-a but cannot create a sandbox from it. Add workspace to SandboxFromTemplateSpec and forward it here.
| &workspace, | ||
| &resolved.encode_to_vec(), | ||
| labels_json.as_deref(), | ||
| WriteCondition::MustCreate, |
There was a problem hiding this comment.
[P2] Make the workspace quota atomic
The count and MustCreate write are separate operations. Concurrent creates near the 1000-template limit can all observe available capacity and then succeed, exceeding the documented safety limit. Enforce the quota transactionally or serialize template admission per workspace.
| environment: { FEATURE_FLAG: 'on' }, | ||
| resources: { cpu: '1', memory: '512Mi' }, | ||
| }, | ||
| driverConfig: { kubernetes: { runtime_class_name: 'kata-containers' } }, |
There was a problem hiding this comment.
[P2] Fix the Kubernetes driver-config example
The Kubernetes driver uses deny_unknown_fields and expects kubernetes.pod.runtime_class_name. This example supplies runtime_class_name directly beneath kubernetes, so template creation succeeds but sandbox creation fails driver validation. sdk/go/docs/src/api/sandbox-templates.md:40 has the same issue and also uses camelCase.
|
|
||
| resolvedSpec := sandboxSpecFromWorkloadTemplate(template) | ||
| resolvedSpec.Providers = copyStringSlice(spec.Providers) | ||
| resolvedSpec.Policy = copySandboxPolicy(spec.Policy) |
There was a problem hiding this comment.
[P2] Match production governance validation in the fake
The real Go client and server reject workload fields in a template-backed create, but the fake silently discards them and succeeds. Consumer tests can therefore pass with an Environment, Template, LogLevel, or GPU override that fails in production. Apply the same validation before resolving the template.
There was a problem hiding this comment.
The independent pass found the same parity problem on the adjacent template CRUD fake: fake.NewClient().SandboxTemplates().Create(ctx, "default", &v1.SandboxWorkloadTemplate{}) succeeds, while the gateway rejects the missing name/spec/workload (and similarly accepts invalid environment or zero-GPU inputs). Please cover both fake create paths when addressing this thread, with at least required-name/workload and governance-override parity tests.
| // Latest user-facing observed status derived by the gateway. | ||
| SandboxStatus status = 3; | ||
| // Read-only provenance for sandboxes created from a reusable workload template. | ||
| SandboxWorkloadTemplateProvenance created_from_workload_template = 20; |
There was a problem hiding this comment.
[P2] Carry provenance through the supported client surfaces
This field is introduced as read-only audit provenance, but the primary CLI and the curated Rust, TypeScript, and Python SandboxRef conversions discard it. A user who creates from a template and later uses ordinary get/list operations cannot determine the template name or revision without dropping to raw gRPC (Go is the exception). Please add an optional provenance shape to those curated references and include it in CLI sandbox detail output, or explicitly document that provenance is raw-only if that is the intended contract.
| } | ||
| if !metadata.labels.is_empty() { | ||
| obj.insert("labels".to_string(), serde_json::json!(metadata.labels)); | ||
| } |
There was a problem hiding this comment.
[P2] Include template annotations in machine-readable output
sandbox template create --annotation owner=platform persists the annotation, but this converter emits labels and omits metadata.annotations. As a result, both create/get JSON or YAML output silently lose supported metadata and cannot be round-tripped by automation. Serialize annotations alongside labels and add a focused converter test.
| let name = template.object_name().to_string(); | ||
| validate_dns1123_label(&name, "template.metadata.name")?; | ||
| let spec = sandbox_spec_from_workload_template(template)?; | ||
| validate_sandbox_spec(&name, &spec)?; |
There was a problem hiding this comment.
[Suggestion] Validate startup service-level durations at the server boundary
The CLI rejects non-positive ready_within values, but raw and generated SDK callers can persist negative or malformed protobuf durations because template validation never inspects desired_service_level. This is only a stored hint today, so it is non-blocking, but validating protobuf duration normalization and positivity here avoids admitting invalid state before warm-pool or service-level consumers begin relying on it.
Summary
Introduces reusable sandbox workload templates — workspace-scoped named resources that define a portable workload shape (image, environment, resources, driver config, service-level settings). Workspace admins create templates once; sandbox creation by template name inherits the workload while callers retain control over policy, providers, and labels.
This is a non-breaking alternative to #2781 that avoids tyign in the cleanup suggested in that PR. This does mean that another name was needed for the message representing sandbox templates, as SandboxTemplate is a inline section of the SandboxSpec. It also means the choice between template name or workload configuration is not explicit in the proto. These are aspects that can be cleanup in a subsequent breaking step when the time is right.
Proto: Four new RPCs (Create/Get/List/Delete SandboxTemplate), SandboxWorkloadTemplate resource with SandboxWorkloadConfig and SandboxResources, workload_template_name on CreateSandboxRequest (field 6),
SandboxWorkloadTemplateProvenance on Sandbox (field 20). No existing field numbers changed or reserved.
Server: Template CRUD handlers with workspace-scoped persistence, workspace deletion blocks on templates, governance/workload split enforcement on create-from-template (rejects inline environment, template,
resource_requirements, log_level), template-to-SandboxSpec resolution, provenance tracking, telemetry for WorkloadTemplate source.
CLI: openshell sandbox template create|get|list|delete subcommands, --template flag on sandbox create with conflicts_with_all against inline workload flags, table/JSON/names-only output formats.
SDKs: Full template CRUD across Rust, Go, Python, and TypeScript SDKs with workspace-scoped and default-workspace surfaces, mock/fake test infrastructure, and docs.
Related Issue
Fixes the sandbox template aspect of (#2680), though not the cleanup which would be treated as part of a wider effort across the proto surface.
Changes
Testing
mise run pre-commitpassesChecklist