util/multiprovider: Implement Info by vvoland · Pull Request #4558 · moby/buildkit

vvoland · 2024-01-15T14:03:07Z

related: Update import and export to allow references to missing content containerd/containerd#9554

client.Export in v2 will need an content.Info implementation.

util/contentutil/multiprovider.go

AkihiroSuda

https://github.com/moby/buildkit/pull/4558/files#r1452964290

vvoland · 2024-01-16T09:32:42Z

Fixed, thanks for spotting!

tonistiigi

Should we maybe just switch the underlying map to InfoProvider? What would that break? These runtime type checks are easy to miss on future code changes.

vvoland · 2024-01-19T12:48:23Z

Not sure if something would break. But that would require changing a bunch of content.Providers across buildkit codebase (possibly breaking the public API?).
I can do that if you want to see how that would look.

tonistiigi · 2024-01-19T19:27:13Z

@vvoland I think we should at least try that if we have codepaths that actually call into Info() when Provider is passed. If there is some complicated issue why it doesn't work then we can reconsider. Breaking public Go API (not sure if actual breakage for this would really happen) is not a problem.

vvoland

Looks like this also needs an Info implementation for the remote cache providers, but not all data is available there to construct the full content.Info.

Any idea on these? @tonistiigi @crazy-max

cache/remotecache/gha/gha.go

cache/remotecache/s3/s3.go

util/contentutil/fetcher.go

cache/remotecache/gha/gha.go

cache/remotecache/s3/s3.go

util/contentutil/fetcher.go

cache/remotecache/gha/gha.go

cache/remotecache/s3/s3.go

tonistiigi · 2024-02-09T21:56:22Z

util/contentutil/fetcher.go

 	return &readerAt{Reader: rc, Closer: rc, size: desc.Size}, nil
 }

+func (p *fetchedProvider) Info(ctx context.Context, dgst digest.Digest) (content.Info, error) {


Did you check if this was needed for anything actually (apart from places in this PR that use it in cache import where it shouldn't be needed) or can FromFetcher keep returning content.Provider. This returns wrong size so not ideal if it actually gets called.

I did check by replacing the Info implementation with a panic and running the buildkit tests. They all passed so I think it's not actually needed.

Also, I just implemented the Info for DescriptorProviderPair instead of doing it for each cache implementation.

cache/remotecache/azblob/importer.go

tonistiigi · 2024-02-14T17:40:52Z

Validate failing with:

------
 > [linux/amd64->freebsd/amd64 golangci-lint 1/1] RUN --mount=target=/go/src/github.com/moby/buildkit     --mount=target=/root/.cache,type=cache,id=lint-cache-default-freebsd/amd64   xx-go --wrap &&   golangci-lint run --build-tags "" &&   touch /golangci-lint.done:
661.7 cache/remotecache/v1/chains.go:130:26: use of `fmt.Errorf` forbidden because "use errors\\.Errorf instead" (forbidigo)
661.7 		return content.Info{}, fmt.Errorf("content not found %s", dgst)
661.7 		                       ^
------

Require base provides to implement both content.InfoReaderProvider. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

outdated

ktock · 2024-02-26T05:19:50Z

cache/remotecache/v1/parse.go


 			remote.Descriptors = append(remote.Descriptors, descPair.Descriptor)
-			mp.Add(descPair.Descriptor.Digest, descPair.Provider)
+			mp.Add(descPair.Descriptor.Digest, descPair)


Related to #4686 :

The original provider should be passed to the multiprovider without hiding methods with the wrapped struct (DescriptorProviderPair). Or, DescriptorProviderPair should export methods multiprovider relies on ( UnlazySession and SnapshotLabels). If this PR is still needed, I can work on this PR.

Thanks! Feel free to carry this PR :)

AkihiroSuda reviewed Jan 16, 2024

View reviewed changes

util/contentutil/multiprovider.go Outdated Show resolved Hide resolved

AkihiroSuda previously requested changes Jan 16, 2024

View reviewed changes

vvoland force-pushed the multiprovider-info branch from ff8aed0 to 8df0cc3 Compare January 16, 2024 09:31

vvoland mentioned this pull request Jan 16, 2024

c8d/exporter: Use WithSkipMissing moby/moby#46978

Merged

tonistiigi reviewed Jan 16, 2024

View reviewed changes

dmcgowan mentioned this pull request Feb 7, 2024

Update to containerd 2.0 #4630

Closed

dmcgowan approved these changes Feb 7, 2024

View reviewed changes

vvoland force-pushed the multiprovider-info branch 2 times, most recently from 539094d to e3f7093 Compare February 8, 2024 11:23

vvoland marked this pull request as draft February 8, 2024 11:24

vvoland force-pushed the multiprovider-info branch from e3f7093 to 1cbd233 Compare February 8, 2024 14:46

vvoland commented Feb 8, 2024

View reviewed changes

cache/remotecache/gha/gha.go Outdated Show resolved Hide resolved

cache/remotecache/s3/s3.go Outdated Show resolved Hide resolved

util/contentutil/fetcher.go Outdated Show resolved Hide resolved

vvoland marked this pull request as ready for review February 8, 2024 14:51

vvoland force-pushed the multiprovider-info branch from 1cbd233 to 66a524b Compare February 8, 2024 15:14

tonistiigi reviewed Feb 8, 2024

View reviewed changes

cache/remotecache/gha/gha.go Outdated Show resolved Hide resolved

cache/remotecache/s3/s3.go Outdated Show resolved Hide resolved

util/contentutil/fetcher.go Outdated Show resolved Hide resolved

vvoland force-pushed the multiprovider-info branch from 66a524b to 075a022 Compare February 9, 2024 12:38

tonistiigi reviewed Feb 9, 2024

View reviewed changes

vvoland force-pushed the multiprovider-info branch from 075a022 to ef637f6 Compare February 14, 2024 12:07

util/multiprovider: Implement Info

55afcdb

Require base provides to implement both content.InfoReaderProvider. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the multiprovider-info branch from ef637f6 to 55afcdb Compare February 14, 2024 17:42

AkihiroSuda added this to the v0.13.0 milestone Feb 19, 2024

tonistiigi approved these changes Feb 23, 2024

View reviewed changes

tonistiigi merged commit 85e9df3 into moby:master Feb 23, 2024

tonistiigi mentioned this pull request Feb 23, 2024

Revert "util/multiprovider: Implement Info" #4686

Merged

ktock reviewed Feb 26, 2024

View reviewed changes

ktock mentioned this pull request Feb 26, 2024

[Carry 4558] util/multiprovider: Implement Info #4694

Merged

tonistiigi mentioned this pull request Mar 18, 2024

remotecache: fix missing CheckDescriptor method #4771

Merged

Conversation

vvoland commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Jan 16, 2024

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland commented Jan 19, 2024

Uh oh!

tonistiigi commented Jan 19, 2024

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonistiigi Feb 9, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonistiigi commented Feb 14, 2024

Uh oh!

ktock Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

vvoland Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vvoland commented Jan 15, 2024 •

edited

Loading