c8d: Implement push #44963

vvoland · 2023-02-09T13:12:15Z

upstreams: containerdstore: Initial push support rumpl/moby#2
upstreams: c8d/push: Push lazy blobs with distribution source labels rumpl/moby#74
upstreams: c8d/push: Fetch missing resources that can't be mounted rumpl/moby#80
upstreams: containerd: Implement push rumpl/moby#15

Implements docker push under containerd image store. When
pushing manifest lists that reference a content which is not present in
the local content store, it will attempt to perform the cross-repo mount
the content if possible.

Considering this scenario:

$ docker pull docker.io/library/busybox

This will download manifest list and only host platform-specific
manifest and blobs.

Note, tagging to a different repository (but still the same registry) and pushing:

$ docker tag docker.io/library/busybox docker.io/private-repo/mybusybox
$ docker push docker.io/private-repo/mybusybox

will result in error, because the neither we nor the target repository
doesn't have the manifests that the busybox manifest list references
(because manifests can't be cross-repo mounted).

If for some reason the manifests and configs for all other platforms
would be present in the content store, but only layer blobs were
missing, then the push would work, because the blobs can be cross-repo
mounted (only if we push to the same registry).

The container.io/distribution.source label is taken only from the target blob we are pushing. It's a change from fork's behavior.
In fork for each missing content we did search the entire content store for a manifest that includes it and took it's distribution source label:

moby/daemon/containerd/lazy_push.go

Lines 367 to 489 in 2670c94

    
           // collectSources walks the content store and looks for content which can 
        
           // provide a source registry and repository for the provided descriptors from 
        
           // the containerd.io/distribution.source labels 
        
           func collectSources(ctx context.Context, toCollect []ocispec.Descriptor, store content.Store, sources map[digest.Digest]distributionSource) error { 
        
           	// Nothing to do. 
        
           	if len(toCollect) == 0 { 
        
           		return nil 
        
           	} 
        
           	// Make a copy of the missing descriptors as we will be removing 
        
           	// the descriptors that we found a source for. 
        
           	missing := make([]ocispec.Descriptor, len(toCollect)) 
        
           	copy(missing, toCollect) 
        
           	success := errors.New("success, found the source but can't return earlier without an error") 
        
           	err := store.Walk(ctx, func(i content.Info) error { 
        
           		source := extractDistributionSource(i.Labels) 
        
           		log := logrus. 
        
           			WithField("digest", i.Digest) 
        
           		log.Debug("walk") 
        
           		// Nah, we're looking for a parent of this lazy child. 
        
           		// This one will not provide us with the source. 
        
           		if source.value == "" { 
        
           			return nil 
        
           		} 
        
           		desc := ocispec.Descriptor{Digest: i.Digest} 
        
           		// Do a simple peek of the content to avoid big blobs which definitely aren't json. 
        
           		notJson, err := peekNotJson(ctx, store, desc) 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           		if notJson { 
        
           			log.Debug("skipping, definitely not a json") 
        
           			return nil 
        
           		} 
        
           		// Read the manifest 
        
           		blob, err := content.ReadBlob(ctx, store, desc) 
        
           		if err != nil { 
        
           			log.WithError(err).Error("error reading blob") 
        
           			return err 
        
           		} 
        
           		// Manifests and indexes have different children. 
        
           		// Index stores other manifests and manifests store layers. 
        
           		// To avoid unmarshaling the blob separately as manifest and index 
        
           		// this holds fields that contains them both and the media type. 
        
           		var indexOrManifest struct { 
        
           			MediaType string               `json:"mediaType,omitempty"` 
        
           			Manifests []ocispec.Descriptor `json:"manifests,omitempty"` 
        
           			Layers    []ocispec.Descriptor `json:"layers,omitempty"` 
        
           			Config    ocispec.Descriptor   `json:"config,omitempty"` 
        
           		} 
        
           		err = json.Unmarshal(blob, &indexOrManifest) 
        
           		if err != nil { 
        
           			log.WithError(err).Debug("unmarshal failed") 
        
           			return nil 
        
           		} 
        
           		mediaType := indexOrManifest.MediaType 
        
           		// Just in case, check if it really is manifest or index. 
        
           		if !containerdimages.IsManifestType(mediaType) && !containerdimages.IsIndexType(mediaType) { 
        
           			log.Debug("not a manifest/index") 
        
           			return nil 
        
           		} 
        
           		children := append(indexOrManifest.Layers, indexOrManifest.Manifests...) 
        
           		if indexOrManifest.Config.Digest != digest.Digest("") { 
        
           			children = append(children, indexOrManifest.Config) 
        
           		} 
        
           		if len(children) == 0 { 
        
           			log.Debug("empty a manifest/index") 
        
           			return nil 
        
           		} 
        
           		// Look if this manifest/index specifies any of the missing content 
        
           		for _, layer := range children { 
        
           			for idx := 0; idx < len(missing); idx += 1 { 
        
           				wanted := missing[idx] 
        
           				if layer.Digest == wanted.Digest { 
        
           					// Found it! 
        
           					sources[wanted.Digest] = source 
        
           					log.WithField("wanted", wanted.Digest.String()).Debug("found") 
        
           					// Don't look for it anymore 
        
           					if len(missing) > 1 { 
        
           						lastIdx := len(missing) - 1 
        
           						missing[idx] = missing[lastIdx] 
        
           						missing = missing[:lastIdx] 
        
           						idx -= 1 
        
           					} else { 
        
           						// We found all missing, let's end the walk. 
        
           						missing = missing[:0] 
        
           						return success 
        
           					} 
        
           				} 
        
           			} 
        
           		} 
        
           		return nil 
        
           	}) 
        
           	if err == success { 
        
           		err = nil 
        
           	} 
        
           	if len(missing) > 0 { 
        
           		msg := "missing blobs with no source: " 
        
           		for idx, c := range missing { 
        
           			if idx != 0 { 
        
           				msg += ", " 
        
           			} 
        
           			msg += c.Digest.String() 
        
           		} 
        
           		err = errdefs.NotFound(errors.New(msg)) 
        
           	} 
        
           	return err 
        
           }

This was mostly a workaround for buildkit image exporter not fetching all platform-specific content (only the host's platform). Eg. doing FROM 'ubuntu' | docker buildx build --platform <host>,<other> - would only fetch ubuntu rootfs for <host> platform, but not for <other>. Because the built image didn't have any distribution.source that would point to the original docker.io/library/ubuntu - it was impossible to know where to obtain the blob from, based on the built image alone. This was only possible by traversing all manifests in content store, checking if they have a distribution.source label and then traversing them to check if they happen to define this blob.
Since this is no longer needed as buildkit already does store=true by default, I decided to change this.

- What I did
Implemented push

- How I did it
Use containerd remote to push the content
Some missing content can be cross-repo mounted. In this case fake their existence by wrapping the content.Store to return fake content.Info that includes the containerd.io/distribution.source label with the source repository.
That makes the push not end with NotFound error and allows it to perform cross-repo mount.

**How to verify it [click to expand]**

TODO: New examples

- A picture of a cute animal (not mandatory but encouraged)

daemon/containerd/image_push.go

vvoland · 2023-02-09T14:57:20Z

Arghh, wanted to make the PushImage accept reference.NamedTagged, but forgot about --all-tags case in which there's no tag at all 🤦🏻

=== Failed
=== FAIL: amd64.integration-cli TestDockerRegistrySuite/TestPushMultipleTags (0.16s)
    docker_cli_push_test.go:79: assertion failed: 
        Command:  /usr/local/cli/docker push 127.0.0.1:5000/dockercli/busybox
        ExitCode: 1
        Error:    exit status 1
        Stdout:   The push refers to repository [127.0.0.1:5000/dockercli/busybox]
        
        Stderr:   tag does not exist: 127.0.0.1:5000/dockercli/busybox:latest
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error
    check_test.go:384: [deae1ffcfb672] daemon is not started
    --- FAIL: TestDockerRegistrySuite/TestPushMultipleTags (0.16s)

AkihiroSuda · 2023-02-09T17:19:33Z

I'm not sure whether it is safe to push a manifest without its layer blobs.
From my comment in nerdctl, some registry doesn't/didn't accept it? (In 2021? containerd/nerdctl@6760ebf)

https://github.com/containerd/nerdctl/blob/b0701ba875880db5a6e7585df148f0eaea36283a/pkg/cmd/image/push.go#L92-L106

// Push fails with "400 Bad Request" when the manifest is multi-platform but we do not locally have multi-platform blobs.
// So we create a tmp reduced-platform image to avoid the error.

AkihiroSuda · 2023-02-09T17:22:26Z

daemon/containerd/image_push.go

+	return *s, nil
+}
+
+const labelDistributionSource = "containerd.io/distribution.source."


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)

Suggested change

const labelDistributionSource = "containerd.io/distribution.source."

const labelDistributionSource = "containerd.io/distribution.source." // TODO https://github.com/containerd/containerd/pull/8224 in containerd 1.7+

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 👀)

vvoland · 2023-02-10T10:00:41Z

I'm not sure whether it is safe to push a manifest without its layer blobs.

Yes, registry (DockerHub at least) will reject the manifest because it references blobs not present in the repository.
The reason why I do prepareMissing only for manifest lists and not manifests, is that I don't expect that image which targets platform-specific manifest will have any blobs missing.

Removing that check for manifests wouldn't hurt though, but if there's a case in which the one-platform image has layers missing then probably there's something wrong going on.

thaJeztah · 2023-03-07T15:38:46Z

api/server/router/image/image_routes.go

+	// Tag is empty only in case ImagePushOptions.All is true.
+	if tag != "" {


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)

daemon/containerd/image_push.go

tianon

Overall this looks pretty good to me -- in the maintainers call where we discussed this, I was concerned about having docker push end up pulling content locally, but just doing as much cross-blob mounting as we can for objects we don't have locally seems 100% sane and honestly really, really clever 👍

I've got a few of what I think are minor nits and would love to hear more maintainer opinions about exposing platform in the API for this endpoint before we've really ironed out the UX (because it's much harder to walk that external API back later if we decide we want something different than it is to just not introduce it for now), but I don't feel so strongly as to block it if I'm alone in my concerns. 🙇 ❤️

tianon · 2023-03-23T23:18:37Z

daemon/images/image_push.go

-		return err
-	}
-	if tag != "" {
-		// Push by digest is not supported, so only tags are supported.


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. 🤔

tianon · 2023-03-23T23:27:52Z

daemon/containerd/image_push.go

+	if _, ok := targetRef.(reference.Tagged); !ok {
+		return errdefs.NotImplemented(errors.New("push all tags is not implemented"))
+	}


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. 😂

daemon/containerd/image_push.go

tianon · 2023-03-23T23:32:47Z

daemon/containerd/image_push.go

+	}))
+	defer finishProgress()
+
+	var limiter *semaphore.Weighted = nil // TODO: Respect max concurrent downloads/uploads


👀 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. 👍

tianon · 2023-03-23T23:49:40Z

daemon/containerd/image_push.go

+				err = errdefs.NotFound(fmt.Errorf(
+					"missing content: %w\n"+
+						"Note: You're trying to push a manifest list/index which "+
+						"references multiple platform specific manifests, but not all of them are available locally "+
+						"or available to the remote repository.\n"+
+						"Make sure you have all the referenced content and try again.",
+					err))


This is really good, IMO 🙇

tianon · 2023-03-23T23:57:04Z

daemon/containerd/image_push.go

+	return *s, nil
+}
+
+const labelDistributionSource = "containerd.io/distribution.source."


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)

Suggested change

const labelDistributionSource = "containerd.io/distribution.source."

const labelDistributionSource = "containerd.io/distribution.source." // TODO https://github.com/containerd/containerd/pull/8224 in containerd 1.7+

tianon · 2023-03-24T00:02:31Z

daemon/containerd/image_push.go

+	return *s, nil
+}
+
+const labelDistributionSource = "containerd.io/distribution.source."


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 😂 🙈)

tianon · 2023-03-24T00:03:58Z

daemon/containerd/image_push.go

+	if containerdimages.IsConfigType(mediaType) {
+		return false
+	}


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!

tianon · 2023-03-24T00:09:25Z

api/swagger.yaml

+        - name: "platform"
+          in: "query"
+          description: |
+            Platform in the format `os[/arch[/variant]]` used to select
+            a platform-specific manifest if the image is an index/manifest list.
+
+            If the option is not set, then no specific manifest is select and the
+            whole image index is pushed.
+
+            When specified, the daemon checks if the image has any manifest that
+            matches the requested platform and pushes it instead of the whole index.
+            If there's no matching manifest it returns a `404` status.
+          type: "string"


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. 🙇

tianon · 2023-03-27T18:51:13Z

Discussed in the maintainers meeting today, and we'd like to hold off on changes to the push API (specifically the addition of platform) until we have the larger UX discussion about how push should look in a world where we have a more complex underlying image store and how we can better take advantage of that. 🙇

I think concretely that means pulling e39998f out for now (and the related code in the previous commit). 👀

Push the reference parsing from repo and tag names into the api and pass a reference object to the ImageService. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland · 2023-03-30T16:10:12Z

Removed platform parameter and related code. Also added the code to append distribution source layers to the pushed content. Can remove it if it's too much though.

AkihiroSuda · 2023-03-30T17:21:23Z

daemon/containerd/image_push.go

+
+// shouldDownload returns if the given descriptor can be cross-repo mounted
+// when pushing it to a remote reference ref.
+func canBeMounted(desc ocispec.Descriptor, targetRef reference.Named, isInsecureFunc func(string) bool, source distributionSource) bool {


Suggested change

func canBeMounted(desc ocispec.Descriptor, targetRef reference.Named, isInsecureFunc func(string) bool, source distributionSource) bool {

func canBeMounted(mediaType string, targetRef reference.Named, isInsecureFunc func(string) bool, source distributionSource) bool {

thaJeztah · 2023-03-30T17:36:17Z

daemon/containerd/image_push.go

+	return err
+}
+
+// prepareMissing will walk the target descriptor recursively and returns up


Looks like the docs doesn't match the function name

Thanks! Fixed.

thaJeztah · 2023-03-30T17:37:29Z

daemon/containerd/image_push.go

+	return reference.WithDigest(source.registryRef, dgst)
+}
+
+// shouldDownload returns if the given descriptor can be cross-repo mounted


Thanks! Fixed.

This implements `docker push` under containerd image store. When pushing manifest lists that reference a content which is not present in the local content store, it will attempt to perform the cross-repo mount the content if possible. Considering this scenario: ```bash $ docker pull docker.io/library/busybox ``` This will download manifest list and only host platform-specific manifest and blobs. Note, tagging to a different repository (but still the same registry) and pushing: ```bash $ docker tag docker.io/library/busybox docker.io/private-repo/mybusybox $ docker push docker.io/private-repo/mybusybox ``` will result in error, because the neither we nor the target repository doesn't have the manifests that the busybox manifest list references (because manifests can't be cross-repo mounted). If for some reason the manifests and configs for all other platforms would be present in the content store, but only layer blobs were missing, then the push would work, because the blobs can be cross-repo mounted (only if we push to the same registry). Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah

"LGTM" from a UX perspective (as discussed) I'm good with bringing this one in, but would appreciate a few eyes on the code

tianon · 2023-03-30T21:07:46Z

daemon/containerd/progress.go

+
+		if status.Committed && status.Offset >= status.Total {
+			if p.isMountable(j.Digest) {
+				progress.Update(out, id, "Mounted")


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:

moby/distribution/push_v2.go

Line 379 in 0656059

progress.Updatef(progressOutput, pd.ID(), "Mounted from %s", err.From.Name())

tianon

One minor comment, but probably fine for a follow-up 👍

thaJeztah

Looked a bit at the code; overall looks good, but left some nits, and a question about concurrency

thaJeztah · 2023-03-30T21:44:26Z

daemon/containerd/image_push.go

+	if err != nil {
+		return err
+	}
+	defer release(leasedCtx)


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

thaJeztah · 2023-03-30T21:45:26Z

daemon/containerd/image_push.go

+	store := i.client.ContentStore()
+
+	resolver, tracker := i.newResolverFromAuthConfig(authConfig)
+	pushProgress := pushProgress{Tracker: tracker}


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

Done in #45243

thaJeztah · 2023-03-30T21:47:03Z

daemon/containerd/image_push.go

+
+	resolver, tracker := i.newResolverFromAuthConfig(authConfig)
+	pushProgress := pushProgress{Tracker: tracker}
+	jobs := newJobs()


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

thaJeztah · 2023-03-30T21:51:46Z

daemon/containerd/image_push.go

+			for _, c := range children {
+				jobs.Add(c)
+			}


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

thaJeztah · 2023-03-30T21:56:20Z

daemon/containerd/image_push.go

+
+			for _, source := range sources {
+				if canBeMounted(desc.MediaType, targetRef, i.registryService.IsInsecureRegistry, source) {
+					mountableBlobs[desc.Digest] = source


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.

thaJeztah · 2023-03-30T22:00:57Z

daemon/containerd/image_push.go

+		registry := strings.TrimPrefix(k, labelDistributionSource)
+		if registry != k {
+			ref, err := reference.ParseNamed(registry + "/" + v)


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);

Suggested change

registry := strings.TrimPrefix(k, labelDistributionSource)

if registry != k {

ref, err := reference.ParseNamed(registry + "/" + v)

if reg := strings.TrimPrefix(k, labelDistributionSource); reg != k {

ref, err := reference.ParseNamed(reg + "/" + v)

Done in: #45243

thaJeztah · 2023-03-30T22:02:43Z

daemon/containerd/image_push.go

+		return false
+	}
+
+	registry := reference.Domain(targetRef)


same here (registry colliding with import)

Done in: #45243

thaJeztah · 2023-03-30T22:08:57Z

(those should be fine for a follow-up though 😂 because it's already in 😂)

vvoland added area/api area/images containerd-integration Issues and PRs related to containerd integration labels Feb 9, 2023

vvoland commented Feb 9, 2023

View reviewed changes

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

vvoland force-pushed the c8d-push-upstream branch from 11b21c5 to 31376c9 Compare February 9, 2023 13:24

vvoland force-pushed the c8d-push-upstream branch 3 times, most recently from 7d7c0c3 to b39819b Compare February 9, 2023 15:10

AkihiroSuda reviewed Feb 9, 2023

View reviewed changes

vvoland added the status/2-code-review label Feb 22, 2023

thaJeztah added this to the v-next milestone Feb 23, 2023

thaJeztah reviewed Mar 7, 2023

View reviewed changes

daemon/containerd/image_push.go Outdated Show resolved Hide resolved

thaJeztah mentioned this pull request Mar 7, 2023

labels: Add LabelDistributionSource containerd/containerd#8224

Merged

vvoland requested review from corhere, cpuguy83, neersighted and rumpl March 13, 2023 17:42

vvoland marked this pull request as draft March 16, 2023 16:36

vvoland force-pushed the c8d-push-upstream branch 2 times, most recently from 3bed76b to e39998f Compare March 22, 2023 17:34

tianon reviewed Mar 24, 2023

View reviewed changes

vvoland force-pushed the c8d-push-upstream branch from e39998f to 5e632df Compare March 30, 2023 16:08

images/push: Accept reference

e7393d0

Push the reference parsing from repo and tag names into the api and pass a reference object to the ImageService. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the c8d-push-upstream branch from 5e632df to 3d97a3d Compare March 30, 2023 16:08

vvoland marked this pull request as ready for review March 30, 2023 16:10

vvoland force-pushed the c8d-push-upstream branch from 3d97a3d to 8df1428 Compare March 30, 2023 16:43

AkihiroSuda reviewed Mar 30, 2023

View reviewed changes

thaJeztah reviewed Mar 30, 2023

View reviewed changes

vvoland force-pushed the c8d-push-upstream branch from 8df1428 to a75354c Compare March 30, 2023 17:54

thaJeztah approved these changes Mar 30, 2023

View reviewed changes

tianon mentioned this pull request Mar 30, 2023

Don't allow pulling all tags by default for the ImageCreate API endpoint #42053

Open

tianon reviewed Mar 30, 2023

View reviewed changes

tianon approved these changes Mar 30, 2023

View reviewed changes

tianon mentioned this pull request Mar 30, 2023

remotes/docker: Add Mounted and Exists push status containerd/containerd#8330

Merged

tianon merged commit f537ef5 into moby:master Mar 30, 2023

thaJeztah reviewed Mar 30, 2023

View reviewed changes

thaJeztah mentioned this pull request Mar 30, 2023

c8d: push: addressing some nits #45243

Merged

vvoland mentioned this pull request Mar 31, 2023

c8d/push: Follow up fixes #45249

Merged

	// collectSources walks the content store and looks for content which can
	// provide a source registry and repository for the provided descriptors from
	// the containerd.io/distribution.source labels
	func collectSources(ctx context.Context, toCollect []ocispec.Descriptor, store content.Store, sources map[digest.Digest]distributionSource) error {
	// Nothing to do.
	if len(toCollect) == 0 {
	return nil
	}

	// Make a copy of the missing descriptors as we will be removing
	// the descriptors that we found a source for.
	missing := make([]ocispec.Descriptor, len(toCollect))
	copy(missing, toCollect)

	success := errors.New("success, found the source but can't return earlier without an error")
	err := store.Walk(ctx, func(i content.Info) error {
	source := extractDistributionSource(i.Labels)
	log := logrus.
	WithField("digest", i.Digest)

	log.Debug("walk")

	// Nah, we're looking for a parent of this lazy child.
	// This one will not provide us with the source.
	if source.value == "" {
	return nil
	}

	desc := ocispec.Descriptor{Digest: i.Digest}

	// Do a simple peek of the content to avoid big blobs which definitely aren't json.
	notJson, err := peekNotJson(ctx, store, desc)
	if err != nil {
	return err
	}
	if notJson {
	log.Debug("skipping, definitely not a json")
	return nil
	}

	// Read the manifest
	blob, err := content.ReadBlob(ctx, store, desc)
	if err != nil {
	log.WithError(err).Error("error reading blob")
	return err
	}

	// Manifests and indexes have different children.
	// Index stores other manifests and manifests store layers.
	// To avoid unmarshaling the blob separately as manifest and index
	// this holds fields that contains them both and the media type.
	var indexOrManifest struct {
	MediaType string `json:"mediaType,omitempty"`
	Manifests []ocispec.Descriptor `json:"manifests,omitempty"`
	Layers []ocispec.Descriptor `json:"layers,omitempty"`
	Config ocispec.Descriptor `json:"config,omitempty"`
	}

	err = json.Unmarshal(blob, &indexOrManifest)
	if err != nil {
	log.WithError(err).Debug("unmarshal failed")
	return nil
	}

	mediaType := indexOrManifest.MediaType
	// Just in case, check if it really is manifest or index.
	if !containerdimages.IsManifestType(mediaType) && !containerdimages.IsIndexType(mediaType) {
	log.Debug("not a manifest/index")
	return nil
	}
	children := append(indexOrManifest.Layers, indexOrManifest.Manifests...)
	if indexOrManifest.Config.Digest != digest.Digest("") {
	children = append(children, indexOrManifest.Config)
	}

	if len(children) == 0 {
	log.Debug("empty a manifest/index")
	return nil
	}

	// Look if this manifest/index specifies any of the missing content
	for _, layer := range children {
	for idx := 0; idx < len(missing); idx += 1 {
	wanted := missing[idx]
	if layer.Digest == wanted.Digest {
	// Found it!
	sources[wanted.Digest] = source
	log.WithField("wanted", wanted.Digest.String()).Debug("found")

	// Don't look for it anymore
	if len(missing) > 1 {
	lastIdx := len(missing) - 1
	missing[idx] = missing[lastIdx]
	missing = missing[:lastIdx]
	idx -= 1
	} else {
	// We found all missing, let's end the walk.
	missing = missing[:0]
	return success
	}
	}
	}
	}

	return nil
	})

	if err == success {
	err = nil
	}
	if len(missing) > 0 {
	msg := "missing blobs with no source: "
	for idx, c := range missing {
	if idx != 0 {
	msg += ", "
	}
	msg += c.Digest.String()
	}
	err = errdefs.NotFound(errors.New(msg))
	}

	return err
	}

	const labelDistributionSource = "containerd.io/distribution.source."
	const labelDistributionSource = "containerd.io/distribution.source." // TODO https://github.com/containerd/containerd/pull/8224 in containerd 1.7+

		// Tag is empty only in case ImagePushOptions.All is true.
		if tag != "" {

	func canBeMounted(desc ocispec.Descriptor, targetRef reference.Named, isInsecureFunc func(string) bool, source distributionSource) bool {
	func canBeMounted(mediaType string, targetRef reference.Named, isInsecureFunc func(string) bool, source distributionSource) bool {

c8d: Implement push #44963

c8d: Implement push #44963

Conversation

vvoland commented Feb 9, 2023 • edited

vvoland commented Feb 9, 2023

AkihiroSuda commented Feb 9, 2023 • edited

Choose a reason for hiding this comment

vvoland Mar 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvoland commented Feb 10, 2023

thaJeztah Mar 7, 2023 • edited by tianon

Choose a reason for hiding this comment

tianon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Mar 27, 2023

vvoland commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Mar 30, 2023

vvoland commented Feb 9, 2023 •

edited

AkihiroSuda commented Feb 9, 2023 •

edited

vvoland Mar 7, 2023 •

edited

thaJeztah Mar 7, 2023 •

edited by tianon