nit: The const should be publicized on containerd side: https://github.com/containerd/containerd/blob/97480afdac09c947d48f5e3a134db86c78f4bfa6/remotes/docker/handler.go#L35

Can be done later though

Opened a PR: containerd/containerd#8224

Maybe we could leave a breadcrumb in a comment? Something like this? (I'm sure it could be better, but just a hint as to how to update this in the future)

Does containerd append these labels on push, too? (or only pull?)

If I build something, push it somewhere, then build something else based on that first thing and push it to the same registry, it can use cross-repo blob mounts too. 😅

(see also the related #44757 😂 🙈)

Nope, containerd only appends these on pull.
But I just made this code do it and it seems to work!

quick test:

$ docker run --name test -it alpine
# cat /dev/random >/file  # ctr-c after a while
$ docker commit test 172.172.0.2:5000/big-image
$ docker push 172.172.0.2:5000/big-image
$ docker run --name test2 -it 172.172.0.2:5000/big-image
# apk add fish
...
$ docker commit test2 172.172.0.2:5000/big-image-with-small-change
$ docker push 172.172.0.2:5000/big-image-with-small-change
Using default tag: latest
f8d2deb24c5c: Pushed
c41833b44d91: Pushed
8b626a5feef1: Pushed
140adfd2fa47: Pushed
c76cc252a7ff: Pushed # <- pushes only this changed layer

The status is still Pushed though because the containerd doesn't actually expose that information (I'm working on a containerd PR for that one though).

That's really neat!! Thank you for testing/working on it 🙇

(For interested parties, I think containerd/containerd#8330 is the relevant containerd PR 👀)

I just recalled we still have an open ticket for this to change this at the API level as well;

• client.ImagePush(): default to ":latest" instead of "all tags" #40302 changed the client to require an explicit opt-in for "all tags"
• implement docker push -a/--all-tags docker/cli#2220 changed the CLI to use it
• Don't allow pulling all tags by default for the ImageCreate API endpoint #42053 has a proposal to change the API to have an explicit all-tags options

Maybe we should start working on making that API change 🤔 (we could mention the linked ticket here as a TODO)

Ooooooh, I hadn't fully realized until just now that containerd integration means we could support push-by-digest now! 👀

It probably requires an API change too though? I'm guessing we don't want the tag to also be able to specify digests? 👀
Possibly, we could think about deprecating separate repo and tag arguments (but still support it), and have ref instead as a preferred way to reference an image?

Doh, yeah, I forgot the API separated those. 😭 Totally something to punt for now, but yeah, I guess we should probably reconsider that as we consider what we want the "better push" API to look like. 🤔

Longer term, we should probably implement some other means of telling this level of the code that we want to push "all" tags, right? (so we can support push-by-digest? I guess we could implement that by checking for reference.Digested here too? future concerns IMO, but worth raising)

I'm kinda wondering if push "all" tags is still useful with the multi-platform images. I can imagine it being useful when you had to do separate tags for multi-platform images (eg alpine:amd64, alphine:arm64 etc), but with multi-platform images there might be less use cases for it?
Obviously, I might be missing some more context behind it, so happy to hear what others think 😅

I still use it every now and then, but only for very small scale pushes where I've built a bunch of related things locally and want to batch pushing them all. 😅

I wouldn't be bothered one bit by it going away, but I'm sure there are some users who might have different opinions/use cases. 😂

👀 this one had some upstream blocker, right? could we maybe link to an upstream discussion somewhere about what's stopping us from implementing this?

Actually we can support it for push just fine because we use the remotes.PushContent directly. It's the pull that uses the c8d's client.Push method that can't use the same limiter for multiple concurrent operations.
For pull, I expect that at some point we would have the need to do something more than the c8d client.Pull provides and we will have to use the lower level functions anyway, which would allow us to support MaxConcurrentDownloads too.
So perhaps it would be fine (at least for now) to only have "MaxConcurrentUploads" implemented?

Yeah, that seems reasonable to me, although IMO doesn't have to be part of this PR unless you think the implementation is really straightforward/quick. 🙇

Given this isn't blocked on upstream (as I'd mistakenly crossed the upload/download signals in my head), probably fine to leave the comment as-is too. 👍

This is really good, IMO 🙇

Maybe we could leave a breadcrumb in a comment? Something like this? (I'm sure it could be better, but just a hint as to how to update this in the future)

Does containerd append these labels on push, too? (or only pull?)

If I build something, push it somewhere, then build something else based on that first thing and push it to the same registry, it can use cross-repo blob mounts too. 😅

(see also the related #44757 😂 🙈)

This one is a little over-zealous -- manifest objects are special (indexes and image manifests), but config objects are just regular blobs (like layers), so if they already exist, they're 100% fair game for cross-blob mounts 👀

Ah, indeed. I was sure that configs are uploaded as manifests too, but this was probably some mistake when developing the initial fork version. Thanks!

As I've noted in my other comment, IMO we should spend a bit more time thinking about the UX we want from docker push WRT platforms before we implement this in the API ("no is temporary, yes is forever"), but I seem to recall being in a minority on that in the maintainers call where we discussed this, so I'm not going personally to block this if other maintainers don't agree. 🙇

Thanks!

Looks like the docs doesn't match the function name

Thanks! Fixed.

Same here

Thanks! Fixed.

I think the current output from docker push on a cross-blob mount specifies where it was mounted from; is it possible to surface that data somehow here as well? (is that already in here and I missed it? 🙈)

Aha, found the ref:

Looks like release() can return an error; is this something we should log if it does?

Technically, if it returned an error then the lease couldn't be deleted, so that might be something worthy of logging. However, looking at how containerd itself uses this, it's "common" (I haven't really seen any code that does it differently) to just call it and ignore the error.
Looking at the code, the only case where this could happen if the lease deletion fails. This can happen if the lease no longer exists (which is already the state we want) or the c8d connection is broken, which will pop out in some other place anyway.

Gotcha; yeah, TBH, containerd ignoring things could sometimes be a guideline, but we should definitely check ourselves what makes sense (mistakes are made by everyone, so it's always possible containerd is on the wrong on these, in which case we could contribute there as well).

• "not found" errors; yup definitely we should ignore
• more curious; could "something else using the files / mounts / whatever" cause things to not be cleaned up? (in which case we may want to have a log to backtrace we potentially leaked things somewhere); of course that is if that wouldn't trigger "false positives" (race conditions, things being "eventually" cleaned up etc).

This only deletes a lease - which is created only in this call-site and deleting it shouldn't fail unless the lease was already deleted, containerd connection is broken or containerd state is corrupted. In which case we will have loud errors in other places. Even if we fail to delete the lease and containerd is fully operational with the lease state still being present, the lease will expire after 24h.
It doesn't harm to log it though.. Except it's just bloating a simple one line defer to a deferred anonymous function call with a conditional log invocation 😅 But maybe it can save us from some trouble if the Lease manager has some major future changes from the current state, so I'm not that much against logging it.

Thanks! Yes I didn't know if actual mounts and/or files were involved here. I'm mostly "fine" either way I guess

pushProgress is shadowing the pushProgress type here. Perhaps we could name it just progress

Done in #45243

Same for jobs here (not sure what a good alternative name is. We could rename one of them to queue perhaps, but 😅)

heh, now would've been nice if

func (j *jobs) Add(desc ocispec.Descriptor)

Had a ...ocispec.Descriptor as signature. That way we also wouldn't have to lock/unlock the mutex for each item;

// Add adds a descriptor to be tracked
func (j *jobs) Add(desc ...ocispec.Descriptor) {
	j.mu.Lock()
	defer j.mu.Unlock()

	for _, d := range desc {
		if _, ok := j.descs[d.Digest]; ok {
			continue
		}
		j.descs[d.Digest] = d
	}
}

Done in #45243

Is it possible that this handler runs concurrently? In that case, does mountableBlobs need synchronisation?

Yes, good catch! This is explicitly called concurrently due to Dispatch instead of Walk so there's a data race here.

nit: Looks like registry collides with the "github.com/docker/docker/api/types/registry" import. perhaps reg is an option (perhaps also inline it, as it's only used inside the if);