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

@@ -57,7 +57,7 @@ func Commit(ctx context.Context, client *containerd.Client, rawRef string, req s
 			if found.MatchCount > 1 {
 				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.

Signed-off-by: apostasie <[email protected]>

apostasie · 2024-09-25T05:19:31Z

CI is green (save for the windows regression #3437).

@AkihiroSuda This is how far I will go with this.

I already hate this PR, and there are still glaring issues to fix in all that stuff, especially the whole tmp-reduced-platform contorsions, and overall most things touching platform code (which definitely stems from containerd btw - just staring at stevvooe's 10 years old code and comments in that regard make my eyes bleed).

Anyhow, I still believe this PR here will help a lot, as the issue being fixed has been one of our most pervasive and egregious.

Credit for this fix should go to @lingdie who did the research and correctly identified the underlying containerd issue, then pointed me in the right direction.

PTAL at your convenience.

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

cmd/nerdctl/container/container_commit_linux_test.go

@@ -55,26 +52,45 @@ func TestKubeCommitPush(t *testing.T) {
 		cmd := testutil.KubectlHelper(base, "get", "pods", tID, "-o", "jsonpath={ .status.containerStatuses[0].containerID }")
 		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

 )

-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

@@ -125,7 +125,8 @@ func TestImagesFilterDangling(t *testing.T) {
 	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

@@ -23,6 +23,7 @@ readonly root

 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

@@ -31,7 +32,7 @@ import (
 func Tag(ctx context.Context, client *containerd.Client, options types.ImageTagOptions) error {
 	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

@@ -126,6 +127,13 @@ func Commit(ctx context.Context, client *containerd.Client, container containerd
 		return emptyDigest, err
 	}

+	// 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 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

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

apostasie force-pushed the bug-3425-missing-layer branch 2 times, most recently from 5d966e5 to d5e41e8 Compare September 24, 2024 23:52

apostasie marked this pull request as draft September 25, 2024 00:11

apostasie added 6 commits September 24, 2024 21:27

Add fetch and EnsureAllContent methods

9539f6b

Signed-off-by: apostasie <[email protected]>

EnsureAllContent on commit

8ba7ce7

Signed-off-by: apostasie <[email protected]>

EnsureAllContent on convert

ef09d19

Signed-off-by: apostasie <[email protected]>

EnsureAllContent on save

8e10d87

Signed-off-by: apostasie <[email protected]>

EnsureAllContent on tag

1e52bf5

Signed-off-by: apostasie <[email protected]>

Fix tests and add regression tests for containerd#3425

764a2aa

Signed-off-by: apostasie <[email protected]>

apostasie force-pushed the bug-3425-missing-layer branch from d5e41e8 to 764a2aa Compare September 25, 2024 04:30

apostasie marked this pull request as ready for review September 25, 2024 05:20

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

Draft

Test rework, part 3 #3464

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

apostasie commented Sep 18, 2024 •

edited

Loading

apostasie Sep 18, 2024

apostasie Sep 18, 2024

apostasie commented Sep 19, 2024

lingdie Sep 19, 2024

apostasie Sep 19, 2024

apostasie Sep 19, 2024 •

edited

Loading

lingdie Sep 19, 2024

apostasie Sep 19, 2024

apostasie Sep 19, 2024

lingdie Sep 20, 2024

apostasie commented Sep 25, 2024 •

edited

Loading

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie Sep 25, 2024

apostasie commented Sep 25, 2024

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

Are you sure you want to change the base?

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

Conversation

apostasie commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostasie commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostasie Sep 19, 2024 • edited Loading

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

apostasie commented Sep 25, 2024 • edited Loading

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

apostasie commented Sep 25, 2024

apostasie commented Sep 18, 2024 •

edited

Loading

apostasie Sep 19, 2024 •

edited

Loading

apostasie commented Sep 25, 2024 •

edited

Loading