fix podman container exists and diff for storage containers #7908

rhatdan · 2020-10-03T11:28:14Z

Current these commands only check if a container exists in libpod. With
this fix, the commands will also check if they are in containers/storage.

This allows users to look at differences within a buildah or CRI-O container.

Currently buildah diff does not exists, so this helps out in that situation
as well as in CRI-O since the cri does not implement a diff command.

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

rhatdan · 2020-10-03T11:28:34Z

@mrunalp PTAL
@containers/podman-maintainers PTAL

edsantiago · 2020-10-03T11:52:31Z

test/system/030-run.bats

+    run_podman container exists myctr
+
+    # Create a container that podman does not know about
+    run buildah from $IMAGE


We cannot guarantee having buildah in the test environment. I have a test for this (storage containers) in the works, but (sigh) it's in a side branch and I forgot to submit it last week. Basically it runs 'podman build', with a RUN sleep 15 but with PODMAN_TIMEOUT=5 so the build is likely to leave behind a buildah container. Unfortunately it also requires other changes to helpers.bash. May I submit that next week?

What do we need to do to make sure buildah is installed? It would seem a better solution especially as I add more container/storage features into podman,

in CI: go through the build-new-VMs dance. In Fedora/RHEL: add Requires: buildah to the -tests subpackage.

@edsantiago is this OK now?

Tests would not pass if it was not.

It's OK in CI, but gating tests are failing because of missing buildah. On my list for today.

mheon · 2020-10-03T13:36:35Z

I do not agree with 'exists'. Openstack requested that command to make sure containers were present and *ready to use* so they did not need to recreate them. We are breaking the intent of the command if it reports that a container is present but said container is Buildah or CRI-O.

…

On Sat, Oct 3, 2020, 08:34 Ed Santiago ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/system/030-run.bats <#7908 (comment)>: > @@ -432,4 +432,19 @@ json-file | f is "$output" "$expect" "podman run with --tz=local, matches host" } ***@***.*** "podman container exists" { + rand=$(random_string 30) + run_podman 1 container exists myctr + + run_podman create --name myctr $IMAGE /bin/true + run_podman container exists myctr + + # Create a container that podman does not know about + run buildah from $IMAGE in CI: go through the build-new-VMs dance. In Fedora/RHEL: add Requires: buildah to the -tests subpackage. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#7908 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCC34VW3VOUA2DKW54LSI4K5HANCNFSM4SCTFPVQ> .

rhatdan · 2020-10-04T11:17:41Z

Again, these will not happen by accident, you have to check by name if a container exists. I would be willing to add a flag to say we want a specific type, (But I would guess no one would ever use it, but by default we should do what human beings would expect).

mheon · 2020-10-05T00:50:57Z

It looks like Github managed to lose my response email, so pasting here:

My concern is that the exists command will answer true after a partially failed deletion, where we remove the container in the database but fail to remove it in c/storage. This has come up on a regular basis in Openstack. They use scripts to check that containers exist and work with them, recreating if necessary. I think what you are doing has real potential to break this workflow.

rhatdan · 2020-10-15T11:42:58Z

@edsantiago this tests just hit the timeout also "8 seconds"

rhatdan · 2020-10-15T13:10:19Z

@containers/podman-maintainers PTAL
This looks like it is ready to merge.

rhatdan · 2020-10-15T13:10:56Z

@mheon PTAL, this now does not do exists by default, you have to specify --external.

mheon · 2020-10-15T13:20:30Z

LGTM

TomSweeneyRedHat · 2020-10-15T13:36:11Z

cmd/podman/containers/exists.go

@@ -12,10 +12,10 @@ var (
 	containerExistsDescription = `If the named container exists in local storage, podman container exists exits with 0, otherwise the exit code will be 1.`

 	existsCommand = &cobra.Command{
-		Use:   "exists CONTAINER",
+		Use:   "exists [flags] CONTAINER",


You've 'flags' here and 'options' in the man page, I think it should be options everywhere.

Suggested change

Use: "exists [flags] CONTAINER",

Use: "exists [options] CONTAINER",

Let's do this in a different PR. We exchange flags and options all over the place, and one of the tests is looking for flags.

grep -r Use.*'[flags' cmd | wc -l
82

TomSweeneyRedHat · 2020-10-15T13:37:07Z

completions/bash/podman

@@ -2696,7 +2695,7 @@ _podman_ps() {
 	  --pod -p
 	  --quiet -q
 	  --size -s
-	  --storage
+	  --external


since you've another nit to fix, can you alphabetize this please? Also namespace on the line below.

I would prefer to do this in a separate PR. Lets go through the entire contrib and alphabetize it all.

TomSweeneyRedHat · 2020-10-15T13:38:15Z

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


 **--pod**, **-p**

 Display the pods the containers are associated with

-**--storage**
+**--external**


alphabetize this option please

TomSweeneyRedHat · 2020-10-15T13:42:10Z

docs/source/markdown/podman-container-exists.1.md

@@ -17,22 +17,30 @@ was an issue accessing the local storage.
 **-h**, **--help**
 Print usage statement

+**--external**=*true|false*


move above help please

TomSweeneyRedHat · 2020-10-15T13:46:31Z

I'm fine with picking the man page nits in another PR. If @edsantiago thinks the testing end is gtg, then LGTM.

Current these commands only check if a container exists in libpod. With this fix, the commands will also check if they are in containers/storage. This allows users to look at differences within a buildah or CRI-O container. Currently buildah diff does not exists, so this helps out in that situation as well as in CRI-O since the cri does not implement a diff command. Signed-off-by: Daniel J Walsh <[email protected]>

External containers are containers created outside of Podman. For example Buildah and CRI-O Containers. $ buildah from alpine alpine-working-container $ buildah run alpine-working-container touch /test $ podman container exists --external alpine-working-container $ podman container diff alpine-working-container C /etc A /test Added --external flag to refer to external containers, rather then --storage. Added --external for podman container exists and modified podman ps to use --external rather then --storage. It was felt that --storage would confuse the user into thinking about changing the storage driver or options. --storage is still supported through the use of aliases. Finally podman contianer diff, does not require the --external flag, since it there is little change of users making the mistake, and would just be a pain for the user to remember the flag. podman container exists --external is required because it could fool scripts that rely on the existance of a Podman container, and there is a potential for a partial deletion of a container, which could mess up existing users. Signed-off-by: Daniel J Walsh <[email protected]>

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

rhatdan · 2020-10-16T13:26:16Z

@containers/podman-maintainers @saschagrunert This is ready to merge.

saschagrunert

/lgtm

openshift-ci-robot · 2020-10-19T06:44:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

rhatdan force-pushed the diff branch from 1bb6871 to 12c870a Compare October 3, 2020 11:31

edsantiago reviewed Oct 3, 2020

View reviewed changes

rhatdan force-pushed the diff branch 2 times, most recently from da923e5 to 8509107 Compare October 5, 2020 00:49

rhatdan force-pushed the diff branch 4 times, most recently from 43c892e to c396443 Compare October 12, 2020 11:33

rhatdan force-pushed the diff branch from c396443 to d4c0a3a Compare October 15, 2020 09:07

TomSweeneyRedHat reviewed Oct 15, 2020

View reviewed changes

rhatdan added 3 commits October 15, 2020 09:51

Fix sorting issues in completions

571ae9d

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

rhatdan force-pushed the diff branch from d4c0a3a to 571ae9d Compare October 15, 2020 14:18

saschagrunert approved these changes Oct 19, 2020

View reviewed changes

openshift-ci-robot assigned saschagrunert Oct 19, 2020

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

openshift-merge-robot merged commit 7ffcab0 into containers:master Oct 19, 2020

edsantiago mentioned this pull request Oct 19, 2020

Cirrus CI: agent stopped responding #8068

Closed

edsantiago mentioned this pull request Feb 24, 2021

rawhide: podman diff of buildah container: error creating overlay mount #9507

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

fix podman container exists and diff for storage containers #7908

fix podman container exists and diff for storage containers #7908

rhatdan commented Oct 3, 2020

rhatdan commented Oct 3, 2020

edsantiago Oct 3, 2020

rhatdan Oct 3, 2020

edsantiago Oct 3, 2020

TomSweeneyRedHat Oct 15, 2020

rhatdan Oct 15, 2020

edsantiago Oct 19, 2020

mheon commented Oct 3, 2020 via email

rhatdan commented Oct 4, 2020

mheon commented Oct 5, 2020

rhatdan commented Oct 15, 2020

rhatdan commented Oct 15, 2020

rhatdan commented Oct 15, 2020

mheon commented Oct 15, 2020

TomSweeneyRedHat Oct 15, 2020

rhatdan Oct 15, 2020

rhatdan Oct 15, 2020

TomSweeneyRedHat Oct 15, 2020

rhatdan Oct 15, 2020

TomSweeneyRedHat Oct 15, 2020

rhatdan Oct 15, 2020

TomSweeneyRedHat Oct 15, 2020

rhatdan Oct 15, 2020

TomSweeneyRedHat commented Oct 15, 2020

rhatdan commented Oct 16, 2020

saschagrunert left a comment

openshift-ci-robot commented Oct 19, 2020

	Use: "exists [flags] CONTAINER",
	Use: "exists [options] CONTAINER",

fix podman container exists and diff for storage containers #7908

fix podman container exists and diff for storage containers #7908

Conversation

rhatdan commented Oct 3, 2020

rhatdan commented Oct 3, 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

mheon commented Oct 3, 2020 via email

rhatdan commented Oct 4, 2020

mheon commented Oct 5, 2020

rhatdan commented Oct 15, 2020

rhatdan commented Oct 15, 2020

rhatdan commented Oct 15, 2020

mheon commented Oct 15, 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

TomSweeneyRedHat commented Oct 15, 2020

rhatdan commented Oct 16, 2020

saschagrunert left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 19, 2020