I'm still a bit annoyed that we give the ref access to the generic progress.Writer and the VertexDigest, VertexName that are solver specific concepts. Also, not sure how it works with FromRemote as don't see it passed then. I think we should only pass the narrow interface for the actions cache pkg needs.

Okay, so what do you think about CacheKey just passing some callback funcs used to start/complete the progress?

I think define an interface for the methods you need in the cache pkg and then pass in implementation for it when needed.

NeedsRemoteProviderError ?

I think this should not use interface{}. Worker is ok level, where to do the type cast.

I recently took sessionID out of context in #1551 so makes it weird to add it in here. Maybe you can even combine it with the Group interface I added for sessions. This can be follow-up as well.

Actually, I think this PR reintroduces one of the things fixed in #1551 . Eg. the fetcher created in pull is already connected to a session but that session might change later. Would be better if the real session.Group is added to the request in here so when Provider is accessed it already is connected to updated session. Let me know if this gets too messy for you. We can merge without it and I can carry/fix this later.

Okay, I'll take a closer look at #1551 and give integrating with the Group a shot

Alright, took a look, my understanding is the issue comes down to the fact that we construct a fetcher here from the resolver, at which time credentials will be called with whatever groups are present at the time. That's bad because it could be much later and under a different vtx that the actual de-lazy happens but the provider will still use that possibly stale fetcher.

If that's correct then I guess the fix is, in part, to recreate the fetcher from the resolver when the actual pull occurs rather than use the one created earlier, which is easy enough. My only question is if you think it's okay at that time (i.e. this line) to use the same resolver as before (which should be updated w/ sessions in calls to CacheMap and Snapshot such as here), or do you think we need to make a new resolver using the session.Group from the vtx where the actual de-lazy is occurring (i.e. here or in the exporter)?

Basically, should the de-lazy use the session.Groups from the original vtx where the lazy ref got created, or the session.Group from the vtx where the de-lazy is occurring? Or is it irrelevant (I haven't traced through #1551 carefully enough yet to tell)?

My only question is if you think it's okay at that time (i.e. this line) to use the same resolver as before

Yes, ideally we should reuse as much as possible. New resolver means new tcp connections and new authentication roundtrips that should be avoided.

Basically, should the de-lazy use the session.Groups from the original vtx where the lazy ref got created, or the session.Group from the vtx where the de-lazy is occurring?

From where the de-lazy is occurring. Old may not be valid anymore.

I added this SessionAuthenticator https://github.com/moby/buildkit/blob/master/source/containerimage/pull.go#L127 concept that allows adding new sessions while reusing the resolver https://github.com/moby/buildkit/blob/master/source/containerimage/pull.go#L167 . Was actually thinking to take this further and write a custom containerd authenticator and resolver pool (limited resolver pool already exists for pull) that can remember tokens and reuse connections cross builds(after checking auth). Something like resolverpool.Get(ref, scope ("pull/push"), g session.Group). (Maybe also resolver.WithSession(session.Group) resolver if that helps). That could help simplify this as well. Maybe I have time to work on this tomorrow.

Okay, for now all I did was make it so the fetcher gets reconstructed at the time of de-lazy.

But it is still using the resolver that is only ever updated w/ session.Groups from calls to CacheKey/Snapshot. So it will not be updated with the session.Groups from the vertex where actual de-lazy is occurring. I'm not sure what the best way to implement that would be as it seems like it would require plumbing the session.Groups for the current vtx all the way through to the provider implementation, where it can be used to update the resolver. I guess we could put the session group in the ctx, but it seems like that was something you were trying to avoid in the previous PR? Let me know what you think or if I'm misunderstanding.

Lets try to avoid this dependency in the solver package.

Okay, I didn't like this either but felt a little better when I realized solver already had a transitive dependency on cache :)

The import is only needed for the Remote struct. I originally wanted to leave Remote in its original place in solver, but Remote is now needed in cache due to the GetRemote method moving there and trying to import solver into cache created an import loop. Moving Remote to cache worked, but what else would you suggest? I guess cache/remotecache seems like an obvious choice, but are you okay with solver having a direct import from that?

I realized solver already had a transitive dependency on cache :)

go list -json ./solver | grep cache on master doesn't show a dependency on cache

I... must have lost my mind at some point in all of this or am misremembering the issue but yeah you're right. I'll try to switch Remote back to solver

doesn't this function need synchronization against parallel calls?

I previously looked through the underlying implementation of the content store methods it's calling and concluded the synchronization wasn't necessary here, but I'll go through it all again to double-check and get back

Okay yeah, I still believe this basically works as is. There's a few relevant pieces of code:

1. We always use contentutil.Copy when de-lazying, which in turn uses a FetchHandler
2. The FetchHandler internally opens a content writer with a writer ref consistent for a given oci descriptor. It also uses content.OpenWriter, which internally will check to see if there's already a writer open for that ref (and thus that descriptor) and, if so, wait until that writer is closed before opening it again.
3. fetch will also always check to see if opening the writer or committing an already completed writer returns an AlreadyExists err, in which case it knows the content was already successfully committed by something else and returns a nil err. Same goes for the content.Copy func it calls internally.

So, given the above, if we check isLazy and it returns true but by the time we actually try to de-lazy it's already been de-lazied by something else, then we should just get a nil error returned by contentutil.Copy and can move on successfully. If something else is in the middle of de-lazying, we'll block until that thing is done de-lazying and, if it succeeded, continue on or if it failed, try de-lazying it again.

The only slightly weird corner case I can think of this is if you loaded the same immutable ref twice but with different desc handlers and then those two were both getting de-lazied at the exact same time, it's possible for you to technically pull it from one remote but then the other cache ref wins the race to setting the Description in the metadata and it will be marked there as having been pulled from a different remote image ref. It seems like the description is mostly just informational and if two instances of the same cache ref are being pulled at the same time it is not really significant which one's image ref ends up in the Description, but let me know if that matters or if there are other cases I'm not thinking of.

We should avoid the case where to parallel Unlazy requests make an extra HTTP request to the registry, even if some data is ignored later. Also the locking in content.Writer isn't very well thought out, it doesn't handle cancellation of one request gracefully and I believe doesn't even release the lock until a time-based trigger completes. Usually flightcontrol.Do that is used extensively in this repo is a much better solution.

Ah sure, it should be easy to wrap in a flightcontrol.Do too, will do.

I don't really get this HashOnlyIngester logic. As your comment says just wrap the Applier so that it calls ReaderAt on Provider(or calls Unlazy() etc). This double hashing seems unnecessary and this construction is very hard to understand.

Just to be clear, the point wasn't just to take the hash, it was to read the content in full from remote.Provider which had the (magic) side effect of de-lazying it into the local content store (due to the implementation in the cache package). However, now that you mention "double hashing" I do realize that when we do the magic de-lazy to the local content store, we are already doing a hash-verification, so copying to a HashOnlyIngester is actually unnecessary and I can at minimum change this to copy to an ingester that straight up discards data it reads without even taking the hash. I'll do that if nothing else.

But I also agree this all kind of sucks and the approach I suggested in the comment of just having the Applier use remote.Provider directly would be cleaner. However, I didn't see any way of doing so that didn't involve even more large-scale refactoring on top of the already large change here (happy to be wrong though). The problem is the actual diff.Applier interface only has this method

Apply(ctx context.Context, desc ocispec.Descriptor, mount []mount.Mount, opts ...ApplyOpt) (ocispec.Descriptor, error)

which doesn't include a content.Provider. The Applier implementation appears to internally use the content.Store it's hooked up to much earlier (i.e. here and here) as its content.Provider.

So the options I'm seeing besides what I already have are:

1. Wrap just the Apply method with something that takes the desc argument, de-lazies it (using a similar technique as now, just w/out the double-hashing you mentioned) and then calls the wrapped Apply method.
   • I have no opposition to this, I just didn't do it originally because it seemed like basically the same as what I'm currently doing but hidden under an Apply wrapper. Totally fine with updating to that if you think it's clearer though.
2. Change the original Applier to be constructed in the worker implementations with a special content store that can somehow be updated on the fly to use providers for given descriptors as set from the remote.Provider we obtain in the exporter here.
   • I'm not entirely sure how this would work, but the idea is appealing on some level. My main concern is that even if it's possible it would expand this PR way too much, but I could of course be overestimating and/or not seeing a simple path to implementing it.

Let me know if this makes any sense and what you think.

Wrap just the Apply method with something that takes the desc argument, de-lazies it (using a similar technique as now, just w/out the double-hashing you mentioned) and then calls the wrapped Apply method.

I think that is the same that I had in mind.

wrappedApplier(applier, remote/provider) Applier implements Apply that when called invokes copy to be made to contentstore, and then just calls the regular applier that does the extraction from contentstore to snapshotter.

If this turns out problematic for some reason(eg. the current parallelization issue) then lets add explicit method to ref/remote for this and avoid magic constructions with side-effects.

Had some more thoughts; just to be super explicit about this since the code has kind of gone back and forth between different approaches here throughout the PR, here's what I'm thinking.

There's 3 places where a de-lazy may need to occur:

1. During ImmutableRef.Extract()
2. During a local export/unpack (such as the Applier case we were discussing)
3. Sometimes during a push; specifically when a lazy ref is going to be pushed to a registry that doesn't contain the blob yet (but not when the registry already contains the blob in one or more of its repos)

There's also 2 general approaches for how the provider in the Remote returned by ImmutableRef.GetRemote() can be implemented:

1. The current approach w/ magic side-effects where the provider is implemented such that opening a reader for it will result first in the entire descriptor blob to be pulled to the local content store and then a reader for the local content store to be returned.
2. An alternative, less magical approach where it simply checks to see if the ref is lazy and, if so, set the provider to the one from the remote repo or, if not lazy, sets the local content store as the provider. This means that all callers of GetRemote() will have to check whether the descriptors in the returned remote are available locally before trying to use them.
   • Callers can be helped out with this via a Unlazy (or what I called before EnsureContentExists) method on the Remote or ImmutableRef object

In theory, approach 2 is preferable from the standpoint of being less magical and side-effecty, but there's a fairly significant downside when you consider the de-lazies that occur during a push (place 3 in the list above). Namely, all of the logic that determines during a push whether or not the content will need to exist locally or if pull can be skipped is fairly deep in external code (e.g. one place is here where it's determined if you can do a cross-repo push). I'm guessing we can't modify that code to do the checks/calls required in approach 2, so the only other option would be to put all the logic that determines whether we can skip the local pull of content directly in the push util itself. That seems like it would most likely be a fairly large amount of copy/paste and hardcoding of behavior that should just be handled by the library we're calling out to.

So given that, I'm still inclined for approach 1 (though, like I said, I'll fix the whole double-hashing issue). However, I don't really see any reason why we couldn't do approach 1 but also add a util method to Remote called Unlazy that at least makes the code less confusing to read. I'll use remote.Unlazy in the wrapper around the Applier like we were talking about and also in the extract() code path. Internally, Unlazy will just do contentutil.Copy from the remote provider to an ingester that discards everything (without even checking hash) in order to trigger the magic side effect of de-lazying, but at least it will be wrapped in a more explicit method.

Let me know how that all sounds.

Internally Unlazy will call ref.GetRemote().Provider.ReaderAt(ctx,ref.Desc) that will copy from remote provider to the local contentstore?

Almost but not exactly, due to the issues I mentioned before I think we have to leave GetRemote and the provider it returns the same as it currently is, where when the provider gets opened it first copies from the remote provider to the local content store before returning a reader for the local content store (de-lazies as a side-effect of opening the reader).

If it weren't for the case of de-lazies occurring during pushes that I mentioned in the previous comment, I think we could do what you suggest (ref.GetRemote().Provider.ReaderAt(ctx,ref.Desc)) where the Remote returned by GetRemote is just the actual remote provider w/out any de-lazying as a side-effect, but like I said before I just don't know how to make that work with the push handler code that exists outside this repo (and thus doesn't know how to call Unlazy).

So, given that, if Unlazy is going to be a helper function, it would have to do something like contentutil.Copy(ctx, discardIngester, ref.GetRemote.Provider), where discardIngester is just the equivalent of ioutil.Discard but for ingesters.

However, if we make Unlazy be a method on Remote or ImmutableRef, we could probably make it not have to do the separate contentutil.Copy, it could be coded specially to just copy from the remote provider from DescHandler to the local content store. I think this is what I'd prefer since the contentutil.Copy is obviously sort of wasteful even if it's just writing to a discard ingester.

If it weren't for the case of de-lazies occurring during pushes that I mentioned in the previous comment, I think we could do what you suggest (ref.GetRemote().Provider.ReaderAt(ctx,ref.Desc)) where the Remote returned by GetRemote is just the actual remote provider w/out any de-lazying as a side-effect,

I'm bit confused about what causes the conflict between push and de-lazy on ref.GetRemote().Provider.ReaderAt . The example you linked https://github.com/containerd/containerd/blob/0f2b15b7af41896190fcb6e7b597be789c11756b/remotes/docker/pusher.go#L129 directly works with descriptor and I don't see it requiring local data. So I don't think it would ever call ReaderAt if descriptor exists or can be cross-pushed. It only calls it if it needs local data and de-lazy is required then anyway.

Right, what I'm saying is that there are times where de-lazy is required for push and there are times where it isn't required for push. However, the determination of whether it's needed happens deep in external code. So, for buildkit's code, if we don't have the "de-lazy as a side-effect" approach, then we would be forced to just always de-lazy before a push because we don't know ahead of time whether it needs to be present locally. But if we did that we'd be breaking some of the crucial features of lazy refs, namely that you don't have to de-lazy when just re-tagging or doing a cross-repo push.

Of course the other option would be to have buildkit's code do the determination of whether a de-lazy is needed. I was ruling that out for now because it seems like that functionality (such as that cross-repo logic I linked to) isn't public in containerd push handler code and is fairly far from trivial. Let me know though if there's some easier way of determining it, that would obviously change this consideration.

We discussed this more on Slack, we were actually in agreement, I was just confused about one point. We’ll go with the side effect reader and Unlazy will use that internally, but it will just open the reader to trigger the side effect rather than copy to a discard ingester.

Updated with this change. I did make one superficial tweak to it, namely, instead of having Unlazy be a util func, I made an Unlazier interface and then callers can check to see if a remote's provider implements that interface, calling it if so. The only small improvement over Unlazy as a util func is that you can avoid having to open and immediately close a reader to the local content store, otherwise it's basically the same in my mind. It's a fairly minor change so if you disagree w/ the approach just let me know and I have zero issue switching to Unlazy as a util func.

afaics, this should happen for all layers. Also, a blob digest doesn't need to have the same parents so it doesn't look correct to assume refs parents come from the same remote (if only top digest is tracked).

My thinking here (which I should probably add a comment for if nothing else) is that while it's true a parent desc could be in the chain of multiple refs and thus come from multiple remotes, it actually won't matter.

Say here you have a chain of descriptors from parent->child of A->B->C (topDesc is thus C), but in a different image you end up with just the chain A->B (where topDesc is thus B) and use an entirely different remote (and progress writer, etc.). If you end up creating lazy-refs for both these desc chains, say then that the A->B chain gets de-lazied first.

When you later get around to de-lazying the A->B->C chain, the implementation in cache realizes that A->B was already de-lazied by something else and should skip de-lazying those descriptors. It will end up just de-lazying what it needs to, C in this case. It thus shouldn't matter that there were some descriptors that had different remotes in different refs.

In addition, for a single ref, all the descriptors in the chain for that ref are going to have the same DescHandler values, so setting the DescHandler for each in the chain seemed unnecessary.

That's the theory anyways; you asked in some other comments about synchronization issues which I need to go look deeper at as obviously that's important to get right for all the above to work. Let me know if this makes sense or if I'm missing something.

But what if you have A->B->C and A->D->C competing?

I forgot that the digest of a layer descriptor doesn't include the digest of its parent (this is all still settling in my brain), but I actually still feel like it would still work. If A->B->C and A->D->C are getting de-lazied at the same, each of the individual descriptors will still get de-lazied just once (again, assuming the locking/synchronization is done correctly). It's possible to end up with a mix of like A and C got de-lazied using remote 1 and B got de-lazied using remote 2, but at the end of the day they are the same bytes no matter the remote and end up in the local content store, which is what matters, right? What am I missing still?

A and C got de-lazied using remote 1 and B got de-lazied using remote 2, but at the end of the day they are the same bytes

I'm not following on the "but at the end of the day they are the same bytes". The case I'm thinking is when there are 2 separate lazy refs (different bytes) that have the same top layer. Afaics when they both get unlazy request only one deschandler is used. So ref that was based on A->B->C but is pulled with a deschandler for A->D->C there could be a problem.

Right, makes total sense, I was still somehow getting hung up on the layer digest not including its parent digest, but I get it now. I'll change this to set the DescHandler for each desc in the chain, thanks for bearing with me

This is bit confusing to me. Don't think there is a case where the platform is allowed to be nil.

Updated to have platform not be a pointer