Show c/storage (Buildah/CRI-O) containers in ps #7290

rhatdan · 2020-08-11T12:43:22Z

The podman ps --all command will now show containers that
are under the control of other c/storage container systems and
the new ps --external option will show only containers that are
in c/storage but are not controlled by libpod.

In the below examples, the '*working-container' entries were created
by Buildah.

podman ps -a
CONTAINER ID  IMAGE                             COMMAND  CREATED       STATUS                   PORTS  NAMES
9257ef8c786c  docker.io/library/busybox:latest  ls /etc  8 hours ago   Exited (0) 8 hours ago          gifted_jang
d302c81856da  docker.io/library/busybox:latest  buildah  30 hours ago  external                        busybox-working-container
7a5a7b099d33  localhost/tom:latest              ls -alF  30 hours ago  Exited (0) 30 hours ago         hopeful_hellman
01d601fca090  localhost/tom:latest              ls -alf  30 hours ago  Exited (1) 30 hours ago         determined_panini
ee58f429ff26  localhost/tom:latest              buildah  33 hours ago  external                        alpine-working-container

podman ps --external
CONTAINER ID  IMAGE                             COMMAND  CREATED       STATUS    PORTS  NAMES
d302c81856da  docker.io/library/busybox:latest  buildah  30 hours ago  external         busybox-working-container
ee58f429ff26  localhost/tom:latest              buildah  33 hours ago  external         alpine-working-container

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

mheon · 2020-08-11T13:56:55Z

cmd/podman/containers/ps.go

@@ -56,10 +57,10 @@ func init() {
 func listFlagSet(flags *pflag.FlagSet) {
 	flags.BoolVarP(&listOpts.All, "all", "a", false, "Show all the containers, default is only running containers")
 	flags.StringSliceVarP(&filters, "filter", "f", []string{}, "Filter output based on conditions given")
+	flags.BoolVar(&listOpts.External, "external", true, "Show containers in storage not controlled by Podman")


Think we should use --storage here for consistency with podman rm --storage

I am fine with this.

mheon · 2020-08-25T13:25:07Z

libpod/runtime_ctr.go

+// StorageContainers returns a list of containers from containers/storage that
+// are not currently known to Podman.  The list of containers passed in are all
+// of the containers known to Podman from the GetContainers() function.
+func (r *Runtime) StorageContainers() ([]*Container, error) {


We can't make this return Libpod containers - having a bunch of semi-functional things wandering around calling themselves a libpod.Container is asking for trouble (they don't have valid locks, for example, we'll panic the moment someone tries to call a real Libpod function on one of them). I think we need a separate API for this, that returns something like []*StorageContainer - we can translate those into the PS container struct in pkg/domain, so the code in PS will remain unchanged.

TomSweeneyRedHat · 2020-08-25T18:33:29Z

docs/source/markdown/podman-ps.1.md

+
+Display containers that are not controlled by Podman but are stored in containers storage.  These containers
+are generally created via other container technology such as Buildah or CRI-O and may depend on the
+same container images that Podman is using.  These containers are denoted with a 'NA' in the COMMAND and


Does the 'NA' still apply? Your example shows 'Buildah`

Nope, good catch.

TomSweeneyRedHat · 2020-08-25T18:34:13Z

docs/source/markdown/podman-ps.1.md

@@ -32,12 +32,19 @@ all the containers information.  By default it lists:

 **--all**, **-a**

-Show all the containers, default is only running containers
+Show all the containers including those created by other container technologies such as Buildah and CRI-O.  The default displays only running containers created by Podman.  Containers not created by Podman have an 'NA' status.


ditto 'NA' as to below.

mheon · 2020-08-25T22:11:34Z

libpod/runtime_ctr.go

+
+	// We only have a store when doing all, storage or size, so skip this otherwise.
+	if r.store == nil {
+		return retCtrs, nil


This should probably be an error

Are you sure? All of the other checks ignore this issue and just return success?

Having no store is something that we deliberately did ourselves - we should never be calling this in a case where the store is not set up, so returning an error seems correct

mheon · 2020-08-25T22:12:17Z

libpod/runtime_ctr.go

+		return retCtrs, errors.Wrapf(err, "error reading list of all containers")
+	}
+	for _, container := range storeContainers {
+		if _, err := r.state.Container(container.ID); err == nil {


Can use HasContainer instead - is a lot faster

TomSweeneyRedHat · 2020-08-26T17:38:44Z

@rhatdan gating issue, looks like a man page needs tweaking.

edsantiago

I've been wanting this feature for a long time, thank you!

I'd love to see tests, but that's hard: you need to interrupt a podman-build in just the right way. I'll see if I can figure it out for a followup PR.

edsantiago · 2020-09-01T12:05:12Z

/hold

Possible bug. Will report in full in a few minutes, but I need time to investigate. Please do not merge yet.

edsantiago

Logic bug. I happened to have leftover buildah containers on my system; as submitted, this PR was only showing them when I specified --all but not with --storage. Trivial fix.

(And, if you agree with my "non-running" doc change below, please include that too)

edsantiago · 2020-09-01T12:10:26Z

cmd/podman/containers/ps.go

@@ -56,9 +57,9 @@ func init() {
 func listFlagSet(flags *pflag.FlagSet) {
 	flags.BoolVarP(&listOpts.All, "all", "a", false, "Show all the containers, default is only running containers")
 	flags.StringSliceVarP(&filters, "filter", "f", []string{}, "Filter output based on conditions given")
+	flags.BoolVar(&listOpts.Storage, "storage", true, "Show containers in storage not controlled by Podman")


Should default to false

edsantiago · 2020-09-01T12:10:47Z

pkg/ps/ps.go

@@ -68,7 +69,31 @@ func GetContainerLists(runtime *libpod.Runtime, options entities.ContainerListOp
 			return nil, err
 		}
 		pss = append(pss, listCon)
+	}
+
+	if options.All && options.Storage {


Should be ||, not &&'

openshift-ci-robot · 2020-09-01T12:13: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

mheon · 2020-09-01T12:57:07Z

I'd prefer it if we took the all flag out of the picture entirely, and only gated this behind the storage flag.

…

On Tue, Sep 1, 2020, 08:13 OpenShift CI Robot ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *APPROVED* This pull-request has been approved by: *rhatdan <#7290#>* The full list of commands accepted by this bot can be found here <https://go.k8s.io/bot-commands?repo=containers%2Fpodman>. The pull request process is described here <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> Needs approval from an approver in each of these files: - OWNERS <https://github.com/containers/podman/blob/master/OWNERS> [rhatdan] Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7290 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCBNKYMPN2TXLB6YYHLSDTQOLANCNFSM4P274LFA> .

edsantiago · 2020-09-01T13:14:23Z

I'd prefer it if we took the all flag out of the picture entirely

I was ambivalent at first -- it does significantly change the behavior of ps -a -- but the more I think about it the more I really want it in ps -a.

Disturbingly often I find myself in annoying situations where podman rm and rmi start complaining at me about "in use by a container" or something like that. podman ps -a shows nothing. I've trained myself to run buildah containers and buildah rm, and that usually fixes things (typically leftovers from a failed podman build). I think normal humans might not knoow to do this, and would appreciate ps -a showing something useful.

This still leaves the problem of removing those containers. podman rm and rm --storage aren't actually very helpful here. But we can address that later.

mheon · 2020-09-01T13:24:29Z

I would really prefer the opposite, because I don't want people to see these containers and then try using them with other Podman commands. The output of ps may not be as complete as it should be, but I strongly suspect people will not be able to identify what is a storage container and what is a Podman container on first glance, and they will try and interact with these containers using the standard Podman commands (podman start, podman rm, podman inspect, etc) and will get a series of wonderfully contradictory "that container does not exist" errors. We need to make this an explicit thing people opt into to prevent normal operation of Podman from becoming horribly confusing.

This is really a no-win scenario - we can either show them and confuse people as to why they don't act like normal Podman containers, or not show them and confuse people as to why they see errors that their image is "in use".

edsantiago · 2020-09-01T13:43:32Z

That's a good point. I defer to you in this matter. And I agree that it's a no-win.

edsantiago · 2020-09-01T14:23:00Z

Proposed test: run a long build with a short timeout:

@test "podman ps --storage" {
    PODMAN_TIMEOUT=5 run_podman 124 build -t thiswillneverexist - <<EOF
FROM $IMAGE
RUN sleep 15
EOF

    run_podman ps --storage
    is "${lines[1]}" \
       "[0-9a-f]\{12\} \+$IMAGE *buildah .* seconds ago .* storage .* ${PODMAN_TEST_IMAGE_NAME}-working-container" \
       "podman ps --storage"

    run_podman rm --storage -f "${lines[1]:0:12}"
}

Requires minor surgery to helpers.bash, so please don't include in your PR. I'm commenting here just to see if anyone has better ideas on how to test. I don't like this because it adds 15 seconds to the already-long system test run time. Any other ideas?

rhatdan · 2020-09-02T11:08:16Z

We can discuss this next week, but I am preparing to make these containers first class within podman, IE allow podman ps -a to show them by default. Allow podman rm to remove them by default. Allow podman diff to show the difference by default (Something requested by CRI-O).

Users do not understand these and do not understand how to use them. If we make --storage the default and properly show that they are not libpod containers, I believe the user experience will be better for the users.

$ ./bin/podman ps -a
CONTAINER ID  IMAGE   COMMAND  CREATED  STATUS  PORTS   NAMES
$ buildah from alpine
alpine-working-container
 $ ./bin/podman ps -a
CONTAINER ID  IMAGE                            COMMAND  CREATED        STATUS   PORTS   NAMES
84c690fc961c  docker.io/library/alpine:latest  buildah  2 seconds ago  storage          alpine-working-container
$ ./bin/podman rm alpine-working-container
alpine-working-container

These containers are unknown by libpod but Podman should be able to deal with them.
We can allow the storage flag to be set in containers.conf to allow us to get back to the current behaviour if users and distros want, but I think that these containers not being usable by default is a User Interface failure.

TomSweeneyRedHat · 2020-09-02T19:08:14Z

I second the thought of a post scrum chat on how ps should show the variety of containers next week, fwiw.

mheon · 2020-09-02T19:19:58Z

Concur. I still have serious doubts (we can maybe get a few commands to work in an OK-ish way, but the vast majority of Podman will never work with these containers)

vrothberg · 2020-09-07T13:47:46Z

Just a friendly ping to bump the issue up in the mailboxes. Let's chat in tomorrows water cooler.

rhatdan · 2020-09-08T20:14:37Z

@vrothberg @giuseppe @mheon @baude @jwhonce @ashley-cui @QiWang19 This is less controversial now since the --format is defaulted to false.

The `podman ps --all` command will now show containers that are under the control of other c/storage container systems and the new `ps --storage` option will show only containers that are in c/storage but are not controlled by libpod. In the below examples, the '*working-container' entries were created by Buildah. ``` podman ps -a CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES 9257ef8c786c docker.io/library/busybox:latest ls /etc 8 hours ago Exited (0) 8 hours ago gifted_jang d302c81856da docker.io/library/busybox:latest buildah 30 hours ago storage busybox-working-container 7a5a7b099d33 localhost/tom:latest ls -alF 30 hours ago Exited (0) 30 hours ago hopeful_hellman 01d601fca090 localhost/tom:latest ls -alf 30 hours ago Exited (1) 30 hours ago determined_panini ee58f429ff26 localhost/tom:latest buildah 33 hours ago storage alpine-working-container podman ps --external CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES d302c81856da docker.io/library/busybox:latest buildah 30 hours ago external busybox-working-container ee58f429ff26 localhost/tom:latest buildah 33 hours ago external alpine-working-container ``` Signed-off-by: TomSweeneyRedHat <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2020-09-09T10:11:28Z

I added more documentation to explain what these are to the podman build man page and tried to standardize on the term external containers in all of the effected man pages.

mheon · 2020-09-09T14:02:34Z

LGTM

mheon · 2020-09-09T14:02:44Z

Restarted two tests (hopefully flakes)

vrothberg

/lgtm

vrothberg · 2020-09-09T16:13:03Z

/hold cancel

openshift-ci-robot requested review from baude and mrunalp August 11, 2020 12:43

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2020

rhatdan mentioned this pull request Aug 11, 2020

WIP - Show c/storage (Buildah/CRI-O) containers in ps #4053

Closed

rhatdan force-pushed the external branch from 6644905 to 62bf5f0 Compare August 11, 2020 12:52

rhatdan changed the title ~~Show c/storage (Buildah/CRI-O) containers in ps~~ [WIP] Show c/storage (Buildah/CRI-O) containers in ps Aug 11, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2020

rhatdan force-pushed the external branch from 62bf5f0 to cc808dd Compare August 11, 2020 12:57

mheon reviewed Aug 11, 2020

View reviewed changes

rhatdan force-pushed the external branch 4 times, most recently from d25a551 to aeef06e Compare August 16, 2020 11:42

rhatdan force-pushed the external branch 3 times, most recently from ff2b7a7 to 985e7aa Compare August 25, 2020 12:19

rhatdan changed the title ~~[WIP] Show c/storage (Buildah/CRI-O) containers in ps~~ Show c/storage (Buildah/CRI-O) containers in ps Aug 25, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2020

mheon reviewed Aug 25, 2020

View reviewed changes

rhatdan force-pushed the external branch from 985e7aa to f6c0bbb Compare August 25, 2020 17:44

TomSweeneyRedHat reviewed Aug 25, 2020

View reviewed changes

rhatdan force-pushed the external branch from f6c0bbb to 0d881ab Compare August 25, 2020 22:10

mheon reviewed Aug 25, 2020

View reviewed changes

rhatdan force-pushed the external branch 3 times, most recently from f1686ce to 4cc6c75 Compare August 26, 2020 10:06

rhatdan force-pushed the external branch from 4cc6c75 to de3e870 Compare August 26, 2020 21:41

edsantiago approved these changes Sep 1, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020

edsantiago suggested changes Sep 1, 2020

View reviewed changes

rhatdan force-pushed the external branch from ef0fc09 to 2c8e24e Compare September 8, 2020 17:37

rhatdan force-pushed the external branch from 2c8e24e to 581afbb Compare September 9, 2020 10:10

vrothberg reviewed Sep 9, 2020

View reviewed changes

openshift-ci-robot assigned vrothberg Sep 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2020

openshift-merge-robot merged commit 49cb0ed into containers:master Sep 9, 2020

edsantiago mentioned this pull request Sep 11, 2020

APIv2: DELETE libpod/containers/foo?force=true: unlinkat: directory not empty #7535

Closed

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show c/storage (Buildah/CRI-O) containers in ps #7290

Show c/storage (Buildah/CRI-O) containers in ps #7290

rhatdan commented Aug 11, 2020

mheon Aug 11, 2020

rhatdan Aug 11, 2020

mheon Aug 25, 2020

TomSweeneyRedHat Aug 25, 2020

rhatdan Aug 25, 2020

TomSweeneyRedHat Aug 25, 2020

mheon Aug 25, 2020

rhatdan Aug 26, 2020

mheon Aug 26, 2020

mheon Aug 25, 2020

TomSweeneyRedHat commented Aug 26, 2020

edsantiago left a comment

edsantiago commented Sep 1, 2020

edsantiago left a comment

edsantiago Sep 1, 2020

edsantiago Sep 1, 2020

openshift-ci-robot commented Sep 1, 2020

mheon commented Sep 1, 2020 via email

edsantiago commented Sep 1, 2020

mheon commented Sep 1, 2020

edsantiago commented Sep 1, 2020

edsantiago commented Sep 1, 2020

rhatdan commented Sep 2, 2020

TomSweeneyRedHat commented Sep 2, 2020

mheon commented Sep 2, 2020

vrothberg commented Sep 7, 2020 •

edited

Loading

rhatdan commented Sep 8, 2020

rhatdan commented Sep 9, 2020

mheon commented Sep 9, 2020

mheon commented Sep 9, 2020

vrothberg left a comment

vrothberg commented Sep 9, 2020

Show c/storage (Buildah/CRI-O) containers in ps #7290

Show c/storage (Buildah/CRI-O) containers in ps #7290

Conversation

rhatdan commented Aug 11, 2020

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 commented Aug 26, 2020

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago commented Sep 1, 2020

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 1, 2020

mheon commented Sep 1, 2020 via email

edsantiago commented Sep 1, 2020

mheon commented Sep 1, 2020

edsantiago commented Sep 1, 2020

edsantiago commented Sep 1, 2020

rhatdan commented Sep 2, 2020

TomSweeneyRedHat commented Sep 2, 2020

mheon commented Sep 2, 2020

vrothberg commented Sep 7, 2020 • edited Loading

rhatdan commented Sep 8, 2020

rhatdan commented Sep 9, 2020

mheon commented Sep 9, 2020

mheon commented Sep 9, 2020

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Sep 9, 2020

vrothberg commented Sep 7, 2020 •

edited

Loading