diff --git a/cmd/podman/pods/rm.go b/cmd/podman/pods/rm.go index 041d411521..69eeae21fd 100644 --- 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 index fb157c4c09..6ed38df1d8 100644 --- 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 index f371fdb39c..4ed7d5a2d8 100644 --- 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 index 189889b092..29d77b61f1 100644 --- 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 index 25e48de14c..de840afeb0 100644 --- 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 index 7709fbc4b1..0ebb2b0cac 100644 --- 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,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 - // 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,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 } diff --git a/pkg/api/handlers/libpod/pods.go b/pkg/api/handlers/libpod/pods.go index 43acebb6cb..c13af63136 100644 --- 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 index 585b0e6ca5..9a39ff32a5 100644 --- 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 index f648a392c0..b51ba764bd 100644 --- 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 index e54e287d7a..65c16afa21 100644 --- 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 index 03f15f1903..d8759a9314 100644 --- 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) } }