Merge pull request #9014 from rhatdan/rm

Fix handling of container remove

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