docker/compose
1
0
Fork 0
mirror of https://github.com/docker/compose.git synced 2026-10-12 02:13:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: tolerate a dangling image still in use like every other resource

Browse Source 
A dangling image still in use failed the whole op, while a tagged
image or volume in the same state only gets a swallowed warning via
removeResource — same daemon condition, inconsistent outcome. Dangling
images now get the same tolerance. Also stop claiming "Removed" when
a removal failure means nothing actually was, bound the tagged-image
fan-out the same way the dangling one already is, and share the
listing+filter logic between the down and watch paths instead of
duplicating it.

Signed-off-by: Guillaume Lours <[email protected]>
This commit is contained in:
Guillaume Lours
2026-09-29 16:19:33 +02:00
committed by Nicolas De loof
parent f0b464fc0b
commit 5aac30946a
4 changed files with 87 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/compose/down.go
+14 -15
View File
@@ -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))
pkg/compose/down_test.go
+36 -5
View File
@@ -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
pkg/compose/image_pruner.go
+26 -6
View File
@@ -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)
}
pkg/compose/image_pruner_test.go
+11 -6
View File
@@ -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