Pods now return what containers were removed with them

This probably should have been in the API since the beginning,
but it's not too late to start now.

The extra information is returned (both via the REST API, and to
the CLI handler for `podman rm`) but is not yet printed - it
feels like adding it to the output could be a breaking change?

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

cmd/podman/pods/rm.go

@@ -128,6 +128,11 @@ func removePods(namesOrIDs []string, rmOptions entities.PodRmOptions, printIDs b
 			}
 			setExitCode(r.Err)
 			errs = append(errs, r.Err)
+			for ctr, err := range r.RemovedCtrs {
+				if err != nil {
+					errs = append(errs, fmt.Errorf("error removing container %s from pod %s: %w", ctr, r.Id, err))
+				}
+			}
 		}
 	}
 	return errs

libpod/reset.go

@@ -97,10 +97,15 @@ func (r *Runtime) reset(ctx context.Context) error {
 		return err
 	}
 	for _, p := range pods {
-		if err := r.RemovePod(ctx, p, true, true, timeout); err != nil {
+		if ctrs, err := r.RemovePod(ctx, p, true, true, timeout); err != nil {
 			if errors.Is(err, define.ErrNoSuchPod) {
 				continue
 			}
+			for ctr, err := range ctrs {
+				if err != nil {
+					logrus.Errorf("Error removing pod %s container %s: %v", p.ID(), ctr, err)
+				}
+			}
 			logrus.Errorf("Removing Pod %s: %v", p.ID(), err)
 		}
 	}

libpod/runtime_ctr.go

@@ -751,7 +751,11 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo
 				continue
 			}
 			logrus.Infof("Removing pod %s as container %s is its service container", depPod.ID(), c.ID())
-			if err := r.RemovePod(ctx, depPod, true, force, timeout); err != nil {
+			podRemovedCtrs, err := r.RemovePod(ctx, depPod, true, force, timeout)
+			for ctr, err := range podRemovedCtrs {
+				removedCtrs[ctr] = err
+			}
+			if err != nil {
 				removedPods[depPod.ID()] = err
 				retErr = fmt.Errorf("error removing container %s dependency pods: %w", c.ID(), err)
 				return
@@ -769,7 +773,11 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo
 		}
 
 		logrus.Infof("Removing pod %s (dependency of container %s)", pod.ID(), c.ID())
-		if err := r.removePod(ctx, pod, true, force, timeout); err != nil {
+		podRemovedCtrs, err := r.removePod(ctx, pod, true, force, timeout)
+		for ctr, err := range podRemovedCtrs {
+			removedCtrs[ctr] = err
+		}
+		if err != nil {
 			removedPods[pod.ID()] = err
 			retErr = fmt.Errorf("error removing container %s pod: %w", c.ID(), err)
 			return

libpod/runtime_img.go

@@ -42,7 +42,7 @@ func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage
 				if err != nil {
 					return fmt.Errorf("container %s is in pod %s, but pod cannot be retrieved: %w", ctr.ID(), ctr.config.Pod, err)
 				}
-				if err := r.removePod(ctx, pod, true, true, timeout); err != nil {
+				if _, err := r.removePod(ctx, pod, true, true, timeout); err != nil {
 					return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err)
 				}
 			} else {

libpod/runtime_pod.go

@@ -27,16 +27,16 @@ type PodFilter func(*Pod) bool
 // If force is specified with removeCtrs, all containers will be stopped before
 // being removed
 // Otherwise, the pod will not be removed if any containers are running
-func (r *Runtime) RemovePod(ctx context.Context, p *Pod, removeCtrs, force bool, timeout *uint) error {
+func (r *Runtime) RemovePod(ctx context.Context, p *Pod, removeCtrs, force bool, timeout *uint) (map[string]error, error) {
 	if !r.valid {
-		return define.ErrRuntimeStopped
+		return nil, define.ErrRuntimeStopped
 	}
 
 	if !p.valid {
 		if ok, _ := r.state.HasPod(p.ID()); !ok {
 			// Pod probably already removed
 			// Or was never in the runtime to begin with
-			return nil
+			return make(map[string]error), nil
 		}
 	}
 
@@ -180,7 +180,7 @@ func (r *Runtime) PrunePods(ctx context.Context) (map[string]error, error) {
 	}
 	for _, pod := range pods {
 		var timeout *uint
-		err := r.removePod(context.TODO(), pod, true, false, timeout)
+		_, err := r.removePod(context.TODO(), pod, true, false, timeout)
 		response[pod.ID()] = err
 	}
 	return response, nil

libpod/runtime_pod_common.go

@@ -124,8 +124,9 @@ func (r *Runtime) SavePod(pod *Pod) error {
 
 // DO NOT USE THIS FUNCTION DIRECTLY. Use removePod(), below. It will call
 // removeMalformedPod() if necessary.
-func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Container, force bool, timeout *uint, ctrNamedVolumes map[string]*ContainerNamedVolume) error {
+func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Container, force bool, timeout *uint, ctrNamedVolumes map[string]*ContainerNamedVolume) (map[string]error, error) {
-	var removalErr error
+	removedCtrs := make(map[string]error)
+	errored := false
 	for _, ctr := range ctrs {
 		err := func() error {
 			ctrLock := ctr.lock
@@ -145,15 +146,24 @@ func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Contai
 			_, _, err := r.removeContainer(ctx, ctr, force, false, true, true, false, false, timeout)
 			return err
 		}()
+		removedCtrs[ctr.ID()] = err
+		if err != nil {
+			errored = true
+		}
+	}
 
-		if removalErr == nil {
+	// So, technically, no containers have been *removed*.
-			removalErr = err
+	// They're still in the DB.
-		} else {
+	// So just return nil for removed containers. Squash all the errors into
-			logrus.Errorf("Removing container %s from pod %s: %v", ctr.ID(), p.ID(), err)
+	// a multierror so we don't lose them.
+	if errored {
+		var allErrors error
+		for ctr, err := range removedCtrs {
+			if err != nil {
+				allErrors = multierror.Append(allErrors, fmt.Errorf("removing container %s: %w", ctr, err))
 			}
 		}
-	if removalErr != nil {
+		return nil, fmt.Errorf("no containers were removed due to the following errors: %w", allErrors)
-		return removalErr
 	}
 
 	// Clear infra container ID before we remove the infra container.
@@ -162,7 +172,7 @@ func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Contai
 	// later - we end up with a reference to a nonexistent infra container.
 	p.state.InfraContainerID = ""
 	if err := p.save(); err != nil {
-		return err
+		return nil, err
 	}
 
 	// Remove all containers in the pod from the state.
@@ -170,20 +180,22 @@ func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Contai
 		// If this fails, there isn't much more we can do.
 		// The containers in the pod are unusable, but they still exist,
 		// so pod removal will fail.
-		return err
+		return nil, err
 	}
 
-	return nil
+	return removedCtrs, nil
 }
 
-func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool, timeout *uint) error {
+func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool, timeout *uint) (map[string]error, error) {
+	removedCtrs := make(map[string]error)
+
 	if err := p.updatePod(); err != nil {
-		return err
+		return nil, err
 	}
 
 	ctrs, err := r.state.PodContainers(p)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	numCtrs := len(ctrs)
 
@@ -194,7 +206,7 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool,
 		force = true
 	}
 	if !removeCtrs && numCtrs > 0 {
-		return fmt.Errorf("pod %s contains containers and cannot be removed: %w", p.ID(), define.ErrCtrExists)
+		return nil, fmt.Errorf("pod %s contains containers and cannot be removed: %w", p.ID(), define.ErrCtrExists)
 	}
 
 	var removalErr error
@@ -206,14 +218,15 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool,
 		// We have to allow the pod to be removed.
 		// But let's only do it if force is set.
 		if !force {
-			return fmt.Errorf("cannot create container graph for pod %s: %w", p.ID(), err)
+			return nil, fmt.Errorf("cannot create container graph for pod %s: %w", p.ID(), err)
 		}
 
 		removalErr = fmt.Errorf("creating container graph for pod %s failed, fell back to loop removal: %w", p.ID(), err)
 
-		if err := r.removeMalformedPod(ctx, p, ctrs, force, timeout, ctrNamedVolumes); err != nil {
+		removedCtrs, err = r.removeMalformedPod(ctx, p, ctrs, force, timeout, ctrNamedVolumes)
+		if err != nil {
 			logrus.Errorf("Error creating container graph for pod %s: %v. Falling back to loop removal.", p.ID(), err)
-			return err
+			return removedCtrs, err
 		}
 	} else {
 		ctrErrors := make(map[string]error)
@@ -223,16 +236,13 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool,
 			removeNode(ctx, node, p, force, timeout, false, ctrErrors, ctrsVisited, ctrNamedVolumes)
 		}
 
-		// This is gross, but I don't want to change the signature on
+		// Finalize the removed containers list
-		// removePod - especially since any change here eventually has
+		for ctr, _ := range ctrsVisited {
-		// to map down to one error unless we want to make a breaking
+			removedCtrs[ctr] = ctrErrors[ctr]
-		// API change.
-		if len(ctrErrors) > 0 {
-			var allErrs error
-			for id, err := range ctrErrors {
-				allErrs = multierror.Append(allErrs, fmt.Errorf("removing container %s from pod %s: %w", id, p.ID(), err))
 		}
-			return allErrs
+
+		if len(ctrErrors) > 0 {
+			return removedCtrs, fmt.Errorf("not all containers could be removed from pod %s: %w", p.ID(), define.ErrCtrExists)
 		}
 	}
 
@@ -319,7 +329,7 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool,
 	}
 
 	if err := p.maybeRemoveServiceContainer(); err != nil {
-		return err
+		return removedCtrs, err
 	}
 
 	// Remove pod from state
@@ -327,7 +337,7 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool,
 		if removalErr != nil {
 			logrus.Errorf("%v", removalErr)
 		}
-		return err
+		return removedCtrs, err
 	}
 
 	// Mark pod invalid
@@ -343,5 +353,5 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool,
 		}
 	}
 
-	return removalErr
+	return removedCtrs, removalErr
 }

pkg/api/handlers/libpod/pods.go

@@ -246,11 +246,12 @@ func PodDelete(w http.ResponseWriter, r *http.Request) {
 		utils.PodNotFound(w, name, err)
 		return
 	}
-	if err := runtime.RemovePod(r.Context(), pod, true, query.Force, query.Timeout); err != nil {
+	ctrs, err := runtime.RemovePod(r.Context(), pod, true, query.Force, query.Timeout)
+	if err != nil {
 		utils.Error(w, http.StatusInternalServerError, err)
 		return
 	}
-	report := entities.PodRmReport{Id: pod.ID()}
+	report := entities.PodRmReport{Id: pod.ID(), RemovedCtrs: ctrs}
 	utils.WriteResponse(w, http.StatusOK, report)
 }
 

pkg/domain/entities/pods.go

@@ -105,6 +105,7 @@ type PodRmOptions struct {
 }
 
 type PodRmReport struct {
+	RemovedCtrs map[string]error
 	Err         error
 	Id          string //nolint:revive,stylecheck
 }

pkg/domain/infra/abi/network.go

@@ -133,7 +133,7 @@ func (ic *ContainerEngine) NetworkRm(ctx context.Context, namesOrIds []string, o
 					if err != nil {
 						return reports, err
 					}
-					if err := ic.Libpod.RemovePod(ctx, pod, true, true, options.Timeout); err != nil {
+					if _, err := ic.Libpod.RemovePod(ctx, pod, true, true, options.Timeout); err != nil {
 						return reports, err
 					}
 				} else if err := ic.Libpod.RemoveContainer(ctx, c, true, true, options.Timeout); err != nil && !errors.Is(err, define.ErrNoSuchCtr) {

pkg/domain/infra/abi/pods.go

@@ -274,10 +274,11 @@ func (ic *ContainerEngine) PodRm(ctx context.Context, namesOrIds []string, optio
 	reports := make([]*entities.PodRmReport, 0, len(pods))
 	for _, p := range pods {
 		report := entities.PodRmReport{Id: p.ID()}
-		err := ic.Libpod.RemovePod(ctx, p, true, options.Force, options.Timeout)
+		ctrs, err := ic.Libpod.RemovePod(ctx, p, true, options.Force, options.Timeout)
 		if err != nil {
 			report.Err = err
 		}
+		report.RemovedCtrs = ctrs
 		reports = append(reports, &report)
 	}
 	return reports, nil
@@ -376,8 +377,11 @@ func (ic *ContainerEngine) PodClone(ctx context.Context, podClone entities.PodCl
 
 	if podClone.Destroy {
 		var timeout *uint
-		err = ic.Libpod.RemovePod(ctx, p, true, true, timeout)
+		_, err = ic.Libpod.RemovePod(ctx, p, true, true, timeout)
 		if err != nil {
+			// TODO: Possibly should handle case where containers
+			// failed to remove - maybe compact the errors into a
+			// multierror and return that?
 			return &entities.PodCloneReport{Id: pod.ID()}, err
 		}
 	}

pkg/specgen/generate/pod_create.go

@@ -22,7 +22,7 @@ func MakePod(p *entities.PodSpec, rt *libpod.Runtime) (_ *libpod.Pod, finalErr e
 	var createdPod *libpod.Pod
 	defer func() {
 		if finalErr != nil && createdPod != nil {
-			if err := rt.RemovePod(context.Background(), createdPod, true, true, nil); err != nil {
+			if _, err := rt.RemovePod(context.Background(), createdPod, true, true, nil); err != nil {
 				logrus.Errorf("Removing pod: %v", err)
 			}
 		}