Add --time option for podman * rm -f flag #11763

rhatdan · 2021-09-28T01:32:46Z

Add --time options to

podman container rm
podman pod rm
podman volume rm
podman network rm

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2021-09-28T01:33:10Z

Might make @edsantiago happy in #11762

vrothberg

Other than the mini nits, LGTM

cmd/podman/containers/rm.go

vrothberg · 2021-09-28T11:06:34Z

test/e2e/checkpoint_test.go

@@ -159,7 +159,7 @@ var _ = Describe("Podman checkpoint", func() {
 		Expect(result).Should(Exit(2))
 		Expect(podmanTest.NumberOfContainersRunning()).To(Equal(1))

-		result = podmanTest.Podman([]string{"rm", "-f", cid})
+		result = podmanTest.Podman([]string{"rm", "-t", 0, "-f", cid})


0 -> "0" in all _test.go files.

This is why I shouldn't code late at night.

Luap99 · 2021-09-28T11:28:37Z

pkg/domain/infra/abi/containers.go

+	if options.Force && options.Timeout != nil {
+		// Ignore errors, ic.removeContainer will catch them
+		parallelctr.ContainerOp(ctx, ctrs, func(c *libpod.Container) error {
+			return c.StopWithTimeout(*options.Timeout)


Can this be moved inside the other ContainerOp function below. I don't think it is necessary to stop all container before we start to remove them.
Also should we really ignore all errors?

rhatdan · 2021-09-28T13:00:05Z

@Luap99 good idea.
Updated and hopefully fixed tests.

vrothberg · 2021-09-28T13:10:27Z

@rhatdan needs a rebase

edsantiago · 2021-09-28T13:12:31Z

Yeah, my fault with the bats-cleanup PR. Sorry! And, thank you!

mheon · 2021-09-28T13:16:23Z

pkg/domain/infra/tunnel/containers.go

@@ -193,6 +193,10 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 		}
 		reports := make([]*entities.RmReport, 0, len(ctrs))
 		for _, c := range ctrs {
+			if opts.Force && opts.Timeout != nil {
+				// Ignore errors, containers.Remove will catch them
+				containers.Stop(ic.ClientCtx, c.ID, &containers.StopOptions{Timeout: opts.Timeout})


Why are we doing this? Remove already stops containers if force is specified?

This just strikes me as potentially buggy - we ignore all errors that could occur with stopping the container... Seems like it would be a lot safer to add a Timeout arg to the libpod RemoveContainer function?

There is some nasty plumbing to do. We will still try to stop the container within the remove stage if it is not stopped, so I don't believe there is any risk.

This allows us to stop them first without a timeout of 0.

This can be removed now I think

edsantiago · 2021-09-28T15:28:01Z

This CI error actually looks real:

  podman checkpoint container with export (migration)
  ...
 Running: podman [options] ps --all --format={{.Status}}
 Up 2 seconds ago
 Up 1 second ago
 Running: podman [options] rm -t 0 -fa
         7b425b2420aceccdd7fcafbe9a5ffcc3c48c37257f8df53cbe958e440a1ca40b
         Error: container e6f1e20fd52ba325a469ca432d36a25f65a016115a44d687c61cefbcd0961fb0 does not exist in database: no such container

rhatdan · 2021-09-28T18:04:39Z

@mheon I expanded the PR to handle, pod, network and volume, and fully plumbed this into libpod.

TomSweeneyRedHat · 2021-09-28T19:55:35Z

cmd/podman/volumes/rm.go

@@ -43,6 +45,10 @@ func init() {
 	flags := rmCommand.Flags()
 	flags.BoolVarP(&rmOptions.All, "all", "a", false, "Remove all volumes")
 	flags.BoolVarP(&rmOptions.Force, "force", "f", false, "Remove a volume by force, even if it is being used by a container")
+	timeFlagName := "time"
+	flags.UintVarP(&stopTimeout, timeFlagName, "t", containerConfig.Engine.StopTimeout, "Seconds to wait for pod stop before killing the container")


Is this message right? I'm not sure of it or my suggestion...

Suggested change

flags.UintVarP(&stopTimeout, timeFlagName, "t", containerConfig.Engine.StopTimeout, "Seconds to wait for pod stop before killing the container")

flags.UintVarP(&stopTimeout, timeFlagName, "t", containerConfig.Engine.StopTimeout, "Seconds to wait for the container to stop before removing the volume")

TomSweeneyRedHat · 2021-09-28T19:56:49Z

docs/source/markdown/podman-network-rm.1.md

@@ -15,6 +15,10 @@ Delete one or more Podman networks.
 The `force` option will remove all containers that use the named network. If the container is
 running, the container will be stopped and removed.

+#### **--time**, **-t**=*time*
+
+Time to wait before forcibly stopping the container. The --force option must be specified to use the --time option.


container or network?

or maybe "the container that the network is attached to."

TomSweeneyRedHat · 2021-09-28T19:57:01Z

docs/source/markdown/podman-pod-rm.1.md

@@ -33,6 +33,10 @@ Stop running containers and delete all stopped containers before removal of pod.

 Read pod ID from the specified file and remove the pod.  Can be specified multiple times.

+#### **--time**, **-t**=*time*
+
+Time to wait before forcibly stopping the container. The --force option must be specified to use the --time option.


pod or container?

or "containers within the pod".

TomSweeneyRedHat · 2021-09-28T20:00:39Z

docs/source/markdown/podman-volume-rm.1.md

@@ -28,6 +28,9 @@ If it is being used by containers, the containers will be removed first.

 Print usage statement

+#### **--time**, **-t**=*time*
+
+Time to wait before forcibly stopping the container. The --force option must be specified to use the --time option.


ditto here if you take similar comments.

TomSweeneyRedHat · 2021-09-28T20:06:22Z

test/test_podman_pods.sh

@@ -114,4 +114,4 @@ podman pod kill foobar
 ########
 # Remove all pods and their containers
 ########
-podman pod rm -fa
+podman pod rm -t 0 -fa


Maybe I missed it, but I don't recall seeing a -t value for anything other than zero. It would be nice to have at least a small handful with values other than zero.

Added some.

Luap99

There is still remote wiring to do, pkg/domain/infra/tunnel does not set the timeout option for the network/volume rm bindings.

Luap99 · 2021-10-04T08:26:06Z

docs/source/markdown/podman-network-rm.1.md

@@ -15,6 +15,10 @@ Delete one or more Podman networks.
 The `force` option will remove all containers that use the named network. If the container is
 running, the container will be stopped and removed.

+#### **--time**, **-t**=*time*


Suggested change

#### **--time**, **-t**=*time*

#### **--time**, **-t**=*seconds*

Please change this to seconds everywhere. A user has no idea what time means.

Luap99 · 2021-10-04T08:26:56Z

pkg/domain/infra/tunnel/containers.go

@@ -193,6 +193,10 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 		}
 		reports := make([]*entities.RmReport, 0, len(ctrs))
 		for _, c := range ctrs {
+			if opts.Force && opts.Timeout != nil {
+				// Ignore errors, containers.Remove will catch them
+				containers.Stop(ic.ClientCtx, c.ID, &containers.StopOptions{Timeout: opts.Timeout})


This can be removed now I think

Add --time flag to podman container rm Add --time flag to podman pod rm Add --time flag to podman volume rm Add --time flag to podman network rm Signed-off-by: Daniel J Walsh <[email protected]>

Luap99

LGTM

openshift-ci · 2021-10-04T19:09:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2021-10-04T19:09:51Z

/lgtm

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2021

rhatdan force-pushed the timeout branch 2 times, most recently from 571823e to ba902af Compare September 28, 2021 01:37

vrothberg reviewed Sep 28, 2021

View reviewed changes

cmd/podman/containers/rm.go Outdated Show resolved Hide resolved

vrothberg reviewed Sep 28, 2021

View reviewed changes

Luap99 reviewed Sep 28, 2021

View reviewed changes

rhatdan force-pushed the timeout branch from dbcbd31 to 800a7eb Compare September 28, 2021 12:59

edsantiago changed the title ~~Add --time out for podman rm -f flag~~ Add --time option for podman rm -f flag Sep 28, 2021

mheon reviewed Sep 28, 2021

View reviewed changes

rhatdan force-pushed the timeout branch 2 times, most recently from bbaa835 to 3c7017e Compare September 28, 2021 13:57

rhatdan force-pushed the timeout branch from 3c7017e to ef87fc0 Compare September 28, 2021 18:03

rhatdan changed the title ~~Add --time option for podman rm -f flag~~ Add --time option for podman * rm -f flag Sep 28, 2021

TomSweeneyRedHat reviewed Sep 28, 2021

View reviewed changes

rhatdan force-pushed the timeout branch 6 times, most recently from b2e7372 to b7df5f9 Compare September 30, 2021 18:47

rhatdan force-pushed the timeout branch 3 times, most recently from 9571746 to 6fe5496 Compare October 1, 2021 18:48

Luap99 requested changes Oct 4, 2021

View reviewed changes

Add --time out for podman * rm -f commands

21c9dc3

Add --time flag to podman container rm Add --time flag to podman pod rm Add --time flag to podman volume rm Add --time flag to podman network rm Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the timeout branch from 6fe5496 to 21c9dc3 Compare October 4, 2021 11:08

Luap99 approved these changes Oct 4, 2021

View reviewed changes

openshift-ci bot assigned Luap99 Oct 4, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2021

openshift-merge-robot merged commit a866a2f into containers:main Oct 4, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --time option for podman * rm -f flag #11763

Add --time option for podman * rm -f flag #11763

rhatdan commented Sep 28, 2021 •

edited

Loading

rhatdan commented Sep 28, 2021

vrothberg left a comment

vrothberg Sep 28, 2021

rhatdan Sep 28, 2021

Luap99 Sep 28, 2021

rhatdan Sep 28, 2021

rhatdan commented Sep 28, 2021

vrothberg commented Sep 28, 2021

edsantiago commented Sep 28, 2021

mheon Sep 28, 2021

mheon Sep 28, 2021

rhatdan Sep 28, 2021

rhatdan Sep 28, 2021

Luap99 Oct 4, 2021

edsantiago commented Sep 28, 2021

rhatdan commented Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

TomSweeneyRedHat Sep 28, 2021

rhatdan Sep 29, 2021

Luap99 left a comment

Luap99 Oct 4, 2021

Luap99 Oct 4, 2021

Luap99 left a comment

openshift-ci bot commented Oct 4, 2021

Luap99 commented Oct 4, 2021

	flags.UintVarP(&stopTimeout, timeFlagName, "t", containerConfig.Engine.StopTimeout, "Seconds to wait for pod stop before killing the container")
	flags.UintVarP(&stopTimeout, timeFlagName, "t", containerConfig.Engine.StopTimeout, "Seconds to wait for the container to stop before removing the volume")

	#### --time, -t=time
	#### --time, -t=seconds

Add --time option for podman * rm -f flag #11763

Add --time option for podman * rm -f flag #11763

Conversation

rhatdan commented Sep 28, 2021 • edited Loading

rhatdan commented Sep 28, 2021

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Sep 28, 2021

vrothberg commented Sep 28, 2021

edsantiago commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Sep 28, 2021

rhatdan commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 4, 2021

Luap99 commented Oct 4, 2021

rhatdan commented Sep 28, 2021 •

edited

Loading