libimage: report all removed images

Fix a bug where not all removed images were actually reported as such. A regression test will be added to Podman. Signed-off-by: Valentin Rothberg <[email protected]>
containers · Jul 20, 2021 · 39e7214 · 39e7214
1 parent b487f0f
commit 39e7214
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 25 deletions.
diff --git a/libimage/image.go b/libimage/image.go
@@ -276,7 +276,7 @@ type RemoveImageReport struct {
 
 // remove removes the image along with all dangling parent images that no other
 // image depends on.  The image must not be set read-only and not be used by
-// containers.
+// containers.  Returns IDs of removed/untagged images in order.
 //
 // If the image is used by containers return storage.ErrImageUsedByContainer.
 // Use force to remove these containers.
@@ -287,7 +287,12 @@ type RemoveImageReport struct {
 //
 // This function is internal.  Users of libimage should always use
 // `(*Runtime).RemoveImages()`.
-func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, referencedBy string, options *RemoveImagesOptions) error {
+func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport, referencedBy string, options *RemoveImagesOptions) ([]string, error) {
+	processedIDs := []string{}
+	return i.removeRecursive(ctx, rmMap, processedIDs, referencedBy, options)
+}
+
+func (i *Image) removeRecursive(ctx context.Context, rmMap map[string]*RemoveImageReport, processedIDs []string, referencedBy string, options *RemoveImagesOptions) ([]string, error) {
 	// If referencedBy is empty, the image is considered to be removed via
 	// `image remove --all` which alters the logic below.
 
@@ -299,7 +304,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 	logrus.Debugf("Removing image %s", i.ID())
 
 	if i.IsReadOnly() {
-		return errors.Errorf("cannot remove read-only image %q", i.ID())
+		return processedIDs, errors.Errorf("cannot remove read-only image %q", i.ID())
 	}
 
 	if i.runtime.eventChannel != nil {
@@ -311,7 +316,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 	if exists {
 		// If the image has already been removed, we're done.
 		if report.Removed {
-			return nil
+			return processedIDs, nil
 		}
 	} else {
 		report = &RemoveImageReport{ID: i.ID()}
@@ -338,7 +343,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 	if options.WithSize {
 		size, err := i.Size()
 		if handleError(err) != nil {
-			return err
+			return processedIDs, err
 		}
 		report.Size = size
 	}
@@ -359,18 +364,18 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 		byDigest := strings.HasPrefix(referencedBy, "sha256:")
 		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())
+				return processedIDs, 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())
+				return processedIDs, 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.
 		if !byID && !byDigest {
 			if err := i.Untag(referencedBy); handleError(err) != nil {
-				return err
+				return processedIDs, err
 			}
 			report.Untagged = append(report.Untagged, referencedBy)
 
@@ -379,14 +384,15 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 		}
 	}
 
+	processedIDs = append(processedIDs, i.ID())
 	if skipRemove {
-		return nil
+		return processedIDs, nil
 	}
 
 	// Perform the actual removal. First, remove containers if needed.
 	if options.Force {
 		if err := i.removeContainers(options.RemoveContainerFunc); err != nil {
-			return err
+			return processedIDs, err
 		}
 	}
 
@@ -412,7 +418,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 	}
 
 	if _, err := i.runtime.store.DeleteImage(i.ID(), true); handleError(err) != nil {
-		return err
+		return processedIDs, err
 	}
 	report.Untagged = append(report.Untagged, i.Names()...)
 
@@ -422,7 +428,7 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 
 	// Check if can remove the parent image.
 	if parent == nil {
-		return nil
+		return processedIDs, nil
 	}
 
 	// Only remove the parent if it's dangling, that is being untagged and
@@ -435,11 +441,11 @@ func (i *Image) remove(ctx context.Context, rmMap map[string]*RemoveImageReport,
 		danglingParent = false
 	}
 	if !danglingParent {
-		return nil
+		return processedIDs, nil
 	}
 
 	// Recurse into removing the parent.
-	return parent.remove(ctx, rmMap, "", options)
+	return parent.removeRecursive(ctx, rmMap, processedIDs, "", options)
 }
 
 // Tag the image with the specified name and store it in the local containers

diff --git a/libimage/runtime.go b/libimage/runtime.go
@@ -587,11 +587,10 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem
 		rmErrors = append(rmErrors, err)
 	}
 
-	orderedIDs := []string{}                // determinism and relative order
 	deleteMap := make(map[string]*deleteMe) // ID -> deleteMe
-
+	toDelete := []string{}
 	// Look up images in the local containers storage and fill out
-	// orderedIDs and the deleteMap.
+	// toDelete and the deleteMap.
 	switch {
 	case len(names) > 0:
 		// Look up the images one-by-one.  That allows for removing
@@ -605,15 +604,12 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem
 			}
 			dm, exists := deleteMap[img.ID()]
 			if !exists {
-				orderedIDs = append(orderedIDs, img.ID())
+				toDelete = append(toDelete, img.ID())
 				dm = &deleteMe{image: img}
 				deleteMap[img.ID()] = dm
 			}
 			dm.referencedBy = append(dm.referencedBy, resolvedName)
 		}
-		if len(orderedIDs) == 0 {
-			return nil, rmErrors
-		}
 
 	default:
 		filteredImages, err := r.ListImages(ctx, nil, &ListImagesOptions{Filters: options.Filters})
@@ -622,14 +618,21 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem
 			return nil, rmErrors
 		}
 		for _, img := range filteredImages {
-			orderedIDs = append(orderedIDs, img.ID())
+			toDelete = append(toDelete, img.ID())
 			deleteMap[img.ID()] = &deleteMe{image: img}
 		}
 	}
 
+	// Return early if there's no image to delete.
+	if len(deleteMap) == 0 {
+		return nil, rmErrors
+	}
+
 	// Now remove the images in the given order.
 	rmMap := make(map[string]*RemoveImageReport)
-	for _, id := range orderedIDs {
+	orderedIDs := []string{}
+	visitedIDs := make(map[string]bool)
+	for _, id := range toDelete {
 		del, exists := deleteMap[id]
 		if !exists {
 			appendError(errors.Errorf("internal error: ID %s not in found in image-deletion map", id))
@@ -639,9 +642,17 @@ func (r *Runtime) RemoveImages(ctx context.Context, names []string, options *Rem
 			del.referencedBy = []string{""}
 		}
 		for _, ref := range del.referencedBy {
-			if err := del.image.remove(ctx, rmMap, ref, options); err != nil {
+			processedIDs, err := del.image.remove(ctx, rmMap, ref, options)
+			if err != nil {
 				appendError(err)
-				continue
+			}
+			// NOTE: make sure to add given ID only once to orderedIDs.
+			for _, id := range processedIDs {
+				if visited := visitedIDs[id]; visited {
+					continue
+				}
+				orderedIDs = append(orderedIDs, id)
+				visitedIDs[id] = true
 			}
 		}
 	}