WIP - Show c/storage (Buildah/CRI-O) containers in ps #4053
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
|
Can they be treated as if they belong to namespaces? eg. podman --namespace=crio ps |
|
bot, retest this please. |
libpod/runtime_ctr.go
Outdated
libpod/runtime_ctr.go
Outdated
There was a problem hiding this comment.
Horrible Function name. GetNonLibpodContainers? GetExternalContainers?
pkg/adapter/runtime.go
Outdated
There was a problem hiding this comment.
Just return r.Runtime.ImageRuntime().GetImage(imageID)
There was a problem hiding this comment.
Actually looking at it further, I don't need this function and don't call it. Backing this change out.
pkg/adapter/runtime_remote.go
Outdated
|
I hate the name of the option. I would figure --storage would be to return all containers in storage and Can we change it to --external podman ps -a --storage external @AkihiroSuda has a good idea, what the user would really want is something like --namespace buildah or --namespace podman --namespace CRI-O, but all we can identify right now is whether or not the container was created via libpod or not. Since Buildah and CRI-O do not use LIBPOD yet, we have no good way to identify where the storage containers came from. |
|
We already have a |
|
--namespace undefined? --namespace external?
If CRI-O eventually grows libpod support
|
|
|
|
Also, I've just taken a brief look at the PR, and I'm pretty concerned that the logic here is leaking into libpod. Libpod should never see a container not in the DB. We added separate functions for dealing with c/storage containers in |
libpod/runtime_ctr.go
Outdated
There was a problem hiding this comment.
I would not expect building containers like this to work; soe of this, like lock allocation, has some definite undesirable side-effects.
We should confine the storage logic to cmd/podman, and ideally keep it separate from the rest of ps. If a storage container hits Libpod, I do not expect good things to happen.
|
I've moved the external container list building logic from libpod into the podman ps logic. I still need to test further, especially the remote stuff, but thought I'd put this up for review. I've also renamed the option to |
|
We should discuss this more on Monday - I still don't think that returning a I'd honestly prefer if we did not reuse the display logic from the rest of |
|
Postscrum chat sounds good to me. FWIW, the main reason I'm stuffing a storage container into a libpod container object is so that it can be shown in the The other approach would be to just forget about showing the storage containers with the --all command. But I think that's helpful for some one looking for one of these containers after they can't remove an image that has a storage container and not a libpod container still attached. FWIW, I think I've found the remote compilation issue. I'd to update a struct in |
|
I think removing display in --all sounds fine; I'd prefer not to
intermingle containers from libpod and not from libpod. Would like to get a
consensus with Dan and Brent though.
…On Sun, Sep 22, 2019, 13:38 Tom Sweeney ***@***.***> wrote:
Postscrum chat sounds good to me.
FWIW, the main reason I'm stuffing a storage container into a libpod
container object is so that it can be shown in the ps --all and go
through any filtering that a user asks to be done. Assuming we keep this
approach, is there something I should do to deallocate the locks at the end
of the run? I assumed, maybe badly, that they'd just go away at the end of
the run when the in memory objects got cleaned up.
The other approach would be to just forget about showing the storage
containers with the --all command. But I think that's helpful for some one
looking for one of these containers after they can't remove an image that
has a storage container and not a libpod container still attached.
FWIW, I think I've found the remote compilation issue. I'd to update a
struct in vi ./cmd/podman/shared/container.go. 'make podman-remote'
worked in my sandbox, does the CI do other compiling/linking? It failed as
I'd not included External to the PsOpts struct in that file.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4053>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB3AOCATTEVXC4DKTJU5IMDQK6UPVANCNFSM4IXQKN3Q>
.
|
|
☔ The latest upstream changes (presumably #4355) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@TomSweeneyRedHat Needs a rebase. Is this something we should talk about at the f2f? |
|
I think we're all set talk wise, I just need to carve out some time and get back to this. |
|
This pull request had no activity for 30 days. In the absence of activity or the "do-not-close" label, the pull request will be automatically closed within 7 days. |
openshift-ci-robot
added
the
needs-rebase
|
@TomSweeneyRedHat I guess this has bubbled back up to importance, at least do a rebase, Please. |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@TomSweeneyRedHat Could you rebase this one, and we can start the conversation again. |
Ready for review. I still have some work to do with testing, varlink and documentation. But thought I'd get this out for a look/feel kind of review at the moment. 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 NA 30 hours ago NA 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 NA 33 hours ago NA alpine-working-container podman ps --external CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES d302c81856da docker.io/library/busybox:latest NA 30 hours ago NA busybox-working-container ee58f429ff26 localhost/tom:latest NA 33 hours ago NA alpine-working-container ``` Signed-off-by: TomSweeneyRedHat <tsweeney@redhat.com>
openshift-ci-robot
removed
the
needs-rebase
|
I've just rebased this, no changes yet, so please don't review. |
|
☔ The latest upstream changes (presumably #4953) made this pull request unmergeable. Please resolve the merge conflicts. |
openshift-ci-robot
added
the
needs-rebase
|
@TomSweeneyRedHat: PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Replacing with #7290 |
github-actions
bot
added
the
locked - please file new issue/PR
7 participants
WIP - Show c/storage (Buildah/CRI-O) containers in ps
Ready for review. I still have some work to do with testing, varlink and documentation. But thought I'd get this out for a look/feel kind of review at the moment.
The
podman ps --allcommand will now show containers that are under the control of other c/storage container systems and the newps --externaloption will show only containers that arein c/storage but are not controlled by libpod.
In the below examples, the '*working-container' entries were created by Buildah.
Signed-off-by: TomSweeneyRedHat tsweeney@redhat.com