Skip to content

Commit

Permalink
The removeContainer function now accepts a struct
Browse files Browse the repository at this point in the history 
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 <matthew.heon@pm.me>
  • Loading branch information
@mheon
mheon committed May 9, 2023
1 parent 361c0ed commit 86840de
Show file tree
Hide file tree
Showing 6 changed files with 101 additions and 26 deletions.
9 changes: 8 additions & 1 deletion libpod/container_graph.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,14 @@ 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,
RemoveVolume: true,
RemovePod: true,
Timeout: timeout,
}

if _, _, err := node.container.runtime.removeContainer(ctx, node.container, opts); err != nil {
ctrErrored = true
ctrErrors[node.id] = err
}
Expand Down
7 changes: 6 additions & 1 deletion libpod/pod_api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
91 changes: 70 additions & 21 deletions libpod/runtime_ctr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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.
Expand All @@ -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)

Expand All @@ -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
}
Expand Down Expand Up @@ -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()
}
Expand All @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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.
Expand All @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
Expand All @@ -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.
Expand Down Expand Up @@ -977,7 +1021,7 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo

c.newContainerEvent(events.Remove)

if !removeVolume {
if !opts.RemoveVolume {
return
}

Expand All @@ -986,7 +1030,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()
Expand Down Expand Up @@ -1040,7 +1084,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.
Expand Down
6 changes: 5 additions & 1 deletion libpod/runtime_img.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
8 changes: 7 additions & 1 deletion libpod/runtime_pod_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion libpod/runtime_volume_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down

0 comments on commit 86840de

Please sign in to comment.