Merge pull request #4062 from apostasie/qa-fixes

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

Author: fahedouch

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 {
 						return err
 					}
 					if _, err = imageService.Create(ctx, image); err != nil {

pkg/cmd/image/convert.go

@@ -190,7 +190,7 @@ func Convert(ctx context.Context, client *containerd.Client, srcRawRef, targetRa
 	}
 
 	// converter.Convert() gains the lease by itself
-	newImg, err := converter.Convert(ctx, client, targetRef, srcRef, convertOpts...)
+	newImg, err := converterutil.Convert(ctx, client, targetRef, srcRef, convertOpts...)
 	if err != nil {
 		return err
 	}
@@ -208,7 +208,7 @@ func Convert(ctx context.Context, client *containerd.Client, srcRawRef, targetRa
 			return err
 		}
 		is := client.ImageService()
-		_ = is.Delete(ctx, newI.Name)
+		_ = is.Delete(ctx, newI.Name, images.SynchronousDelete())
 		finimg, err := is.Create(ctx, *newI)
 		if err != nil {
 			return err

pkg/cmd/image/crypt.go

@@ -30,6 +30,7 @@ import (
 	"github.com/containerd/imgcrypt/v2/images/encryption/parsehelpers"
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
+	nerdconverter "github.com/containerd/nerdctl/v2/pkg/imgutil/converter"
 	"github.com/containerd/nerdctl/v2/pkg/platformutil"
 	"github.com/containerd/nerdctl/v2/pkg/referenceutil"
 )
@@ -93,7 +94,7 @@ func Crypt(ctx context.Context, client *containerd.Client, srcRawRef, targetRawR
 	convertOpts = append(convertOpts, converter.WithIndexConvertFunc(convertFunc))
 
 	// converter.Convert() gains the lease by itself
-	newImg, err := converter.Convert(ctx, client, targetRef, srcRef, convertOpts...)
+	newImg, err := nerdconverter.Convert(ctx, client, targetRef, srcRef, convertOpts...)
 	if err != nil {
 		return err
 	}

pkg/cmd/image/push.go

@@ -43,6 +43,7 @@ import (
 
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/errutil"
+	nerdconverter "github.com/containerd/nerdctl/v2/pkg/imgutil/converter"
 	"github.com/containerd/nerdctl/v2/pkg/imgutil/dockerconfigresolver"
 	"github.com/containerd/nerdctl/v2/pkg/imgutil/push"
 	"github.com/containerd/nerdctl/v2/pkg/ipfs"
@@ -119,7 +120,12 @@ func Push(ctx context.Context, client *containerd.Client, rawRef string, options
 		pushRef = ref + "-tmp-reduced-platform"
 		// 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.
-		platImg, err := converter.Convert(ctx, client, pushRef, ref, converter.WithPlatform(platMC))
+		// Ensure all the layers are here: https://github.com/containerd/nerdctl/issues/3425
+		err = EnsureAllContent(ctx, client, ref, platMC, options.GOptions)
+		if err != nil {
+			return err
+		}
+		platImg, err := nerdconverter.Convert(ctx, client, pushRef, ref, converter.WithPlatform(platMC))
 		if err != nil {
 			if len(options.Platforms) == 0 {
 				return fmt.Errorf("failed to create a tmp single-platform image %q: %w", pushRef, err)
@@ -132,7 +138,7 @@ func Push(ctx context.Context, client *containerd.Client, rawRef string, options
 
 	if options.Estargz {
 		pushRef = ref + "-tmp-esgz"
-		esgzImg, err := converter.Convert(ctx, client, pushRef, ref, converter.WithPlatform(platMC), converter.WithLayerConvertFunc(eStargzConvertFunc()))
+		esgzImg, err := nerdconverter.Convert(ctx, client, pushRef, ref, converter.WithPlatform(platMC), converter.WithLayerConvertFunc(eStargzConvertFunc()))
 		if err != nil {
 			return fmt.Errorf("failed to convert to eStargz: %v", err)
 		}

pkg/cmd/image/remove.go

@@ -79,6 +79,8 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio
 
 			if cid, ok := runningImages[found.Image.Name]; ok {
 				if options.Force {
+					// FIXME: this is suspicious, but passing the opt seem to break some tests
+					// if err = is.Delete(ctx, found.Image.Name, delOpts...); err != nil {
 					if err = is.Delete(ctx, found.Image.Name); err != nil {
 						return err
 					}
@@ -126,6 +128,8 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio
 
 			if cid, ok := runningImages[found.Image.Name]; ok {
 				if options.Force {
+					// FIXME: this is suspicious, but passing the opt seem to break some tests
+					// if err = is.Delete(ctx, found.Image.Name, delOpts...); err != nil {
 					if err = is.Delete(ctx, found.Image.Name); err != nil {
 						return false, err
 					}

pkg/cmd/image/tag.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	containerd "github.com/containerd/containerd/v2/client"
+	"github.com/containerd/containerd/v2/core/images"
 	"github.com/containerd/errdefs"
 	"github.com/containerd/log"
 
@@ -81,7 +82,7 @@ func Tag(ctx context.Context, client *containerd.Client, options types.ImageTagO
 	img.Name = parsedReference.String()
 	if _, err = imageService.Create(ctx, img); err != nil {
 		if errdefs.IsAlreadyExists(err) {
-			if err = imageService.Delete(ctx, img.Name); err != nil {
+			if err = imageService.Delete(ctx, img.Name, images.SynchronousDelete()); err != nil {
 				return err
 			}
 			if _, err = imageService.Create(ctx, img); err != nil {

pkg/imgutil/converter/convert.go

@@ -0,0 +1,55 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package converter
+
+import (
+	"context"
+
+	"github.com/containerd/containerd/v2/core/images"
+	"github.com/containerd/containerd/v2/core/images/converter"
+)
+
+// 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.
+// This here is to workaround the bug, by manually creating the image first,
+// then converting it in place (which avoid the problematic code-path).
+// See containerd upstream discussion https://github.com/containerd/containerd/pull/11628 and
+// nerdctl issues:
+// https://github.com/containerd/nerdctl/issues/3509#issuecomment-2398236766
+// https://github.com/containerd/nerdctl/issues/3513
+// Note this should be remove if/when containerd merges in a fix.
+
+func Convert(ctx context.Context, client converter.Client, dstRef, srcRef string, opts ...converter.Opt) (*images.Image, error) {
+	imageService := client.ImageService()
+
+	img, err := imageService.Get(ctx, srcRef)
+	if err != nil {
+		return nil, err
+	}
+
+	img.Name = dstRef
+
+	_ = imageService.Delete(ctx, img.Name, images.SynchronousDelete())
+
+	if _, err = imageService.Create(ctx, img); err != nil {
+		return nil, err
+	}
+
+	return converter.Convert(ctx, client, dstRef, dstRef, opts...)
+}