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 <[email protected]>
mheon · Jun 1, 2023 · 8cb5d39 · 8cb5d39
1 parent bc1a31c
commit 8cb5d39
Show file tree

Hide file tree

Showing 11 changed files with 81 additions and 47 deletions.
diff --git a/cmd/podman/pods/rm.go b/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

diff --git a/libpod/reset.go b/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)
 		}
 	}

diff --git a/libpod/runtime_ctr.go b/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

diff --git a/libpod/runtime_img.go b/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 {

diff --git a/libpod/runtime_pod.go b/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

diff --git a/libpod/runtime_pod_common.go b/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 {
-	var removalErr error
+func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Container, force bool, timeout *uint, ctrNamedVolumes map[string]*ContainerNamedVolume) (map[string]error, 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
 		}()
-
-		if removalErr == nil {
-			removalErr = err
-		} else {
-			logrus.Errorf("Removing container %s from pod %s: %v", ctr.ID(), p.ID(), err)
+		removedCtrs[ctr.ID()] = err
+		if err != nil {
+			errored = true
 		}
 	}
-	if removalErr != nil {
-		return removalErr
+
+	// So, technically, no containers have been *removed*.
+	// They're still in the DB.
+	// So just return nil for removed containers. Squash all the errors into
+	// 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))
+			}
+		}
+		return nil, fmt.Errorf("no containers were removed due to the following errors: %w", allErrors)
 	}
 
 	// Clear infra container ID before we remove the infra container.
@@ -162,28 +172,30 @@ 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.
 	if err := r.state.RemovePodContainers(p); err != nil {
 		// 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
-		// removePod - especially since any change here eventually has
-		// to map down to one error unless we want to make a breaking
-		// API change.
+		// Finalize the removed containers list
+		for ctr, _ := range ctrsVisited {
+			removedCtrs[ctr] = ctrErrors[ctr]
+		}
+
 		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
+			return removedCtrs, fmt.Errorf("not all containers could be removed from pod %s: %w", p.ID(), define.ErrCtrExists)
 		}
 	}
 
@@ -319,15 +329,15 @@ 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
 	if err := r.state.RemovePod(p); err != nil {
 		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
 }
diff --git a/pkg/api/handlers/libpod/pods.go b/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)
 }
 

diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go
@@ -105,8 +105,9 @@ type PodRmOptions struct {
 }
 
 type PodRmReport struct {
-	Err error
-	Id  string //nolint:revive,stylecheck
+	RemovedCtrs map[string]error
+	Err         error
+	Id          string //nolint:revive,stylecheck
 }
 
 // PddSpec is an abstracted version of PodSpecGen designed to eventually accept options

diff --git a/pkg/domain/infra/abi/network.go b/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) {

diff --git a/pkg/domain/infra/abi/pods.go b/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
 		}
 	}

diff --git a/pkg/specgen/generate/pod_create.go b/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)
 			}
 		}