This PR allows users to remove external containers directly #7891

rhatdan · 2020-10-02T12:15:10Z

Currenly if a user specifies the name or ID of an external storage
container, we report an error to them.

buildah from scratch
working-container-2
podman rm working-container-2
Error: no container with name or ID working-container-2 found: no such container

Since the user specified the correct name and the container is in storage we
force them to specify --storage to remove it. This is a bad experience for the
user.

This change will just remove the container from storage. If the container
is known by libpod, it will remove the container from libpod as well.

If the user species the --storage option, and the container is known to libpod
the command will fail.

Also cleaned documented options that are not available to podman-remote.

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

openshift-ci-robot · 2020-10-02T12:15:14Z

[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-10-02T18:11:42Z

pkg/domain/infra/abi/containers.go

+	// Attempt to remove named containers directly from storage, if container is defined in libpod
+	// this will fail, if options.Storage is not specified, code will fall through to removing
+	// the container from libpod.`
+	i := 0


Please just make a new array - the memory savings/speed optimization here is going to be minor and these is a lot less readable than appending failed names to a fresh array.

mheon · 2020-10-02T18:12:19Z

pkg/domain/infra/abi/containers.go

+			report.Err = err
+			if options.Storage {
+				reports = append(reports, &report)
+				return reports, nil


This doesn't seem correct - the old --storage behavior was to try all images, not fail when only one failed.

No it was not, the code, would attempt to remove the nameOrIDs from containerStorage. But now it will do it if the container exists in storage and not in libpod. Making the storage flag useless. I am going to just hide it and remove it from docs.

What I am saying is that the new code will return an error after any single image fails. The old code tried all images, even if one failed.

I think we just remove the storage option altogether, if the image does not exists in libpod, it will report and error now, and it will be removed if it is just in container/storage.

TomSweeneyRedHat · 2020-10-03T00:45:28Z

I'm a bit confused by the change. I thought we wanted to not allow this so that someone would have to think before removing a storage container.... I could see changing the error message to be more explanatory in that regard, but are we all comfortable with just removing a container that wasn't created by Podman? I'm going to think some people will no like that and I'm not sure which is the lesser evil if you will.

rhatdan · 2020-10-03T09:54:27Z

@TomSweeneyRedHat The only way to remove a container would be to fully specify the name. This is not going to happen via --latest or --all. You basically would have to find the name of the container via podman ps --storage or an error message perhaps in podman stop.

Then you need to do podman stop UUID. The chance of someone doing this by mistake is nil.

Currently the UI does

# buildah from alpine
alpine-working-container
# podman rm alpine-working-container
Error: no container with name or ID alpine-working-container found: no such container

Which is a lie, and changing this to something that said to me to add --storage just feels like a bad UI.
It is not like the user just typed these random characters and accidentally hit a container name.

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

Does that example actually work? I seem to remember 'rm --storage' requiring a full ID to work.

…

On Sat, Oct 3, 2020, 05:54 Daniel J Walsh ***@***.***> wrote: @TomSweeneyRedHat <https://github.com/TomSweeneyRedHat> The only way to remove a container would be to fully specify the name. This is not going to happen via --latest or --all. You basically would have to find the name of the container via podman ps --storage or an error message perhaps in podman stop. Then you need to do podman stop UUID. The chance of someone doing this by mistake is nil. Currently the UI does # buildah from alpine alpine-working-container # podman rm alpine-working-container Error: no container with name or ID alpine-working-container found: no such container Which is a lie, and changing this to something that said to me to add --storage just feels like a bad UI. It is not like the user just typed these random characters and accidentally hit a container name. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#7891 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCBUZFH4OFKNJ3IDMUTSI3YF5ANCNFSM4SBT5MWQ> .

rhatdan · 2020-10-04T11:51:50Z

Yes this example works It is the primary reason for this PR.

This test added in this PR makes the change.
@test "podman rm container from storage" {

mheon · 2020-10-08T18:55:28Z

pkg/domain/infra/abi/containers.go

+	if len(tmpNames) < len(names) {
+		names = tmpNames
+	}
+


Should we have a check here to return immediately if len(names) == 0 (everything was a storage container)?

No because we have not processed --all or --latest yet.
But there was a bug below.

TomSweeneyRedHat

LGTM

QiWang19 · 2020-10-08T20:52:55Z

LGTM with mheon's suggestion addressed.

Currenly if a user specifies the name or ID of an external storage container, we report an error to them. buildah from scratch working-container-2 podman rm working-container-2 Error: no container with name or ID working-container-2 found: no such container Since the user specified the correct name and the container is in storage we force them to specify --storage to remove it. This is a bad experience for the user. This change will just remove the container from storage. If the container is known by libpod, it will remove the container from libpod as well. The podman rm --storage option has been deprecated, and removed from docs. Also cleaned documented options that are not available to podman-remote. Signed-off-by: Daniel J Walsh <[email protected]>

mheon · 2020-10-09T13:17:55Z

/lgtm
/hold

rhatdan · 2020-10-09T14:49:52Z

/hold cancel

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

rhatdan force-pushed the rm branch from ddb4fc6 to 842a045 Compare October 2, 2020 13:46

mheon reviewed Oct 2, 2020

View reviewed changes

rhatdan force-pushed the rm branch 4 times, most recently from 00cb33d to da1e691 Compare October 2, 2020 20:36

rhatdan force-pushed the rm branch 2 times, most recently from f45ca63 to 039289b Compare October 7, 2020 18:48

mheon reviewed Oct 8, 2020

View reviewed changes

TomSweeneyRedHat reviewed Oct 8, 2020

View reviewed changes

rhatdan force-pushed the rm branch from 039289b to fea78d5 Compare October 9, 2020 10:07

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

openshift-ci-robot assigned mheon Oct 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 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 Oct 9, 2020

openshift-merge-robot merged commit fa01b83 into containers:master Oct 9, 2020

edsantiago mentioned this pull request Oct 19, 2020

Cirrus CI: agent stopped responding #8068

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

This PR allows users to remove external containers directly #7891

This PR allows users to remove external containers directly #7891

rhatdan commented Oct 2, 2020

openshift-ci-robot commented Oct 2, 2020

mheon Oct 2, 2020

rhatdan Oct 2, 2020

mheon Oct 2, 2020

rhatdan Oct 2, 2020

mheon Oct 2, 2020

rhatdan Oct 2, 2020

TomSweeneyRedHat commented Oct 3, 2020

rhatdan commented Oct 3, 2020

mheon commented Oct 3, 2020 via email

rhatdan commented Oct 4, 2020

mheon Oct 8, 2020

rhatdan Oct 9, 2020

TomSweeneyRedHat left a comment

QiWang19 commented Oct 8, 2020

mheon commented Oct 9, 2020

rhatdan commented Oct 9, 2020

This PR allows users to remove external containers directly #7891

This PR allows users to remove external containers directly #7891

Conversation

rhatdan commented Oct 2, 2020

openshift-ci-robot commented Oct 2, 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

TomSweeneyRedHat commented Oct 3, 2020

rhatdan commented Oct 3, 2020

mheon commented Oct 3, 2020 via email

rhatdan commented Oct 4, 2020

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

QiWang19 commented Oct 8, 2020

mheon commented Oct 9, 2020

rhatdan commented Oct 9, 2020