diff --git a/pkg/cmd/image/remove.go b/pkg/cmd/image/remove.go index 5e5c9ccd9fd..0482e60a445 100644 --- a/pkg/cmd/image/remove.go +++ b/pkg/cmd/image/remove.go @@ -22,8 +22,11 @@ import ( "fmt" "strings" + "github.com/opencontainers/go-digest" + containerd "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/core/images" + "github.com/containerd/errdefs" "github.com/containerd/log" "github.com/containerd/nerdctl/v2/pkg/api/types" @@ -32,6 +35,15 @@ import ( "github.com/containerd/nerdctl/v2/pkg/platformutil" ) +// danglingImageName returns the synthetic name given to an image that is force-removed while +// still referenced by a container: it must be unique per digest so that force-removing several +// such images in the same invocation does not collide on a shared name in the image store (see +// https://github.com/containerd/nerdctl/issues/4109). Consumers that need to recognize dangling +// images (e.g. pkg/imgutil filtering) match on the leading ":". +func danglingImageName(dgst digest.Digest) string { + return ":" + dgst.String() +} + // Remove removes a list of `images`. func Remove(ctx context.Context, client *containerd.Client, args []string, options types.ImageRemoveOptions) error { var delOpts []images.DeleteOpt @@ -83,11 +95,13 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio if cid, ok := runningImages[found.Image.Name]; ok { if options.Force { - // This is a running image, so, we need to keep a ref on it so that containerd does not GC the layers - // First create the new image with an empty name + // This is a running image, so, we need to keep a ref on it so that containerd does not GC the layers. + // First create the new dangling image, named uniquely per digest so that force-removing + // several running images in a row does not collide on a shared name (see + // https://github.com/containerd/nerdctl/issues/4109). originalName := found.Image.Name - found.Image.Name = ":" - if _, err = is.Create(ctx, found.Image); err != nil { + found.Image.Name = danglingImageName(found.Image.Target.Digest) + if _, err = is.Create(ctx, found.Image); err != nil && !errdefs.IsAlreadyExists(err) { return err } @@ -136,11 +150,13 @@ func Remove(ctx context.Context, client *containerd.Client, args []string, optio if cid, ok := runningImages[found.Image.Name]; ok { if options.Force { - // This is a running image, so, we need to keep a ref on it so that containerd does not GC the layers - // First create the new image with an empty name + // This is a running image, so, we need to keep a ref on it so that containerd does not GC the layers. + // First create the new dangling image, named uniquely per digest so that force-removing + // several running images in a row does not collide on a shared name (see + // https://github.com/containerd/nerdctl/issues/4109). originalName := found.Image.Name - found.Image.Name = ":" - if _, err = is.Create(ctx, found.Image); err != nil { + found.Image.Name = danglingImageName(found.Image.Target.Digest) + if _, err = is.Create(ctx, found.Image); err != nil && !errdefs.IsAlreadyExists(err) { return false, err } diff --git a/pkg/cmd/image/remove_test.go b/pkg/cmd/image/remove_test.go new file mode 100644 index 00000000000..f5743eb90de --- /dev/null +++ b/pkg/cmd/image/remove_test.go @@ -0,0 +1,77 @@ +/* + 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 image + +import ( + "context" + "testing" + + "github.com/opencontainers/go-digest" + "gotest.tools/v3/assert" + + "github.com/containerd/containerd/v2/core/images" + "github.com/containerd/errdefs" +) + +// fakeImageStore is a minimal images.Store that reproduces containerd's real constraint that +// Create fails with errdefs.ErrAlreadyExists when the image name already exists. +type fakeImageStore struct { + images.Store + byName map[string]images.Image +} + +func newFakeImageStore() *fakeImageStore { + return &fakeImageStore{byName: map[string]images.Image{}} +} + +func (f *fakeImageStore) Create(_ context.Context, image images.Image) (images.Image, error) { + if _, ok := f.byName[image.Name]; ok { + return images.Image{}, errdefs.ErrAlreadyExists + } + f.byName[image.Name] = image + return image, nil +} + +// TestDanglingImageNameIsUniquePerDigest reproduces +// https://github.com/containerd/nerdctl/issues/4109: force-removing several images that are each +// in use by a running container used to create every kept-alive dangling ref under the exact same +// literal name (":"), so the second `is.Create` call in the same run failed with +// "image \":\": already exists". Naming the dangling ref after its digest (danglingImageName) +// keeps names unique across images, so both creations succeed. +func TestDanglingImageNameIsUniquePerDigest(t *testing.T) { + store := newFakeImageStore() + ctx := context.Background() + + digestA := digest.FromString("image-a") + digestB := digest.FromString("image-b") + + // Old, buggy behavior: every dangling ref reused the same literal ":" name. + const buggyName = ":" + _, err := store.Create(ctx, images.Image{Name: buggyName}) + assert.NilError(t, err) + _, err = store.Create(ctx, images.Image{Name: buggyName}) + assert.Assert(t, errdefs.IsAlreadyExists(err), "expected the second create with a shared name to collide, got %v", err) + + // Fixed behavior: naming the dangling ref after its digest avoids the collision. + store = newFakeImageStore() + _, err = store.Create(ctx, images.Image{Name: danglingImageName(digestA)}) + assert.NilError(t, err) + _, err = store.Create(ctx, images.Image{Name: danglingImageName(digestB)}) + assert.NilError(t, err, "force-removing a second running image must not collide on the dangling ref name") + + assert.Assert(t, danglingImageName(digestA) != danglingImageName(digestB)) +} diff --git a/pkg/imgutil/filtering.go b/pkg/imgutil/filtering.go index 30764f163c6..eba947c4cbd 100644 --- a/pkg/imgutil/filtering.go +++ b/pkg/imgutil/filtering.go @@ -323,8 +323,9 @@ func matchesAllLabels(imageCfgLabels map[string]string, filterLabels map[string] func matchesReferences(image images.Image, referencePatterns []string) (bool, error) { var matches int - // Containerd returns ":" for dangling untagged images - see https://github.com/containerd/nerdctl/issues/3852 - if image.Name == ":" { + // Dangling untagged images are named ":" or ":" - see + // https://github.com/containerd/nerdctl/issues/3852 and https://github.com/containerd/nerdctl/issues/4109 + if strings.HasPrefix(image.Name, ":") { return false, nil }