Merge pull request #9095 from rhatdan/ps

podman-remote ps --external --pod --sort do not work.

13 files changed, 81 insertions, 35 deletions

diff --git a/cmd/podman/containers/ps.go b/cmd/podman/containers/ps.go
index d23771fc5..31f44d92f 100644
--- a/cmd/podman/containers/ps.go
+++ b/cmd/podman/containers/ps.go
@@ -78,7 +78,7 @@ func listFlagSet(cmd *cobra.Command) {
 	flags := cmd.Flags()
 
 	flags.BoolVarP(&listOpts.All, "all", "a", false, "Show all the containers, default is only running containers")
-	flags.BoolVar(&listOpts.Storage, "external", false, "Show containers in storage not controlled by Podman")
+	flags.BoolVar(&listOpts.External, "external", false, "Show containers in storage not controlled by Podman")
 
 	filterFlagName := "filter"
 	flags.StringSliceVarP(&filters, filterFlagName, "f", []string{}, "Filter output based on conditions given")
@@ -132,10 +132,10 @@ func checkFlags(c *cobra.Command) error {
 	}
 	cfg := registry.PodmanConfig()
 	if cfg.Engine.Namespace != "" {
-		if c.Flag("storage").Changed && listOpts.Storage {
-			return errors.New("--namespace and --storage flags can not both be set")
+		if c.Flag("storage").Changed && listOpts.External {
+			return errors.New("--namespace and --external flags can not both be set")
 		}
-		listOpts.Storage = false
+		listOpts.External = false
 	}
 
 	return nil
diff --git a/cmd/podman/containers/rm.go b/cmd/podman/containers/rm.go
index ea616b6e5..884ad05f4 100644
--- a/cmd/podman/containers/rm.go
+++ b/cmd/podman/containers/rm.go
@@ -140,6 +140,10 @@ func removeContainers(namesOrIDs []string, rmOptions entities.RmOptions, setExit
 }
 
 func setExitCode(err error) {
+	// If error is set to no such container, do not reset
+	if registry.GetExitCode() == 1 {
+		return
+	}
 	cause := errors.Cause(err)
 	switch {
 	case cause == define.ErrNoSuchCtr:
diff --git a/pkg/api/handlers/compat/containers.go b/pkg/api/handlers/compat/containers.go
index aa12afc82..0f91a4362 100644
--- a/pkg/api/handlers/compat/containers.go
+++ b/pkg/api/handlers/compat/containers.go
@@ -73,7 +73,7 @@ func RemoveContainer(w http.ResponseWriter, r *http.Request) {
 		utils.InternalServerError(w, err)
 		return
 	}
-	if report[0].Err != nil {
+	if len(report) > 0 && report[0].Err != nil {
 		utils.InternalServerError(w, report[0].Err)
 		return
 	}
diff --git a/pkg/api/handlers/libpod/containers.go b/pkg/api/handlers/libpod/containers.go
index 6b07b1cc5..a0cb1d49e 100644
--- a/pkg/api/handlers/libpod/containers.go
+++ b/pkg/api/handlers/libpod/containers.go
@@ -12,7 +12,6 @@ import (
 	"github.com/containers/podman/v2/pkg/api/handlers/utils"
 	"github.com/containers/podman/v2/pkg/domain/entities"
 	"github.com/containers/podman/v2/pkg/domain/infra/abi"
-	"github.com/containers/podman/v2/pkg/ps"
 	"github.com/gorilla/schema"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -63,6 +62,7 @@ func ListContainers(w http.ResponseWriter, r *http.Request) {
 	decoder := r.Context().Value("decoder").(*schema.Decoder)
 	query := struct {
 		All       bool                `schema:"all"`
+		External  bool                `schema:"external"`
 		Filters   map[string][]string `schema:"filters"`
 		Last      int                 `schema:"last"` // alias for limit
 		Limit     int                 `schema:"limit"`
@@ -90,17 +90,22 @@ func ListContainers(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}
 	opts := entities.ContainerListOptions{
 		All:       query.All,
+		External:  query.External,
 		Filters:   query.Filters,
 		Last:      limit,
-		Size:      query.Size,
-		Sort:      "",
 		Namespace: query.Namespace,
-		Pod:       true,
-		Sync:      query.Sync,
+		// Always return Pod, should not be part of the API.
+		// https://github.com/containers/podman/pull/7223
+		Pod:  true,
+		Size: query.Size,
+		Sync: query.Sync,
 	}
-	pss, err := ps.GetContainerLists(runtime, opts)
+	pss, err := containerEngine.ContainerList(r.Context(), opts)
 	if err != nil {
 		utils.InternalServerError(w, err)
 		return
diff --git a/pkg/api/server/register_containers.go b/pkg/api/server/register_containers.go
index 74a04b2e6..e30747800 100644
--- a/pkg/api/server/register_containers.go
+++ b/pkg/api/server/register_containers.go
@@ -48,6 +48,11 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error {
 	//    default: false
 	//    description: Return all containers. By default, only running containers are shown
 	//  - in: query
+	//    name: external
+	//    type: boolean
+	//    default: false
+	//    description: Return containers in storage not controlled by Podman
+	//  - in: query
 	//    name: limit
 	//    description: Return this number of most recently created containers, including non-running ones.
 	//    type: integer
diff --git a/pkg/bindings/containers/types.go b/pkg/bindings/containers/types.go
index 24604fa83..3fb1ab733 100644
--- a/pkg/bindings/containers/types.go
+++ b/pkg/bindings/containers/types.go
@@ -106,6 +106,7 @@ type MountedContainerPathsOptions struct{}
 // ListOptions are optional options for listing containers
 type ListOptions struct {
 	All       *bool
+	External  *bool
 	Filters   map[string][]string
 	Last      *int
 	Namespace *bool
diff --git a/pkg/bindings/containers/types_list_options.go b/pkg/bindings/containers/types_list_options.go
index 43326fa59..c363dcd32 100644
--- a/pkg/bindings/containers/types_list_options.go
+++ b/pkg/bindings/containers/types_list_options.go
@@ -103,6 +103,22 @@ func (o *ListOptions) GetAll() bool {
 	return *o.All
 }
 
+// WithExternal
+func (o *ListOptions) WithExternal(value bool) *ListOptions {
+	v := &value
+	o.External = v
+	return o
+}
+
+// GetExternal
+func (o *ListOptions) GetExternal() bool {
+	var external bool
+	if o.External == nil {
+		return external
+	}
+	return *o.External
+}
+
 // WithFilters
 func (o *ListOptions) WithFilters(value map[string][]string) *ListOptions {
 	v := value
diff --git a/pkg/domain/entities/containers.go b/pkg/domain/entities/containers.go
index 4c1bd6a7d..2c32f792f 100644
--- a/pkg/domain/entities/containers.go
+++ b/pkg/domain/entities/containers.go
@@ -297,8 +297,8 @@ type ContainerListOptions struct {
 	Pod       bool
 	Quiet     bool
 	Size      bool
+	External  bool
 	Sort      string
-	Storage   bool
 	Sync      bool
 	Watch     uint
 }
diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
index 524b29553..0c61714c3 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -173,19 +173,32 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st
 }
 
 func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, opts entities.RmOptions) ([]*entities.RmReport, error) {
-	ctrs, err := getContainersByContext(ic.ClientCtx, opts.All, opts.Ignore, namesOrIds)
-	if err != nil {
-		return nil, err
-	}
 	// TODO there is no endpoint for container eviction.  Need to discuss
-	reports := make([]*entities.RmReport, 0, len(ctrs))
-	options := new(containers.RemoveOptions).WithForce(opts.Force).WithVolumes(opts.Volumes)
-	for _, c := range ctrs {
+	options := new(containers.RemoveOptions).WithForce(opts.Force).WithVolumes(opts.Volumes).WithIgnore(opts.Ignore)
+
+	if opts.All {
+		ctrs, err := getContainersByContext(ic.ClientCtx, opts.All, opts.Ignore, namesOrIds)
+		if err != nil {
+			return nil, err
+		}
+		reports := make([]*entities.RmReport, 0, len(ctrs))
+		for _, c := range ctrs {
+			reports = append(reports, &entities.RmReport{
+				Id:  c.ID,
+				Err: containers.Remove(ic.ClientCtx, c.ID, options),
+			})
+		}
+		return reports, nil
+	}
+
+	reports := make([]*entities.RmReport, 0, len(namesOrIds))
+	for _, name := range namesOrIds {
 		reports = append(reports, &entities.RmReport{
-			Id:  c.ID,
-			Err: containers.Remove(ic.ClientCtx, c.ID, options),
+			Id:  name,
+			Err: containers.Remove(ic.ClientCtx, name, options),
 		})
 	}
+
 	return reports, nil
 }
 
@@ -601,7 +614,7 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 
 func (ic *ContainerEngine) ContainerList(ctx context.Context, opts entities.ContainerListOptions) ([]entities.ListContainer, error) {
 	options := new(containers.ListOptions).WithFilters(opts.Filters).WithAll(opts.All).WithLast(opts.Last)
-	options.WithNamespace(opts.Namespace).WithSize(opts.Size).WithSync(opts.Sync)
+	options.WithNamespace(opts.Namespace).WithSize(opts.Size).WithSync(opts.Sync).WithExternal(opts.External)
 	return containers.List(ic.ClientCtx, options)
 }
 
diff --git a/pkg/ps/ps.go b/pkg/ps/ps.go
index dc577890a..42f9e1d39 100644
--- a/pkg/ps/ps.go
+++ b/pkg/ps/ps.go
@@ -69,7 +69,7 @@ func GetContainerLists(runtime *libpod.Runtime, options entities.ContainerListOp
 		pss = append(pss, listCon)
 	}
 
-	if options.All && options.Storage {
+	if options.All && options.External {
 		externCons, err := runtime.StorageContainers()
 		if err != nil {
 			return nil, err
diff --git a/test/e2e/ps_test.go b/test/e2e/ps_test.go
index 13701fc3b..d12534219 100644
--- a/test/e2e/ps_test.go
+++ b/test/e2e/ps_test.go
@@ -396,11 +396,14 @@ var _ = Describe("Podman ps", func() {
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
 
-		session = podmanTest.Podman([]string{"ps", "--pod", "--no-trunc"})
-
+		session = podmanTest.Podman([]string{"ps", "--no-trunc"})
 		session.WaitWithDefaultTimeout()
 		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(Not(ContainSubstring(podid)))
 
+		session = podmanTest.Podman([]string{"ps", "--pod", "--no-trunc"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
 		Expect(session.OutputToString()).To(ContainSubstring(podid))
 	})
 
diff --git a/test/e2e/rm_test.go b/test/e2e/rm_test.go
index ca142d7f3..4c50a61ef 100644
--- a/test/e2e/rm_test.go
+++ b/test/e2e/rm_test.go
@@ -132,7 +132,7 @@ var _ = Describe("Podman rm", func() {
 
 		latest := "-l"
 		if IsRemote() {
-			latest = "test1"
+			latest = cid
 		}
 		result := podmanTest.Podman([]string{"rm", latest})
 		result.WaitWithDefaultTimeout()
diff --git a/test/system/040-ps.bats b/test/system/040-ps.bats
index 0ae8b0ce0..ae27c479f 100644
--- a/test/system/040-ps.bats
+++ b/test/system/040-ps.bats
@@ -82,11 +82,10 @@ load helpers
     run_podman rm -a
 }
 
-@test "podman ps -a --storage" {
-    skip_if_remote "ps --storage does not work over remote"
+@test "podman ps -a --external" {
 
     # Setup: ensure that we have no hidden storage containers
-    run_podman ps --storage -a
+    run_podman ps --external -a
     is "${#lines[@]}" "1" "setup check: no storage containers at start of test"
 
     # Force a buildah timeout; this leaves a buildah container behind
@@ -98,18 +97,18 @@ EOF
     run_podman ps -a
     is "${#lines[@]}" "1" "podman ps -a does not see buildah container"
 
-    run_podman ps --storage -a
-    is "${#lines[@]}" "2" "podman ps -a --storage sees buildah container"
+    run_podman ps --external -a
+    is "${#lines[@]}" "2" "podman ps -a --external sees buildah container"
     is "${lines[1]}" \
        "[0-9a-f]\{12\} \+$IMAGE *buildah .* seconds ago .* storage .* ${PODMAN_TEST_IMAGE_NAME}-working-container" \
-       "podman ps --storage"
+       "podman ps --external"
 
     cid="${lines[1]:0:12}"
 
     # 'rm -a' should be a NOP
     run_podman rm -a
-    run_podman ps --storage -a
-    is "${#lines[@]}" "2" "podman ps -a --storage sees buildah container"
+    run_podman ps --external -a
+    is "${#lines[@]}" "2" "podman ps -a --external sees buildah container"
 
     # We can't rm it without -f, but podman should issue a helpful message
     run_podman 2 rm "$cid"
@@ -118,7 +117,7 @@ EOF
     # With -f, we can remove it.
     run_podman rm -f "$cid"
 
-    run_podman ps --storage -a
+    run_podman ps --external -a
     is "${#lines[@]}" "1" "storage container has been removed"
 }