[P0] Fix #3513 (`[MAJOR] content digest not found`) #4062

apostasie · 2025-03-31T16:37:16Z

Current status

Discussion and patch upstream: containerd/containerd#11628

TL;DR here: this PR here on nerdctl does seem to fix our issues (many runs of the CI with no issue), because it entirely bypasses converter.Convert Delete logic which seems to trigger some underlying containerd issue related to lease and deletion (<- my opinion, unverified).

My patch upstream does ALSO fix our issue, with a clear reproducer, although it probably just avoids the underlying containerd problem.

Original post below

It looks like containerd imagestore.Delete is asynchronous by default.

If you want a synchronous delete, you have to pass an additional opt.

Of course, this seems to be a problem when you:

check if the image exist
delete it (async) if it does
create a new version of it

This seem to be the case (actually, it is worse) for https://github.com/containerd/containerd/blob/af013606262f9df1de4cc80688725a15a375fb3a/core/images/converter/converter.go#L120-L121
... where convert.Convert just forcefully delete, whether or not the image existed, async...

This of course is also something we are doing internally.

This PR addresses these issues:

ensure we delete SYNCHRONOUSLY where it matters
honor the async option when there is one
wrap / workaround containerd (* maybe) faulty implementation of convert

At this point, this is still a hunch - but for the first time I feel confident that we may be reaching the bottom of it, as this explanation closely matches the soul-destroying symptoms we have witnessed in eg: #3509 (comment)

and more generally, with anything touching containerd Convert (see #3513).

I will close an open this PR many times and confirm that the infamous #3513 is finally gone, but I would like the expert opinion of @lingdie on this - if you have time for a quick review.

Upstream PR: containerd/containerd#11628

Note that this PR here does not try to do more than that - there MAY still be places where we would need to EnsureContent.

EDIT: looks like containerd maintainer disagree with this, so, maybe I am misreading this.
Anyhow, I will keep closing / opening and run the CI - we are currently at:

four runs without failure
fifth run failed on Github cache
sixth run failure is [CI]: buildg times-out #4046

apostasie · 2025-03-31T20:32:27Z

Green once. 🤔

apostasie · 2025-03-31T20:42:06Z

pkg/cmd/builder/build.go

@@ -127,7 +127,7 @@ func Build(ctx context.Context, client *containerd.Client, options types.Builder
 			if _, err := imageService.Create(ctx, image); err != nil {
 				// if already exists; skip.
 				if errors.Is(err, errdefs.ErrAlreadyExists) {
-					if err = imageService.Delete(ctx, targetRef); err != nil {
+					if err = imageService.Delete(ctx, targetRef, images.SynchronousDelete()); err != nil {


This may explain some of the "internal: not found" errrors we sometimes see when building.

Indeed this behavior is not intuitive at all. I expect that delete to be sync by default and the async is activated by parameter, Good catch

apostasie · 2025-03-31T21:17:04Z

Green twice. 😓

pkg/imgutil/converter/convert.go

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

fahedouch · 2025-04-01T16:48:23Z

pkg/cmd/builder/build.go

@@ -127,7 +127,7 @@ func Build(ctx context.Context, client *containerd.Client, options types.Builder
 			if _, err := imageService.Create(ctx, image); err != nil {
 				// if already exists; skip.
 				if errors.Is(err, errdefs.ErrAlreadyExists) {
-					if err = imageService.Delete(ctx, targetRef); err != nil {
+					if err = imageService.Delete(ctx, targetRef, images.SynchronousDelete()); err != nil {


Indeed this behavior is not intuitive at all. I expect that delete to be sync by default and the async is activated by parameter, Good catch

fahedouch · 2025-04-01T16:58:18Z

pkg/imgutil/converter/convert.go

+
+	img.Name = dstRef
+
+	_ = imageService.Delete(ctx, img.Name, images.SynchronousDelete())


do we need to delete/re-create destRef if it already exists ?

It does not seem like we do need to delete indeed (note that I am not 100% familiar with it yet).

Indeed, my containerd patch does away with the delete (which is the source of the problem), and instead tries to update, then if it fails tries to create.

This patch here is opting for the least amount of change, but of course happy to clean-up a bit more if we want.

lmk your preference.

@fahedouch

Actually, I don't think this is the end of it (though I am convinced this patch will significantly enhance the situation).

I think in a follow-up PR we should thoroughly review our codebase and:

make sure we use leases where need be (GC might still happen and trigger similar symptoms)

evaluate why we are calling delete and what happens if we do not

LGTM to unblock the situation. But in the short and medium term, I don't want to move the containerd logic to nerdctl.

fahedouch · 2025-04-01T17:01:17Z

pkg/imgutil/converter/convert.go

+// Something seems wrong in converter.Convert.
+// When dstRef != srcRef, convert will first forcefully delete dstRef,
+// *asynchronously*, then create the image.
+// This seems to cause a race conditions, and the deletion may kick in after the creation.


the lease is not maintained during the creation ? 🤔

No clue.

My gut intuition is that this thing faceplants when the old ref is actually exactly the same image as new ref.

Note that patching containerd to also use a sync delete does NOT fix the issue. 🤔

Anyhow, for me the bottom-line really is:

do not use leases + delete - clealy buggy

do not use async delete - counter-intuitive

fahedouch

lgtm to fix these problems that have been going on for a long time but the real fix should be done on the containerd side

apostasie · 2025-04-02T01:17:29Z

lgtm to fix these problems that have been going on for a long time but the real fix should be done on the containerd side

Purely for the converter.Convert part, proposal is there: containerd/containerd#11628 if you feel like it is right.

I will eventually get to the true bottom of this pit and figure out if there is indeed a bug / race in the deletion mechanism (which I do believe there is), but I need time and motivation...

apostasie · 2025-04-04T21:29:03Z

Suggesting we merge this soon.

I would like to get as much CI watching time with that patch merged-in before it gets into the general audience with the next patch release.

fahedouch · 2025-04-06T08:07:46Z

Suggesting we merge this soon.

I would like to get as much CI watching time with that patch merged-in before it gets into the general audience with the next patch release.

I would have preferred to see @AkihiroSuda 's opinion on this, but he is certainly busy with KubeCon. Let's merge this and roll back later if needed.

fahedouch

I believe that the real fix should be done on the daemon side, but to unblock multiple issues we have on the CLI, let's merge the fix and see what the patch on the containerd side will bring.

AkihiroSuda · 2025-04-06T11:48:58Z

Thanks, LGTM

apostasie changed the title ~~[WIP] Fix content digest not found for tmp reduced platform~~ [DEBUGGING] [WIP] Fix content digest not found for tmp reduced platform Mar 31, 2025

apostasie force-pushed the qa-fixes branch from fed3758 to 2c3c91e Compare March 31, 2025 19:27

apostasie changed the title ~~[DEBUGGING] [WIP] Fix content digest not found for tmp reduced platform~~ [DEBUGGING] Tentatively fix #3513 Mar 31, 2025

apostasie marked this pull request as draft March 31, 2025 19:37

apostasie force-pushed the qa-fixes branch from 2c3c91e to e2671b4 Compare March 31, 2025 19:43

apostasie closed this Mar 31, 2025