Repository: containerd/containerd
File: internal/cri/server/images/image_pull.go (Go)

Code before:
		return "", fmt.Errorf("failed to parse image reference %q: %w", name, err)
	}

	ref := namedRef.String()
	if ref != name {
		log.G(ctx).Debugf("PullImage using normalized image ref: %q", ref)
	}

	imagePullProgressTimeout, err := time.ParseDuration(c.config.ImagePullProgressTimeout)
	if err != nil {
		return "", fmt.Errorf("failed to parse image_pull_progress_timeout %q: %w", c.config.ImagePullProgressTimeout, err)
	}

	snapshotter, err := c.snapshotterFromPodSandboxConfig(ctx, ref, sandboxConfig, runtimeHandler)
	if err != nil {
		return "", err
	}

	// If the image exists but content was garbage collected (metadata remains,
	// blobs missing), delete the stale image to force a clean re-pull.
	// Stale snapshots are handled by the retry logic in pullWithStaleSnapshotRetry.
	//
	// We intentionally do NOT short-circuit here when the image is already
	// properly unpacked: PullImage may be called with AlwaysPull policy,
	// which should always contact the registry to check for updates.
	if _, _, err := c.imageForSnapshotter(ctx, ref, snapshotter); errdefs.IsFailedPrecondition(err) {
		log.G(ctx).Warnf("Image %q not properly unpacked for snapshotter %q (possible stale metadata), forcing fresh pull; if this persists try: ctr image rm %s", ref, snapshotter, ref)
		if delErr := c.images.Delete(ctx, ref, containerdimages.SynchronousDelete()); delErr != nil {
			log.G(ctx).WithError(delErr).Warnf("Failed to delete image %q, will try pull anyway", ref)
		}
	}

	// If ref is a digest (sha256:...), we need to find a pullable reference.
	// Registry pull requires a full reference like docker.io/library/nginx:latest,
	// not just sha256:abc123...
	if img, err := c.LocalResolve(ref); err == nil && len(img.References) > 0 {
		// Find a pullable reference (prefer one that's not a digest)
		for _, imgRef := range img.References {
			if !strings.Contains(imgRef, "@sha256:") {
				ref = imgRef
				break
			}
		}
		// If all references are digest-based, just use the first one
		if strings.HasPrefix(ref, "sha256:") && len(img.References) > 0 {
			ref = img.References[0]
		}
	}

	span.SetAttributes(
		tracing.Attribute("image.ref", ref),

Review comment (security):
Could you please revert the change in PullImage?

It makes the function harder to maintain, and it does not actually work in practice.

1. For non-pause images, even if there is stable content as you mentioned, this approach still cannot guarantee success because it does not have credentials. For missing content, we can address that by setting `discard_unpacked_layers = false`, or by handling it on the kubelet side. If this change is only for the pause image, I don’t think it is worth doing.

2. Deleting an image cannot guarantee snapshotter cleanup.

Assume image A is built on top of image B, and both are unpacked by nydus without labels. In that case, you cannot clean up the snapshotter data related to B until image A is also cleaned up.

3. Based on the discussion here: https://github.com/containerd/containerd/pull/12835#discussion_r2836708333

I’m not a fan of handling image deletion in PullImage. In my opinion, that is not a good design.
**If the goal is to handle data migration in this patch, I personally cannot accept this approach.**

The nydus snapshotter should return an error during unpacking if the required information is missing.

------------

I think the workflow should be:

Check whether it is already unpacked. If not, unpack it with the label.

https://github.com/containerd/containerd/blob/591de240579f5e043c09c2853ed06ef8d5a87491/internal/cri/server/container_create.go#L353-L361

Currently, containerd has the `WithNewSnapshot` option, which can unpack an image with a specific snapshotter. This patch should use the proper snapshotter based on the runtime config, and include the image reference in the label.

Code after:
		return "", fmt.Errorf("failed to parse image reference %q: %w", name, err)
	}

	ref := namedRef.String()
	if ref != name {
		log.G(ctx).Debugf("PullImage using normalized image ref: %q", ref)
	}

	imagePullProgressTimeout, err := time.ParseDuration(c.config.ImagePullProgressTimeout)
	if err != nil {
		return "", fmt.Errorf("failed to parse image_pull_progress_timeout %q: %w", c.config.ImagePullProgressTimeout, err)
	}

	snapshotter, err := c.snapshotterFromPodSandboxConfig(ctx, ref, sandboxConfig, runtimeHandler)
	if err != nil {
		return "", err
	}

	span.SetAttributes(
		tracing.Attribute("image.ref", ref),
		tracing.Attribute("snapshotter.name", snapshotter),
	)
	labels := c.getLabels(ctx, ref)

	// If UseLocalImagePull is true, use client.Pull to pull the image, else use transfer service by default.
	//
	// Transfer service does not currently support all the CRI image config options.
	// TODO: Add support for DisableSnapshotAnnotations, DiscardUnpackedLayers, ImagePullWithSyncFs and unpackDuplicationSuppressor
	var image containerd.Image
	if c.config.UseLocalImagePull {
		image, err = c.pullImageWithLocalPull(ctx, ref, credentials, snapshotter, labels, imagePullProgressTimeout)
	} else {
		image, err = c.pullImageWithTransferService(ctx, ref, credentials, snapshotter, labels, imagePullProgressTimeout)
	}

	if err != nil {
		return "", err
	}

	span.AddEvent("Pull and unpack image complete")

	configDesc, err := image.Config(ctx)
	if err != nil {
		return "", fmt.Errorf("get image config descriptor: %w", err)
	}
	imageID := configDesc.Digest.String()

	repoDigest, repoTag := util.GetRepoDigestAndTag(namedRef, image.Target().Digest)
	for _, r := range []string{imageID, repoTag, repoDigest} {
		if r == "" {
			continue