Ensure all layers are here when tagging, committing, saving, or converting #3435

apostasie · 2024-09-18T19:15:52Z

See #3425 and linked issues for context.

fix Issues related to missing layer (likely caused by https://github.com/containerd/containerd/issues/8973 ) #3425
fix Cannot commit and push container image from Kubernetes pods #827 (commit)
fix layer is not pulled when using the 'nerdctl pull' #2357 (save)
fix flaky test: content digest sha256:xxx: not found in CI #2327 (convert)
fix nerdctl push command not working when using minikube with containerd (invalid checksum digest format) #2506 (tag)
fix nerdctl push level=fatal msg="content digest sha256:xxx: not found" #3026 (commit)

Unconfirmed (no test), but cannot reproduce anymore with the patch:

fix Build fail with "parent snapshot does not exist" #3420

Unconfirmed, but probably fixed as well:

nerdctl image save fails when discard_unpacked_layers=true is enabled #2588 (untested)

Reviewers, most of this is test wrangling:

enabling Kube test which was missing a few bits
fixing broken image inspect tests that are now more likely to show leftovers

cmd/nerdctl/container/container_commit_linux_test.go

apostasie · 2024-09-18T19:28:19Z

pkg/cmd/container/commit.go

 				return fmt.Errorf("multiple IDs found with provided prefix: %s", found.Req)
 			}
-			imageID, err := commit.Commit(ctx, client, found.Container, opts)
+			imageID, err := commit.Commit(ctx, client, found.Container, opts, options.GOptions)


Might be better to add GOptions to the commit opts struct, but this is good enough short term.

apostasie · 2024-09-18T19:29:08Z

pkg/imgutil/imgutil.go

+	}
+	refDomain := distributionref.Domain(named)
+
+	// Get a resolver


We should put that away somewhere with the login code refactor. Resolver is repetitive and error prone...

apostasie · 2024-09-19T00:11:39Z

@AkihiroSuda can you poke the CI?

lingdie · 2024-09-19T03:09:57Z

pkg/cmd/image/tag.go

+	err = imgutil.EnsureAllContent(ctx, client, srcName, img.Target, "", options.GOptions)
+	if err != nil {
+		return err
+	}


When tagging, do we need to return errors for failed pulls? This might not meet expectations.

Yes, that is a valid concern.
Then if at a later point we try to push it, it will fail…
Not sure how to deal with that…

I do not like any of the code involved in there.
The "single-platform tmp-reduced-platform image" gymnastic / convert intermediary step on push just smells fishy.
Then again, this is containerd design issues on display here.

We could introduce a new flag called ensure-image for both the tag and commit commands.

When this flag is set, we would attempt to pull any missing layers. If any errors occur during this process, they would be returned.

The thing that is bugging me with the new flag approach is that it would leave the default behavior broken, which I don't think is right.

So, what about this: instead of hard error when the fetch fail, we just print a loud warning but we continue?
wdyt @lingdie ?

Latest push implements this.
We no longer hard error, but warn the user.
PTAL at your convenience @lingdie

look good to me.

apostasie · 2024-09-25T20:36:27Z

cmd/nerdctl/container/container_commit_linux_test.go

 		cmd.Run()
 		containerID = strings.TrimPrefix(cmd.Out(), "containerd://")
+
+		// This below is missing configuration to allow for plain http communication


Was not able / interested enough to finish setting up a cluster registry.
This here is a start, and can be completed later on, either by me or someone else.

apostasie · 2024-09-25T20:36:51Z

cmd/nerdctl/container/container_commit_linux_test.go


 	tearDown := func() {
-		testutil.KubectlHelper(base, "delete", "pod", "-f", tID).Run()
+		testutil.KubectlHelper(base, "delete", "pod", "--all").Run()


For some reason that escapes me, kubectl was not cleaning up with the pod name

apostasie · 2024-09-25T20:37:09Z

cmd/nerdctl/container/container_commit_linux_test.go

-			Err:      "failed to create a tmp single-platform image",
-		})
+		base.Cmd("commit", containerID, "testcommitsave").AssertOK()
+		base.Cmd("save", "testcommitsave").AssertOK()


This correctly reproduce issue #827 I believe.

apostasie · 2024-09-25T20:37:44Z

cmd/nerdctl/image/image_inspect_test.go

+	"github.com/containerd/nerdctl/v2/pkg/testutil/test"
 )

-func TestImageInspectContainsSomeStuff(t *testing.T) {


This turned out flaky as I was debugging this new code.
Rewriting against the new toolkit to fix that.

apostasie · 2024-09-25T20:37:54Z

cmd/nerdctl/image/image_inspect_test.go


 func TestImageInspectDifferentValidReferencesForTheSameImage(t *testing.T) {
-	testutil.DockerIncompatible(t)
+	nerdtest.Setup()


Same as the other test.

apostasie · 2024-09-25T20:39:00Z

cmd/nerdctl/image/image_list_test.go

 	testutil.RequiresBuild(t)
 	testutil.RegisterBuildCacheCleanup(t)
 	base := testutil.NewBase(t)
-	base.Cmd("images", "prune", "--all").AssertOK()


This (obviously?) was doing nothing and leftovers from other tests were having an impact here.
Because we depend on build here, we cannot use a private namespace, so, making sure we clean house.

apostasie · 2024-09-25T20:40:05Z

cmd/nerdctl/issues/issues_linux_test.go

@@ -0,0 +1,133 @@
+/*


Since the underlying issue is impacting a lot of different functions, it did not feel right to scatter it over in different commands.
Suggesting instead to have a new /issues/ folder for these type of stuff (eg: regression testing for complex / multi-command issues)

apostasie · 2024-09-25T20:40:47Z

hack/build-integration-kubernetes.sh


 GO_VERSION=1.23
 KIND_VERSION=v0.24.0
+CNI_PLUGINS_VERSION=v1.5.1


Somehow when I came up with the Kube tooling, I did not test networking.
We (obviously?) do need cni here for nerdctl to work outside of kube itself.

apostasie · 2024-09-25T20:41:13Z

pkg/cmd/image/tag.go

 	imageService := client.ImageService()
 	var srcName string
-	imagewalker := &imagewalker.ImageWalker{
+	walker := &imagewalker.ImageWalker{


Do not shadow the package name.

apostasie · 2024-09-25T20:42:03Z

pkg/cmd/image/tag.go

+	// Ensure all the layers are here: https://github.com/containerd/nerdctl/issues/3425
+	err = EnsureAllContent(ctx, client, srcName, options.GOptions)
+	if err != nil {
+		log.G(ctx).Warn("Unable to fetch missing layers before committing. " +


On tag, we do not hard error if we fail to retrieve the missing layers.
This is to preserve the previous behavior, in case we are offline.
Warn the user instead, and point it to a special purpose ticket that provides explanations.

apostasie · 2024-09-25T20:42:25Z

pkg/imgutil/commit/commit.go


+	// Ensure all the layers are here: https://github.com/containerd/nerdctl/issues/3425
+	err = image.EnsureAllContent(ctx, client, baseImg.Name(), globalOptions)
+	if err != nil {


Same as tag, we do not hard error on commit.

apostasie · 2024-09-25T20:43:03Z

pkg/cmd/image/convert.go

+	// Ensure all the layers are here: https://github.com/containerd/nerdctl/issues/3425
+	err = EnsureAllContent(ctx, client, srcRawRef, options.GOptions)
+	if err != nil {
+		return err


Convert and Save DO hard error, since the operation cannot complete if there is missing content.

apostasie · 2024-09-25T20:43:53Z

Added some inline comments to ease review.

apostasie · 2024-10-01T22:42:48Z

@AkihiroSuda with containerd rc5 just out - it would be nice to have this one in for the next nerdctl rc so that we can get it some soak time.

AkihiroSuda

Thanks

ChrisBr · 2024-12-13T13:15:25Z

@apostasie we seem to run into this issue when using the GKE image streaming snapshotter. When we commit an image locally (e.g. with docker) and don't push it, the converter tries to pull the image from remote. Is this expected?

Example

> docker run --name foo alpine
> docker commit foo gcr.io/$repo/cbruckmayer:foo

# I can use the image with nerdctl
> nerdctl --address /var/run/docker/containerd/containerd.sock --namespace moby --snapshotter gcfs run -it --network none gcr.io/$repo/cbruckmayer:foo

# However, when trying to convert the image, it will try to pull it from remote which fails with not found

> nerdctl --address /var/run/docker/containerd/containerd.sock --namespace moby --snapshotter gcfs image convert --zstd --oci gcr.io/$repo/cbruckmayer:foo gcr.io/$repo/cbruckmayer:foo-zstd
gcr.io/$repo/cbruckmayer:foo: resolving      |--------------------------------------|
elapsed: 0.1 s                                total:   0.0 B (0.0 B/s)
INFO[0000] fetch failed after status: 404 Not Found      host=gcr.io
FATA[0000] failed to resolve reference "gcr.io/$repo/cbruckmayer:foo": gcr.io/$repo/cbruckmayer:foo: not found
root@docker-daemon-rzhtc:/app# nerdctl --debug-full --address /var/run/docker/containerd/containerd.sock --namespace moby --snapshotter gcfs image convert --zstd --oci gcr.io/$repo/cbruckmayer:foo gcr.io/$repo/cbruckmayer:foo-zstd
DEBU[0000] fetching                                      image="gcr.io/$repo/cbruckmayer:foo"
DEBU[0000] resolving                                     host=gcr.io
DEBU[0000] do request                                    host=gcr.io request.header.accept="application/vnd.docker.distribution.manifest.v2+json, application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.oci.image.manifest.v1+json, application/vnd.oci.image.index.v1+json, */*" request.header.user-agent=containerd/2.0.0+unknown request.method=HEAD url="https://gcr.io/v2/$repo/cbruckmayer/manifests/foo"
DEBU[0000] fetch response received                       host=gcr.io response.header.accept-ranges=none response.header.content-type=application/json response.header.date="Fri, 13 Dec 2024 13:10:25 GMT" response.header.docker-distribution-api-version=registry/2.0 response.header.server="Docker Registry" response.header.vary=Accept-Encoding response.header.www-authenticate="Bearer realm=\"https://gcr.io/v2/token\",service=\"gcr.io\",scope=\"repository:$repo/cbruckmayer:pull\"" response.header.x-frame-options=SAMEORIGIN response.header.x-xss-protection=0 response.status="401 Unauthorized" url="https://gcr.io/v2/$repo/cbruckmayer/manifests/foo"
DEBU[0000] Unauthorized                                  header="Bearer realm=\"https://gcr.io/v2/token\",service=\"gcr.io\",scope=\"repository:$repo/cbruckmayer:pull\"" host=gcr.io
DEBU[0000] do request                                    host=gcr.io request.header.accept="application/vnd.docker.distribution.manifest.v2+json, application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.oci.image.manifest.v1+json, application/vnd.oci.image.index.v1+json, */*" request.header.user-agent=containerd/2.0.0+unknown request.method=HEAD url="https://gcr.io/v2/$repo/cbruckmayer/manifests/foo"
DEBU[0000] fetch response received                       host=gcr.io response.header.accept-ranges=none response.header.content-type=application/json response.header.date="Fri, 13 Dec 2024 13:10:25 GMT" response.header.docker-distribution-api-version=registry/2.0 response.header.server="Docker Registry" response.header.vary=Accept-Encoding response.header.x-frame-options=SAMEORIGIN response.header.x-xss-protection=0 response.status="404 Not Found" url="https://gcr.io/v2/$repo/cbruckmayer/manifests/foo"
INFO[0000] fetch failed after status: 404 Not Found      host=gcr.io
FATA[0000] failed to resolve reference "gcr.io/$repo/cbruckmayer:foo": gcr.io/shopify-docker-images/cbruckmayer:foo: not found

apostasie · 2024-12-13T17:03:45Z

@ChrisBr
Unfortunately, yes, this is expected, as it is necessary to mitigate the unforeseen effects of containerd "lazy" pulling.

Though, in this case, maybe we can relax to a soft error if the pull fails.

Do you mind opening a new issue with these details?
I'll look into it ASAP.

ChrisBr · 2024-12-16T09:42:52Z

@apostasie #3764.

Thanks for looking into it, really appreciated.

apostasie force-pushed the bug-3425-missing-layer branch from 8c55dbd to 82daf62 Compare September 18, 2024 19:23

apostasie commented Sep 18, 2024

View reviewed changes

cmd/nerdctl/container/container_commit_linux_test.go Outdated Show resolved Hide resolved

apostasie commented Sep 18, 2024

View reviewed changes

apostasie force-pushed the bug-3425-missing-layer branch 2 times, most recently from 58a9dd3 to 6aa644c Compare September 18, 2024 19:36

apostasie changed the title ~~Ensure all layers are here when tagging~~ Ensure all layers are here when tagging or committing Sep 18, 2024

apostasie marked this pull request as draft September 18, 2024 20:13

apostasie changed the title ~~Ensure all layers are here when tagging or committing~~ [WIP] Ensure all layers are here when tagging or committing Sep 18, 2024

apostasie force-pushed the bug-3425-missing-layer branch 14 times, most recently from ccdb533 to d2905da Compare September 18, 2024 23:23

apostasie marked this pull request as ready for review September 19, 2024 00:11

apostasie changed the title ~~[WIP] Ensure all layers are here when tagging or committing~~ Ensure all layers are here when tagging or committing Sep 19, 2024

lingdie reviewed Sep 19, 2024

View reviewed changes

apostasie force-pushed the bug-3425-missing-layer branch 2 times, most recently from 297233b to c64c494 Compare September 19, 2024 22:47

apostasie requested a review from lingdie September 19, 2024 22:48

AkihiroSuda added this to the v2.0.0 milestone Sep 25, 2024

AkihiroSuda self-requested a review September 25, 2024 10:52

apostasie commented Sep 25, 2024

View reviewed changes

This was referenced Sep 25, 2024

[WIP] Test rework, next #3461

Closed

Test rework, part 3 #3464

Merged

AkihiroSuda approved these changes Oct 2, 2024

View reviewed changes

AkihiroSuda merged commit b943664 into containerd:main Oct 2, 2024
22 checks passed

This was referenced Oct 3, 2024

IPFS: layer not found - follow-up to #3425 #3489

Closed

content digest not found: platform-reduced push, follow-up to #3425 #3509

Closed

[MAJOR] content digest not found #3513

Closed

ChrisBr mentioned this pull request Dec 16, 2024

Converting image present in snapshotter fails #3764

Open

apostasie mentioned this pull request Jan 31, 2025

Unused unpacked snapshot left in content store after nerdctl system prune #2372

Closed

This was referenced Feb 6, 2025

Fix CI by exclude the .ansible in .ansible-lint & remove ctr image pull workaround kubernetes-sigs/kubespray#11948

Merged

The nerdctl save command pulls the image when the image is already pulled by the ctr i pull. #3866

Closed

Uh oh!

Ensure all layers are here when tagging, committing, saving, or converting #3435

Ensure all layers are here when tagging, committing, saving, or converting #3435

Uh oh!

Conversation

apostasie commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apostasie commented Sep 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apostasie Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apostasie commented Sep 25, 2024

Uh oh!

apostasie commented Oct 1, 2024

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisBr commented Dec 13, 2024

Uh oh!

apostasie commented Dec 13, 2024

Uh oh!

ChrisBr commented Dec 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apostasie commented Sep 18, 2024 •

edited

Loading

apostasie Sep 19, 2024 •

edited

Loading