Fix handling of container remove

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 <dwalsh@redhat.com>

10 files changed, 139 insertions, 57 deletions

diff --git a/cmd/podman/containers/rm.go b/cmd/podman/containers/rm.go
index ee9dc4c78..ea616b6e5 100644
--- a/cmd/podman/containers/rm.go
+++ b/cmd/podman/containers/rm.go
@@ -3,6 +3,7 @@ package containers
 import (
 	"context"
 	"fmt"
+	"io/ioutil"
 	"strings"
 
 	"github.com/containers/common/pkg/completion"
@@ -54,6 +55,7 @@ var (
 
 var (
 	rmOptions = entities.RmOptions{}
+	cidFiles  = []string{}
 )
 
 func rmFlags(cmd *cobra.Command) {
@@ -65,7 +67,7 @@ func rmFlags(cmd *cobra.Command) {
 	flags.BoolVarP(&rmOptions.Volumes, "volumes", "v", false, "Remove anonymous volumes associated with the container")
 
 	cidfileFlagName := "cidfile"
-	flags.StringArrayVarP(&rmOptions.CIDFiles, cidfileFlagName, "", nil, "Read the container ID from the file")
+	flags.StringArrayVar(&cidFiles, cidfileFlagName, nil, "Read the container ID from the file")
 	_ = cmd.RegisterFlagCompletionFunc(cidfileFlagName, completion.AutocompleteDefault)
 
 	if !registry.IsRemote() {
@@ -92,7 +94,16 @@ func init() {
 	validate.AddLatestFlag(containerRmCommand, &rmOptions.Latest)
 }
 
-func rm(_ *cobra.Command, args []string) error {
+func rm(cmd *cobra.Command, args []string) error {
+	for _, cidFile := range cidFiles {
+		content, err := ioutil.ReadFile(string(cidFile))
+		if err != nil {
+			return errors.Wrap(err, "error reading CIDFile")
+		}
+		id := strings.Split(string(content), "\n")[0]
+		args = append(args, id)
+	}
+
 	return removeContainers(args, rmOptions, true)
 }
 
diff --git a/pkg/api/handlers/compat/containers.go b/pkg/api/handlers/compat/containers.go
index a8f850823..de8366587 100644
--- a/pkg/api/handlers/compat/containers.go
+++ b/pkg/api/handlers/compat/containers.go
@@ -14,7 +14,9 @@ import (
 	"github.com/containers/podman/v2/libpod/define"
 	"github.com/containers/podman/v2/pkg/api/handlers"
 	"github.com/containers/podman/v2/pkg/api/handlers/utils"
+	"github.com/containers/podman/v2/pkg/domain/entities"
 	"github.com/containers/podman/v2/pkg/domain/filters"
+	"github.com/containers/podman/v2/pkg/domain/infra/abi"
 	"github.com/containers/podman/v2/pkg/ps"
 	"github.com/containers/podman/v2/pkg/signal"
 	"github.com/docker/docker/api/types"
@@ -30,9 +32,11 @@ import (
 func RemoveContainer(w http.ResponseWriter, r *http.Request) {
 	decoder := r.Context().Value("decoder").(*schema.Decoder)
 	query := struct {
-		Force bool `schema:"force"`
-		Vols  bool `schema:"v"`
-		Link  bool `schema:"link"`
+		All     bool `schema:"all"`
+		Force   bool `schema:"force"`
+		Ignore  bool `schema:"ignore"`
+		Link    bool `schema:"link"`
+		Volumes bool `schema:"v"`
 	}{
 		// override any golang type defaults
 	}
@@ -50,34 +54,31 @@ func RemoveContainer(w http.ResponseWriter, r *http.Request) {
 	}
 
 	runtime := r.Context().Value("runtime").(*libpod.Runtime)
+	// Now use the ABI implementation to prevent us from having duplicate
+	// code.
+	containerEngine := abi.ContainerEngine{Libpod: runtime}
 	name := utils.GetName(r)
-	con, err := runtime.LookupContainer(name)
-	if err != nil && errors.Cause(err) == define.ErrNoSuchCtr {
-		// Failed to get container. If force is specified, get the container's ID
-		// and evict it
-		if !query.Force {
+	options := entities.RmOptions{
+		All:     query.All,
+		Force:   query.Force,
+		Volumes: query.Volumes,
+		Ignore:  query.Ignore,
+	}
+	report, err := containerEngine.ContainerRm(r.Context(), []string{name}, options)
+	if err != nil {
+		if errors.Cause(err) == define.ErrNoSuchCtr {
 			utils.ContainerNotFound(w, name, err)
 			return
 		}
 
-		if _, err := runtime.EvictContainer(r.Context(), name, query.Vols); err != nil {
-			if errors.Cause(err) == define.ErrNoSuchCtr {
-				logrus.Debugf("Ignoring error (--allow-missing): %q", err)
-				w.WriteHeader(http.StatusNoContent)
-				return
-			}
-			logrus.Warn(errors.Wrapf(err, "failed to evict container: %q", name))
-			utils.InternalServerError(w, err)
-			return
-		}
-		w.WriteHeader(http.StatusNoContent)
+		utils.InternalServerError(w, err)
 		return
 	}
-
-	if err := runtime.RemoveContainer(r.Context(), con, query.Force, query.Vols); err != nil {
-		utils.InternalServerError(w, err)
+	if report[0].Err != nil {
+		utils.InternalServerError(w, report[0].Err)
 		return
 	}
+
 	utils.WriteResponse(w, http.StatusNoContent, nil)
 }
 
diff --git a/pkg/bindings/containers/containers.go b/pkg/bindings/containers/containers.go
index 73e4d1d3d..40fcfbded 100644
--- a/pkg/bindings/containers/containers.go
+++ b/pkg/bindings/containers/containers.go
@@ -71,8 +71,10 @@ func Prune(ctx context.Context, options *PruneOptions) ([]*reports.PruneReport,
 }
 
 // Remove removes a container from local storage.  The force bool designates
-// that the container should be removed forcibly (example, even it is running).  The volumes
-// bool dictates that a container's volumes should also be removed.
+// that the container should be removed forcibly (example, even it is running).
+// The volumes bool dictates that a container's volumes should also be removed.
+// The All option indicates that all containers should be removed
+// The Ignore option indicates that if a container did not exist, ignore the error
 func Remove(ctx context.Context, nameOrID string, options *RemoveOptions) error {
 	if options == nil {
 		options = new(RemoveOptions)
@@ -85,9 +87,15 @@ func Remove(ctx context.Context, nameOrID string, options *RemoveOptions) error
 	if v := options.GetVolumes(); options.Changed("Volumes") {
 		params.Set("v", strconv.FormatBool(v))
 	}
+	if all := options.GetAll(); options.Changed("All") {
+		params.Set("all", strconv.FormatBool(all))
+	}
 	if force := options.GetForce(); options.Changed("Force") {
 		params.Set("force", strconv.FormatBool(force))
 	}
+	if ignore := options.GetIgnore(); options.Changed("Ignore") {
+		params.Set("ignore", strconv.FormatBool(ignore))
+	}
 	response, err := conn.DoRequest(nil, http.MethodDelete, "/containers/%s", params, nil, nameOrID)
 	if err != nil {
 		return err
diff --git a/pkg/bindings/containers/types.go b/pkg/bindings/containers/types.go
index 43cb58a54..24604fa83 100644
--- a/pkg/bindings/containers/types.go
+++ b/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
 	Volumes *bool
 }
diff --git a/pkg/bindings/containers/types_remove_options.go b/pkg/bindings/containers/types_remove_options.go
index e21fb41f7..3ef32fa03 100644
--- a/pkg/bindings/containers/types_remove_options.go
+++ b/pkg/bindings/containers/types_remove_options.go
@@ -87,6 +87,38 @@ func (o *RemoveOptions) ToParams() (url.Values, error) {
 	return params, nil
 }
 
+// WithAll
+func (o *RemoveOptions) WithAll(value bool) *RemoveOptions {
+	v := &value
+	o.All = v
+	return o
+}
+
+// GetAll
+func (o *RemoveOptions) GetAll() bool {
+	var all bool
+	if o.All == nil {
+		return all
+	}
+	return *o.All
+}
+
+// WithIgnore
+func (o *RemoveOptions) WithIgnore(value bool) *RemoveOptions {
+	v := &value
+	o.Ignore = v
+	return o
+}
+
+// GetIgnore
+func (o *RemoveOptions) GetIgnore() bool {
+	var ignore bool
+	if o.Ignore == nil {
+		return ignore
+	}
+	return *o.Ignore
+}
+
 // WithForce
 func (o *RemoveOptions) WithForce(value bool) *RemoveOptions {
 	v := &value
diff --git a/pkg/domain/entities/containers.go b/pkg/domain/entities/containers.go
index d8576c101..4c1bd6a7d 100644
--- a/pkg/domain/entities/containers.go
+++ b/pkg/domain/entities/containers.go
@@ -128,12 +128,11 @@ type RestartReport struct {
 }
 
 type RmOptions struct {
-	All      bool
-	CIDFiles []string
-	Force    bool
-	Ignore   bool
-	Latest   bool
-	Volumes  bool
+	All     bool
+	Force   bool
+	Ignore  bool
+	Latest  bool
+	Volumes bool
 }
 
 type RmReport struct {
diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
index a8f4d44a8..48a32817d 100644
--- a/pkg/domain/infra/abi/containers.go
+++ b/pkg/domain/infra/abi/containers.go
@@ -264,30 +264,30 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 	reports := []*entities.RmReport{}
 
 	names := namesOrIds
-	for _, cidFile := range options.CIDFiles {
-		content, err := ioutil.ReadFile(cidFile)
-		if err != nil {
-			return nil, errors.Wrap(err, "error reading CIDFile")
-		}
-		id := strings.Split(string(content), "\n")[0]
-		names = append(names, id)
-	}
-
 	// Attempt to remove named containers directly from storage, if container is defined in libpod
 	// this will fail and code will fall through to removing the container from libpod.`
 	tmpNames := []string{}
 	for _, ctr := range names {
 		report := entities.RmReport{Id: ctr}
-		if err := ic.Libpod.RemoveStorageContainer(ctr, options.Force); err != nil {
+		report.Err = ic.Libpod.RemoveStorageContainer(ctr, options.Force)
+		switch errors.Cause(report.Err) {
+		case nil:
 			// remove container names that we successfully deleted
-			tmpNames = append(tmpNames, ctr)
-		} else {
 			reports = append(reports, &report)
+		case define.ErrNoSuchCtr:
+			// There is still a potential this is a libpod container
+			tmpNames = append(tmpNames, ctr)
+		default:
+			if _, err := ic.Libpod.LookupContainer(ctr); errors.Cause(err) == define.ErrNoSuchCtr {
+				// remove container failed, but not a libpod container
+				reports = append(reports, &report)
+				continue
+			}
+			// attempt to remove as a libpod container
+			tmpNames = append(tmpNames, ctr)
 		}
 	}
-	if len(tmpNames) < len(names) {
-		names = tmpNames
-	}
+	names = tmpNames
 
 	ctrs, err := getContainersByContext(options.All, options.Latest, names, ic.Libpod)
 	if err != nil && !(options.Ignore && errors.Cause(err) == define.ErrNoSuchCtr) {
diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
index 84a07f8e9..524b29553 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -173,14 +173,6 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st
 }
 
 func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, opts entities.RmOptions) ([]*entities.RmReport, error) {
-	for _, cidFile := range opts.CIDFiles {
-		content, err := ioutil.ReadFile(cidFile)
-		if err != nil {
-			return nil, errors.Wrap(err, "error reading CIDFile")
-		}
-		id := strings.Split(string(content), "\n")[0]
-		namesOrIds = append(namesOrIds, id)
-	}
 	ctrs, err := getContainersByContext(ic.ClientCtx, opts.All, opts.Ignore, namesOrIds)
 	if err != nil {
 		return nil, err
diff --git a/test/e2e/rm_test.go b/test/e2e/rm_test.go
index 524c07cc6..ca142d7f3 100644
--- a/test/e2e/rm_test.go
+++ b/test/e2e/rm_test.go
@@ -215,6 +215,40 @@ var _ = Describe("Podman rm", func() {
 		Expect(result.ExitCode()).To(Equal(125))
 	})
 
+	It("podman rm --all", func() {
+		session := podmanTest.Podman([]string{"create", ALPINE, "ls"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(podmanTest.NumberOfContainers()).To(Equal(1))
+
+		session = podmanTest.Podman([]string{"create", ALPINE, "ls"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(podmanTest.NumberOfContainers()).To(Equal(2))
+
+		session = podmanTest.Podman([]string{"rm", "--all"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(podmanTest.NumberOfContainers()).To(Equal(0))
+	})
+
+	It("podman rm --ignore", func() {
+		session := podmanTest.Podman([]string{"create", ALPINE, "ls"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		cid := session.OutputToStringArray()[0]
+		Expect(podmanTest.NumberOfContainers()).To(Equal(1))
+
+		session = podmanTest.Podman([]string{"rm", "bogus", cid})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(1))
+
+		session = podmanTest.Podman([]string{"rm", "--ignore", "bogus", cid})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(podmanTest.NumberOfContainers()).To(Equal(0))
+	})
+
 	It("podman rm bogus container", func() {
 		session := podmanTest.Podman([]string{"rm", "bogus"})
 		session.WaitWithDefaultTimeout()
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