Fix podman rm -fa with dependencies

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
 		}

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)
 			}

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 {

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)
 				}
 			}

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 {

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)
 			}
 		}

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,46 +440,14 @@ 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 {
 		return nil, err
 	}
 
 	for ctr, err := range errMap {
-		if alreadyRemoved[ctr.ID()] {
-			continue
-		}
 		report := new(reports.RmReport)
 		report.Id = ctr.ID()
 		report.Err = err