diff --git a/pkg/compose/down.go b/pkg/compose/down.go index 140c8d6d8..43577b344 100644 --- a/pkg/compose/down.go +++ b/pkg/compose/down.go @@ -195,6 +195,7 @@ func (s *composeService) removeTaggedImagesOp(ctx context.Context, project *type } eg, ctx := errgroup.WithContext(ctx) + eg.SetLimit(s.maxConcurrency) for i := range images { img := images[i] eg.Go(func() error { @@ -214,33 +215,31 @@ func (s *composeService) removeTaggedImagesOp(ctx context.Context, project *type // them). // // It stays silent when there's nothing to remove, mirroring removeNetwork's -// silent skip. A listing failure is reported as a visible error without the -// "Dangling images" label, since at that point we don't know whether it -// would have been a no-op or a real removal. A partial removal failure -// keeps the "Removed" label — some images did get removed — and adds the -// error as a visible warning alongside it rather than replacing it. +// silent skip. A listing failure surfaces under the "Dangling images" label +// as a plain error rather than the normal Removing/Removed progression, +// since at that point we don't know whether it would have been a no-op or +// a real removal. A removal failure keeps the "Removed" label only if at +// least one image actually got removed — with the error visible alongside +// it, not replacing it — otherwise it's reported as a plain error too. func (s *composeService) removeDanglingImagesOp(ctx context.Context, projectName string, keep func(image.Summary) bool) error { eventID := "Dangling images" - images, err := s.danglingImages(ctx, projectName) + eligible, err := s.eligibleDanglingImages(ctx, projectName, keep) if err != nil { s.events.On(errorEvent(eventID, err.Error())) return err } - - var eligible []image.Summary - for _, img := range images { - if !keep(img) { - eligible = append(eligible, img) - } - } if len(eligible) == 0 { return nil } s.events.On(removingEvent(eventID)) - _, err = s.removeImages(ctx, eligible) + removed, err := s.removeImages(ctx, eligible) if err != nil { - s.events.On(newEvent(eventID, api.Warning, "Removed", err.Error())) + if len(removed) == 0 { + s.events.On(errorEvent(eventID, err.Error())) + } else { + s.events.On(newEvent(eventID, api.Warning, "Removed", err.Error())) + } return err } s.events.On(removedEvent(eventID)) diff --git a/pkg/compose/down_test.go b/pkg/compose/down_test.go index af8414fcb..beff4ec66 100644 --- a/pkg/compose/down_test.go +++ b/pkg/compose/down_test.go @@ -740,19 +740,19 @@ func TestEnsureImagesDown_PartialRemovalFailureStaysVisibleAlongsideRemoved(t *t Filters: projectFilter("prj").Add("dangling", "true"), }).Return(client.ImageListResult{Items: []image.Summary{ {ID: "sha256:ok"}, - {ID: "sha256:in-use"}, + {ID: "sha256:fails"}, }}, nil) apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:ok", client.ImageRemoveOptions{}). Return(client.ImageRemoveResult{}, nil) - apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:in-use", client.ImageRemoveOptions{}). - Return(client.ImageRemoveResult{}, errdefs.ErrConflict.WithMessage("image is being used by a container")) + apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:fails", client.ImageRemoveOptions{}). + Return(client.ImageRemoveResult{}, errdefs.ErrPermissionDenied.WithMessage("permission denied")) ops := svc.ensureImagesDown(t.Context(), project, compose.DownOptions{Images: "local", RemoveOrphans: true}) assert.Equal(t, len(ops), 2) // tagged-images op + dangling-images op assert.NilError(t, ops[0]()) // tagged-images op: nothing to prune, trivially succeeds opErr := ops[1]() - assert.ErrorContains(t, opErr, "sha256:in-use") + assert.ErrorContains(t, opErr, "sha256:fails") assert.Equal(t, len(rec.resources), 2) assert.Equal(t, rec.resources[0].ID, "Dangling images") @@ -760,7 +760,38 @@ func TestEnsureImagesDown_PartialRemovalFailureStaysVisibleAlongsideRemoved(t *t assert.Equal(t, rec.resources[1].ID, "Dangling images") assert.Equal(t, rec.resources[1].Text, "Removed") assert.Equal(t, rec.resources[1].Status, compose.Warning) - assert.ErrorContains(t, errors.New(rec.resources[1].Details), "sha256:in-use") + assert.ErrorContains(t, errors.New(rec.resources[1].Details), "sha256:fails") +} + +// TestEnsureImagesDown_TotalRemovalFailureReportsPlainError guards the fix +// suggested in review: when NO eligible dangling image actually gets +// removed (every one hits a real error), the "Removed" label must not be +// shown at all — claiming "Removed" when nothing was would be dishonest — +// a plain error is reported instead. +func TestEnsureImagesDown_TotalRemovalFailureReportsPlainError(t *testing.T) { + apiClient, svc, rec, project := newDanglingImagesFixture(t) + apiClient.EXPECT().ImageList(gomock.Any(), client.ImageListOptions{ + Filters: projectFilter("prj").Add("dangling", "true"), + }).Return(client.ImageListResult{Items: []image.Summary{ + {ID: "sha256:fails"}, + }}, nil) + apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:fails", client.ImageRemoveOptions{}). + Return(client.ImageRemoveResult{}, errdefs.ErrPermissionDenied.WithMessage("permission denied")) + + ops := svc.ensureImagesDown(t.Context(), project, compose.DownOptions{Images: "local", RemoveOrphans: true}) + assert.Equal(t, len(ops), 2) // tagged-images op + dangling-images op + + assert.NilError(t, ops[0]()) // tagged-images op: nothing to prune, trivially succeeds + opErr := ops[1]() + assert.ErrorContains(t, opErr, "sha256:fails") + + assert.Equal(t, len(rec.resources), 2) + assert.Equal(t, rec.resources[0].ID, "Dangling images") + assert.Equal(t, rec.resources[0].Text, "Removing") + assert.Equal(t, rec.resources[1].ID, "Dangling images") + assert.Equal(t, rec.resources[1].Status, compose.Error) + assert.Equal(t, rec.resources[1].Text, compose.StatusError) + assert.ErrorContains(t, errors.New(rec.resources[1].Details), "sha256:fails") } // TestDownRemovesRetainedPreStartHookContainers verifies that compose down finds and diff --git a/pkg/compose/image_pruner.go b/pkg/compose/image_pruner.go index 92fecd63c..c67c91e9f 100644 --- a/pkg/compose/image_pruner.go +++ b/pkg/compose/image_pruner.go @@ -175,8 +175,10 @@ func (s *composeService) danglingImages(ctx context.Context, projectName string) // removeImages removes the given images in parallel, tolerating individual // failures so one bad image doesn't abort the rest: an image already gone -// (a benign race with something else removing it concurrently) is treated -// as a no-op, counted neither as removed nor as failed; any other removal +// (a benign race with something else removing it concurrently) or still in +// use is treated as a no-op, counted neither as removed nor as failed — +// same tolerance removeResource already gives a tagged image still in use +// ("Resource is still in use" warning, not an error); any other removal // error is joined into err so callers reporting failures to the user keep // the actual daemon error instead of just an image ID. func (s *composeService) removeImages(ctx context.Context, images []image.Summary) (removed []string, err error) { @@ -192,6 +194,12 @@ func (s *composeService) removeImages(ctx context.Context, images []image.Summar logrus.Debugf("dangling image %s already removed: %v", img.ID, err) return nil } + if errdefs.IsConflict(err) { + // still in use: the same benign skip the tagged-image + // path reports via removeResource's conflict branch + logrus.Debugf("dangling image %s still in use: %v", img.ID, err) + return nil + } mu.Lock() errs = append(errs, fmt.Errorf("image %s: %w", img.ID, err)) mu.Unlock() @@ -207,10 +215,11 @@ func (s *composeService) removeImages(ctx context.Context, images []image.Summar return removed, errors.Join(errs...) } -// removeDanglingImages lists a project's dangling images and removes those -// not spared by keep. Shared by down --rmi and watch --prune, which differ -// only in what keep spares. -func (s *composeService) removeDanglingImages(ctx context.Context, projectName string, keep func(image.Summary) bool) ([]string, error) { +// eligibleDanglingImages lists a project's dangling images and filters out +// those spared by keep. Shared by down --rmi (which also needs the list +// itself, to decide whether there's anything to report) and watch --prune, +// so the two don't drift apart. +func (s *composeService) eligibleDanglingImages(ctx context.Context, projectName string, keep func(image.Summary) bool) ([]image.Summary, error) { images, err := s.danglingImages(ctx, projectName) if err != nil { return nil, err @@ -222,6 +231,17 @@ func (s *composeService) removeDanglingImages(ctx context.Context, projectName s eligible = append(eligible, img) } } + return eligible, nil +} + +// removeDanglingImages lists a project's dangling images and removes those +// not spared by keep. Shared by down --rmi and watch --prune, which differ +// only in what keep spares. +func (s *composeService) removeDanglingImages(ctx context.Context, projectName string, keep func(image.Summary) bool) ([]string, error) { + eligible, err := s.eligibleDanglingImages(ctx, projectName, keep) + if err != nil { + return nil, err + } return s.removeImages(ctx, eligible) } diff --git a/pkg/compose/image_pruner_test.go b/pkg/compose/image_pruner_test.go index 8f0c7ea55..d84c964a9 100644 --- a/pkg/compose/image_pruner_test.go +++ b/pkg/compose/image_pruner_test.go @@ -29,10 +29,12 @@ import ( // TestRemoveDanglingImages_FiltersKeepsAndToleratesFailures guards the // shared helper used by both `down --rmi` and `watch --prune`: images the // keep predicate spares must never be removed; an image already gone (a -// benign race with something else removing it concurrently) is tolerated -// and not reported as a failure; a genuine removal error doesn't stop the -// others, but is joined into the returned error (preserving the actual -// daemon error, not just the image ID) for callers that need to surface it. +// benign race with something else removing it concurrently) or still in +// use (the same tolerance removeResource already gives a tagged image in +// the same situation) is tolerated and not reported as a failure; a +// genuine removal error doesn't stop the others, but is joined into the +// returned error (preserving the actual daemon error, not just the image +// ID) for callers that need to surface it. func TestRemoveDanglingImages_FiltersKeepsAndToleratesFailures(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -48,21 +50,24 @@ func TestRemoveDanglingImages_FiltersKeepsAndToleratesFailures(t *testing.T) { {ID: "sha256:keep"}, {ID: "sha256:removed"}, {ID: "sha256:already-gone"}, + {ID: "sha256:in-use"}, {ID: "sha256:fails"}, }}, nil) apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:removed", client.ImageRemoveOptions{}). Return(client.ImageRemoveResult{}, nil) apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:already-gone", client.ImageRemoveOptions{}). Return(client.ImageRemoveResult{}, errdefs.ErrNotFound.WithMessage("already removed")) - apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:fails", client.ImageRemoveOptions{}). + apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:in-use", client.ImageRemoveOptions{}). Return(client.ImageRemoveResult{}, errdefs.ErrConflict.WithMessage("image is being used")) + apiClient.EXPECT().ImageRemove(gomock.Any(), "sha256:fails", client.ImageRemoveOptions{}). + Return(client.ImageRemoveResult{}, errdefs.ErrPermissionDenied.WithMessage("permission denied")) // no expectation for "sha256:keep" — a call to ImageRemove for it fails the test keep := func(img image.Summary) bool { return img.ID == "sha256:keep" } removed, err := svc.removeDanglingImages(t.Context(), "prj", keep) assert.DeepEqual(t, removed, []string{"sha256:removed"}) assert.ErrorContains(t, err, "sha256:fails") - assert.ErrorContains(t, err, "image is being used") + assert.ErrorContains(t, err, "permission denied") } // TestRemoveDanglingImages_NoneFound guards that an empty dangling-image