Fix handling of container remove #9014

rhatdan · 2021-01-18T20:00:49Z

I found several problems with container remove

podman-remote rm --all
Was not handled

podman-remote rm --ignore
Was not handled

Return better errors when attempting to remove an --external container.
Currently we return the container does not exists, as opposed to container
is an external container that is being used.

This patch also consolidates the tunnel code to use the same code for
removing the container, as the local API, removing duplication of code
and potential problems.

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

openshift-ci-robot · 2021-01-18T20:00:51Z

[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

rhatdan · 2021-01-18T20:01:48Z

Fixes: #8735

rhatdan · 2021-01-18T20:01:59Z

@edsantiago PTAL

edsantiago · 2021-01-18T20:19:47Z

pkg/bindings/containers/types.go

@@ -122,6 +122,8 @@ type PruneOptions struct {
 //go:generate go run ../generator/generator.go RemoveOptions
 // RemoveOptions are optional options for removing containers
 type RemoveOptions struct {
+	All     *bool
+	Ignore  *bool
 	Force   *bool


I'm going to assume that Tom will ask you to sort those

edsantiago · 2021-01-18T20:24:43Z

podman-remote rm --all Was not handled

That's quite a hole, I'm trying to figure out how the system tests didn't detect it. Thanks for catching it.

edsantiago · 2021-01-18T20:36:06Z

Friendly suggestion for system tests:

diff --git a/test/system/040-ps.bats b/test/system/040-ps.bats
index 0447122b1..0ae8b0ce0 100644
--- a/test/system/040-ps.bats
+++ b/test/system/040-ps.bats
@@ -111,8 +111,11 @@ EOF
     run_podman ps --storage -a
     is "${#lines[@]}" "2" "podman ps -a --storage sees buildah container"

-    # This is what deletes the container
-    # FIXME: why doesn't "podman rm --storage $cid" do anything?
+    # We can't rm it without -f, but podman should issue a helpful message
+    run_podman 2 rm "$cid"
+    is "$output" "Error: container .* is mounted and cannot be removed without using force: container state improper" "podman rm <buildah container> without -f"
+
+    # With -f, we can remove it.
     run_podman rm -f "$cid"

     run_podman ps --storage -a

Is the exit status (2) intentional? I was surprised that it wasn't 125.

edsantiago · 2021-01-18T20:36:31Z

OBTW CI is failing

mheon · 2021-01-18T22:13:41Z

pkg/api/handlers/compat/containers.go

@@ -29,9 +31,11 @@ import (
 func RemoveContainer(w http.ResponseWriter, r *http.Request) {


This is the compat endpoint. We shouldn't be modifying its parameters.

This is the only endpoint we have for removing containers. All of the Docker parameters still work, is it invalid to use additional libpod params on a compat endpoint?

@baude @jwhonce WDYT?

You can make it conditional which is something other endpoints are doing. utils.IsLibpodRequest(r) returns a boolean that indicates if the request was made against a libpod/... endpoint.

mheon · 2021-01-18T22:19:03Z

pkg/domain/infra/abi/containers.go

 			reports = append(reports, &report)
+		case define.ErrNoSuchCtr:
+			// remove container names that does not exist
+			reports = append(reports, &report)


This seems dubious - podman rm doesnotexist needs to throw an error, and I'm not sure it does after this.

Also, I think this is actually unsafe - RemoveStorageContainer can potentially return ErrNoSuchCtr in cases where the container does not exist in c/storage but still exists in Libpod

Ok, I am not sure that is possible, but I can change this to fall though, since it does not change the error.

We should not be erroring in that case, we should be proceeding to normal RemoveContainer without appending the report.

Right it does not add the report now.

mheon · 2021-01-18T22:21:57Z

pkg/domain/infra/abi/containers.go

+			// remove container names that does not exist
+			reports = append(reports, &report)
+		default:
+			if _, err := ic.Libpod.LookupContainer(ctr); errors.Cause(err) == define.ErrNoSuchCtr {


This bit seems unnecessary - I don't see any way we fail in a fashion that's not ErrNoSuchCtr before RemoveStorageContainer checks Libpod to see if the container exists.

The use case we are trying to fix #8735, is the case where we created a buildah container that is in use, and fails. This would end up in this block, and now we check to see if it is a libpod container. Since it is not, we return the error.
Thus we end up with an error telling us the container is in use, and the user has to add --force.
In the current code we fail over to trying to remove a libpod container, and we report
container does not exist.

I'm not sure you're right here - the check for whether a container is a Libpod container in RemoveStorageContainer happens before we try to remove the container, so we should already have gotten an ErrCtrExists

If you run @edsantiago test you will see this happen.

Then we need to catch this inside RemoveStorageContainer - why work around broken behavior when we can just fix the broken API

RemoveStorageContainer is not broken, it is returning a valid error, that we are returning to the user. The problem is we are ignoring the error and then falling through to the libpod remove, which finds that there is no Container libpod container, and reports container does not exists.

I found several problems with container remove podman-remote rm --all Was not handled podman-remote rm --ignore Was not handled Return better errors when attempting to remove an --external container. Currently we return the container does not exists, as opposed to container is an external container that is being used. This patch also consolidates the tunnel code to use the same code for removing the container, as the local API, removing duplication of code and potential problems. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2021-01-20T20:02:45Z

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

pkg/domain/infra/abi/containers.go

jwhonce · 2021-01-20T22:30:06Z

LGTM

TomSweeneyRedHat · 2021-01-20T22:45:29Z

pkg/api/handlers/compat/containers.go

+		All:     query.All,
+		Force:   query.Force,
+		Volumes: query.Volumes,
+		Ignore:  query.Ignore,


nit, alpha order this plz.

TomSweeneyRedHat · 2021-01-20T22:46:54Z

LGTM
I'm going to push this one over the finish line as I know you've another one tagging behind this. Please fix my nit and address any remaining concerns from @mheon in that PR.

TomSweeneyRedHat · 2021-01-20T22:47:00Z

/lgtm

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2021

edsantiago reviewed Jan 18, 2021

View reviewed changes

mheon reviewed Jan 18, 2021

View reviewed changes

rhatdan force-pushed the rm branch 3 times, most recently from 7a0597d to 4edbc81 Compare January 20, 2021 11:26

rhatdan force-pushed the rm branch from 4edbc81 to e7df73e Compare January 20, 2021 14:10

edsantiago mentioned this pull request Jan 20, 2021

flake: error reading /etc/cni/net.d/something #9041

Closed

mheon reviewed Jan 20, 2021

View reviewed changes

pkg/domain/infra/abi/containers.go Show resolved Hide resolved

TomSweeneyRedHat reviewed Jan 20, 2021

View reviewed changes

openshift-ci-robot assigned TomSweeneyRedHat Jan 20, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021

openshift-merge-robot merged commit 14443cc into containers:master Jan 20, 2021

edsantiago mentioned this pull request Jan 20, 2021

podman rm external-container: sometimes needs -f #8735

Closed

edsantiago mentioned this pull request Jan 28, 2021

flake: pod rm: remove /sys/fs/cgroup/systemd/etc/etc: device or resource busy #9026

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of container remove #9014

Fix handling of container remove #9014

rhatdan commented Jan 18, 2021

openshift-ci-robot commented Jan 18, 2021

rhatdan commented Jan 18, 2021

rhatdan commented Jan 18, 2021

edsantiago Jan 18, 2021

edsantiago commented Jan 18, 2021

edsantiago commented Jan 18, 2021

edsantiago commented Jan 18, 2021

mheon Jan 18, 2021

rhatdan Jan 19, 2021

vrothberg Jan 19, 2021

mheon Jan 18, 2021

mheon Jan 18, 2021

rhatdan Jan 19, 2021

mheon Jan 19, 2021

rhatdan Jan 19, 2021

mheon Jan 18, 2021

rhatdan Jan 19, 2021

mheon Jan 19, 2021

rhatdan Jan 19, 2021

mheon Jan 19, 2021

rhatdan Jan 19, 2021

rhatdan commented Jan 20, 2021

jwhonce commented Jan 20, 2021

TomSweeneyRedHat Jan 20, 2021

TomSweeneyRedHat commented Jan 20, 2021

TomSweeneyRedHat commented Jan 20, 2021

		@@ -29,9 +31,11 @@ import (
		func RemoveContainer(w http.ResponseWriter, r *http.Request) {

Fix handling of container remove #9014

Fix handling of container remove #9014

Conversation

rhatdan commented Jan 18, 2021

openshift-ci-robot commented Jan 18, 2021

rhatdan commented Jan 18, 2021

rhatdan commented Jan 18, 2021

Choose a reason for hiding this comment

edsantiago commented Jan 18, 2021

edsantiago commented Jan 18, 2021

edsantiago commented Jan 18, 2021

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

rhatdan commented Jan 20, 2021

jwhonce commented Jan 20, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jan 20, 2021

TomSweeneyRedHat commented Jan 20, 2021