manifest,zstd: give priority to `zstd` compressed images when pulling image from a manifest list #1789

flouthoc · 2023-01-12T11:31:46Z

While perform a pull from manifest list of type application/vnd.oci.image.index.v1+json make sure that if more than one image exists for the same platform then give priority to the one compressed with zstd.

Annotation io.github.containers.compression.zstd is expected to be set for the image compressed with zstd.

For users who wish to test this feature with buildah/podman

# create manifest list
buildah manifest create host/repo:latest
# create image 
buildah build -t test1 .
# create another image
buildah build -t test2 .

# add test1 to manifest list
buildah manifest add host/repo:latest sha:<test1>
# add test2 to manifest list
buildah manifest add host/repo:latest sha:<test2>

# tag zstd compressed image
buildah manifest annotate --annotation io.github.containers.compression.zstd=true host/repo:latest sha256:<test1>
# push to the repo
buildah manifest push --all host/repo:latest docker://host/repo:latest

# Fresh system !!! Pull from manifest list should give priority to zstd compressed image
buildah pull host/repo:latest

giuseppe

LGTM

manifest/oci_index.go

internal/image/fixtures/schemazstdselection.json

manifest/oci_index.go

mtrmac · 2023-01-12T15:56:55Z

Fresh system !!! Pull from manifest list should give priority to zstd compressed image

I’d expect skopeo copy [--multi-arch=system] docker://… docker://… to expose this logic without needing to wipe c/storage.

manifest/oci_index.go

flouthoc · 2023-01-13T08:58:59Z

@vrothberg @mtrmac PTAL again as discussed before I have made it configurable option and later on it can be relayed to SystemContext via copyOptions.

TomSweeneyRedHat · 2023-01-13T16:28:32Z

@flouthoc, I think you need a rebase.

manifest/oci_index.go

mtrmac · 2023-01-13T16:41:20Z

I’m afraid I’ll only return to this carefully next week.

Just to give a brief (aesthetic ?) preference: Eventually most of the compression conversion options (add a variant, drop a variant, ensure a variant exists) are going to exist in copy.Options, and make no sense outside of copy.Options. So I’m not too enthusiastic about adding an option to types.SystemContext now — we would keep it forever mostly just as an artifact of the order in which we develop things; that seems entirely avoidable. Or are we in some rush to deliver this immediately? (OTOH, admittedly, adding an option next to things like ArchitectureChoice would be fairly clean.)

So I’d prefer starting with making manifest.List private, and an option in copy.Options, not SystemContext. (In general, SystemContext is a bit of a mistake.)

But I also desire an option in a configuration file. I think that registries.conf can be a nice host for that.

But wouldn’t such an option be necessary also for the two OCI transports? That wouldn’t work with registries.conf. Especially if the thinking is that this should be a fail-safe that is disabled by default, that option seems like something that shouldn’t be transport-specific.

It might well make sense to add a registries.conf, per-scope, override mechanism, but that seems to me to be a more advanced feature, not essential for the very first version. (It would also require passing that choice through private.ImageDestination, because registries.conf is docker://-specific.)

flouthoc · 2023-01-13T17:27:01Z

@mtrmac adding it to copy.Options and passing it to manifest.List SGTM. Just a small doubt since we had a discussion on the before and there was a preference to do both of these in a different PR. Should I create a new PR for it or are you fine in this PR itself ?

Or are we in some rush to deliver this immediately?

I don't think there is a rush, I think getting this feature in the right way would be better :)

mtrmac · 2023-01-13T17:32:37Z

I’d prefer a separate PR to introduce (probably) internal/manifest.List as an extension / replacement (not 100% sure which one, preferably an extension if possible) for the stable public manifest.List; with no new added functionality.

Then this PR can add/change the internal ChooseInstance (which no longer has public API restrictions) to add a compression option, and use that from copy.

vrothberg · 2023-01-16T09:13:42Z

Extending copy.Options SGTM. That will make the behavior opt-in and address my fear.

But wouldn’t such an option be necessary also for the two OCI transports? That wouldn’t work with registries.conf.

That's a fair point. containers.conf may be better-suited for that BUT neither skopeo nor CRI-O are using it yet.

types/types.go

flouthoc · 2023-01-17T07:08:24Z

@mtrmac PR for flipping dependency and introducing internal.manifest.List #1791 PTAL

manifest/oci_index.go

manifest/list_test.go

manifest/oci_index.go

types/types.go

mtrmac

One last line I think

internal/manifest/oci_index.go

mtrmac

LGTM, assuming tests pass.

Thanks!

While perform a pull from manifest list of type `application/vnd.oci.image.index.v1+json` make sure that if more than one image exists for the same platform then give priority to the one compressed with `zstd`. Annotation `io.github.containers.compression.zstd` is expected to be set for the image compressed with `zstd`. Signed-off-by: Aditya R <arajan@redhat.com>

Instrument copy.Image to use ChooseInstanceByCompression so pulling of zstd images can be given priority. Signed-off-by: Aditya R <arajan@redhat.com>

* When d.platform == nil there is no need to iterate `len(manifests)*len(wantedPlatforms)` times and move processing for case `d.platform == nil` outside of the wanted platform loop * Retrofit tests to verify use-cases when platform is nil. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc · 2023-02-27T18:56:12Z

@giuseppe @rhatdan @vrothberg @TomSweeneyRedHat PTAL

vrothberg

LGTM, nice work!

Setting SystemContext.ArchitectureChoice to "" does not mean "match any/the first platform"; it's the default behavior of SystemContext, and it means "choose for the current runtime architecture". (Originally discussed in containers#1789 (comment) ) I.e. on amd64 these two test cases are redundant with the first two instances above, and on other achitectures (notably ARM) they cause failures. So just drop them. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Setting SystemContext.ArchitectureChoice to "" does not mean "match any/the first platform"; it's the default behavior of SystemContext, and it means "choose for the current runtime architecture". (Originally discussed in containers#1789 (comment) ) I.e. on amd64 these two test cases are redundant with the first two instances above, and on other architectures (notably ARM) they cause failures. So just drop them. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

flouthoc marked this pull request as draft January 12, 2023 11:31

flouthoc force-pushed the list-zst branch from 766bf33 to ade081b Compare January 12, 2023 11:32

flouthoc marked this pull request as ready for review January 12, 2023 11:40

giuseppe approved these changes Jan 12, 2023

View reviewed changes

vrothberg reviewed Jan 12, 2023

View reviewed changes

manifest/oci_index.go Outdated Show resolved Hide resolved

internal/image/fixtures/schemazstdselection.json Outdated Show resolved Hide resolved

flouthoc force-pushed the list-zst branch from ade081b to 484144f Compare January 12, 2023 12:25

mtrmac reviewed Jan 12, 2023

View reviewed changes

manifest/oci_index.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

flouthoc force-pushed the list-zst branch 4 times, most recently from 2284e25 to 765fc08 Compare January 13, 2023 06:52

vrothberg reviewed Jan 13, 2023

View reviewed changes

manifest/oci_index.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

flouthoc force-pushed the list-zst branch 3 times, most recently from 42b2cf4 to 25f81e7 Compare January 13, 2023 08:54

flouthoc requested review from vrothberg and mtrmac and removed request for vrothberg January 13, 2023 08:59

TomSweeneyRedHat reviewed Jan 13, 2023

View reviewed changes

manifest/oci_index.go Outdated Show resolved Hide resolved

giuseppe reviewed Jan 16, 2023

View reviewed changes

types/types.go Outdated Show resolved Hide resolved

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Feb 3, 2023

mtrmac reviewed Feb 8, 2023

View reviewed changes

manifest/oci_index.go Outdated Show resolved Hide resolved

manifest/list_test.go Outdated Show resolved Hide resolved

manifest/oci_index.go Outdated Show resolved Hide resolved

types/types.go Outdated Show resolved Hide resolved

types/types.go Outdated Show resolved Hide resolved

flouthoc force-pushed the list-zst branch from 16abd54 to d125ec8 Compare February 27, 2023 18:14

mtrmac reviewed Feb 27, 2023

View reviewed changes

internal/manifest/oci_index.go Outdated Show resolved Hide resolved

flouthoc force-pushed the list-zst branch 3 times, most recently from 1af410c to 05ea212 Compare February 27, 2023 18:36

mtrmac approved these changes Feb 27, 2023

View reviewed changes

flouthoc added 3 commits February 28, 2023 00:07

copy: Image use ChooseInstanceByCompression

a43b3c3

Instrument copy.Image to use ChooseInstanceByCompression so pulling of zstd images can be given priority. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the list-zst branch from 05ea212 to 030d369 Compare February 27, 2023 18:38

flouthoc requested review from mtrmac, giuseppe, rhatdan and TomSweeneyRedHat and removed request for mtrmac, giuseppe, rhatdan and TomSweeneyRedHat February 27, 2023 18:55

vrothberg approved these changes Feb 28, 2023

View reviewed changes

vrothberg merged commit d18058b into containers:main Feb 28, 2023

mtrmac mentioned this pull request Jul 15, 2023

Fix TestOCI1IndexChooseInstanceByCompression on non-amd64 #2043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

manifest,zstd: give priority to `zstd` compressed images when pulling image from a manifest list #1789

manifest,zstd: give priority to `zstd` compressed images when pulling image from a manifest list #1789

flouthoc commented Jan 12, 2023 •

edited

Loading

giuseppe left a comment

mtrmac commented Jan 12, 2023 •

edited

Loading

flouthoc commented Jan 13, 2023 •

edited

Loading

TomSweeneyRedHat commented Jan 13, 2023

mtrmac commented Jan 13, 2023

flouthoc commented Jan 13, 2023

mtrmac commented Jan 13, 2023

vrothberg commented Jan 16, 2023

flouthoc commented Jan 17, 2023

mtrmac left a comment

mtrmac left a comment

flouthoc commented Feb 27, 2023

vrothberg left a comment

manifest,zstd: give priority to zstd compressed images when pulling image from a manifest list #1789

manifest,zstd: give priority to zstd compressed images when pulling image from a manifest list #1789

Conversation

flouthoc commented Jan 12, 2023 • edited Loading

giuseppe left a comment

Choose a reason for hiding this comment

mtrmac commented Jan 12, 2023 • edited Loading

flouthoc commented Jan 13, 2023 • edited Loading

TomSweeneyRedHat commented Jan 13, 2023

mtrmac commented Jan 13, 2023

flouthoc commented Jan 13, 2023

mtrmac commented Jan 13, 2023

vrothberg commented Jan 16, 2023

flouthoc commented Jan 17, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Feb 27, 2023

vrothberg left a comment

Choose a reason for hiding this comment

manifest,zstd: give priority to `zstd` compressed images when pulling image from a manifest list #1789

manifest,zstd: give priority to `zstd` compressed images when pulling image from a manifest list #1789

flouthoc commented Jan 12, 2023 •

edited

Loading

mtrmac commented Jan 12, 2023 •

edited

Loading

flouthoc commented Jan 13, 2023 •

edited

Loading