libimage: support parallel tag/untag

The c/storage SetNames API is depracated because it is not race free to
first get the list of names and then append our new name then write the
full list back. Instead a better Add/RemovesNames API has been added.

Tag and Untag should use these to prevent race conditions that can be
easily reproduce using podman tag in parallel. Tests have been added to
ensure it is working correctly.

Fixes containers/podman#17515

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Author: Luap99

libimage/image.go

@@ -580,17 +580,13 @@ func (i *Image) Tag(name string) error {
 		defer i.runtime.writeEvent(&Event{ID: i.ID(), Name: name, Time: time.Now(), Type: EventTypeImageTag})
 	}
 
-	newNames := append(i.Names(), ref.String())
-	if err := i.runtime.store.SetNames(i.ID(), newNames); err != nil {
+	if err := i.runtime.store.AddNames(i.ID(), []string{ref.String()}); err != nil {
 		return err
 	}
 
 	return i.reload()
 }
 
-// to have some symmetry with the errors from containers/storage.
-var errTagUnknown = errors.New("tag not known")
-
 // TODO (@vrothberg) - `docker rmi sha256:` will remove the digest from the
 // image.  However, that's something containers storage does not support.
 var errUntagDigest = errors.New("untag by digest not supported")
@@ -625,21 +621,7 @@ func (i *Image) Untag(name string) error {
 		defer i.runtime.writeEvent(&Event{ID: i.ID(), Name: name, Time: time.Now(), Type: EventTypeImageUntag})
 	}
 
-	removedName := false
-	newNames := []string{}
-	for _, n := range i.Names() {
-		if n == name {
-			removedName = true
-			continue
-		}
-		newNames = append(newNames, n)
-	}
-
-	if !removedName {
-		return fmt.Errorf("%s: %w", name, errTagUnknown)
-	}
-
-	if err := i.runtime.store.SetNames(i.ID(), newNames); err != nil {
+	if err := i.runtime.store.RemoveNames(i.ID(), []string{name}); err != nil {
 		return err
 	}
 

libimage/image_test.go

@@ -3,7 +3,9 @@ package libimage
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
+	"sync"
 	"testing"
 
 	"github.com/containers/common/pkg/config"
@@ -261,21 +263,8 @@ func TestInspectHealthcheck(t *testing.T) {
 }
 
 func TestTag(t *testing.T) {
-	// Note: this will resolve pull from the GCR registry (see
-	// testdata/registries.conf).
-	busyboxLatest := "docker.io/library/busybox:latest"
-
-	runtime, cleanup := testNewRuntime(t)
+	runtime, image, cleanup := getImageAndRuntime(t)
 	defer cleanup()
-	ctx := context.Background()
-
-	pullOptions := &PullOptions{}
-	pullOptions.Writer = os.Stdout
-	pulledImages, err := runtime.Pull(ctx, busyboxLatest, config.PullPolicyMissing, pullOptions)
-	require.NoError(t, err)
-	require.Len(t, pulledImages, 1)
-
-	image := pulledImages[0]
 
 	digest := "sha256:adab3844f497ab9171f070d4cae4114b5aec565ac772e2f2579405b78be67c96"
 
@@ -307,26 +296,65 @@ func TestTag(t *testing.T) {
 	}
 
 	// Check for specific error.
-	err = image.Tag("foo@" + digest)
+	err := image.Tag("foo@" + digest)
 	require.True(t, errors.Is(err, errTagDigest), "check for specific digest error")
 }
 
-func TestUntag(t *testing.T) {
-	// Note: this will resolve pull from the GCR registry (see
-	// testdata/registries.conf).
-	busyboxLatest := "docker.io/library/busybox:latest"
-
-	runtime, cleanup := testNewRuntime(t)
+func TestTagAndUntagParallel(t *testing.T) {
+	runtime, image, cleanup := getImageAndRuntime(t)
 	defer cleanup()
-	ctx := context.Background()
 
-	pullOptions := &PullOptions{}
-	pullOptions.Writer = os.Stdout
-	pulledImages, err := runtime.Pull(ctx, busyboxLatest, config.PullPolicyMissing, pullOptions)
-	require.NoError(t, err)
-	require.Len(t, pulledImages, 1)
+	tagCount := 10
+	wg := sync.WaitGroup{}
 
-	image := pulledImages[0]
+	origNames := image.Names()
+
+	names := make([]string, 0, tagCount)
+	names = append(names, origNames...)
+
+	// Test tag in parallel, the extra go routine is critical for the test do not remove that.
+	wg.Add(tagCount)
+	for i := 0; i < tagCount; i++ {
+		name := fmt.Sprintf("localhost/tag-%d:latest", i)
+		names = append(names, name)
+		go func(name string) {
+			defer wg.Done()
+			err := image.Tag(name)
+			require.NoError(t, err, "parallel tag should have succeeded")
+		}(name)
+	}
+
+	// wait for all routines to finish
+	wg.Wait()
+
+	newImg, _, err := runtime.LookupImage(image.ID(), nil)
+	require.NoError(t, err, "image should have resolved locally")
+	// Note use ElementsMatch because teh order is unspecified to the parallel nature
+	require.ElementsMatch(t, names, newImg.Names(), "tag image names should contain same elements")
+
+	// Test untag in parallel
+	wg.Add(tagCount)
+	for i := 0; i < tagCount; i++ {
+		name := fmt.Sprintf("localhost/tag-%d:latest", i)
+		names = append(names, name)
+		go func(name string) {
+			defer wg.Done()
+			err := image.Untag(name)
+			require.NoError(t, err, "parallel untag should have succeeded")
+		}(name)
+	}
+	// wait for all routines to finish
+	wg.Wait()
+
+	newImg, _, err = runtime.LookupImage(image.ID(), nil)
+	require.NoError(t, err, "image should have resolved locally")
+	// Note use ElementsMatch because teh order is unspecified to the parallel nature
+	require.ElementsMatch(t, origNames, newImg.Names(), "untag image names should contain same elements")
+}
+
+func TestUntag(t *testing.T) {
+	runtime, image, cleanup := getImageAndRuntime(t)
+	defer cleanup()
 
 	digest := "sha256:adab3844f497ab9171f070d4cae4114b5aec565ac772e2f2579405b78be67c96"
 
@@ -360,6 +388,25 @@ func TestUntag(t *testing.T) {
 	}
 
 	// Check for specific error.
-	err = image.Untag(digest)
+	err := image.Untag(digest)
 	require.True(t, errors.Is(err, errUntagDigest), "check for specific digest error")
 }
+
+func getImageAndRuntime(t *testing.T) (*Runtime, *Image, func()) {
+	// Note: this will resolve pull from the GCR registry (see
+	// testdata/registries.conf).
+	busyboxLatest := "docker.io/library/busybox:latest"
+
+	runtime, cleanup := testNewRuntime(t)
+	ctx := context.Background()
+
+	pullOptions := &PullOptions{}
+	pullOptions.Writer = os.Stdout
+	pulledImages, err := runtime.Pull(ctx, busyboxLatest, config.PullPolicyMissing, pullOptions)
+	require.NoError(t, err)
+	require.Len(t, pulledImages, 1)
+
+	image := pulledImages[0]
+
+	return runtime, image, cleanup
+}