docker, BlobInfoCache: try to reuse blobs from set of all known compressed blobs when pushing across registries #1645

flouthoc · 2022-09-01T05:59:58Z

It seems we try to reuse blobs only for the specified registry, however
we can have valid known compressed digests across registry as well
following pr attempts to use that by doing following steps.

CandidateLocations2 now processes all known blobs and appends them
to returned candidates at the lowest priority. As a result when
TryReusingBlob tries to process these candidates and if the blobs
filtered by the Opaque set by the transport fail to match then
attempt is made against all known blobs (ones which do not belong to the
current registry).
Increase the sample set of potential blob reuse to all known
compressed digests , also involving the one which do not belong to
current registry.
If a blob is found match it against the registry where we are
attempting to push. If blob is already there consider it a CACHE HIT! and reply skipping blob, since its already there.

How to verify this ?

$ skopeo copy docker://registry.fedoraproject.org/fedora-minimal docker://quay.io/fl/test:some-tag
$ buildah pull registry.fedoraproject.org/fedora-minimal
$ buildah tag registry.fedoraproject.org/fedora-minimal quay.io/fl/test
$ buildah push quay.io/fl/test

Getting image source signatures
Copying blob a3497ca15bbf skipped: already exists
Copying config f7e02de757 done
Writing manifest to image destination
Storing signatures

Alternative to: #1461

flouthoc · 2022-09-01T07:00:19Z

@mtrmac Following patch works for me and unlike #1461 current patch works without breaking/cleaning existing written blob-info-cache-v1.boltdb.

However since CandidateLocations2 now returns all blobs with lower priority so i think some tests needs to be retrofitted, but I'll do that once you give a quick look to the patch and verify if its on the right track.

mtrmac

I think we really need to have an explicit concept of “BICLocationReference not set” (and not create those entries that for the v1 callers). Then we can avoid the O(BICLocationReference) loop and adding all the O(BICLocationReference) individual entries.
- A downside of doing just that, which I didn’t originally realize, is that we would not have any timestamp for the location-less entries.
  - We might just live with that, there are not likely to be that many digests of the same blob floating around. I think that’s good enough for now, with an explicit comment.
  - Alternatively, it would help to replace the existing knownLocationsBucket organized transport→scope→digest→(location, timestamp), with a new version organized transport→digest→scope→(location, timestamp); that way we would only need to iterate through the scopes (=registries) that contain the digest, not all scopes, scaling hopefully much better.
It seems to me this could basically be a small addition to the appendReplacementCandidates function, adding one more BICLocationReference-unknown entry, and then teaching …Prioritize… to handle that.

docker/docker_image_dest.go

pkg/blobinfocache/boltdb/boltdb.go

mtrmac · 2022-09-01T16:44:59Z

 * We might just live with that, there are not likely to be _that_ many digests of the same blob floating around.

Right now my cache doesn’t have a single digestByUncompressed record with more than one entry. That’s probably not representative, but really, would there likely to be more than 3 different gzip implementations? This might become more relevant with widespread, but not overwhelming, use of Zstd.

flouthoc · 2022-09-02T12:21:47Z

 * We might just live with that, there are not likely to be _that_ many digests of the same blob floating around.
Right now my cache doesn’t have a single digestByUncompressed record with more than one entry. That’s probably not representative, but really, would there likely to be more than 3 different gzip implementations? This might become more relevant with widespread, but not overwhelming, use of Zstd.

@mtrmac Did not completely understood this comment. Is this comment more like a future note or some change which we are expecting in this PR but since it looks like a different scope so i'm expecting that this is a note for future.

mtrmac · 2022-09-02T17:48:26Z

 * We might just live with that, there are not likely to be _that_ many digests of the same blob floating around.
Right now my cache doesn’t have a single digestByUncompressed record with more than one entry. …
@mtrmac Did not completely understood this comment.

This is my argument why we might not need to collect the timestamps to sort the location-less candidates accurately, at least in this version.

internal/blobinfocache/types.go

pkg/blobinfocache/boltdb/boltdb.go

pkg/blobinfocache/memory/memory.go

pkg/blobinfocache/boltdb/boltdb.go

docker/docker_image_dest.go

pkg/blobinfocache/memory/memory.go

flouthoc · 2022-09-05T10:19:33Z

All changes addressed except modifying heuristic check which i'll push soon.

pkg/blobinfocache/boltdb/boltdb.go

pkg/blobinfocache/memory/memory.go

docker/docker_image_dest.go

pkg/blobinfocache/boltdb/boltdb.go

pkg/blobinfocache/memory/memory.go

pkg/blobinfocache/boltdb/boltdb.go

mtrmac

The BoltDB comments mostly apply to memory equally.

pkg/blobinfocache/boltdb/boltdb.go

pkg/blobinfocache/internal/prioritize/prioritize.go

pkg/blobinfocache/memory/memory.go

pkg/blobinfocache/boltdb/boltdb.go

pkg/blobinfocache/internal/prioritize/prioritize.go

mtrmac · 2023-09-21T20:22:21Z

Yeah, I think it would be best not to worry about the new without-from feature in this PR. This is quite enough work for one PR already, and we can add that feature later as implementations appear; let’s just keep accurate comments in TryReusingBlob… to help future maintainers.

flouthoc · 2023-09-22T09:30:26Z

@mtrmac Done, addressed comments as well as some old comments. ~~I'll add new tests once overall PR looks good to you~~ ( there are already some tests in prioritize_test.go, integration tests must be added on podman side )

mtrmac

This looks reasonable overall.

I think the one major missing part is expanding the tests in pkg/blobinfocache/internal/test , so that the new code in storage backends code is tested. The logic is not quite trivial, so I’d prefer to have a good test coverage.

pkg/blobinfocache/memory/memory.go

pkg/blobinfocache/internal/prioritize/prioritize_test.go

docker/docker_image_dest.go

flouthoc · 2023-10-18T10:14:29Z

@mtrmac PTAL

flouthoc · 2023-10-30T16:15:13Z

@mtrmac PTAL

@mtrmac Friendly ping :)

mtrmac

Thanks!

Hopefully the last round…

pkg/blobinfocache/sqlite/sqlite.go

pkg/blobinfocache/internal/prioritize/prioritize_test.go

docker/docker_image_dest.go

pkg/blobinfocache/internal/test/test.go

…oss registries It seems we try to reuse blobs only for the specified registry, however we can have valid known compressed digests across registry as well following pr attempts to use that by doing following steps. * `CandidateLocations2` now processes all known blobs and appends them to returned candidates at the lowest priority. As a result when `TryReusingBlob` tries to process these candidates and if the blobs filtered by the `Opaque` set by the `transport` fail to match then attempt is made against all known blobs (ones which do not belong to the current registry). * Increase the sample set of potential blob reuse to all known compressed digests , also involving the one which do not belong to current registry. * If a blob is found match it against the registry where we are attempting to push. If blob is already there consider it a `CACHE HIT!` and reply skipping blob, since its already there. How to verify this ? * Remove all images `buildah rmi --all` // needed so all new blobs can be tagged again in common bucket * Remove any previous `blob-info-cache` by ```console rm /home/<user>/.local/share/containers/cache/blob-info-cache-v1.boltdb ``` ```console $ skopeo copy docker://registry.fedoraproject.org/fedora-minimal docker://quay.io/fl/test:some-tag $ buildah pull registry.fedoraproject.org/fedora-minimal $ buildah tag registry.fedoraproject.org/fedora-minimal quay.io/fl/test $ buildah push quay.io/fl/test ``` ```console Getting image source signatures Copying blob a3497ca15bbf skipped: already exists Copying config f7e02de757 done Writing manifest to image destination Storing signatures ``` Signed-off-by: Aditya R <[email protected]>

flouthoc · 2023-11-01T05:53:49Z

@mtrmac PTAL again.

mtrmac

Thanks!

pkg/blobinfocache/sqlite/sqlite.go

Following check is not needed and can be removed. Followup fix from containers#1645 (comment) Signed-off-by: Aditya R <[email protected]>

TomSweeneyRedHat · 2023-11-01T22:09:31Z

Nice work on this @flouthoc and @mtrmac

It seems we try to reuse blobs only for the specified registry, however we can have valid known compressed digests across registry as well following pr attempts to use that by doing following steps. * `CandidateLocations2` now processes all known blobs and appends them to returned candidates at the lowest priority. As a result when `TryReusingBlob` tries to process these candidates and if the blobs filtered by the `Opaque` set by the `transport` fail to match then attempt is made against all known blobs (ones which do not belong to the current registry). * Increase the sample set of potential blob reuse to all known compressed digests , also involving the one which do not belong to current registry. * If a blob is found match it against the registry where we are attempting to push. If blob is already there consider it a `CACHE HIT!` and reply skipping blob, since its already there. ---- ```console $ skopeo copy docker://registry.fedoraproject.org/fedora-minimal docker://quay.io/fl/test:some-tag $ buildah pull registry.fedoraproject.org/fedora-minimal $ buildah tag registry.fedoraproject.org/fedora-minimal quay.io/fl/test $ buildah push quay.io/fl/test ``` ```console Getting image source signatures Copying blob a3497ca15bbf skipped: already exists Copying config f7e02de757 done Writing manifest to image destination Storing signatures ``` Testing: containers/image#1645 Signed-off-by: Aditya R <[email protected]>

flouthoc mentioned this pull request Sep 1, 2022

docker, BlobInfoCache: try to reuse compressed blobs from all known compressed digests #1461

Closed

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 5 times, most recently from 53183a7 to 1dbb4a3 Compare September 1, 2022 06:55

mtrmac reviewed Sep 1, 2022

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 2 times, most recently from 67235a1 to 0b5a3be Compare September 2, 2022 09:46

mtrmac reviewed Sep 2, 2022

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 2 times, most recently from 0580587 to 600a8b9 Compare September 5, 2022 10:05

mtrmac reviewed Sep 5, 2022

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch from 600a8b9 to 0fcbf47 Compare September 7, 2022 09:30

mtrmac reviewed Sep 7, 2022

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 4 times, most recently from 822867a to 26a55a9 Compare September 8, 2022 07:15

mtrmac reviewed Sep 8, 2022

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 2 times, most recently from 78bdd13 to e2a1ceb Compare September 9, 2022 07:36

mtrmac reviewed Sep 9, 2022

View reviewed changes

pkg/blobinfocache/boltdb/boltdb.go Outdated Show resolved Hide resolved

pkg/blobinfocache/internal/prioritize/prioritize.go Outdated Show resolved Hide resolved

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 2 times, most recently from 6c2de11 to 7c41d67 Compare September 12, 2022 07:35

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch from f2fb916 to ca111f7 Compare September 21, 2023 10:04

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch from ca111f7 to 92cdb4f Compare September 22, 2023 09:28

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch from 92cdb4f to d99cfa9 Compare September 22, 2023 11:05

mtrmac reviewed Oct 11, 2023

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch 4 times, most recently from 4d3fe76 to a7cd83c Compare October 18, 2023 10:13

flouthoc requested a review from mtrmac October 18, 2023 10:14

mtrmac reviewed Oct 30, 2023

View reviewed changes

flouthoc force-pushed the CandidateLocations2-process-all-blobs branch from a7cd83c to 539f9ee Compare November 1, 2023 05:49

mtrmac approved these changes Nov 1, 2023

View reviewed changes

pkg/blobinfocache/sqlite/sqlite.go Show resolved Hide resolved

mtrmac merged commit 6c186a9 into containers:main Nov 1, 2023
9 checks passed

flouthoc added a commit to flouthoc/image that referenced this pull request Nov 1, 2023

blobinfocache,sqlite: remove unnecessary compression check

e405750

Following check is not needed and can be removed. Followup fix from containers#1645 (comment) Signed-off-by: Aditya R <[email protected]>

flouthoc mentioned this pull request Nov 1, 2023

blobinfocache,sqlite: remove unnecessary compression check #2168

Merged

flouthoc mentioned this pull request Nov 3, 2023

image: replace GetStoreImage with ResolveReference and bump c/image to 373c52a9466f containers/buildah#5129

Merged

flouthoc mentioned this pull request Nov 10, 2023

blobcacheinfo,test: blobs must be resued when pushing across registry containers/buildah#5153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker, BlobInfoCache: try to reuse blobs from set of all known compressed blobs when pushing across registries #1645

docker, BlobInfoCache: try to reuse blobs from set of all known compressed blobs when pushing across registries #1645

flouthoc commented Sep 1, 2022 •

edited

Loading

flouthoc commented Sep 1, 2022 •

edited

Loading

mtrmac left a comment

mtrmac commented Sep 1, 2022

flouthoc commented Sep 2, 2022

mtrmac commented Sep 2, 2022

flouthoc commented Sep 5, 2022

mtrmac left a comment

mtrmac commented Sep 21, 2023

flouthoc commented Sep 22, 2023 •

edited

Loading

mtrmac left a comment

flouthoc commented Oct 18, 2023

flouthoc commented Oct 30, 2023

mtrmac left a comment

flouthoc commented Nov 1, 2023

mtrmac left a comment

TomSweeneyRedHat commented Nov 1, 2023

docker, BlobInfoCache: try to reuse blobs from set of all known compressed blobs when pushing across registries #1645

docker, BlobInfoCache: try to reuse blobs from set of all known compressed blobs when pushing across registries #1645

Conversation

flouthoc commented Sep 1, 2022 • edited Loading

How to verify this ?

flouthoc commented Sep 1, 2022 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Sep 1, 2022

flouthoc commented Sep 2, 2022

mtrmac commented Sep 2, 2022

flouthoc commented Sep 5, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Sep 21, 2023

flouthoc commented Sep 22, 2023 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Oct 18, 2023

flouthoc commented Oct 30, 2023

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Nov 1, 2023

mtrmac left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Nov 1, 2023

flouthoc commented Sep 1, 2022 •

edited

Loading

flouthoc commented Sep 1, 2022 •

edited

Loading

flouthoc commented Sep 22, 2023 •

edited

Loading