Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove containers when pod prune & pod rm. #4443

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Remove containers when pod prune & pod rm. #4443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/podman/pod_rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

var (
podRmCommand cliconfig.PodRmValues
podRmDescription = fmt.Sprintf(`podman rm will remove one or more pods from the host.
podRmDescription = fmt.Sprintf(`podman rm will remove one or more stopped pods and their containers from the host.

The pod name or ID can be used. A pod with containers will not be removed without --force. If --force is specified, all containers will be stopped, then removed.`)
_podRmCommand = &cobra.Command{
Expand Down
4 changes: 2 additions & 2 deletions cmd/podman/pods_prune.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ var (
_prunePodsCommand = &cobra.Command{
Use: "prune",
Args: noSubArgs,
Short: "Remove all stopped pods",
Short: "Remove all stopped pods and their containers",
Long: podPruneDescription,
RunE: func(cmd *cobra.Command, args []string) error {
podPruneCommand.InputArgs = args
Expand All @@ -32,7 +32,7 @@ func init() {
podPruneCommand.SetHelpTemplate(HelpTemplate())
podPruneCommand.SetUsageTemplate(UsageTemplate())
flags := podPruneCommand.Flags()
flags.BoolVarP(&podPruneCommand.Force, "force", "f", false, "Force removal of a running pods. The default is false")
flags.BoolVarP(&podPruneCommand.Force, "force", "f", false, "Force removal of all running pods. The default is false")
}

func podPruneCmd(c *cliconfig.PodPruneValues) error {
Expand Down
1 change: 0 additions & 1 deletion cmd/podman/system_prune.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ Are you sure you want to continue? [y/N] `, volumeString)
fmt.Println("Deleted Pods")
pruneValues := cliconfig.PodPruneValues{
PodmanCommand: c.PodmanCommand,
Force: c.Force,
}
ctx := getContext()
ok, failures, lasterr := runtime.PrunePods(ctx, &pruneValues)
Expand Down
11 changes: 8 additions & 3 deletions docs/source/markdown/podman-pod-prune.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
% podman-pod-prune(1)

## NAME
podman-pod-prune - Remove all stopped pods
podman-pod-prune - Remove all stopped pods and their containers

## SYNOPSIS
**podman pod prune**

## DESCRIPTION
**podman pod prune** removes all stopped pods from local storage.
**podman pod prune** removes all stopped pods and their containers from local storage.

## OPTIONS

**--force** **-f**
Force removal of all running pods and their containers. The default is false.

## EXAMPLES
Remove all stopped pods from local storage
Remove all stopped pods and their containers from local storage
```
$ sudo podman pod prune
22b8813332948064b6566370088c5e0230eeaf15a58b1c5646859fd9fc364fe7
Expand Down
4 changes: 2 additions & 2 deletions docs/source/markdown/podman-pod-rm.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
% podman-pod-rm(1)

## NAME
podman\-pod\-rm - Remove one or more pods
podman\-pod\-rm - Remove one or more stopped pods and containers

## SYNOPSIS
**podman pod rm** [*options*] *pod*

## DESCRIPTION
**podman pod rm** will remove one or more pods from the host. The pod name or ID can be used. The \-f option stops all containers and then removes them before removing the pod. Without the \-f option, a pod cannot be removed if it has associated containers.
**podman pod rm** will remove one or more stopped pods and their containers from the host. The pod name or ID can be used. The \-f option stops all containers and then removes them before removing the pod.

## OPTIONS

Expand Down
4 changes: 2 additions & 2 deletions docs/source/markdown/podman-pod.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ podman pod is a set of subcommands that manage pods, or groups of containers.
| inspect | [podman-pod-inspect(1)](podman-pod-inspect.1.md) | Displays information describing a pod. |
| kill | [podman-pod-kill(1)](podman-pod-kill.1.md) | Kill the main process of each container in one or more pods. |
| pause | [podman-pod-pause(1)](podman-pod-pause.1.md) | Pause one or more pods. |
| prune | [podman-pod-prune(1)](podman-pod-prune.1.md) | Remove all stopped pods. |
| prune | [podman-pod-prune(1)](podman-pod-prune.1.md) | Remove all stopped pods and their containers. |
| ps | [podman-pod-ps(1)](podman-pod-ps.1.md) | Prints out information about pods. |
| restart | [podman-pod-restart(1)](podman-pod-restart.1.md) | Restart one or more pods. |
| rm | [podman-pod-rm(1)](podman-pod-rm.1.md) | Remove one or more pods. |
| rm | [podman-pod-rm(1)](podman-pod-rm.1.md) | Remove one or more stopped pods and containers. |
| start | [podman-pod-start(1)](podman-pod-start.1.md) | Start one or more pods. |
| stats | [podman-pod-stats(1)](podman-pod-stats.1.md) | Display a live stream of resource usage stats for containers in one or more pods. |
| stop | [podman-pod-stop(1)](podman-pod-stop.1.md) | Stop one or more pods. |
Expand Down
4 changes: 2 additions & 2 deletions docs/source/pod.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ Pod

:doc:`pause <markdown/podman-pause.1>` Pause one or more pods

:doc:`prune <markdown/podman-pod-prune.1>` Remove all stopped pods
:doc:`prune <markdown/podman-pod-prune.1>` Remove all stopped pods and their containers

:doc:`ps <markdown/podman-pod-ps.1>` List pods

:doc:`restart <markdown/podman-pod-restart.1>` Restart one or more pods

:doc:`rm <markdown/podman-pod-rm.1>` Remove one or more pods
:doc:`rm <markdown/podman-pod-rm.1>` Remove one or more stopped pods and containers

:doc:`start <markdown/podman-pod-start.1>` Start one or more pods

Expand Down
4 changes: 2 additions & 2 deletions pkg/adapter/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func (r *LocalRuntime) PrunePods(ctx context.Context, cli *cliconfig.PodPruneVal
pool.Add(shared.Job{
ID: p.ID(),
Fn: func() error {
err := r.Runtime.RemovePod(ctx, p, cli.Force, cli.Force)
err := r.Runtime.RemovePod(ctx, p, true, cli.Force)
if err != nil {
logrus.Debugf("Failed to remove pod %s: %s", p.ID(), err.Error())
}
Expand All @@ -101,7 +101,7 @@ func (r *LocalRuntime) RemovePods(ctx context.Context, cli *cliconfig.PodRmValue
}

for _, p := range pods {
if err := r.Runtime.RemovePod(ctx, p, cli.Force, cli.Force); err != nil {
if err := r.Runtime.RemovePod(ctx, p, true, cli.Force); err != nil {
errs = append(errs, err)
} else {
podids = append(podids, p.ID())
Expand Down
2 changes: 1 addition & 1 deletion pkg/varlinkapi/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (i *LibpodAPI) RemovePod(call iopodman.VarlinkCall, name string, force bool
if err != nil {
return call.ReplyPodNotFound(name, err.Error())
}
if err = i.Runtime.RemovePod(ctx, pod, force, force); err != nil {
if err = i.Runtime.RemovePod(ctx, pod, true, force); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the issue for podman remote pod prune remove stop containers.
But podman remote pod rm will use the same logic to remove pod and stopped containers from the pod.
Do we allow podman pod rm to remove pod if containers are stopped?
@rhatdan @mheon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should, that is the change we made to podman pod rm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the tests failures podman pod rm doesn't remove a pod with a container https://github.com/containers/libpod/blob/b3d8b99b82d466ab9a325e66c1a240ff989eeb66/test/e2e/pod_rm_test.go#L80-L89

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then remove this test. Podman pod will always have a container. I don't see a reason to not remove the pod. Too much work on the user to have to remove all containers before removing the pod. Force says remove the pod and containers if they are running.
Containers in Pods do not standalone, so pod includes containers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We handle the infra container explicitly. I would prefer that we keep the current behavior - --force to remove pods with anything other than an infra container

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why. Containers are part of the Pod and should be treated as such. Similar to built in volumes. I don't want to add yet another option that will confuse the users.

When people think in terms of pods, they do not consider the containers as separate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And podman rm doesn't remove volumes unless you tell it to - you need --volumes to do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, we should just get a session together about pods at the F2F - it's a good opportunity to revisit these decisions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

podman rm removes the builtin volumes, IE Volumes that are not used by other entities, I see Pod containers the same way. But yes we can talk about this at the Face2Face.

@haircommander @mrunalp What does kubernetes do. Does it first remove all of the containers from a pod, before removing the pod, or when you tell CRI-O to remove the pod, then it removes all of the containers and finally the POD?

Copy link
Collaborator

@haircommander haircommander Nov 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the kubelet asks cri-o to remove a pod, it is removed, regardless of containers that live in it, I am pretty sure. The kubelet making a request of cri-o is a bit different than a user typing cli, though. I think ultimately I'm with @rhatdan here, a pod should be treated as a unit in removal.

return call.ReplyErrorOccurred(err.Error())
}

Expand Down
23 changes: 20 additions & 3 deletions test/e2e/pod_prune_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,24 @@ var _ = Describe("Podman pod prune", func() {
Expect(result.ExitCode()).To(Equal(0))
})

It("podman pod prune doesn't remove a pod with a container", func() {
It("podman pod prune doesn't remove a pod with a running container", func() {
_, ec, podid := podmanTest.CreatePod("")
Expect(ec).To(Equal(0))

ec2 := podmanTest.RunTopContainerInPod("", podid)
ec2.WaitWithDefaultTimeout()
Expect(ec2.ExitCode()).To(Equal(0))

result := podmanTest.Podman([]string{"pod", "prune"})
result.WaitWithDefaultTimeout()
Expect(result.ExitCode()).To((Equal(0)))

result = podmanTest.Podman([]string{"ps", "-qa"})
result.WaitWithDefaultTimeout()
Expect(len(result.OutputToStringArray())).To(Equal(1))
})

It("podman pod prune removes a pod with a stopped container", func() {
_, ec, podid := podmanTest.CreatePod("")
Expect(ec).To(Equal(0))

Expand All @@ -50,11 +67,11 @@ var _ = Describe("Podman pod prune", func() {

result := podmanTest.Podman([]string{"pod", "prune"})
result.WaitWithDefaultTimeout()
Expect(result.ExitCode()).To(Equal(125))
Expect(result.ExitCode()).To(Equal(0))

result = podmanTest.Podman([]string{"ps", "-qa"})
result.WaitWithDefaultTimeout()
Expect(len(result.OutputToStringArray())).To(Equal(1))
Expect(len(result.OutputToStringArray())).To(Equal(0))
})

It("podman pod prune -f does remove a running container", func() {
Expand Down
8 changes: 4 additions & 4 deletions test/e2e/pod_rm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ var _ = Describe("Podman pod rm", func() {
Expect(result.OutputToString()).To(Not(ContainSubstring(podid2)))
})

It("podman pod rm doesn't remove a pod with a container", func() {
It("podman pod rm removes a pod with a container", func() {
_, ec, podid := podmanTest.CreatePod("")
Expect(ec).To(Equal(0))

Expand All @@ -86,11 +86,11 @@ var _ = Describe("Podman pod rm", func() {

result := podmanTest.Podman([]string{"pod", "rm", podid})
result.WaitWithDefaultTimeout()
Expect(result.ExitCode()).To(Equal(125))
Expect(result.ExitCode()).To(Equal(0))

result = podmanTest.Podman([]string{"ps", "-qa"})
result.WaitWithDefaultTimeout()
Expect(len(result.OutputToStringArray())).To(Equal(1))
Expect(len(result.OutputToStringArray())).To(Equal(0))
})

It("podman pod rm -f does remove a running container", func() {
Expand Down Expand Up @@ -136,7 +136,7 @@ var _ = Describe("Podman pod rm", func() {
result := podmanTest.Podman([]string{"pod", "rm", "-a"})
result.WaitWithDefaultTimeout()
Expect(result).To(ExitWithError())
foundExpectedError, _ := result.ErrorGrepString("contains containers and cannot be removed")
foundExpectedError, _ := result.ErrorGrepString("cannot be removed")
Expect(foundExpectedError).To(Equal(true))

num_pods = podmanTest.NumberOfPods()
Expand Down