-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect layer compression #957
Conversation
b1dc3f7
to
4fe708e
Compare
directory/directory_dest.go
Outdated
@@ -100,7 +100,7 @@ func (d *dirImageDestination) SupportsSignatures(ctx context.Context) error { | |||
return nil | |||
} | |||
|
|||
func (d *dirImageDestination) DesiredLayerCompression() types.LayerCompression { | |||
func (d *dirImageDestination) DesiredLayerCompression(info types.BlobInfo) types.LayerCompression { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change to the API. Would be easier to add a new Endpoint. Then we would not have to ref container/image version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, and then mark this one as Deprecated?
Any thoughts about a name? DesiredBlobCompression()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I just updated it per the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s worse than that: callers of e.g. copy.Image
can, in principle, submit their own ImageReference
implementation that returns their own ImageDestination
implementation, so adding any new methods to ImageDestination
is also an API-breaking change. (And there is at least Buildah’s blob caching that actually does provide its own implementation/wrapper.)
We’ve been talking about the need to make ImageDestination
and several other types (mostly) private for some time now; between this, the delta support, and enhancing BlobInfoCache
, we’ll need to figure this out now.
I’ll read through the various PRs and try to figure out a way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… but, looking at this PR more, this might not be quite the concern for this one.
Conceptually, whether a MIME type can be compressed is a property of either that MIME type only, or of a combination of that MIME type and the image format ~ manifest schema (types.Image
implementation); AFAICS is not really the property of the image transport (e.g. dir:
and containers-storage:
can accept/supply several different image formats).
So I think this should conceptually be a new method on types.Image
, not on ImageDestination
, unless there’s something that would make that impractical.
That still has the concern of breaking API by adding extra methods to types.Image
, but the only implementation the copy pipeline cares about is the one returned by image.FromUnparsedImage
, so we don’t have to add anything to the public API. (The ImageReference.NewImage
method is somewhat a historical artifact.)
Tentatively I’ve been thinking:
- Define an
c/image/internal/image.Image
, which embedsc/image/types.Image
but adds the necessary new method. - Move the implementation of
c/image.sourcedImage
toc/image/internal/image
; add the necessary new method there. Make the existing public API inc/image/sourced.go
compatibility wrappers for the internal versions. - (From a quick look it seems that the actual implementation would be a added to the various c/manifest types, which are not interfaces, so we can add methods safely.)
… but I still need to examine the other PRs, or try doing this myself — there may well be something about this plan that doesn’t work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems like maybe the best path forward is the original patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original patch still not solving the problem directly.
c/image needs to deal with the API anyway. It’s completely fair that you shouldn’t have to sign up for that work just to get this fix in — I’m going to be working in this area now, but I can’t promise that it will be done by any specific date.
docker/archive/dest.go
Outdated
@@ -48,7 +48,7 @@ func newImageDestination(sys *types.SystemContext, ref archiveReference) (types. | |||
} | |||
|
|||
// DesiredLayerCompression indicates if layers must be compressed, decompressed or preserved | |||
func (d *archiveImageDestination) DesiredLayerCompression() types.LayerCompression { | |||
func (d *archiveImageDestination) DesiredLayerCompression(info types.BlobInfo) types.LayerCompression { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Change see above.
docker/daemon/daemon_dest.go
Outdated
@@ -88,7 +88,7 @@ func imageLoadGoroutine(ctx context.Context, c *client.Client, reader *io.PipeRe | |||
} | |||
|
|||
// DesiredLayerCompression indicates if layers must be compressed, decompressed or preserved | |||
func (d *daemonImageDestination) DesiredLayerCompression() types.LayerCompression { | |||
func (d *daemonImageDestination) DesiredLayerCompression(info types.BlobInfo) types.LayerCompression { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Change.
As for the test, I can't reproduce. When I run
I think the problem is just that there's another zstd mime type somewhere that doesn't match, but I can't really tell. Is there some trick to getting the test suite to run locally? |
4fe708e
to
d0d543d
Compare
The various ImageDestinations know what types of layers are supported natively and how they should be compressed, but some (e.g. the OCI distribution repo or the OCI image spec) allow arbitrary blobs to be uploaded. We should not compress these blobs by default, since we don't know what they are and may already be compressed (e.g. squashfs). So, let's introduce a new DesiredBlobCompression() and deprecate DesiredLayerCompression() to allow the various backends to tell the core code about what kind of compression they want. We implement mime-type-based compression detection for the docker and OCI backends. As above, OCI supports arbitrary blobs so we shouldn't interfere with ones we don't understand. The docker backend is intended to be the way we interact with OCI dist spec repos, so we should also not compress unknown media types when talking to the docker backend. Signed-off-by: Tycho Andersen <tycho@tycho.ws>
d0d543d
to
4c5851b
Compare
You can use e.g. |
More work on handling unknown blobs / SquashFS is now happening in #1408. This specific approach is not mergeable as is due to the API break, and not correctly targeted, so closing this PR. |
A continuation of #947, this changes the OCI/docker backends to only try to compress layer types that they know about.