From c6578d76fb0da6f6a87d62ac33306945dc616205 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 17 Jun 2021 09:45:53 +0200 Subject: [PATCH] libimage: force remove: only untag on multi tag image When removing an image by name, do not remove the image and all its tags, even if force is set. Instead, just untag the specified name. Note: adjust the load test to preserve the order in the untagged field. Also vendor in the latest HEAD in containers/image to fix a bug revealed in Podman CI. Context: containers/podman/issues/10685 Signed-off-by: Valentin Rothberg --- go.mod | 2 +- go.sum | 4 +- libimage/image.go | 16 ++++--- libimage/load_test.go | 2 +- libimage/remove_test.go | 44 +++++++++++++++++ .../image/v5/storage/storage_image.go | 47 +++++++++---------- .../containers/image/v5/version/version.go | 4 +- vendor/modules.txt | 2 +- 8 files changed, 83 insertions(+), 38 deletions(-) create mode 100644 libimage/remove_test.go diff --git a/go.mod b/go.mod index 6a3dab11b..5ad31f398 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.15 require ( github.com/BurntSushi/toml v0.3.1 - github.com/containers/image/v5 v5.13.1 + github.com/containers/image/v5 v5.13.2-0.20210617132750-db0df5e0cf5e github.com/containers/ocicrypt v1.1.1 github.com/containers/storage v1.32.2 github.com/disiqueira/gotree/v3 v3.0.2 diff --git a/go.sum b/go.sum index 7fe809060..5ac1aede3 100644 --- a/go.sum +++ b/go.sum @@ -202,8 +202,8 @@ github.com/containernetworking/cni v0.8.0/go.mod h1:LGwApLUm2FpoOfxTDEeq8T9ipbpZ github.com/containernetworking/cni v0.8.1/go.mod h1:LGwApLUm2FpoOfxTDEeq8T9ipbpZ61X79hmU3w8FmsY= github.com/containernetworking/plugins v0.8.6/go.mod h1:qnw5mN19D8fIwkqW7oHHYDHVlzhJpcY6TQxn/fUyDDM= github.com/containernetworking/plugins v0.9.1/go.mod h1:xP/idU2ldlzN6m4p5LmGiwRDjeJr6FLK6vuiUwoH7P8= -github.com/containers/image/v5 v5.13.1 h1:eUb9YHEEo+akiQQEfCqZbAa9edg9Y6Mm30BG3NU4MbY= -github.com/containers/image/v5 v5.13.1/go.mod h1:GkWursKDlDcUIT7L7vZf70tADvZCk/Ga0wgS0MuF0ag= +github.com/containers/image/v5 v5.13.2-0.20210617132750-db0df5e0cf5e h1:5Q9jlvOsaPAavVxGbb2gBHcYi/SKKXERzPUCpeTP5E4= +github.com/containers/image/v5 v5.13.2-0.20210617132750-db0df5e0cf5e/go.mod h1:GkWursKDlDcUIT7L7vZf70tADvZCk/Ga0wgS0MuF0ag= github.com/containers/libtrust v0.0.0-20190913040956-14b96171aa3b h1:Q8ePgVfHDplZ7U33NwHZkrVELsZP5fYj9pM5WBZB2GE= github.com/containers/libtrust v0.0.0-20190913040956-14b96171aa3b/go.mod h1:9rfv8iPl1ZP7aqh9YA68wnZv2NUDbXdcdPHVz0pFbPY= github.com/containers/ocicrypt v1.0.1/go.mod h1:MeJDzk1RJHv89LjsH0Sp5KTY3ZYkjXO/C+bKAeWFIrc= diff --git a/libimage/image.go b/libimage/image.go index de0b4b2c5..3bcdbabec 100644 --- a/libimage/image.go +++ b/libimage/image.go @@ -329,17 +329,19 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, // an `rmi foo` will not untag "foo" but instead attempt to remove the // entire image. If there's a container using "foo", we should get an // error. - if options.Force || referencedBy == "" || numNames == 1 { + if referencedBy == "" || numNames == 1 { // DO NOTHING, the image will be removed } else { byID := strings.HasPrefix(i.ID(), referencedBy) byDigest := strings.HasPrefix(referencedBy, "sha256:") - if byID && numNames > 1 { - return errors.Errorf("unable to delete image %q by ID with more than one tag (%s): please force removal", i.ID(), i.Names()) - } else if byDigest && numNames > 1 { - // FIXME - Docker will remove the digest but containers storage - // does not support that yet, so our hands are tied. - return errors.Errorf("unable to delete image %q by digest with more than one tag (%s): please force removal", i.ID(), i.Names()) + if !options.Force { + if byID && numNames > 1 { + return errors.Errorf("unable to delete image %q by ID with more than one tag (%s): please force removal", i.ID(), i.Names()) + } else if byDigest && numNames > 1 { + // FIXME - Docker will remove the digest but containers storage + // does not support that yet, so our hands are tied. + return errors.Errorf("unable to delete image %q by digest with more than one tag (%s): please force removal", i.ID(), i.Names()) + } } // Only try to untag if we know it's not an ID or digest. diff --git a/libimage/load_test.go b/libimage/load_test.go index 0a6d95d3d..8464a3655 100644 --- a/libimage/load_test.go +++ b/libimage/load_test.go @@ -55,7 +55,7 @@ func TestLoad(t *testing.T) { } // Now remove the image. - rmReports, rmErrors := runtime.RemoveImages(ctx, loadedImages, &RemoveImagesOptions{Force: true}) + rmReports, rmErrors := runtime.RemoveImages(ctx, ids, &RemoveImagesOptions{Force: true}) require.Len(t, rmErrors, 0) require.Len(t, rmReports, test.numImages) diff --git a/libimage/remove_test.go b/libimage/remove_test.go new file mode 100644 index 000000000..3500dd2ea --- /dev/null +++ b/libimage/remove_test.go @@ -0,0 +1,44 @@ +package libimage + +import ( + "context" + "os" + "testing" + + "github.com/containers/common/pkg/config" + "github.com/stretchr/testify/require" +) + +func TestRemoveImages(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) + defer cleanup() + ctx := context.Background() + + pullOptions := &PullOptions{} + pullOptions.Writer = os.Stdout + pulledImages, err := runtime.Pull(ctx, busyboxLatest, config.PullPolicyAlways, pullOptions) + require.NoError(t, err) + require.Len(t, pulledImages, 1) + + err = pulledImages[0].Tag("foobar") + require.NoError(t, err) + + // containers/podman/issues/10685 - force removal on image with + // multiple tags will only untag but not remove all tags including the + // image. + rmReports, rmErrors := runtime.RemoveImages(ctx, []string{"foobar"}, &RemoveImagesOptions{Force: true}) + require.Nil(t, rmErrors) + require.Len(t, rmReports, 1) + require.Equal(t, pulledImages[0].ID(), rmReports[0].ID) + require.False(t, rmReports[0].Removed) + require.Equal(t, []string{"localhost/foobar:latest"}, rmReports[0].Untagged) + + // The busybox image is still present even if foobar was force removed. + exists, err := runtime.Exists(busyboxLatest) + require.NoError(t, err) + require.True(t, exists) +} diff --git a/vendor/github.com/containers/image/v5/storage/storage_image.go b/vendor/github.com/containers/image/v5/storage/storage_image.go index 84d35f8c9..7072d6860 100644 --- a/vendor/github.com/containers/image/v5/storage/storage_image.go +++ b/vendor/github.com/containers/image/v5/storage/storage_image.go @@ -76,12 +76,12 @@ type storageImageDestination struct { indexToStorageID map[int]*string // All accesses to below data are protected by `lock` which is made // *explicit* in the code. - blobDiffIDs map[digest.Digest]digest.Digest // Mapping from layer blobsums to their corresponding DiffIDs - fileSizes map[digest.Digest]int64 // Mapping from layer blobsums to their sizes - filenames map[digest.Digest]string // Mapping from layer blobsums to names of files we used to hold them - currentIndex int // The index of the layer to be committed (i.e., lower indices have already been committed) - indexToPulledBlob map[int]*types.BlobInfo // Mapping from layer (by index) to pulled down blob - blobAdditionalLayer map[digest.Digest]storage.AdditionalLayer // Mapping from layer blobsums to their corresponding additional layer + blobDiffIDs map[digest.Digest]digest.Digest // Mapping from layer blobsums to their corresponding DiffIDs + fileSizes map[digest.Digest]int64 // Mapping from layer blobsums to their sizes + filenames map[digest.Digest]string // Mapping from layer blobsums to names of files we used to hold them + currentIndex int // The index of the layer to be committed (i.e., lower indices have already been committed) + indexToPulledLayerInfo map[int]*manifest.LayerInfo // Mapping from layer (by index) to pulled down blob + blobAdditionalLayer map[digest.Digest]storage.AdditionalLayer // Mapping from layer blobsums to their corresponding additional layer } type storageImageCloser struct { @@ -392,17 +392,17 @@ func newImageDestination(sys *types.SystemContext, imageRef storageReference) (* return nil, errors.Wrapf(err, "error creating a temporary directory") } image := &storageImageDestination{ - imageRef: imageRef, - directory: directory, - signatureses: make(map[digest.Digest][]byte), - blobDiffIDs: make(map[digest.Digest]digest.Digest), - blobAdditionalLayer: make(map[digest.Digest]storage.AdditionalLayer), - fileSizes: make(map[digest.Digest]int64), - filenames: make(map[digest.Digest]string), - SignatureSizes: []int{}, - SignaturesSizes: make(map[digest.Digest][]int), - indexToStorageID: make(map[int]*string), - indexToPulledBlob: make(map[int]*types.BlobInfo), + imageRef: imageRef, + directory: directory, + signatureses: make(map[digest.Digest][]byte), + blobDiffIDs: make(map[digest.Digest]digest.Digest), + blobAdditionalLayer: make(map[digest.Digest]storage.AdditionalLayer), + fileSizes: make(map[digest.Digest]int64), + filenames: make(map[digest.Digest]string), + SignatureSizes: []int{}, + SignaturesSizes: make(map[digest.Digest][]int), + indexToStorageID: make(map[int]*string), + indexToPulledLayerInfo: make(map[int]*manifest.LayerInfo), } return image, nil } @@ -751,7 +751,10 @@ func (s *storageImageDestination) queueOrCommit(ctx context.Context, blob types. // caller is the "worker" routine comitting layers. All other routines // can continue pulling and queuing in layers. s.lock.Lock() - s.indexToPulledBlob[index] = &blob + s.indexToPulledLayerInfo[index] = &manifest.LayerInfo{ + BlobInfo: blob, + EmptyLayer: emptyLayer, + } // We're still waiting for at least one previous/parent layer to be // committed, so there's nothing to do. @@ -760,14 +763,10 @@ func (s *storageImageDestination) queueOrCommit(ctx context.Context, blob types. return nil } - for info := s.indexToPulledBlob[index]; info != nil; info = s.indexToPulledBlob[index] { + for info := s.indexToPulledLayerInfo[index]; info != nil; info = s.indexToPulledLayerInfo[index] { s.lock.Unlock() - layerInfo := manifest.LayerInfo{ - BlobInfo: *info, - EmptyLayer: emptyLayer, - } // Note: commitLayer locks on-demand. - if err := s.commitLayer(ctx, layerInfo, index); err != nil { + if err := s.commitLayer(ctx, *info, index); err != nil { return err } s.lock.Lock() diff --git a/vendor/github.com/containers/image/v5/version/version.go b/vendor/github.com/containers/image/v5/version/version.go index ce15bb60b..a3e0fdf6b 100644 --- a/vendor/github.com/containers/image/v5/version/version.go +++ b/vendor/github.com/containers/image/v5/version/version.go @@ -8,10 +8,10 @@ const ( // VersionMinor is for functionality in a backwards-compatible manner VersionMinor = 13 // VersionPatch is for backwards-compatible bug fixes - VersionPatch = 1 + VersionPatch = 2 // VersionDev indicates development branch. Releases will be empty string. - VersionDev = "" + VersionDev = "-dev" ) // Version is the specification version that the package types support. diff --git a/vendor/modules.txt b/vendor/modules.txt index 3c4e26b17..24fb7ec75 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -48,7 +48,7 @@ github.com/containerd/cgroups/stats/v1 github.com/containerd/containerd/errdefs github.com/containerd/containerd/log github.com/containerd/containerd/platforms -# github.com/containers/image/v5 v5.13.1 +# github.com/containers/image/v5 v5.13.2-0.20210617132750-db0df5e0cf5e ## explicit github.com/containers/image/v5/copy github.com/containers/image/v5/directory