oci exporter: support exporting directories #3162
Conversation
jedevc
force-pushed
the
oci-unpack-tar
branch
2 times, most recently
from
fbb2535 to
fac3190
Compare
There was a problem hiding this comment.
I imagined this a bit differently. Instead of unpacking and transferring files via local exporter the client side could open up a contentstore (see how --cache-to type=local works) and instead of setting up tarball at all the daemon side would copy the blobs over to the client-side contentstore. So there is no tarball or extract and when exporting blobs that are already available on the client side they would be just skipped.
The current implementation is just equivalent to build | tar x iiuc.
jedevc
force-pushed
the
oci-unpack-tar
branch
3 times, most recently
from
b5ef5ea to
95e8133
Compare
|
Have done an update to use a contentstore, the oci exporter can then use the I've also added a commit to fix an inconsistency - we were prefixing the stores for local cache with |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| contentStores["export"] = cs |
There was a problem hiding this comment.
If you want to make sure this does not collide then add a random suffix and send the full contentstore id in exporter attributes.
| } | ||
| switch ex.Type { | ||
| case ExporterOCI, ExporterDocker: | ||
| if err := os.MkdirAll(ex.OutputDir, 0755); err != nil { |
There was a problem hiding this comment.
Is this a different behavior from local where directory is created early even for broken builds(not a big issue).
Also looks like local defaults to 0700? https://github.com/moby/buildkit/blob/master/session/filesync/diffcopy.go#L102
There was a problem hiding this comment.
As written the oci unpack=true is more similar to --export-cache type=local than it is to --output type=local (since it uses a content store).
See https://github.com/jedevc/buildkit/blob/oci-unpack-tar/client/solve.go#L466-L472 for how we create the directory using 0755 - maybe it's worth consistentizing all of these?
I don't think we can avoid not making the directory here, we'd otherwise have to wait until the first write to the content store by creating a new interface to defer the creation of the directory.
jedevc
force-pushed
the
oci-unpack-tar
branch
2 times, most recently
from
7a777ec to
e7064e2
Compare
This feature adds support for specifying unpack=true to the oci exporter options to unpack the resulting result for the client. To do this, we setup a content store on the client, and forward it through to the server, which can then copy the exported data into the content store. Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
This mirrors the structure of the names for the local cache directory, as well as the names for the oci exporter (when using a content store). This ensures that we cannot encounter name collisions (intentionally or unintentionally). Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
oci-unpack-tar
branch
from
e7064e2 to
2c3637f
Compare
|
Agreed, I think it might be worth revisiting some of the exporters at some point, especially |
Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
oci-unpack-tar
branch
from
819bd34 to
fea9258
Compare
|
@jedevc Note that this needs buildx update as well. |
Fixes #1219.
This feature adds support for specifying
unpack=trueto the oci exporter options to unpack the resulting result for the client.This requires changes to the client to ensure that when
unpack=trueis provided, we expose a directory instead of a single file.Because of how the containerd API is designed, the image layout file structure is only directly exposed as part of the
archiveexporter.Exportmethod which produces a tar - so we need to unpack it on the buildkit server before syncing it back to the client. I can't quite work out if there's a better way to do this without needing to change the containerd API.