Merge pull request #4860 from vrothberg/v2-top

v2 api: top improvements

7 files changed, 137 insertions, 88 deletions

diff --git a/libpod/container_top_linux.go b/libpod/container_top_linux.go
index 5f4f28130..98edc340a 100644
--- a/libpod/container_top_linux.go
+++ b/libpod/container_top_linux.go
@@ -3,6 +3,8 @@
 package libpod
 
 import (
+	"bufio"
+	"os"
 	"strconv"
 	"strings"
 
@@ -10,6 +12,7 @@ import (
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/psgo"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 // Top gathers statistics about the running processes in a container. It returns a
@@ -36,7 +39,34 @@ func (c *Container) Top(descriptors []string) ([]string, error) {
 			}
 		}
 	}
-	return c.GetContainerPidInformation(psgoDescriptors)
+
+	// If we encountered an ErrUnknownDescriptor error, fallback to executing
+	// ps(1). This ensures backwards compatibility to users depending on ps(1)
+	// and makes sure we're ~compatible with docker.
+	output, psgoErr := c.GetContainerPidInformation(psgoDescriptors)
+	if psgoErr == nil {
+		return output, nil
+	}
+	if errors.Cause(psgoErr) != psgo.ErrUnknownDescriptor {
+		return nil, psgoErr
+	}
+
+	output, err = c.execPS(descriptors)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error executing ps(1) in the container")
+	}
+
+	// Trick: filter the ps command from the output instead of
+	// checking/requiring PIDs in the output.
+	filtered := []string{}
+	cmd := strings.Join(descriptors, " ")
+	for _, line := range output {
+		if !strings.Contains(line, cmd) {
+			filtered = append(filtered, line)
+		}
+	}
+
+	return filtered, nil
 }
 
 // GetContainerPidInformation returns process-related data of all processes in
@@ -65,3 +95,59 @@ func (c *Container) GetContainerPidInformation(descriptors []string) ([]string,
 	}
 	return res, nil
 }
+
+// execPS executes ps(1) with the specified args in the container.
+func (c *Container) execPS(args []string) ([]string, error) {
+	rPipe, wPipe, err := os.Pipe()
+	if err != nil {
+		return nil, err
+	}
+	defer wPipe.Close()
+	defer rPipe.Close()
+
+	rErrPipe, wErrPipe, err := os.Pipe()
+	if err != nil {
+		return nil, err
+	}
+	defer wErrPipe.Close()
+	defer rErrPipe.Close()
+
+	streams := new(AttachStreams)
+	streams.OutputStream = wPipe
+	streams.ErrorStream = wErrPipe
+	streams.AttachOutput = true
+	streams.AttachError = true
+
+	stdout := []string{}
+	go func() {
+		scanner := bufio.NewScanner(rPipe)
+		for scanner.Scan() {
+			stdout = append(stdout, scanner.Text())
+		}
+	}()
+	stderr := []string{}
+	go func() {
+		scanner := bufio.NewScanner(rErrPipe)
+		for scanner.Scan() {
+			stderr = append(stderr, scanner.Text())
+		}
+	}()
+
+	cmd := append([]string{"ps"}, args...)
+	ec, err := c.Exec(false, false, map[string]string{}, cmd, "", "", streams, 0, nil, "")
+	if err != nil {
+		return nil, err
+	} else if ec != 0 {
+		return nil, errors.Errorf("Runtime failed with exit status: %d and output: %s", ec, strings.Join(stderr, " "))
+	}
+
+	if logrus.GetLevel() >= logrus.DebugLevel {
+		// If we're running in debug mode or higher, we might want to have a
+		// look at stderr which includes debug logs from conmon.
+		for _, log := range stderr {
+			logrus.Debugf("%s", log)
+		}
+	}
+
+	return stdout, nil
+}
diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go
index 8b21d6b94..f66999ffa 100644
--- a/pkg/adapter/containers.go
+++ b/pkg/adapter/containers.go
@@ -27,7 +27,6 @@ import (
 	"github.com/containers/libpod/libpod/logs"
 	"github.com/containers/libpod/pkg/adapter/shortcuts"
 	"github.com/containers/libpod/pkg/systemdgen"
-	"github.com/containers/psgo"
 	"github.com/containers/storage"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -913,71 +912,7 @@ func (r *LocalRuntime) Top(cli *cliconfig.TopValues) ([]string, error) {
 		return nil, errors.Wrapf(err, "unable to lookup requested container")
 	}
 
-	output, psgoErr := container.Top(descriptors)
-	if psgoErr == nil {
-		return output, nil
-	}
-
-	// If we encountered an ErrUnknownDescriptor error, fallback to executing
-	// ps(1). This ensures backwards compatibility to users depending on ps(1)
-	// and makes sure we're ~compatible with docker.
-	if errors.Cause(psgoErr) != psgo.ErrUnknownDescriptor {
-		return nil, psgoErr
-	}
-
-	output, err = r.execPS(container, descriptors)
-	if err != nil {
-		// Note: return psgoErr to guide users into using the AIX descriptors
-		// instead of using ps(1).
-		return nil, psgoErr
-	}
-
-	// Trick: filter the ps command from the output instead of
-	// checking/requiring PIDs in the output.
-	filtered := []string{}
-	cmd := strings.Join(descriptors, " ")
-	for _, line := range output {
-		if !strings.Contains(line, cmd) {
-			filtered = append(filtered, line)
-		}
-	}
-
-	return filtered, nil
-}
-
-func (r *LocalRuntime) execPS(c *libpod.Container, args []string) ([]string, error) {
-	rPipe, wPipe, err := os.Pipe()
-	if err != nil {
-		return nil, err
-	}
-	defer wPipe.Close()
-	defer rPipe.Close()
-
-	streams := new(libpod.AttachStreams)
-	streams.OutputStream = wPipe
-	streams.ErrorStream = wPipe
-	streams.InputStream = bufio.NewReader(os.Stdin)
-	streams.AttachOutput = true
-	streams.AttachError = true
-	streams.AttachInput = true
-
-	psOutput := []string{}
-	go func() {
-		scanner := bufio.NewScanner(rPipe)
-		for scanner.Scan() {
-			psOutput = append(psOutput, scanner.Text())
-		}
-	}()
-
-	cmd := append([]string{"ps"}, args...)
-	ec, err := c.Exec(false, false, map[string]string{}, cmd, "", "", streams, 0, nil, "")
-	if err != nil {
-		return nil, err
-	} else if ec != 0 {
-		return nil, errors.Errorf("Runtime failed with exit status: %d and output: %s", ec, strings.Join(psOutput, " "))
-	}
-
-	return psOutput, nil
+	return container.Top(descriptors)
 }
 
 // ExecContainer executes a command in the container
diff --git a/pkg/api/handlers/containers_top.go b/pkg/api/handlers/containers_top.go
index bab559da1..6b7688eb0 100644
--- a/pkg/api/handlers/containers_top.go
+++ b/pkg/api/handlers/containers_top.go
@@ -5,7 +5,6 @@ import (
 	"strings"
 
 	"github.com/containers/libpod/libpod"
-	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/pkg/api/handlers/utils"
 	"github.com/gorilla/mux"
 	"github.com/gorilla/schema"
@@ -16,10 +15,14 @@ func TopContainer(w http.ResponseWriter, r *http.Request) {
 	runtime := r.Context().Value("runtime").(*libpod.Runtime)
 	decoder := r.Context().Value("decoder").(*schema.Decoder)
 
+	defaultValue := "-ef"
+	if utils.IsLibpodRequest(r) {
+		defaultValue = ""
+	}
 	query := struct {
 		PsArgs string `schema:"ps_args"`
 	}{
-		PsArgs: "-ef",
+		PsArgs: defaultValue,
 	}
 	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
 		utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
@@ -28,23 +31,13 @@ func TopContainer(w http.ResponseWriter, r *http.Request) {
 	}
 
 	name := mux.Vars(r)["name"]
-	ctnr, err := runtime.LookupContainer(name)
+	c, err := runtime.LookupContainer(name)
 	if err != nil {
 		utils.ContainerNotFound(w, name, err)
 		return
 	}
 
-	state, err := ctnr.State()
-	if err != nil {
-		utils.InternalServerError(w, err)
-		return
-	}
-	if state != define.ContainerStateRunning {
-		utils.ContainerNotRunning(w, name, errors.Errorf("Container %s must be running to perform top operation", name))
-		return
-	}
-
-	output, err := ctnr.Top([]string{})
+	output, err := c.Top([]string{query.PsArgs})
 	if err != nil {
 		utils.InternalServerError(w, err)
 		return
diff --git a/pkg/api/handlers/generic/containers_stats.go b/pkg/api/handlers/generic/containers_stats.go
index 26c8efa15..e33d37606 100644
--- a/pkg/api/handlers/generic/containers_stats.go
+++ b/pkg/api/handlers/generic/containers_stats.go
@@ -43,7 +43,7 @@ func StatsContainer(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// If the container isn't returning, then let's not bother and return
+	// If the container isn't running, then let's not bother and return
 	// immediately.
 	state, err := ctnr.State()
 	if err != nil {
diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go
index 9edbbdccc..2526a3317 100644
--- a/pkg/api/handlers/types.go
+++ b/pkg/api/handlers/types.go
@@ -135,7 +135,6 @@ type Stats struct {
 
 type ContainerTopOKBody struct {
 	dockerContainer.ContainerTopOKBody
-	ID string `json:"Id"`
 }
 
 type PodCreateConfig struct {
diff --git a/pkg/api/handlers/utils/handler.go b/pkg/api/handlers/utils/handler.go
index 65698bfd3..2fd9bffba 100644
--- a/pkg/api/handlers/utils/handler.go
+++ b/pkg/api/handlers/utils/handler.go
@@ -6,10 +6,18 @@ import (
 	"io"
 	"net/http"
 	"os"
+	"strings"
 
-	log "github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus"
 )
 
+// IsLibpodRequest returns true if the request related to a libpod endpoint
+// (e.g., /v2/libpod/...).
+func IsLibpodRequest(r *http.Request) bool {
+	split := strings.Split(r.URL.String(), "/")
+	return len(split) >= 3 && split[2] == "libpod"
+}
+
 // WriteResponse encodes the given value as JSON or string and renders it for http client
 func WriteResponse(w http.ResponseWriter, code int, value interface{}) {
 	switch v := value.(type) {
@@ -18,14 +26,14 @@ func WriteResponse(w http.ResponseWriter, code int, value interface{}) {
 		w.WriteHeader(code)
 
 		if _, err := fmt.Fprintln(w, v); err != nil {
-			log.Errorf("unable to send string response: %q", err)
+			logrus.Errorf("unable to send string response: %q", err)
 		}
 	case *os.File:
 		w.Header().Set("Content-Type", "application/octet; charset=us-ascii")
 		w.WriteHeader(code)
 
 		if _, err := io.Copy(w, v); err != nil {
-			log.Errorf("unable to copy to response: %q", err)
+			logrus.Errorf("unable to copy to response: %q", err)
 		}
 	default:
 		WriteJSON(w, code, value)
@@ -40,6 +48,6 @@ func WriteJSON(w http.ResponseWriter, code int, value interface{}) {
 	coder := json.NewEncoder(w)
 	coder.SetEscapeHTML(true)
 	if err := coder.Encode(value); err != nil {
-		log.Errorf("unable to write json: %q", err)
+		logrus.Errorf("unable to write json: %q", err)
 	}
 }
diff --git a/pkg/api/server/register_containers.go b/pkg/api/server/register_containers.go
index 8b0b816cd..b275fa4d1 100644
--- a/pkg/api/server/register_containers.go
+++ b/pkg/api/server/register_containers.go
@@ -371,7 +371,7 @@ func (s *APIServer) RegisterContainersHandlers(r *mux.Router) error {
 	//  - in: query
 	//    name: ps_args
 	//    type: string
-	//    description: arguments to pass to ps such as aux
+	//    description: arguments to pass to ps such as aux. Requires ps(1) to be installed in the container if no ps(1) compatible AIX descriptors are used.
 	// produces:
 	// - application/json
 	// responses:
@@ -703,6 +703,34 @@ func (s *APIServer) RegisterContainersHandlers(r *mux.Router) error {
 	//   '500':
 	//      "$ref": "#/responses/InternalError"
 	r.HandleFunc(VersionedPath("/libpod/containers/{name:..*}/stats"), APIHandler(s.Context, generic.StatsContainer)).Methods(http.MethodGet)
+	// swagger:operation GET /libpod/containers/{nameOrID}/top containers topContainer
+	//
+	// List processes running inside a container. Note
+	//
+	// ---
+	// parameters:
+	//  - in: path
+	//    name: nameOrID
+	//    required: true
+	//    description: the name or ID of the container
+	//  - in: query
+	//    name: stream
+	//    type: bool
+	//    default: true
+	//    description: Stream the output
+	//    name: ps_args
+	//    type: string
+	//    description: arguments to pass to ps such as aux. Requires ps(1) to be installed in the container if no ps(1) compatible AIX descriptors are used.
+	// produces:
+	// - application/json
+	// responses:
+	//   '200':
+	//     description: no error
+	//       "ref": "#/responses/DockerTopResponse"
+	//   '404':
+	//       "$ref": "#/responses/NoSuchContainer"
+	//   '500':
+	//      "$ref": "#/responses/InternalError"
 	r.HandleFunc(VersionedPath("/libpod/containers/{name:..*}/top"), APIHandler(s.Context, handlers.TopContainer)).Methods(http.MethodGet)
 	// swagger:operation POST /libpod/containers/{nameOrID}/unpause libpod libpodUnpauseContainer
 	// ---