Add podman system prune and info commands #2252

rhatdan · 2019-02-01T01:42:04Z

We are missing the equivalence of the docker system commands

This patch set adds podman system prune

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

openshift-ci-robot · 2019-02-01T01:42:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rhatdan]

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

baude · 2019-02-01T01:50:03Z

cmd/podman/system_prune.go

+
+func pruneSystemCmd(c *cli.Context) error {
+
+	runtime, err := adapter.GetRuntime(c)


bonus points for using the adapter.Runtime!

baude · 2019-02-01T01:50:48Z

would really like some tests if that is possible

mheon · 2019-02-01T01:50:54Z

cmd/podman/system_prune.go

+			fmt.Println(cid)
+		}
+	}
+	err1 := volumePrune(runtime, getContext(), c.Bool("force"))


I don't think docker system prune actually prunes volumes. The description says exclusively containers, images, networks

It does, see https://docs.docker.com/engine/reference/commandline/system_prune/

There's a --volumes flag.

rhatdan · 2019-02-01T16:10:26Z

/test images

rhatdan · 2019-02-01T16:10:58Z

@vrothberg @mheon @baude @umohnani8 @giuseppe @TomSweeneyRedHat PTAL
@baude Now has tests.

mheon · 2019-02-01T16:13:35Z

cmd/podman/system_prune.go

+	err = pruneContainers(runtime, ctx, shared.Parallelize("rm"), c.Bool("force"))
+	if c.Bool("volumes") {
+		fmt.Println("Deleted Volumes")
+		err1 := volumePrune(runtime, getContext())


Should we be printing IDs here?

volumePrune should print the ids that it is removing.

rhatdan · 2019-02-01T18:34:22Z

/test images

mheon · 2019-02-01T19:42:39Z

LGTM

vrothberg

Nice PR! Just some minor comments regarding --force and making the manpage and help message a bit clearer.

vrothberg · 2019-02-04T08:01:42Z

cmd/podman/system_prune.go

+	pruneSystemFlags = []cli.Flag{
+		cli.BoolFlag{
+			Name:  "all, a",
+			Usage: "remove all unused data",


s/remove/Remove/ for consistency

Fixed these all in the second patch.

vrothberg · 2019-02-04T08:02:14Z

cmd/podman/system_prune.go

+	}
+	pruneSystemCommand = cli.Command{
+		Name:         "prune",
+		Usage:        "Remove unused data",


"data" versus "systems"?

Data is correct. systems was incorrect.

vrothberg · 2019-02-04T08:04:06Z

cmd/podman/system_prune.go

+WARNING! This will remove:
+        - all stopped containers%s
+        - all dangling images
+        - all build cache


Can we add - all volumes if the flag is set?

vrothberg · 2019-02-04T08:04:55Z

cmd/podman/system_prune.go

+
+	ctx := getContext()
+	fmt.Println("Deleted Containers")
+	err = pruneContainers(runtime, ctx, shared.Parallelize("rm"), c.Bool("force"))


s/err/errPruneContainers/

I am not sure if we should use --force here as it should only impact the cli prompt.

vrothberg · 2019-02-04T08:05:32Z

cmd/podman/system_prune.go

+
+	// Call prune; if any cids are returned, print them and then
+	// return err in case an error also came up
+	pruneCids, err1 := runtime.PruneImages(c.Bool("all"))


s/err1/err/PruneImages/ ... just to make the code easier to brain-parse

Ok I changed to use lasterr to make it clearer.

vrothberg · 2019-02-04T08:06:13Z

docs/podman-system-prune.1.md

@@ -0,0 +1,38 @@
+% PODMAN(1) Podman Man Pages
+% Brent Baude
+% December 2018


Copy artifact?

vrothberg · 2019-02-04T08:07:50Z

docs/podman-system-prune.1.md

+does not have any containers based on it.
+
+By default, volumes are not removed to prevent important data from being deleted if there is currently no container using the volume. Use the --volumes flag when running the command to prune volumes as well.
+


Can we define what we mean by systems (i.e., containers, images, build cache)? Same applies to the help message. Currently, it's hard to figure out which data is removed without actually running the command.

This is a global replace issue. System should be Images
Fixed

giuseppe · 2019-02-05T09:37:36Z

LGTM

TomSweeneyRedHat · 2019-02-05T12:17:35Z

cmd/podman/pod_unpause.go

@@ -13,7 +13,7 @@ var (
 	podUnpauseFlags = []cli.Flag{
 		cli.BoolFlag{
 			Name:  "all, a",
-			Usage: "unpause all paused pods",
+			Usage: "Anpause all paused pods",


s/Anpause/Unpause/

TomSweeneyRedHat · 2019-02-05T12:23:48Z

docs/podman-info.1.md

@@ -1,9 +1,9 @@
 % podman-version(1)


Should this be podman-info?

TomSweeneyRedHat · 2019-02-05T12:24:31Z

docs/podman-info.1.md

@@ -1,9 +1,9 @@
 % podman-version(1)

 ## NAME
+podman\-system\-info - Display system information


Should this have it's own page, even if it's mostly a cut/paste job?

No they are identical, This is what we do with all of the podman container commands

podman-container-ps -> podman-ps.
for example. I think we should add the alternate names to the linked images though.

TomSweeneyRedHat · 2019-02-05T12:27:32Z

docs/podman-system.1.md

+
+| Command  | Man Page                                            | Description                                                                  |
+| -------  | --------------------------------------------------- | ---------------------------------------------------------------------------- |
+| info     | [podman-system-info(1)](podman-system-info.1.md)    | Displays Podman related system information.                                  |


I don't think the podman-system-info.1.md exists and I don't see it in this PR.

TomSweeneyRedHat · 2019-02-05T12:28:42Z

docs/podman.1.md

@@ -162,6 +162,7 @@ the exit codes follow the `chroot` standard, see below:
 | [podman-start(1)](podman-start.1.md)      | Starts one or more containers.                                                 |
 | [podman-stats(1)](podman-stats.1.md)      | Display a live stream of one or more container's resource usage statistics.    |
 | [podman-stop(1)](podman-stop.1.md)        | Stop one or more running containers.                                           |
+| [podman-system(1)](podman-system.1.md)    | Manage podman.                                                                 |


I think you should add podman-system-info and podman-system-prune here as well.

We don't add any subcommands on the main man page?

podman container prune is not there so

TomSweeneyRedHat

Any plans to implement podman system df or podman system events to match Docker? Even if they're no-ops?

rhatdan · 2019-02-05T13:29:43Z

Yes we need to add df and events, althoug podman-system-events is just another way of calling podman events, which also does not exist yet.

rh-atomic-bot · 2019-02-05T18:34:21Z

☔ The latest upstream changes (presumably #2227) made this pull request unmergeable. Please resolve the merge conflicts.

We are missing the equivalence of the docker system commands This patch set adds `podman system prune` and `podman system info` Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

We have no consistancy in out option usages and descritions on whether or not the first letter should be capatalized. This patch forces them all to be capatilized. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

openshift-merge-robot · 2019-02-06T14:49:44Z

/retest

vrothberg · 2019-02-06T16:14:04Z

/test validate

baude · 2019-02-06T21:22:51Z

/lgtm

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019

openshift-ci-robot requested review from mheon and TomSweeneyRedHat February 1, 2019 01:42

openshift-ci-robot added the size/L label Feb 1, 2019

rhatdan force-pushed the system branch from cb4c283 to 7837c0d Compare February 1, 2019 01:42

baude reviewed Feb 1, 2019

View reviewed changes

mheon reviewed Feb 1, 2019

View reviewed changes

rhatdan force-pushed the system branch 2 times, most recently from b5c0f01 to 63017fd Compare February 1, 2019 14:12

mheon reviewed Feb 1, 2019

View reviewed changes

vrothberg reviewed Feb 4, 2019

View reviewed changes

rhatdan force-pushed the system branch 2 times, most recently from bd4c286 to fb8a731 Compare February 4, 2019 18:48

TomSweeneyRedHat reviewed Feb 5, 2019

View reviewed changes

rhatdan force-pushed the system branch from fb8a731 to 4a2edb5 Compare February 5, 2019 18:14

Add podman system prune and info commands

74d984e

We are missing the equivalence of the docker system commands This patch set adds `podman system prune` and `podman system info` Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Capatilize all usage and descriptions

0830bb9

We have no consistancy in out option usages and descritions on whether or not the first letter should be capatalized. This patch forces them all to be capatilized. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the system branch from 4a2edb5 to 0830bb9 Compare February 5, 2019 18:42

openshift-ci-robot assigned baude Feb 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2019

openshift-merge-robot merged commit c735072 into containers:master Feb 6, 2019

This was referenced Feb 6, 2019

Fix volume handling in podman #2229

Merged

Add support to migrate containers #2272

Merged

Migrate to cobra CLI #2274

Merged

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 27, 2023

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


		func pruneSystemCmd(c *cli.Context) error {

		runtime, err := adapter.GetRuntime(c)

		does not have any containers based on it.

		By default, volumes are not removed to prevent important data from being deleted if there is currently no container using the volume. Use the --volumes flag when running the command to prune volumes as well.

Add podman system prune and info commands #2252

Add podman system prune and info commands #2252

Conversation

rhatdan commented Feb 1, 2019

openshift-ci-robot commented Feb 1, 2019

Choose a reason for hiding this comment

baude commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

rhatdan commented Feb 1, 2019

rhatdan commented Feb 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Feb 1, 2019

mheon commented Feb 1, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Feb 5, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

rhatdan commented Feb 5, 2019

rh-atomic-bot commented Feb 5, 2019

openshift-merge-robot commented Feb 6, 2019

vrothberg commented Feb 6, 2019

baude commented Feb 6, 2019

vrothberg Feb 1, 2019 •

edited

Loading