Skip to content

Commit

Permalink
remove service container _after_ pods
Browse files Browse the repository at this point in the history
Do not allow for removing the service container unless all associated
pods have been removed.  Previously, the service container could be
removed when all pods have exited which can lead to a number of issues.

Now, the service container is treated like an infra container and can
only be removed along with the pods.

Fixes: containers#16964
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
  • Loading branch information
vrothberg authored and umohnani8 committed Jan 9, 2023
1 parent 576e1aa commit 9a20d94
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 5 deletions.
14 changes: 10 additions & 4 deletions libpod/runtime_ctr.go
Expand Up @@ -687,11 +687,17 @@ func (r *Runtime) removeContainer(ctx context.Context, c *Container, force, remo
}

if c.IsService() {
canStop, err := c.canStopServiceContainer()
if err != nil {
return err
var pods []string
for _, id := range c.state.Service.Pods {
if _, err := c.runtime.LookupPod(id); err != nil {
if errors.Is(err, define.ErrNoSuchPod) {
continue
}
return err
}
pods = append(pods, id)
}
if !canStop {
if len(pods) > 0 {
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, ","))
}
}
Expand Down
13 changes: 12 additions & 1 deletion libpod/service.go
Expand Up @@ -89,6 +89,9 @@ func (c *Container) canStopServiceContainer() (bool, error) {

status, err := pod.GetPodStatus()
if err != nil {
if errors.Is(err, define.ErrNoSuchPod) {
continue
}
return false, err
}

Expand All @@ -112,6 +115,9 @@ func (p *Pod) maybeStopServiceContainer() error {

serviceCtr, err := p.serviceContainer()
if err != nil {
if errors.Is(err, define.ErrNoSuchCtr) {
return nil
}
return fmt.Errorf("getting pod's service container: %w", err)
}
// Checking whether the service can be stopped must be done in
Expand Down Expand Up @@ -195,6 +201,9 @@ func (p *Pod) maybeRemoveServiceContainer() error {

serviceCtr, err := p.serviceContainer()
if err != nil {
if errors.Is(err, define.ErrNoSuchCtr) {
return nil
}
return fmt.Errorf("getting pod's service container: %w", err)
}
// Checking whether the service can be stopped must be done in
Expand All @@ -204,7 +213,9 @@ func (p *Pod) maybeRemoveServiceContainer() error {
logrus.Debugf("Pod %s has a service %s: checking if it can be removed", p.ID(), serviceCtr.ID())
canRemove, err := serviceCtr.canRemoveServiceContainerLocked()
if err != nil {
logrus.Errorf("Checking whether service of container %s can be removed: %v", serviceCtr.ID(), err)
if !errors.Is(err, define.ErrNoSuchCtr) {
logrus.Errorf("Checking whether service container %s can be removed: %v", serviceCtr.ID(), err)
}
return
}
if !canRemove {
Expand Down
2 changes: 2 additions & 0 deletions test/system/700-play.bats
Expand Up @@ -170,6 +170,8 @@ EOF
# Check for an error when trying to remove the service container
run_podman 125 container rm $service_container
is "$output" "Error: container .* is the service container of pod(s) .* and cannot be removed without removing the pod(s)"
run_podman 125 container rm --force $service_container
is "$output" "Error: container .* is the service container of pod(s) .* and cannot be removed without removing the pod(s)"

# Kill the pod and make sure the service is not running
run_podman pod kill test_pod
Expand Down

0 comments on commit 9a20d94

Please sign in to comment.