From e8d7456278f8136eea03a83795e917e9512d1e2d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 14 Apr 2023 11:36:22 -0400 Subject: [PATCH 01/14] Add an API for removing a container and dependencies This is the initial stage of implementation. The current API functions but does not report the additional containers and pods removed. This is necessary to properly display results to the user after `podman rm --all`. The existing remove-dependencies code has been removed in favor of this more native solution. Signed-off-by: Matthew Heon --- libpod/container_graph.go | 2 +- libpod/pod_api.go | 2 +- libpod/runtime_ctr.go | 146 ++++++++++++++++------------- libpod/runtime_img.go | 2 +- libpod/runtime_pod_common.go | 2 +- libpod/runtime_volume_common.go | 2 +- pkg/domain/infra/abi/containers.go | 39 ++------ 7 files changed, 90 insertions(+), 105 deletions(-) diff --git a/libpod/container_graph.go b/libpod/container_graph.go index d43579e4a3..1f0302f7d8 100644 --- a/libpod/container_graph.go +++ b/libpod/container_graph.go @@ -359,7 +359,7 @@ func removeNode(ctx context.Context, node *containerNode, pod *Pod, force bool, } if !ctrErrored { - if err := node.container.runtime.removeContainer(ctx, node.container, force, false, true, false, timeout); err != nil { + if err := node.container.runtime.removeContainer(ctx, node.container, force, false, true, false, false, false, timeout); err != nil { ctrErrored = true ctrErrors[node.id] = err } diff --git a/libpod/pod_api.go b/libpod/pod_api.go index f8e70cc8f5..83add4843e 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -40,7 +40,7 @@ func (p *Pod) startInitContainers(ctx context.Context) error { icLock := initCon.lock icLock.Lock() var time *uint - if err := p.runtime.removeContainer(ctx, initCon, false, false, true, false, time); err != nil { + if err := p.runtime.removeContainer(ctx, initCon, false, false, true, false, false, false, time); err != nil { icLock.Unlock() return fmt.Errorf("failed to remove once init container %s: %w", initCon.ID(), err) } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 563832f457..029feb06be 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -600,14 +600,23 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai return ctr, nil } -// RemoveContainer removes the given container -// If force is specified, the container will be stopped first -// If removeVolume is specified, named volumes used by the container will -// be removed also if and only if the container is the sole user -// Otherwise, RemoveContainer will return an error if the container is running +// RemoveContainer removes the given container. If force is true, the container +// will be stopped first (otherwise, an error will be returned if the container +// is running). If removeVolume is specified, anonymous named volumes used by the +// container will be removed also (iff the container is the sole user of the +// volumes). Timeout sets the stop timeout for the container if it is running. func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) error { // NOTE: container will be locked down the road. - return r.removeContainer(ctx, c, force, removeVolume, false, false, timeout) + return r.removeContainer(ctx, c, force, removeVolume, false, false, false, false, timeout) +} + +// RemoveContainerAndDependencies removes the given container and all its +// dependencies. This may include pods (if the container or any of its +// dependencies is an infra or service container, the associated pod(s) will also +// be removed). Otherwise, it functions identically to RemoveContainer. +func (r *Runtime) RemoveContainerAndDependencies(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) error { + // NOTE: container will be locked down the road. + return r.removeContainer(ctx, c, force, removeVolume, false, false, true, false, timeout) } // Internal function to remove a container. @@ -617,7 +626,10 @@ func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, // remove will handle that). // ignoreDeps is *DANGEROUS* and should not be used outside of a very specific // context (alternate pod removal code, where graph traversal is not possible). -func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, removeVolume, removePod, ignoreDeps bool, timeout *uint) error { +// removeDeps instructs Podman to remove dependency containers (and possible +// a dependency pod if an infra container is involved). removeDeps conflicts +// with removePod - pods have their own dependency management. +func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, removeVolume, removePod, ignoreDeps, removeDeps, noLockPod bool, timeout *uint) error { if !c.valid { if ok, _ := r.state.HasContainer(c.ID()); !ok { // Container probably already removed @@ -626,6 +638,10 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } } + if removePod && removeDeps { + return fmt.Errorf("cannot remove dependencies while also removing a pod: %w", define.ErrInvalidArg) + } + // We need to refresh container config from the DB, to ensure that any // changes (e.g. a rename) are picked up before we start removing. // Since HasContainer above succeeded, we can safely assume the @@ -664,16 +680,22 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } infraID := pod.state.InfraContainerID - if c.ID() == infraID { + if c.ID() == infraID && !removeDeps { return fmt.Errorf("container %s is the infra container of pod %s and cannot be removed without removing the pod", c.ID(), pod.ID()) } } } // For pod removal, the container is already locked by the caller + locked := false if !removePod { c.lock.Lock() - defer c.lock.Unlock() + defer func() { + if locked { + c.lock.Unlock() + } + }() + locked = true } if !r.valid { @@ -685,17 +707,47 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo return err } + serviceForPod := false if c.IsService() { for _, id := range c.state.Service.Pods { - if _, err := c.runtime.LookupPod(id); err != nil { + depPod, err := c.runtime.LookupPod(id) + if err != nil { if errors.Is(err, define.ErrNoSuchPod) { continue } return err } - return fmt.Errorf("container %s is the service container of pod(s) %s and cannot be removed without removing the pod(s)", c.ID(), strings.Join(c.state.Service.Pods, ",")) + if !removeDeps { + return fmt.Errorf("container %s is the service container of pod(s) %s and cannot be removed without removing the pod(s)", c.ID(), strings.Join(c.state.Service.Pods, ",")) + } + // If we are the service container for the pod we are a + // member of: we need to remove that pod last, since + // this container is part of it. + if pod != nil && pod.ID() == depPod.ID() { + serviceForPod = true + 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 { + return err + } } } + if serviceForPod || c.config.IsInfra { + // We're going to remove the pod we are a part of. + // This will get rid of us as well, so we can just return + // immediately after. + if locked { + locked = false + c.lock.Unlock() + } + + logrus.Infof("Removing pod %s (dependency of container %s)", pod.ID(), c.ID()) + if err := r.removePod(ctx, pod, true, force, timeout); err != nil { + return err + } + return nil + } // If we're not force-removing, we need to check if we're in a good // state to remove. @@ -733,9 +785,21 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if err != nil { return err } - if len(deps) != 0 { - depsStr := strings.Join(deps, ", ") - return fmt.Errorf("container %s has dependent containers which must be removed before it: %s: %w", c.ID(), depsStr, define.ErrCtrExists) + if !removeDeps { + if len(deps) != 0 { + depsStr := strings.Join(deps, ", ") + return fmt.Errorf("container %s has dependent containers which must be removed before it: %s: %w", c.ID(), depsStr, define.ErrCtrExists) + } + } + for _, depCtr := range deps { + dep, err := r.GetContainer(depCtr) + if err != nil { + return err + } + logrus.Infof("Removing container %s (dependency of container %s)", dep.ID(), c.ID()) + if err := r.removeContainer(ctx, dep, force, removeVolume, false, false, true, true, timeout); err != nil { + return err + } } } @@ -896,7 +960,7 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol if err == nil { logrus.Infof("Container %s successfully retrieved from state, attempting normal removal", id) // Assume force = true for the evict case - err = r.removeContainer(ctx, tmpCtr, true, removeVolume, false, false, timeout) + err = r.removeContainer(ctx, tmpCtr, true, removeVolume, false, false, false, false, timeout) if !tmpCtr.valid { // If the container is marked invalid, remove succeeded // in kicking it out of the state - no need to continue. @@ -1010,58 +1074,6 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol return id, cleanupErr } -// RemoveDepend removes all dependencies for a container. -// If the container is an infra container, the entire pod gets removed. -func (r *Runtime) RemoveDepend(ctx context.Context, rmCtr *Container, force bool, removeVolume bool, timeout *uint) ([]*reports.RmReport, error) { - logrus.Debugf("Removing container %s and all dependent containers", rmCtr.ID()) - rmReports := make([]*reports.RmReport, 0) - if rmCtr.IsInfra() { - pod, err := r.GetPod(rmCtr.PodID()) - if err != nil { - return nil, err - } - logrus.Debugf("Removing pod %s: depends on infra container %s", pod.ID(), rmCtr.ID()) - podContainerIDS, err := pod.AllContainersByID() - if err != nil { - return nil, err - } - if err := r.RemovePod(ctx, pod, true, force, timeout); err != nil { - return nil, err - } - for _, cID := range podContainerIDS { - rmReports = append(rmReports, &reports.RmReport{Id: cID}) - } - return rmReports, nil - } - - deps, err := r.state.ContainerInUse(rmCtr) - if err != nil { - if err == define.ErrCtrRemoved { - return rmReports, nil - } - return rmReports, err - } - for _, cid := range deps { - ctr, err := r.state.Container(cid) - if err != nil { - if err == define.ErrNoSuchCtr { - continue - } - return rmReports, err - } - - reports, err := r.RemoveDepend(ctx, ctr, force, removeVolume, timeout) - if err != nil { - return rmReports, err - } - rmReports = append(rmReports, reports...) - } - - report := reports.RmReport{Id: rmCtr.ID()} - report.Err = r.removeContainer(ctx, rmCtr, force, removeVolume, false, false, timeout) - return append(rmReports, &report), nil -} - // GetContainer retrieves a container by its ID func (r *Runtime) GetContainer(id string) (*Container, error) { if !r.valid { diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index cf0291e354..d7495f1631 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -46,7 +46,7 @@ func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } else { - if err := r.removeContainer(ctx, ctr, true, false, false, false, timeout); err != nil { + if err := r.removeContainer(ctx, ctr, true, false, false, false, false, false, timeout); err != nil { return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } diff --git a/libpod/runtime_pod_common.go b/libpod/runtime_pod_common.go index 9ca85d9a74..57b6ebce58 100644 --- a/libpod/runtime_pod_common.go +++ b/libpod/runtime_pod_common.go @@ -142,7 +142,7 @@ func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Contai ctrNamedVolumes[vol.Name] = vol } - return r.removeContainer(ctx, ctr, force, false, true, true, timeout) + return r.removeContainer(ctx, ctr, force, false, true, true, false, false, timeout) }() if removalErr == nil { diff --git a/libpod/runtime_volume_common.go b/libpod/runtime_volume_common.go index ae57b17ef6..e9a51cc40f 100644 --- a/libpod/runtime_volume_common.go +++ b/libpod/runtime_volume_common.go @@ -388,7 +388,7 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo logrus.Debugf("Removing container %s (depends on volume %q)", ctr.ID(), v.Name()) - if err := r.removeContainer(ctx, ctr, force, false, false, false, timeout); err != nil { + if err := r.removeContainer(ctx, ctr, force, false, false, false, false, false, timeout); err != nil { return fmt.Errorf("removing container %s that depends on volume %s: %w", ctr.ID(), v.Name(), err) } } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 1c529f3f43..ce9670065b 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -377,7 +377,12 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st } func (ic *ContainerEngine) removeContainer(ctx context.Context, ctr *libpod.Container, options entities.RmOptions) error { - err := ic.Libpod.RemoveContainer(ctx, ctr, options.Force, options.Volumes, options.Timeout) + var err error + if options.All || options.Depend { + err = ic.Libpod.RemoveContainerAndDependencies(ctx, ctr, options.Force, options.Volumes, options.Timeout) + } else { + err = ic.Libpod.RemoveContainer(ctx, ctr, options.Force, options.Volumes, options.Timeout) + } if err == nil { return nil } @@ -435,36 +440,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, } } - alreadyRemoved := make(map[string]bool) - addReports := func(newReports []*reports.RmReport) { - for i := range newReports { - alreadyRemoved[newReports[i].Id] = true - rmReports = append(rmReports, newReports[i]) - } - } - - // First, remove dependent containers. - if options.All || options.Depend { - for _, ctr := range libpodContainers { - // When `All` is set, remove the infra containers and - // their dependencies first. Otherwise, we'd error out. - // - // TODO: All this logic should probably live in libpod. - if alreadyRemoved[ctr.ID()] || (options.All && !ctr.IsInfra()) { - continue - } - reports, err := ic.Libpod.RemoveDepend(ctx, ctr, options.Force, options.Volumes, options.Timeout) - if err != nil { - return rmReports, err - } - addReports(reports) - } - } - errMap, err := parallelctr.ContainerOp(ctx, libpodContainers, func(c *libpod.Container) error { - if alreadyRemoved[c.ID()] { - return nil - } return ic.removeContainer(ctx, c, options) }) if err != nil { @@ -472,9 +448,6 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, } for ctr, err := range errMap { - if alreadyRemoved[ctr.ID()] { - continue - } report := new(reports.RmReport) report.Id = ctr.ID() report.Err = err From bc1a31ce6df6e5b131f32fc8cb75aac020ceec96 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 14 Apr 2023 14:08:22 -0400 Subject: [PATCH 02/14] Make RemoveContainer return containers and pods removed This allows for accurate reporting of dependency removal, but the work is still incomplete: pods can be removed, but do not report the containers they removed as part of said removal. Will add this in a subsequent commit. Major note: I made ignoring no-such-container errors automatic once it has been determined that a container did exist in the first place. I can't think of any case where this would not be a TOCTOU - IE, no reason not to ignore them. The `--ignore` option to `podman rm` should still retain meaning as it will ignore errors from containers that didn't exist in the first place. Signed-off-by: Matthew Heon --- libpod/container_graph.go | 2 +- libpod/pod_api.go | 2 +- libpod/runtime_ctr.go | 125 ++++++++++++++++++++--------- libpod/runtime_img.go | 2 +- libpod/runtime_pod_common.go | 3 +- libpod/runtime_volume_common.go | 2 +- pkg/domain/infra/abi/containers.go | 64 +++++++++------ 7 files changed, 134 insertions(+), 66 deletions(-) diff --git a/libpod/container_graph.go b/libpod/container_graph.go index 1f0302f7d8..d12dc656a4 100644 --- a/libpod/container_graph.go +++ b/libpod/container_graph.go @@ -359,7 +359,7 @@ func removeNode(ctx context.Context, node *containerNode, pod *Pod, force bool, } if !ctrErrored { - if err := node.container.runtime.removeContainer(ctx, node.container, force, false, true, false, false, false, timeout); err != nil { + if _, _, err := node.container.runtime.removeContainer(ctx, node.container, force, false, true, false, false, false, timeout); err != nil { ctrErrored = true ctrErrors[node.id] = err } diff --git a/libpod/pod_api.go b/libpod/pod_api.go index 83add4843e..d2bd7d2c07 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -40,7 +40,7 @@ func (p *Pod) startInitContainers(ctx context.Context) error { icLock := initCon.lock icLock.Lock() var time *uint - if err := p.runtime.removeContainer(ctx, initCon, false, false, true, false, false, false, time); err != nil { + if _, _, err := p.runtime.removeContainer(ctx, initCon, false, false, true, false, false, false, time); err != nil { icLock.Unlock() return fmt.Errorf("failed to remove once init container %s: %w", initCon.ID(), err) } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 029feb06be..f371fdb39c 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -606,16 +606,22 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai // container will be removed also (iff the container is the sole user of the // volumes). Timeout sets the stop timeout for the container if it is running. func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) error { - // NOTE: container will be locked down the road. - return r.removeContainer(ctx, c, force, removeVolume, false, false, false, false, timeout) + // NOTE: container will be locked down the road. There is no unlocked + // version of removeContainer. + _, _, err := r.removeContainer(ctx, c, force, removeVolume, false, false, false, false, timeout) + return err } // RemoveContainerAndDependencies removes the given container and all its // dependencies. This may include pods (if the container or any of its // dependencies is an infra or service container, the associated pod(s) will also // be removed). Otherwise, it functions identically to RemoveContainer. -func (r *Runtime) RemoveContainerAndDependencies(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) error { - // NOTE: container will be locked down the road. +// Returns two arrays: containers removed, and pods removed. These arrays are +// always returned, even if error is set, and indicate any containers that were +// successfully removed prior to the error. +func (r *Runtime) RemoveContainerAndDependencies(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) (map[string]error, map[string]error, error) { + // NOTE: container will be locked down the road. There is no unlocked + // version of removeContainer. return r.removeContainer(ctx, c, force, removeVolume, false, false, true, false, timeout) } @@ -629,17 +635,25 @@ func (r *Runtime) RemoveContainerAndDependencies(ctx context.Context, c *Contain // removeDeps instructs Podman to remove dependency containers (and possible // a dependency pod if an infra container is involved). removeDeps conflicts // with removePod - pods have their own dependency management. -func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, removeVolume, removePod, ignoreDeps, removeDeps, noLockPod bool, timeout *uint) error { +// noLockPod is used for recursive removeContainer calls when the pod is already +// locked. +// TODO: At this point we should just start accepting an options struct +func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, removeVolume, removePod, ignoreDeps, removeDeps, noLockPod bool, timeout *uint) (removedCtrs map[string]error, removedPods map[string]error, retErr error) { + removedCtrs = make(map[string]error) + removedPods = make(map[string]error) + if !c.valid { if ok, _ := r.state.HasContainer(c.ID()); !ok { // Container probably already removed // Or was never in the runtime to begin with - return nil + removedCtrs[c.ID()] = nil + return } } if removePod && removeDeps { - return fmt.Errorf("cannot remove dependencies while also removing a pod: %w", define.ErrInvalidArg) + retErr = fmt.Errorf("cannot remove dependencies while also removing a pod: %w", define.ErrInvalidArg) + return } // We need to refresh container config from the DB, to ensure that any @@ -650,7 +664,8 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // exist once we're done. newConf, err := r.state.GetContainerConfig(c.ID()) if err != nil { - return fmt.Errorf("retrieving container %s configuration from DB to remove: %w", c.ID(), err) + retErr = fmt.Errorf("retrieving container %s configuration from DB to remove: %w", c.ID(), err) + return } c.config = newConf @@ -665,23 +680,27 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if c.config.Pod != "" { pod, err = r.state.Pod(c.config.Pod) if err != nil { - return fmt.Errorf("container %s is in pod %s, but pod cannot be retrieved: %w", c.ID(), pod.ID(), err) + retErr = err + return } if !removePod { // Lock the pod while we're removing container if pod.config.LockID == c.config.LockID { - return fmt.Errorf("container %s and pod %s share lock ID %d: %w", c.ID(), pod.ID(), c.config.LockID, define.ErrWillDeadlock) + retErr = fmt.Errorf("container %s and pod %s share lock ID %d: %w", c.ID(), pod.ID(), c.config.LockID, define.ErrWillDeadlock) + return } pod.lock.Lock() defer pod.lock.Unlock() if err := pod.updatePod(); err != nil { - return err + retErr = err + return } infraID := pod.state.InfraContainerID if c.ID() == infraID && !removeDeps { - return fmt.Errorf("container %s is the infra container of pod %s and cannot be removed without removing the pod", c.ID(), pod.ID()) + retErr = fmt.Errorf("container %s is the infra container of pod %s and cannot be removed without removing the pod", c.ID(), pod.ID()) + return } } } @@ -699,12 +718,14 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } if !r.valid { - return define.ErrRuntimeStopped + retErr = define.ErrRuntimeStopped + return } // Update the container to get current state if err := c.syncContainer(); err != nil { - return err + retErr = err + return } serviceForPod := false @@ -715,10 +736,12 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if errors.Is(err, define.ErrNoSuchPod) { continue } - return err + retErr = err + return } if !removeDeps { - return fmt.Errorf("container %s is the service container of pod(s) %s and cannot be removed without removing the pod(s)", c.ID(), strings.Join(c.state.Service.Pods, ",")) + retErr = fmt.Errorf("container %s is the service container of pod(s) %s and cannot be removed without removing the pod(s)", c.ID(), strings.Join(c.state.Service.Pods, ",")) + return } // If we are the service container for the pod we are a // member of: we need to remove that pod last, since @@ -729,8 +752,11 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } 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 { - return err + removedPods[depPod.ID()] = err + retErr = fmt.Errorf("error removing container %s dependency pods: %w", c.ID(), err) + return } + removedPods[depPod.ID()] = nil } } if serviceForPod || c.config.IsInfra { @@ -744,36 +770,44 @@ 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 { - return err + removedPods[pod.ID()] = err + retErr = fmt.Errorf("error removing container %s pod: %w", c.ID(), err) + return } - return nil + removedPods[pod.ID()] = nil + return } // If we're not force-removing, we need to check if we're in a good // state to remove. if !force { if err := c.checkReadyForRemoval(); err != nil { - return err + retErr = err + return } } if c.state.State == define.ContainerStatePaused { isV2, err := cgroups.IsCgroup2UnifiedMode() if err != nil { - return err + retErr = err + return } // cgroups v1 and v2 handle signals on paused processes differently if !isV2 { if err := c.unpause(); err != nil { - return err + retErr = err + return } } if err := c.ociRuntime.KillContainer(c, 9, false); err != nil { - return err + retErr = err + return } // Need to update container state to make sure we know it's stopped if err := c.waitForExitFileAndSync(); err != nil { - return err + retErr = err + return } } @@ -783,22 +817,33 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if !ignoreDeps { deps, err := r.state.ContainerInUse(c) if err != nil { - return err + retErr = err + return } if !removeDeps { if len(deps) != 0 { depsStr := strings.Join(deps, ", ") - return fmt.Errorf("container %s has dependent containers which must be removed before it: %s: %w", c.ID(), depsStr, define.ErrCtrExists) + retErr = fmt.Errorf("container %s has dependent containers which must be removed before it: %s: %w", c.ID(), depsStr, define.ErrCtrExists) + return } } for _, depCtr := range deps { dep, err := r.GetContainer(depCtr) if err != nil { - return err + retErr = err + return } logrus.Infof("Removing container %s (dependency of container %s)", dep.ID(), c.ID()) - if err := r.removeContainer(ctx, dep, force, removeVolume, false, false, true, true, timeout); err != nil { - return err + ctrs, pods, err := r.removeContainer(ctx, dep, force, removeVolume, false, false, true, true, timeout) + for rmCtr, err := range ctrs { + removedCtrs[rmCtr] = err + } + for rmPod, err := range pods { + removedPods[rmPod] = err + } + if err != nil { + retErr = err + return } } } @@ -812,7 +857,8 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // Ignore ErrConmonDead - we couldn't retrieve the container's // exit code properly, but it's still stopped. if err := c.stop(time); err != nil && !errors.Is(err, define.ErrConmonDead) { - return fmt.Errorf("cannot remove container %s as it could not be stopped: %w", c.ID(), err) + retErr = fmt.Errorf("cannot remove container %s as it could not be stopped: %w", c.ID(), err) + return } // We unlocked as part of stop() above - there's a chance someone @@ -823,17 +869,19 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if ok, _ := r.state.HasContainer(c.ID()); !ok { // When the container has already been removed, the OCI runtime directory remain. if err := c.cleanupRuntime(ctx); err != nil { - return fmt.Errorf("cleaning up container %s from OCI runtime: %w", c.ID(), err) + retErr = fmt.Errorf("cleaning up container %s from OCI runtime: %w", c.ID(), err) + return } - return nil + // Do not add to removed containers, someone else + // removed it. + return } } - var cleanupErr error reportErrorf := func(msg string, args ...any) { err := fmt.Errorf(msg, args...) // Always use fmt.Errorf instead of just logrus.Errorf(…) because the format string probably contains %w - if cleanupErr == nil { - cleanupErr = err + if retErr == nil { + retErr = err } else { logrus.Errorf("%s", err.Error()) } @@ -887,6 +935,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo reportErrorf("removing container %s from database: %w", c.ID(), err) } } + removedCtrs[c.ID()] = nil // Deallocate the container's lock if err := c.lock.Free(); err != nil { @@ -899,7 +948,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo c.newContainerEvent(events.Remove) if !removeVolume { - return cleanupErr + return } for _, v := range c.config.NamedVolumes { @@ -921,7 +970,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } } - return cleanupErr + return } // EvictContainer removes the given container partial or full ID or name, and @@ -960,7 +1009,7 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol if err == nil { logrus.Infof("Container %s successfully retrieved from state, attempting normal removal", id) // Assume force = true for the evict case - err = r.removeContainer(ctx, tmpCtr, true, removeVolume, false, false, false, false, timeout) + _, _, err = r.removeContainer(ctx, tmpCtr, true, removeVolume, false, false, false, false, timeout) if !tmpCtr.valid { // If the container is marked invalid, remove succeeded // in kicking it out of the state - no need to continue. diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index d7495f1631..189889b092 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -46,7 +46,7 @@ func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } else { - if err := r.removeContainer(ctx, ctr, true, false, false, false, false, false, timeout); err != nil { + if _, _, err := r.removeContainer(ctx, ctr, true, false, false, false, false, false, timeout); err != nil { return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } diff --git a/libpod/runtime_pod_common.go b/libpod/runtime_pod_common.go index 57b6ebce58..7709fbc4b1 100644 --- a/libpod/runtime_pod_common.go +++ b/libpod/runtime_pod_common.go @@ -142,7 +142,8 @@ func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Contai ctrNamedVolumes[vol.Name] = vol } - return r.removeContainer(ctx, ctr, force, false, true, true, false, false, timeout) + _, _, err := r.removeContainer(ctx, ctr, force, false, true, true, false, false, timeout) + return err }() if removalErr == nil { diff --git a/libpod/runtime_volume_common.go b/libpod/runtime_volume_common.go index e9a51cc40f..1d22ac8c2a 100644 --- a/libpod/runtime_volume_common.go +++ b/libpod/runtime_volume_common.go @@ -388,7 +388,7 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo logrus.Debugf("Removing container %s (depends on volume %q)", ctr.ID(), v.Name()) - if err := r.removeContainer(ctx, ctr, force, false, false, false, false, false, timeout); err != nil { + if _, _, err := r.removeContainer(ctx, ctr, force, false, false, false, false, false, timeout); err != nil { return fmt.Errorf("removing container %s that depends on volume %s: %w", ctr.ID(), v.Name(), err) } } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index ce9670065b..9a09567f19 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -376,32 +376,24 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st return reports, nil } -func (ic *ContainerEngine) removeContainer(ctx context.Context, ctr *libpod.Container, options entities.RmOptions) error { +func (ic *ContainerEngine) removeContainer(ctx context.Context, ctr *libpod.Container, options entities.RmOptions) (map[string]error, map[string]error, error) { var err error + ctrs := make(map[string]error) + pods := make(map[string]error) if options.All || options.Depend { - err = ic.Libpod.RemoveContainerAndDependencies(ctx, ctr, options.Force, options.Volumes, options.Timeout) + ctrs, pods, err = ic.Libpod.RemoveContainerAndDependencies(ctx, ctr, options.Force, options.Volumes, options.Timeout) } else { err = ic.Libpod.RemoveContainer(ctx, ctr, options.Force, options.Volumes, options.Timeout) } + ctrs[ctr.ID()] = err if err == nil { - return nil + return ctrs, pods, nil } logrus.Debugf("Failed to remove container %s: %s", ctr.ID(), err.Error()) - if errors.Is(err, define.ErrNoSuchCtr) { - // Ignore if the container does not exist (anymore) when either - // it has been requested by the user of if the container is a - // service one. Service containers are removed along with its - // pods which in turn are removed along with their infra - // container. Hence, there is an inherent race when removing - // infra containers with service containers in parallel. - if options.Ignore || ctr.IsService() { - logrus.Debugf("Ignoring error (--allow-missing): %v", err) - return nil - } - } else if errors.Is(err, define.ErrCtrRemoved) { - return nil + if errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved) { + return ctrs, pods, nil } - return err + return ctrs, pods, err } func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, options entities.RmOptions) ([]*reports.RmReport, error) { @@ -440,18 +432,44 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, } } + ctrsMap := make(map[string]error) + mapMutex := sync.Mutex{} + errMap, err := parallelctr.ContainerOp(ctx, libpodContainers, func(c *libpod.Container) error { - return ic.removeContainer(ctx, c, options) + mapMutex.Lock() + if _, ok := ctrsMap[c.ID()]; ok { + return nil + } + mapMutex.Unlock() + + ctrs, _, err := ic.removeContainer(ctx, c, options) + + mapMutex.Lock() + defer mapMutex.Unlock() + for ctr, err := range ctrs { + ctrsMap[ctr] = err + } + + return err }) if err != nil { return nil, err } for ctr, err := range errMap { + if _, ok := ctrsMap[ctr.ID()]; ok { + logrus.Debugf("Multiple results for container %s - attempted multiple removals?", ctr) + } + ctrsMap[ctr.ID()] = err + } + + for ctr, err := range ctrsMap { report := new(reports.RmReport) - report.Id = ctr.ID() - report.Err = err - report.RawInput = idToRawInput[ctr.ID()] + report.Id = ctr + if !(errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved)) { + report.Err = err + } + report.RawInput = idToRawInput[ctr] rmReports = append(rmReports, report) } @@ -945,7 +963,7 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri ExitCode: exitCode, }) if ctr.AutoRemove() { - if err := ic.removeContainer(ctx, ctr.Container, entities.RmOptions{}); err != nil { + if _, _, err := ic.removeContainer(ctx, ctr.Container, entities.RmOptions{}); err != nil { logrus.Errorf("Removing container %s: %v", ctr.ID(), err) } } @@ -981,7 +999,7 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri report.Err = fmt.Errorf("unable to start container %q: %w", ctr.ID(), err) reports = append(reports, report) if ctr.AutoRemove() { - if err := ic.removeContainer(ctx, ctr.Container, entities.RmOptions{}); err != nil { + if _, _, err := ic.removeContainer(ctx, ctr.Container, entities.RmOptions{}); err != nil { logrus.Errorf("Removing container %s: %v", ctr.ID(), err) } } From 8cb5d39d439519121fe19ca0a6bc27aedf8e7965 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 5 May 2023 13:47:23 -0400 Subject: [PATCH 03/14] 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) } } From ef1a22cdea4a24c2750d5d416098eb5a446bd721 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 5 May 2023 13:57:44 -0400 Subject: [PATCH 04/14] Fix a deadlock when removing pods The infra container would try to remove the pod, despite the pod already being in the process of being removed - oops. Add a check to ensure we don't try and remove the pod when called by the `podman pod rm` command. Also, wire up noLockPod - it wasn't previously wired in, which is concerning, and could be related? Finally, make a few minor fixes to un-break lint. Signed-off-by: Matthew Heon --- libpod/runtime_ctr.go | 12 ++++++++---- libpod/runtime_pod_common.go | 2 +- pkg/domain/infra/abi/containers.go | 4 +++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 4ed7d5a2d8..c6856cd34c 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path" "path/filepath" @@ -690,8 +691,10 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo retErr = fmt.Errorf("container %s and pod %s share lock ID %d: %w", c.ID(), pod.ID(), c.config.LockID, define.ErrWillDeadlock) return } - pod.lock.Lock() - defer pod.lock.Unlock() + if !noLockPod { + pod.lock.Lock() + defer pod.lock.Unlock() + } if err := pod.updatePod(); err != nil { retErr = err return @@ -763,7 +766,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo removedPods[depPod.ID()] = nil } } - if serviceForPod || c.config.IsInfra { + if (serviceForPod || c.config.IsInfra) && !removePod { // We're going to remove the pod we are a part of. // This will get rid of us as well, so we can just return // immediately after. @@ -946,7 +949,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo removedCtrs[c.ID()] = nil // Deallocate the container's lock - if err := c.lock.Free(); err != nil { + if err := c.lock.Free(); err != nil && !errors.Is(err, fs.ErrNotExist) { reportErrorf("freeing lock for container %s: %w", c.ID(), err) } @@ -978,6 +981,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } } + //nolint:nakedret return } diff --git a/libpod/runtime_pod_common.go b/libpod/runtime_pod_common.go index 0ebb2b0cac..b22acd586e 100644 --- a/libpod/runtime_pod_common.go +++ b/libpod/runtime_pod_common.go @@ -237,7 +237,7 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool, } // Finalize the removed containers list - for ctr, _ := range ctrsVisited { + for ctr := range ctrsVisited { removedCtrs[ctr] = ctrErrors[ctr] } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 9a09567f19..46bf1d0b61 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -376,6 +376,7 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st return reports, nil } +//nolint:unparam func (ic *ContainerEngine) removeContainer(ctx context.Context, ctr *libpod.Container, options entities.RmOptions) (map[string]error, map[string]error, error) { var err error ctrs := make(map[string]error) @@ -442,6 +443,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, } mapMutex.Unlock() + // TODO: We should report removed pods back somehow. ctrs, _, err := ic.removeContainer(ctx, c, options) mapMutex.Lock() @@ -458,7 +460,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, for ctr, err := range errMap { if _, ok := ctrsMap[ctr.ID()]; ok { - logrus.Debugf("Multiple results for container %s - attempted multiple removals?", ctr) + logrus.Debugf("Multiple results for container %s - attempted multiple removals?", ctr.ID()) } ctrsMap[ctr.ID()] = err } From bafb3d6cc5dc863f0dbd9194c5b1f0be859c0b04 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 5 May 2023 14:00:12 -0400 Subject: [PATCH 05/14] Revert "ginkgo-v2 cleanup workaround for #18180" This reverts commit c4b9f4b34e75da3df5b40fd8e8d42dde224cbd1c. This was a temporary workaround until a fix for #18180 landed. Signed-off-by: Matthew Heon --- test/e2e/common_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index d48845fc99..89289bdec1 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -652,12 +652,7 @@ func (p *PodmanTestIntegration) Cleanup() { // An error would cause it to stop and return early otherwise. Expect(stop).To(Exit(0), "command: %v\nstdout: %s\nstderr: %s", stop.Command.Args, stop.OutputToString(), stop.ErrorToString()) Expect(podrm).To(Exit(0), "command: %v\nstdout: %s\nstderr: %s", podrm.Command.Args, podrm.OutputToString(), podrm.ErrorToString()) - - // FIXME: Remove this special case when the issue is fixed. - // Special case rm -fa is not working correctly with dependencies, https://github.com/containers/podman/issues/18180 - if !strings.Contains(rmall.ErrorToString(), "has dependent containers which must be removed before it") { - Expect(rmall).To(Exit(0), "command: %v\nstdout: %s\nstderr: %s", rmall.Command.Args, rmall.OutputToString(), rmall.ErrorToString()) - } + Expect(rmall).To(Exit(0), "command: %v\nstdout: %s\nstderr: %s", rmall.Command.Args, rmall.OutputToString(), rmall.ErrorToString()) } // CleanupVolume cleans up the volumes and containers. From b75ff3a8faf1e372a504eae613669121c824e59a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 8 May 2023 14:04:44 -0400 Subject: [PATCH 06/14] Add a test for removing dependencies with rm -fa Signed-off-by: Matthew Heon --- test/e2e/rm_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/e2e/rm_test.go b/test/e2e/rm_test.go index 554c458714..77c669d918 100644 --- a/test/e2e/rm_test.go +++ b/test/e2e/rm_test.go @@ -322,4 +322,25 @@ var _ = Describe("Podman rm", func() { Expect(session1).Should(Exit(0)) Expect(session1.OutputToString()).To(BeEquivalentTo(cid4)) }) + + It("podman rm -fa with dependencies", func() { + ctr1Name := "ctr1" + ctr1 := podmanTest.RunTopContainer(ctr1Name) + ctr1.WaitWithDefaultTimeout() + Expect(ctr1).Should(Exit(0)) + cid1 := ctr1.OutputToString() + + ctr2 := podmanTest.Podman([]string{"run", "-d", "--network", fmt.Sprintf("container:%s", ctr1Name), ALPINE, "top"}) + ctr2.WaitWithDefaultTimeout() + Expect(ctr2).Should(Exit(0)) + cid2 := ctr2.OutputToString() + + rm := podmanTest.Podman([]string{"rm", "-fa"}) + rm.WaitWithDefaultTimeout() + Expect(rm).Should(Exit(0)) + Expect(rm.ErrorToString()).To(BeEmpty(), "rm -fa error logged") + Expect(rm.OutputToStringArray()).Should(ConsistOf(cid1, cid2)) + + Expect(podmanTest.NumberOfContainers()).To(Equal(0)) + }) }) From 4e6efbbbb3f60dcaa17cc283388628c2159dae9a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 9 May 2023 13:12:37 -0400 Subject: [PATCH 07/14] Revert "test/e2e: fix "podman run ipcns ipcmk container test"" This reverts commit 9bd833bcfd07bf2b3c258a617d88255327b91669. With the fix for `podman rm -fa` merged, we no longer require this patch. Signed-off-by: Matthew Heon --- test/e2e/run_ns_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/test/e2e/run_ns_test.go b/test/e2e/run_ns_test.go index 53c3645f4e..dfb56f45bd 100644 --- a/test/e2e/run_ns_test.go +++ b/test/e2e/run_ns_test.go @@ -63,7 +63,7 @@ var _ = Describe("Podman run ns", func() { }) It("podman run ipcns ipcmk container test", func() { - setup := podmanTest.Podman([]string{"run", "-d", "--name", "test1", fedoraMinimal, "sleep", "30"}) + setup := podmanTest.Podman([]string{"run", "-d", "--name", "test1", fedoraMinimal, "sleep", "999"}) setup.WaitWithDefaultTimeout() Expect(setup).Should(Exit(0)) @@ -72,12 +72,7 @@ var _ = Describe("Podman run ns", func() { Expect(session).Should(Exit(0)) output := strings.Split(session.OutputToString(), " ") ipc := output[len(output)-1] - session = podmanTest.Podman([]string{"run", "--name=t2", "--ipc=container:test1", fedoraMinimal, "ipcs", "-m", "-i", ipc}) - session.WaitWithDefaultTimeout() - Expect(session).Should(Exit(0)) - - // We have to remove the dependency container first, rm --all fails in the cleanup because of the unknown ordering. - session = podmanTest.Podman([]string{"rm", "t2"}) + session = podmanTest.Podman([]string{"run", "--ipc=container:test1", fedoraMinimal, "ipcs", "-m", "-i", ipc}) session.WaitWithDefaultTimeout() Expect(session).Should(Exit(0)) }) From 2c9f18182ab65c8b6609d9d7f88c489ca93a9e1b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 9 May 2023 13:52:28 -0400 Subject: [PATCH 08/14] The removeContainer function now accepts a struct We had something like 6 different boolean options (removing a container turns out to be rather complicated, because there are a million-odd things that want to do it), and the function signature was getting unreasonably large. Change to a struct to clean things up. Signed-off-by: Matthew Heon --- libpod/container_graph.go | 8 ++- libpod/pod_api.go | 7 ++- libpod/runtime_ctr.go | 91 +++++++++++++++++++++++++-------- libpod/runtime_img.go | 6 ++- libpod/runtime_pod_common.go | 8 ++- libpod/runtime_volume_common.go | 6 ++- 6 files changed, 100 insertions(+), 26 deletions(-) diff --git a/libpod/container_graph.go b/libpod/container_graph.go index d12dc656a4..b508c66b86 100644 --- a/libpod/container_graph.go +++ b/libpod/container_graph.go @@ -359,7 +359,13 @@ func removeNode(ctx context.Context, node *containerNode, pod *Pod, force bool, } if !ctrErrored { - if _, _, err := node.container.runtime.removeContainer(ctx, node.container, force, false, true, false, false, false, timeout); err != nil { + opts := ctrRmOpts{ + Force: force, + RemovePod: true, + Timeout: timeout, + } + + if _, _, err := node.container.runtime.removeContainer(ctx, node.container, opts); err != nil { ctrErrored = true ctrErrors[node.id] = err } diff --git a/libpod/pod_api.go b/libpod/pod_api.go index d2bd7d2c07..c56e6cfb40 100644 --- a/libpod/pod_api.go +++ b/libpod/pod_api.go @@ -40,7 +40,12 @@ func (p *Pod) startInitContainers(ctx context.Context) error { icLock := initCon.lock icLock.Lock() var time *uint - if _, _, err := p.runtime.removeContainer(ctx, initCon, false, false, true, false, false, false, time); err != nil { + opts := ctrRmOpts{ + RemovePod: true, + Timeout: time, + } + + if _, _, err := p.runtime.removeContainer(ctx, initCon, opts); err != nil { icLock.Unlock() return fmt.Errorf("failed to remove once init container %s: %w", initCon.ID(), err) } diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index c6856cd34c..6fcb2561a4 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -607,9 +607,15 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai // container will be removed also (iff the container is the sole user of the // volumes). Timeout sets the stop timeout for the container if it is running. func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) error { + opts := ctrRmOpts{ + Force: force, + RemoveVolume: removeVolume, + Timeout: timeout, + } + // NOTE: container will be locked down the road. There is no unlocked // version of removeContainer. - _, _, err := r.removeContainer(ctx, c, force, removeVolume, false, false, false, false, timeout) + _, _, err := r.removeContainer(ctx, c, opts) return err } @@ -621,9 +627,40 @@ func (r *Runtime) RemoveContainer(ctx context.Context, c *Container, force bool, // always returned, even if error is set, and indicate any containers that were // successfully removed prior to the error. func (r *Runtime) RemoveContainerAndDependencies(ctx context.Context, c *Container, force bool, removeVolume bool, timeout *uint) (map[string]error, map[string]error, error) { + opts := ctrRmOpts{ + Force: force, + RemoveVolume: removeVolume, + RemoveDeps: true, + Timeout: timeout, + } + // NOTE: container will be locked down the road. There is no unlocked // version of removeContainer. - return r.removeContainer(ctx, c, force, removeVolume, false, false, true, false, timeout) + return r.removeContainer(ctx, c, opts) +} + +// Options for removeContainer +type ctrRmOpts struct { + // Whether to stop running container(s) + Force bool + // Whether to remove anonymous volumes used by removing the container + RemoveVolume bool + // Only set by `removePod` as `removeContainer` is being called as part + // of removing a whole pod. + RemovePod bool + // Whether to ignore dependencies of the container when removing + // (This is *DANGEROUS* and should not be used outside of non-graph + // traversal pod removal code). + IgnoreDeps bool + // Remove all the dependencies associated with the container. Can cause + // multiple containers, and possibly one or more pods, to be removed. + RemoveDeps bool + // Do not lock the pod that the container is part of (used only by + // recursive calls of removeContainer, used when removing dependencies) + NoLockPod bool + // Timeout to use when stopping the container. Only used if `Force` is + // true. + Timeout *uint } // Internal function to remove a container. @@ -639,7 +676,7 @@ func (r *Runtime) RemoveContainerAndDependencies(ctx context.Context, c *Contain // noLockPod is used for recursive removeContainer calls when the pod is already // locked. // TODO: At this point we should just start accepting an options struct -func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, removeVolume, removePod, ignoreDeps, removeDeps, noLockPod bool, timeout *uint) (removedCtrs map[string]error, removedPods map[string]error, retErr error) { +func (r *Runtime) removeContainer(ctx context.Context, c *Container, opts ctrRmOpts) (removedCtrs map[string]error, removedPods map[string]error, retErr error) { removedCtrs = make(map[string]error) removedPods = make(map[string]error) @@ -652,7 +689,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } } - if removePod && removeDeps { + if opts.RemovePod && opts.RemoveDeps { retErr = fmt.Errorf("cannot remove dependencies while also removing a pod: %w", define.ErrInvalidArg) return } @@ -685,13 +722,13 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo return } - if !removePod { + if !opts.RemovePod { // Lock the pod while we're removing container if pod.config.LockID == c.config.LockID { retErr = fmt.Errorf("container %s and pod %s share lock ID %d: %w", c.ID(), pod.ID(), c.config.LockID, define.ErrWillDeadlock) return } - if !noLockPod { + if !opts.NoLockPod { pod.lock.Lock() defer pod.lock.Unlock() } @@ -701,7 +738,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } infraID := pod.state.InfraContainerID - if c.ID() == infraID && !removeDeps { + if c.ID() == infraID && !opts.RemoveDeps { retErr = fmt.Errorf("container %s is the infra container of pod %s and cannot be removed without removing the pod", c.ID(), pod.ID()) return } @@ -710,7 +747,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // For pod removal, the container is already locked by the caller locked := false - if !removePod { + if !opts.RemovePod { c.lock.Lock() defer func() { if locked { @@ -742,7 +779,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo retErr = err return } - if !removeDeps { + if !opts.RemoveDeps { retErr = fmt.Errorf("container %s is the service container of pod(s) %s and cannot be removed without removing the pod(s)", c.ID(), strings.Join(c.state.Service.Pods, ",")) return } @@ -754,7 +791,7 @@ 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()) - podRemovedCtrs, err := r.RemovePod(ctx, depPod, true, force, timeout) + podRemovedCtrs, err := r.RemovePod(ctx, depPod, true, opts.Force, opts.Timeout) for ctr, err := range podRemovedCtrs { removedCtrs[ctr] = err } @@ -766,7 +803,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo removedPods[depPod.ID()] = nil } } - if (serviceForPod || c.config.IsInfra) && !removePod { + if (serviceForPod || c.config.IsInfra) && !opts.RemovePod { // We're going to remove the pod we are a part of. // This will get rid of us as well, so we can just return // immediately after. @@ -776,7 +813,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo } logrus.Infof("Removing pod %s (dependency of container %s)", pod.ID(), c.ID()) - podRemovedCtrs, err := r.removePod(ctx, pod, true, force, timeout) + podRemovedCtrs, err := r.removePod(ctx, pod, true, opts.Force, opts.Timeout) for ctr, err := range podRemovedCtrs { removedCtrs[ctr] = err } @@ -791,7 +828,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // If we're not force-removing, we need to check if we're in a good // state to remove. - if !force { + if !opts.Force { if err := c.checkReadyForRemoval(); err != nil { retErr = err return @@ -825,13 +862,13 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // Check that no other containers depend on the container. // Only used if not removing a pod - pods guarantee that all // deps will be evicted at the same time. - if !ignoreDeps { + if !opts.IgnoreDeps { deps, err := r.state.ContainerInUse(c) if err != nil { retErr = err return } - if !removeDeps { + if !opts.RemoveDeps { if len(deps) != 0 { depsStr := strings.Join(deps, ", ") retErr = fmt.Errorf("container %s has dependent containers which must be removed before it: %s: %w", c.ID(), depsStr, define.ErrCtrExists) @@ -845,7 +882,14 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo return } logrus.Infof("Removing container %s (dependency of container %s)", dep.ID(), c.ID()) - ctrs, pods, err := r.removeContainer(ctx, dep, force, removeVolume, false, false, true, true, timeout) + recursiveOpts := ctrRmOpts{ + Force: opts.Force, + RemoveVolume: opts.RemoveVolume, + RemoveDeps: true, + NoLockPod: true, + Timeout: opts.Timeout, + } + ctrs, pods, err := r.removeContainer(ctx, dep, recursiveOpts) for rmCtr, err := range ctrs { removedCtrs[rmCtr] = err } @@ -862,8 +906,8 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo // Check that the container's in a good state to be removed. if c.ensureState(define.ContainerStateRunning, define.ContainerStateStopping) { time := c.StopTimeout() - if timeout != nil { - time = *timeout + if opts.Timeout != nil { + time = *opts.Timeout } // Ignore ErrConmonDead - we couldn't retrieve the container's // exit code properly, but it's still stopped. @@ -958,7 +1002,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo c.newContainerEvent(events.Remove) - if !removeVolume { + if !opts.RemoveVolume { return } @@ -967,7 +1011,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo if !volume.Anonymous() { continue } - if err := runtime.removeVolume(ctx, volume, false, timeout, false); err != nil && !errors.Is(err, define.ErrNoSuchVolume) { + if err := runtime.removeVolume(ctx, volume, false, opts.Timeout, false); err != nil && !errors.Is(err, define.ErrNoSuchVolume) { if errors.Is(err, define.ErrVolumeBeingUsed) { // Ignore error, since podman will report original error volumesFrom, _ := c.volumesFrom() @@ -1021,7 +1065,12 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol if err == nil { logrus.Infof("Container %s successfully retrieved from state, attempting normal removal", id) // Assume force = true for the evict case - _, _, err = r.removeContainer(ctx, tmpCtr, true, removeVolume, false, false, false, false, timeout) + opts := ctrRmOpts{ + Force: true, + RemoveVolume: removeVolume, + Timeout: timeout, + } + _, _, err = r.removeContainer(ctx, tmpCtr, opts) if !tmpCtr.valid { // If the container is marked invalid, remove succeeded // in kicking it out of the state - no need to continue. diff --git a/libpod/runtime_img.go b/libpod/runtime_img.go index 29d77b61f1..c1bec6d22b 100644 --- a/libpod/runtime_img.go +++ b/libpod/runtime_img.go @@ -46,7 +46,11 @@ func (r *Runtime) RemoveContainersForImageCallback(ctx context.Context) libimage return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } else { - if _, _, err := r.removeContainer(ctx, ctr, true, false, false, false, false, false, timeout); err != nil { + opts := ctrRmOpts{ + Force: true, + Timeout: timeout, + } + if _, _, err := r.removeContainer(ctx, ctr, opts); err != nil { return fmt.Errorf("removing image %s: container %s using image could not be removed: %w", imageID, ctr.ID(), err) } } diff --git a/libpod/runtime_pod_common.go b/libpod/runtime_pod_common.go index b22acd586e..22375c4616 100644 --- a/libpod/runtime_pod_common.go +++ b/libpod/runtime_pod_common.go @@ -143,7 +143,13 @@ func (r *Runtime) removeMalformedPod(ctx context.Context, p *Pod, ctrs []*Contai ctrNamedVolumes[vol.Name] = vol } - _, _, err := r.removeContainer(ctx, ctr, force, false, true, true, false, false, timeout) + opts := ctrRmOpts{ + Force: force, + RemovePod: true, + IgnoreDeps: true, + Timeout: timeout, + } + _, _, err := r.removeContainer(ctx, ctr, opts) return err }() removedCtrs[ctr.ID()] = err diff --git a/libpod/runtime_volume_common.go b/libpod/runtime_volume_common.go index 1d22ac8c2a..b771f464d6 100644 --- a/libpod/runtime_volume_common.go +++ b/libpod/runtime_volume_common.go @@ -388,7 +388,11 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo logrus.Debugf("Removing container %s (depends on volume %q)", ctr.ID(), v.Name()) - if _, _, err := r.removeContainer(ctx, ctr, force, false, false, false, false, false, timeout); err != nil { + opts := ctrRmOpts{ + Force: force, + Timeout: timeout, + } + if _, _, err := r.removeContainer(ctx, ctr, opts); err != nil { return fmt.Errorf("removing container %s that depends on volume %s: %w", ctr.ID(), v.Name(), err) } } From 398e48a24a9b43511365a6f3684e7a68e26d78ed Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Thu, 11 May 2023 14:57:45 -0400 Subject: [PATCH 09/14] Change Inherit to use a pointer to a container This fixes a lint issue, but I'm keeping it in its own commit so it can be reverted independently if necessary; I don't know what side effects this may have. I don't *think* there are any issues, but I'm not sure why it wasn't a pointer in the first place, so there may have been a reason. Signed-off-by: Matthew Heon --- cmd/podman/pods/rm.go | 5 ++++- libpod/define/errors.go | 4 ++++ libpod/runtime_pod_common.go | 2 +- pkg/specgen/generate/container_create.go | 4 ++-- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/cmd/podman/pods/rm.go b/cmd/podman/pods/rm.go index 69eeae21fd..9251ff9ebe 100644 --- a/cmd/podman/pods/rm.go +++ b/cmd/podman/pods/rm.go @@ -113,7 +113,10 @@ func removePods(namesOrIDs []string, rmOptions entities.PodRmOptions, printIDs b return nil } setExitCode(err) - return append(errs, err) + errs = append(errs, err) + if !strings.Contains(err.Error(), define.ErrRemovingCtrs.Error()) { + return errs + } } // in the cli, first we print out all the successful attempts diff --git a/libpod/define/errors.go b/libpod/define/errors.go index be471c27e3..9849660042 100644 --- a/libpod/define/errors.go +++ b/libpod/define/errors.go @@ -211,4 +211,8 @@ var ( // ErrConmonVersionFormat is used when the expected version format of conmon // has changed. ErrConmonVersionFormat = "conmon version changed format" + + // ErrRemovingCtrs indicates that there was an error removing all + // containers from a pod. + ErrRemovingCtrs = errors.New("removing pod containers") ) diff --git a/libpod/runtime_pod_common.go b/libpod/runtime_pod_common.go index 22375c4616..56c8a6153f 100644 --- a/libpod/runtime_pod_common.go +++ b/libpod/runtime_pod_common.go @@ -248,7 +248,7 @@ func (r *Runtime) removePod(ctx context.Context, p *Pod, removeCtrs, force bool, } if len(ctrErrors) > 0 { - return removedCtrs, fmt.Errorf("not all containers could be removed from pod %s: %w", p.ID(), define.ErrCtrExists) + return removedCtrs, fmt.Errorf("not all containers could be removed from pod %s: %w", p.ID(), define.ErrRemovingCtrs) } } diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 6ce2eea2ca..65143ca641 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -79,7 +79,7 @@ func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGener compatibleOptions := &libpod.InfraInherit{} var infraSpec *specs.Spec if infra != nil { - options, infraSpec, compatibleOptions, err = Inherit(*infra, s, rt) + options, infraSpec, compatibleOptions, err = Inherit(infra, s, rt) if err != nil { return nil, nil, nil, err } @@ -636,7 +636,7 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l return options, nil } -func Inherit(infra libpod.Container, s *specgen.SpecGenerator, rt *libpod.Runtime) (opts []libpod.CtrCreateOption, infraS *specs.Spec, compat *libpod.InfraInherit, err error) { +func Inherit(infra *libpod.Container, s *specgen.SpecGenerator, rt *libpod.Runtime) (opts []libpod.CtrCreateOption, infraS *specs.Spec, compat *libpod.InfraInherit, err error) { inheritSpec := &specgen.SpecGenerator{} _, compatibleOptions, err := ConfigToSpec(rt, inheritSpec, infra.ID()) if err != nil { From 0e47465e4abf977b56aa4fb102ad7085ddc7aaa2 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 2 Jun 2023 11:41:58 -0400 Subject: [PATCH 10/14] Discard errors when a pod is already removed This was causing some CI flakes. I'm pretty sure that the pods being removed already isn't a bug, but just the result of another container in the pod removing it first - so no reason not to ignore the errors. Signed-off-by: Matthew Heon --- libpod/runtime_ctr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index 6fcb2561a4..cfde14ac11 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -795,7 +795,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, opts ctrRmO for ctr, err := range podRemovedCtrs { removedCtrs[ctr] = err } - if err != nil { + if err != nil && !errors.Is(err, define.ErrNoSuchPod) && !errors.Is(err, define.ErrPodRemoved) { removedPods[depPod.ID()] = err retErr = fmt.Errorf("error removing container %s dependency pods: %w", c.ID(), err) return @@ -817,7 +817,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, opts ctrRmO for ctr, err := range podRemovedCtrs { removedCtrs[ctr] = err } - if err != nil { + if err != nil && !errors.Is(err, define.ErrNoSuchPod) && !errors.Is(err, define.ErrPodRemoved) { removedPods[pod.ID()] = err retErr = fmt.Errorf("error removing container %s pod: %w", c.ID(), err) return From a750cd98769926b5b430f28bfbec4f23882c2752 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Fri, 2 Jun 2023 13:13:13 -0400 Subject: [PATCH 11/14] Fix a race removing multiple containers in the same pod If the first container to get the pod lock is the infra container it's going to want to remove the entire pod, which will also remove every other container in the pod. Subsequent containers will get the pod lock and try to access the pod, only to realize it no longer exists - and that, actually, the container being removed also no longer exists. Signed-off-by: Matthew Heon --- libpod/runtime_ctr.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go index cfde14ac11..dcd0bd53e5 100644 --- a/libpod/runtime_ctr.go +++ b/libpod/runtime_ctr.go @@ -718,6 +718,21 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, opts ctrRmO if c.config.Pod != "" { pod, err = r.state.Pod(c.config.Pod) if err != nil { + // There's a potential race here where the pod we are in + // was already removed. + // If so, this container is also removed, as pods take + // all their containers with them. + // So if it's already gone, check if we are too. + if errors.Is(err, define.ErrNoSuchPod) { + // We could check the DB to see if we still + // exist, but that would be a serious violation + // of DB integrity. + // Mark this container as removed so there's no + // confusion, though. + removedCtrs[c.ID()] = nil + return + } + retErr = err return } @@ -733,6 +748,13 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, opts ctrRmO defer pod.lock.Unlock() } if err := pod.updatePod(); err != nil { + // As above, there's a chance the pod was + // already removed. + if errors.Is(err, define.ErrNoSuchPod) { + removedCtrs[c.ID()] = nil + return + } + retErr = err return } From 310082444c805320bdf51163fdb9c7d10c994510 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 5 Jun 2023 16:22:58 -0400 Subject: [PATCH 12/14] Fix an expected error message from pod removal Signed-off-by: Matthew Heon --- test/e2e/pod_rm_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/pod_rm_test.go b/test/e2e/pod_rm_test.go index ef32e30b22..d5d0e4ccc7 100644 --- a/test/e2e/pod_rm_test.go +++ b/test/e2e/pod_rm_test.go @@ -123,7 +123,7 @@ var _ = Describe("Podman pod rm", func() { result := podmanTest.Podman([]string{"pod", "rm", "-a"}) result.WaitWithDefaultTimeout() Expect(result).To(ExitWithError()) - Expect(result.ErrorToString()).To(ContainSubstring("cannot be removed")) + Expect(result.ErrorToString()).To(ContainSubstring("not all containers could be removed from pod")) numPods = podmanTest.NumberOfPods() ps = podmanTest.Podman([]string{"pod", "ps"}) From f1ecdca4b6dd533d969aa45439e178f8f36ec3c3 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 6 Jun 2023 14:43:13 -0400 Subject: [PATCH 13/14] Ensure our mutexes handle recursive locking properly We use shared-memory pthread mutexes to handle mutual exclusion in Libpod. It turns out that these have configurable options for how to handle a recursive lock (IE, a thread trying to lock a lock that the same thread had previously locked). The mutex can either deadlock, or allow the duplicate lock without deadlocking. Default behavior is, helpfully, unspecified, so if not explicitly set there is no clear indication of which of these behaviors will be seen. Unfortunately, today is the first I learned of this, so our initial implementation did *not* explicitly set our preferred behavior. This turns out to be a major problem with a language like Golang, where multiple goroutines can (and often do) use the same OS thread. So we can have two goroutines trying to stop the same container, and if the no-deadlock mutex behavior is in use, both threads will successfully acquire the lock because the C library, not knowing about Go's lightweight threads, sees the same PID trying to lock a mutex twice, and allows it without question. It appears that, at least on Fedora/RHEL/Debian libc, the default (unspecified) behavior of the locks is the non-deadlocking version - so, effectively, our locks have been of questionable utility within the same Podman process for the last four years. This is somewhat concerning. What's even more concerning is that the Golang-native sync.Mutex that was also in use did nothing to prevent the duplicate locking (I don't know if I like the implications of this). Anyways, this resolves the major issue of our locks not working correctly by explicitly setting the correct pthread mutex behavior. Signed-off-by: Matthew Heon --- libpod/lock/shm/shm_lock.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libpod/lock/shm/shm_lock.c b/libpod/lock/shm/shm_lock.c index 0dae806fcf..9b4bf7ec04 100644 --- a/libpod/lock/shm/shm_lock.c +++ b/libpod/lock/shm/shm_lock.c @@ -136,6 +136,14 @@ shm_struct_t *setup_lock_shm(char *path, uint32_t num_locks, int *error_code) { goto CLEANUP_UNMAP; } + // Ensure that recursive locking of a mutex by the same OS thread (which may + // refer to numerous goroutines) blocks. + ret_code = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_NORMAL); + if (ret_code != 0) { + *error_code = -1 * ret_code; + goto CLEANUP_FREEATTR; + } + // Set mutexes to pshared - multiprocess-safe ret_code = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED); if (ret_code != 0) { From 2ebc9004f40c20916e94ad2e3538571b058c40b5 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 7 Jun 2023 14:44:21 -0400 Subject: [PATCH 14/14] Ignore spurious warnings when killing containers There are certain messages logged by OCI runtimes when killing a container that has already stopped that we really do not care about when stopping a container. Due to our architecture, there are inherent races around stopping containers, and so we cannot guarantee that *we* are the people to kill it - but that doesn't matter because Podman only cares that the container has stopped, not who delivered the fatal signal. Unfortunately, the OCI runtimes don't understand this, and log various warning messages when the `kill` command is invoked on a container that was already dead. These cause our tests to fail, as we now check for clean STDERR when running Podman. To work around this, capture STDERR for the OCI runtime in a buffer only for stopping containers, and go through and discard any of the warnings we identified as spurious. Signed-off-by: Matthew Heon --- libpod/oci_conmon_common.go | 106 ++++++++++++++++++++++++++---------- 1 file changed, 76 insertions(+), 30 deletions(-) diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index d803276b15..95065bf28c 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -356,10 +356,20 @@ func generateResourceFile(res *spec.LinuxResources) (string, []string, error) { // If all is set, send to all PIDs in the container. // All is only supported if the container created cgroups. func (r *ConmonOCIRuntime) KillContainer(ctr *Container, signal uint, all bool) error { + if _, err := r.killContainer(ctr, signal, all, false); err != nil { + return err + } + + return nil +} + +// If captureStderr is requested, OCI runtime STDERR will be captured as a +// *bytes.buffer and returned; otherwise, it is set to os.Stderr. +func (r *ConmonOCIRuntime) killContainer(ctr *Container, signal uint, all, captureStderr bool) (*bytes.Buffer, error) { logrus.Debugf("Sending signal %d to container %s", signal, ctr.ID()) runtimeDir, err := util.GetRuntimeDir() if err != nil { - return err + return nil, err } env := []string{fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir)} var args []string @@ -369,19 +379,27 @@ func (r *ConmonOCIRuntime) KillContainer(ctr *Container, signal uint, all bool) } else { args = append(args, "kill", ctr.ID(), fmt.Sprintf("%d", signal)) } - if err := utils.ExecCmdWithStdStreams(os.Stdin, os.Stdout, os.Stderr, env, r.path, args...); err != nil { + var ( + stderr io.Writer = os.Stderr + stderrBuffer *bytes.Buffer + ) + if captureStderr { + stderrBuffer = new(bytes.Buffer) + stderr = stderrBuffer + } + if err := utils.ExecCmdWithStdStreams(os.Stdin, os.Stdout, stderr, env, r.path, args...); err != nil { // Update container state - there's a chance we failed because // the container exited in the meantime. if err2 := r.UpdateContainerStatus(ctr); err2 != nil { logrus.Infof("Error updating status for container %s: %v", ctr.ID(), err2) } if ctr.ensureState(define.ContainerStateStopped, define.ContainerStateExited) { - return fmt.Errorf("%w: %s", define.ErrCtrStateInvalid, ctr.state.State) + return stderrBuffer, fmt.Errorf("%w: %s", define.ErrCtrStateInvalid, ctr.state.State) } - return fmt.Errorf("sending signal to container %s: %w", ctr.ID(), err) + return stderrBuffer, fmt.Errorf("sending signal to container %s: %w", ctr.ID(), err) } - return nil + return stderrBuffer, nil } // StopContainer stops a container, first using its given stop signal (or @@ -400,23 +418,65 @@ func (r *ConmonOCIRuntime) StopContainer(ctr *Container, timeout uint, all bool) return nil } - if timeout > 0 { - stopSignal := ctr.config.StopSignal - if stopSignal == 0 { - stopSignal = uint(syscall.SIGTERM) + killCtr := func(signal uint) (bool, error) { + stderr, err := r.killContainer(ctr, signal, all, true) + + // Before handling error from KillContainer, convert STDERR to a []string + // (one string per line of output) and print it, ignoring known OCI runtime + // errors that we don't care about + stderrLines := strings.Split(stderr.String(), "\n") + for _, line := range stderrLines { + if line == "" { + continue + } + if strings.Contains(line, "container not running") || strings.Contains(line, "open pidfd: No such process") { + logrus.Debugf("Failure to kill container (already stopped?): logged %s", line) + continue + } + fmt.Fprintf(os.Stderr, "%s\n", line) } - if err := r.KillContainer(ctr, stopSignal, all); err != nil { + + if err != nil { + // There's an inherent race with the cleanup process (see + // #16142, #17142). If the container has already been marked as + // stopped or exited by the cleanup process, we can return + // immediately. + if errors.Is(err, define.ErrCtrStateInvalid) && ctr.ensureState(define.ContainerStateStopped, define.ContainerStateExited) { + return true, nil + } + + // If the PID is 0, then the container is already stopped. + if ctr.state.PID == 0 { + return true, nil + } + // Is the container gone? // If so, it probably died between the first check and // our sending the signal // The container is stopped, so exit cleanly err := unix.Kill(ctr.state.PID, 0) if err == unix.ESRCH { - return nil + return true, nil } + return false, err + } + return false, nil + } + + if timeout > 0 { + stopSignal := ctr.config.StopSignal + if stopSignal == 0 { + stopSignal = uint(syscall.SIGTERM) + } + + stopped, err := killCtr(stopSignal) + if err != nil { return err } + if stopped { + return nil + } if err := waitContainerStop(ctr, time.Duration(timeout)*time.Second); err != nil { logrus.Debugf("Timed out stopping container %s with %s, resorting to SIGKILL: %v", ctr.ID(), unix.SignalName(syscall.Signal(stopSignal)), err) @@ -427,27 +487,13 @@ func (r *ConmonOCIRuntime) StopContainer(ctr *Container, timeout uint, all bool) } } - // If the timeout was set to 0 or if stopping the container with the - // specified signal did not work, use the big hammer with SIGKILL. - if err := r.KillContainer(ctr, uint(unix.SIGKILL), all); err != nil { - // There's an inherent race with the cleanup process (see - // #16142, #17142). If the container has already been marked as - // stopped or exited by the cleanup process, we can return - // immediately. - if errors.Is(err, define.ErrCtrStateInvalid) && ctr.ensureState(define.ContainerStateStopped, define.ContainerStateExited) { - return nil - } - - // If the PID is 0, then the container is already stopped. - if ctr.state.PID == 0 { - return nil - } - // Again, check if the container is gone. If it is, exit cleanly. - if aliveErr := unix.Kill(ctr.state.PID, 0); errors.Is(aliveErr, unix.ESRCH) { - return nil - } + stopped, err := killCtr(uint(unix.SIGKILL)) + if err != nil { return fmt.Errorf("sending SIGKILL to container %s: %w", ctr.ID(), err) } + if stopped { + return nil + } // Give runtime a few seconds to make it happen if err := waitContainerStop(ctr, killContainerTimeout); err != nil {