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>

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)
 }
 

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"
@@ -29,9 +31,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
 	}
@@ -49,34 +53,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)
-		return
-	}
-
-	if err := runtime.RemoveContainer(r.Context(), con, query.Force, query.Vols); err != nil {
 		utils.InternalServerError(w, err)
 		return
 	}
+	if report[0].Err != nil {
+		utils.InternalServerError(w, report[0].Err)
+		return
+	}
+
 	utils.WriteResponse(w, http.StatusNoContent, nil)
 }
 

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

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
 }

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

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 {

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) {

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

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()

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