From 8cb5d39d439519121fe19ca0a6bc27aedf8e7965 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 5 May 2023 13:47:23 -0400 Subject: [PATCH] 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 --- cmd/podman/pods/rm.go | 5 +++ libpod/reset.go | 7 ++- libpod/runtime_ctr.go | 12 ++++- libpod/runtime_img.go | 2 +- libpod/runtime_pod.go | 8 ++-- libpod/runtime_pod_common.go | 72 +++++++++++++++++------------- pkg/api/handlers/libpod/pods.go | 5 ++- pkg/domain/entities/pods.go | 5 ++- pkg/domain/infra/abi/network.go | 2 +- pkg/domain/infra/abi/pods.go | 8 +++- pkg/specgen/generate/pod_create.go | 2 +- 11 files changed, 81 insertions(+), 47 deletions(-) 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) } }