feat(docker): add support for sandboxes from docker archives#2361
feat(docker): add support for sandboxes from docker archives#2361feloy wants to merge 2 commits into
Conversation
Add support for loading pre-built Docker archives into the local daemon when creating sandboxes, enabling air-gapped and macOS VM-based build pipelines where a Docker archive is the natural hand-off artifact. Closes NVIDIA#2175 Signed-off-by: Philippe Martin <phmartin@nvidia.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Verify the full round-trip: docker build → docker save → openshell sandbox create --from archive.tar → marker file present in output. Signed-off-by: Philippe Martin <phmartin@nvidia.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
drew
changed the title
| lower.ends_with(".tar.gz") | ||
| || Path::new(&*lower) | ||
| .extension() | ||
| .is_some_and(|ext| ext == "tar" || ext == "tgz") |
There was a problem hiding this comment.
Question: Why use ends_with in one case and `.extension for the other? Can we use the same logic for all cases?
Also we already have a path? Can't we call .extension() directly on that and then map the resultant string through to_lowercase()?
| /// 1. Existing file whose name contains "Dockerfile" → build from file. | ||
| /// 1b. Existing file with `.tar`/`.tar.gz`/`.tgz` extension → load archive. |
There was a problem hiding this comment.
Split into: A single heading like "Existing file" followed by two points indicating the different sources.
| /// Resolution order: | ||
| /// 1. Existing file whose name contains "Dockerfile" → build from file. | ||
| /// 1b. Existing file with `.tar`/`.tar.gz`/`.tgz` extension → load archive. | ||
| /// 2. Existing directory that contains a `Dockerfile` → build from directory. |
There was a problem hiding this comment.
Question: Why are two "Dockerfile" cases now separated by a archive case? Should the explicit Dockerfile and Dockerfile parent folder cases not be treated the same way?
| /// 1. Existing file whose name contains "Dockerfile" → build from file. | ||
| /// 1b. Existing file with `.tar`/`.tar.gz`/`.tgz` extension → load archive. | ||
| /// 2. Existing directory that contains a `Dockerfile` → build from directory. | ||
| /// 3. Missing explicit local paths → local error, not image pull. |
There was a problem hiding this comment.
Out-of-scope: What are "Missing explicit local paths"?
| @@ -2610,6 +2625,19 @@ fn filename_looks_like_dockerfile(path: &Path) -> bool { | |||
| lower.contains("dockerfile") || lower.ends_with(".dockerfile") | |||
There was a problem hiding this comment.
Out of scope, but the || lower.ends_with(".dockerfile") is redundant since lower.contains("dockerfile")` will ALWAYS be true in that case.
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Thanks for adding archive-based sandbox sources. I think we need to address two issues before merging this:
-
Image ingestion is not compute-driver aware. The current check only distinguishes local from remote gateways. Both Dockerfile builds and this new archive loader use Bollard’s local Docker connection, then assume the resulting image is visible to the active driver. That works reliably for Docker, but not for Podman unless the CLI’s
DOCKER_HOSThappens to point to the exact Podman socket used by the gateway. The VM driver also has its own Docker/Podman resolution behavior. We should either introduce a driver-aware image-ingestion path or explicitly restrict local Dockerfile/archive sources to confirmed Docker-backed gateways. -
The archive is buffered entirely in memory.
std::fs::read()synchronously loads the complete archive before uploading it. Container archives can be several gigabytes. Bollard providesimport_image_stream(), which should be used with an asynchronous, chunked file stream.
These are closely related: Dockerfiles and archives are both ways of ingesting a local image into the active compute runtime. I suggest establishing that capability boundary first, then implementing archive loading through it. A smaller alternative would be to scope this PR explicitly to Docker-backed gateways, reject other drivers clearly, and still switch the upload to the streaming API.
3 participants
Summary
Add support for Docker archives (
.tar,.tar.gz,.tgz) as--fromsources onsandbox create. This enables air-gapped environments and VM-based build pipelines where a Docker archive is the natural hand-off artifact.Related Issue
Closes #2175
Changes
DockerArchive { path }variant toResolvedSourcein the CLI.tar/.tar.gz/.tgzfiles inresolve_from()via extension-based matching (consistent with the existing Dockerfile filename heuristic)load_docker_archive()inopenshell-bootstrapusing Bollard'simport_image()(POST /images/load), parsing the image tag from Docker's"Loaded image: <tag>"responseload_from_docker_archive()in the CLI with the same local-gateway guard as Dockerfile sources--fromhelp text and published docs to mention Docker archive supportNo proto, gateway, or server changes — the archive's embedded tag flows through the existing
SandboxTemplate.imagefield.Testing
mise run pre-commitpassesresolve_fromcorrectly classifies.tar,.tar.gz,.tgzfiles asDockerArchivefilename_looks_like_docker_archivedetects valid extensions (case-insensitive) and rejects non-archivesChecklist