Respect compression_format from containers.conf in push, build --cache-to, and commit#6757
Conversation
|
While fixing the build cache, I decided to also check the pull and commit where compression is used. |
4ce00cc to
b66921a
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
/packit rebuild-failed |
|
PTAL @containers/buildah-maintainers |
| if defaultContainerConfig.Engine.CompressionLevel != nil { | ||
| compressionLevel = defaultContainerConfig.Engine.CompressionLevel | ||
| } |
There was a problem hiding this comment.
| if defaultContainerConfig.Engine.CompressionLevel != nil { | |
| compressionLevel = defaultContainerConfig.Engine.CompressionLevel | |
| } | |
| compressionLevel = defaultContainerConfig.Engine.CompressionLevel |
Definitely non-blocking, it just caught my eye, but the if is redundant, right? compressionLevel would be nil on declaration, so assigning nil to it again would not be a problem.
|
LGTM |
nalind
left a comment
There was a problem hiding this comment.
What's the intent behind not adding to "build" the flags that are being added to "push" and "commit"?
| if err != nil { | ||
| return err | ||
| } | ||
| options.CompressionFormat = &algo |
There was a problem hiding this comment.
Was the bit that checks for c.Flag("compression-format").Changed above intended to be merged into this block, to parallel the similar logic in the next block?
There was a problem hiding this comment.
Ah, that is much cleaner. I will fix that.
| if err != nil { | ||
| return err | ||
| } | ||
| options.CompressionFormat = &algo |
There was a problem hiding this comment.
Was the bit that checks for c.Flag("compression-format").Changed above intended to be merged into this block, to parallel the similar logic in the next block?
I forgot to do that. Good catch. |
|
@nalind I addressed your comments. |
|
PTAL @containers/buildah-maintainers |
|
PTAL @containers/podman-maintainers |
| $cid \ | ||
| dir:$destdir | ||
|
|
||
| run cat $destdir/manifest.json |
There was a problem hiding this comment.
This should check "$status", just in case the "dir:" format changes, however unlikely that is.
| fs.StringVarP(&flags.CWOptions, "cw", "", "", "confidential workload `options`") | ||
| fs.StringVar(&flags.CacheCompressionFormat, "cache-compression-format", "", "compression format to use for cache layers") | ||
| fs.IntVar(&flags.CacheCompressionLevel, "cache-compression-level", 0, "compression level to use for cache layers") | ||
| fs.BoolVar(&flags.CacheForceCompressionFormat, "cache-force-compression", false, "use the specified compression algorithm for cache layers if the destination contains a differently-compressed variant already") |
There was a problem hiding this comment.
build can write the final image to non-local destinations, as commit does. Could these flags also affect those cases?
There was a problem hiding this comment.
These flags are intentionally cache-only (--cache-to). The final image commit already respects the compression_format in containers.conf, and buildah push has its own --compression-format flag for the actual push. Should I clarify this in the docs, or implement flags that affect the entire build?
There was a problem hiding this comment.
Adding CLI flags to "commit" for overriding the defaults from the configuration file when committing the image, and having flags for "build" that do the same thing for cache images, but no equivalent for "build" for the final image just looks like an oversight. Most options for "commit" should also be available for "build".
There was a problem hiding this comment.
I have added support for --compression-format, --compression-level, and --force-compression to apply to the final image during buildah build, not just cache layers. These take effect when the output is a non-local destination (registry, dir:, oci-archive:, etc.).
|
PTAL @nalind |
|
PTAL @containers/buildah-maintainers @containers/podman-maintainers @mheon |
|
PTAL @containers/buildah-maintainers @containers/podman-maintainers @nalind |
nalind
left a comment
There was a problem hiding this comment.
Some questions about the tests that verify compression on --cache-to targets.
39d0f40 to
94fb09a
Compare
|
PTAL @containers/buildah-maintainers @containers/podman-maintainers @nalind |
nalind
left a comment
There was a problem hiding this comment.
Some questions about the tests.
|
PTAL @containers/buildah-maintainers @containers/podman-maintainers @nalind |
nalind
left a comment
There was a problem hiding this comment.
Some nits in the tests, but LGTM.
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Fixes: containers#6660 Fixes: containers#6072 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
|
PTAL @containers/buildah-maintainers @containers/podman-maintainers @nalind |
|
PTAL @containers/buildah-maintainers @containers/podman-maintainers for final review and merge. |
| func NewBlobCache(ref types.ImageReference, directory string, compress types.LayerCompression) (BlobCache, error) { | ||
| return imageBlobCache.NewBlobCache(ref, directory, compress) | ||
| // The optional Option values can further refine behavior. | ||
| func NewBlobCache(ref types.ImageReference, directory string, compress types.LayerCompression, opts ...Option) (BlobCache, error) { |
There was a problem hiding this comment.
(An absolutely non-blocking drive-by, by no means a review:) Maybe the Buildah callers could call the c/image package directly, without extending the API here.
There was a problem hiding this comment.
These were left here to avoid breaking callers when the package's guts were moved.
There was a problem hiding this comment.
Yes, I just meant that that doesn’t require adding more compatibility wrappers. The existing pkg/blobcache could remain as is for compatibility, and callers within Buildah could call the c/image one with the new options.
Anyway this already merged.
| RewriteTimestamp bool | ||
| CreatedAnnotation bool | ||
| SourcePolicyFile string | ||
| TransientRunMounts []string |
There was a problem hiding this comment.
Not to fix now, but I will note that I hate seeing long structs like this that aren't in alpha order. It makes debugging later down the line harder IMHO>
|
LGTM |
|
/lgtm |
buildah pushnow falls back tocompression_formatandcompression_levelfromcontainers.confwhen--compression-formatis not setbuildah build --cache-topassesCompressionFormat/ForceCompressionFormatto cache push, so cached layers use the configured compression instead of defaulting togzipbuildah buildgains--cache-compression-format,--cache-compression-level, and--cache-force-compressionflags for controlling cache layer compression independentlybuildah commitgains--compression-format,--compression-level, and--force-compressionflags withcontainers.conffallbackcommit.goswitch to setDirForceCompressfor all compression algorithms, not justgzippush.goblobcacheto treat any non-uncompressed format as requiring compression--disable-compressionwith--compression-format/--force-compressionFixes: #6660
Fixes: #6072
Depends on: containers/container-libs#731
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?