runc: be able to get the full ps data (ps -f table) #37

erick0z · 2018-02-27T16:33:14Z

This is the first commit of an attempt to change the current state of the
docker top behaviour. Since docker top uses ps -ef to parse the host
processes with the PIDs array provided by runc.Ps() (ps -f json),
this approach won't work for VM-based container runtimes like
Clear Containers, runv, Kata containers. What I have in mind is to construct the
ContainerTopOKBody (from github.com/moby/moby/api/types/container/container_top.go)
with the data already provided by most of the OCI runtimes out there, and by removing
the exec(ps -ef). This commit adds the ability to get the full ps data.

crosbymichael · 2018-02-27T16:44:30Z

How are consumers supposed to consume this programmatically?

erick0z · 2018-02-27T17:39:59Z

@crosbymichael the ps raw output is supposed to be consumed by the docker daemon (since it implements a parsePSOutput function). Programmatically, with this patch, the user can do almost nothing. What I can do is to implement a tweaked version of the parsePSOutput() function in order to return a structure (similar to ContainerTopOKBody) with all the values.

crosbymichael · 2018-03-02T18:54:08Z

Wouldn't it be better to have these runtimes output structured data? This is a hard one since the spec does not define a ps action and result but almost all the specified actions return json as the result.

https://github.com/opencontainers/runtime-spec/blob/master/runtime.md#query-state

crosbymichael · 2018-03-02T19:01:59Z

Ok, I see your latest revision. I think this is more inline and consistent with the rest of the package. lets review from there.

crosbymichael · 2018-03-02T19:02:14Z

runc.go

@@ -379,6 +388,20 @@ func (r *Runc) Ps(context context.Context, id string) ([]int, error) {
 	return pids, nil
 }

+// PsFull lists all the processes inside the container returning the full ps data
+func (r *Runc) PsFull(context context.Context, id string, psOptions string) (*TopBody, error) {


Can you change the function name to Top?

crosbymichael · 2018-03-02T19:03:02Z

runc.go

@@ -21,6 +21,15 @@ import (
 // Format is the type of log formatting options avaliable
 type Format string

+// TopBody represents the structured data of the full ps output
+type TopBody struct {
+	// Each process running in the container, where each is process is an array of values corresponding to the titles


comments include the field name first, just like function comments

crosbymichael · 2018-03-02T19:05:12Z

runc.go

@@ -21,6 +21,15 @@ import (
 // Format is the type of log formatting options avaliable
 type Format string

+// TopBody represents the structured data of the full ps output
+type TopBody struct {


Body is a little off to me, maybe something about TopResults or something better?

crosbymichael · 2018-03-02T19:06:17Z

utils.go

+	topBody.Titles = fieldsASCII(lines[0])
+
+	pidIndex := -1
+	for i, name := range topBody.Titles {


What is this validation for? Does it really matter if Pid is not in the output?

Well, from the containerd perspective and, of course, docker top, PID is a must to have. But from go-runc, I think it doesn't. I will remove this validation.

One purpose is to ensure the struct will contain the PIDs and another is to handle the output when the m option is passed.

crosbymichael · 2018-03-02T19:07:36Z

runc.go

+	Processes [][]string `json:"Processes"`
+
+	// The ps column titles
+	Titles []string `json:"Titles"`


I think this is commonly named Headers or Columns

erick0z · 2018-03-07T03:16:55Z

@crosbymichael I honestly would prefer to have the runtime ps output defined in the OCI spec. I don't know if that's hard to introduce. Anyway, I made the changes you requested and thanks for the feedback.

This is the first commit of an attempt to change the current state of the `docker top` behaviour. Since `docker top` uses `ps -ef` to parse the host processes with the PIDs array provided by runc.Ps() (`ps -f json`), this approach won't work for VM-based container runtimes like (Clear Containers, runv, Kata containers). What I have in mind is to construct the ContainerTopOKBody (from github.com/moby/moby/api/types/container/container_top.go) with the data already provided by most of the OCI runtimes out there, and by removing the exec(ps -ef). This commit adds the ability to get the full ps data returning a struct with all the ps titles and values. Signed-off-by: Erick Cardona <erick.cardona.ruiz@intel.com>

crosbymichael

LGTM

estesp

LGTM

Updates buildkit to current master (github.com/moby/buildkit): moby/buildkit@be6da00...7a4a2a2 Fixes build failures due to gRPC bumps; 18:54:28 vendor/github.com/moby/buildkit/session/filesync/filesync.go:185:8:warning: NewContext not declared by package metadata (gosimple) 18:54:28 vendor/github.com/moby/buildkit/session/filesync/filesync.go:69:13:warning: FromContext not declared by package metadata (gosimple) 18:54:28 vendor/github.com/moby/buildkit/session/filesync/filesync.go:185:8:warning: NewContext not declared by package metadata (interfacer) 18:54:28 vendor/github.com/moby/buildkit/session/filesync/filesync.go:69:13:warning: FromContext not declared by package metadata (interfacer) 18:54:28 vendor/github.com/moby/buildkit/session/filesync/filesync.go:69:13:warning: FromContext not declared by package metadata (unconvert) 18:54:28 vendor/github.com/moby/buildkit/session/filesync/filesync.go:185:8:warning: NewContext not declared by package metadata (unconvert) Update vendored dependencies to match BuildKit: - add github.com/grpc-ecosystem/grpc-opentracing dependency - add github.com/opentracing/opentracing-go dependency - downgrade github.com/pkg/errors to a tagged release: pkg/errors@v0.8.0...839d9e9 - github.com/containerd/continuity containerd/continuity@d8fb858...3e8f2ea - containerd/continuity#110 Add fstest.CreateSocket - github.com/containerd/fifo containerd/fifo@fbfb6a1...3d5202a - Add apache license to files - github.com/containerd/go-runc containerd/go-runc@4f6e87a...f271fa2 - containerd/go-runc#37 runc: be able to get the full ps data (ps -f table) - containerd/go-runc#40 Add ConsoleSocket to RestoreOpts - github.com/containerd/console containerd/console@2748ece...cb7008a - containerd/console#21 Add OpenBSD support - github.com/syndtr/gocapability syndtr/gocapability@2c00dae...db04d3c - syndtr/gocapability#11 Add support for ambient capabilities - syndtr/gocapability#13 Fix issue moby#12: break too early - golang.org/x/net golang/net@5561cd9...0ed95ab - golang.org/x/text golang/text@f72d839...19e5161 - golang.org/x/time golang/time@a4bde12...f51c127 - github.com/tonistiigi/fsutil tonistiigi/fsutil@dea3a0d...93a0fd1 - tonistiigi/fsutil#16 Don not hang on diskwriter errors - tonistiigi/fsutil#17 fix fd leak on send - tonistiigi/fsutil#18 avoid possible receiver panic/deadlock on sender error - tonistiigi/fsutil#20 receive: read stream to EOF on close - tonistiigi/fsutil#21 avoid not-exist error on walker - tonistiigi/fsutil#27 Generalize chtimes() implementation for non-linux platforms - tonistiigi/fsutil#28 walker: handle parents and subdirs in includepatterns Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

crosbymichael requested changes Mar 2, 2018

View reviewed changes

crosbymichael approved these changes Mar 7, 2018

View reviewed changes

estesp approved these changes Mar 7, 2018

View reviewed changes

estesp merged commit 0bcd1fe into containerd:master Mar 7, 2018

thaJeztah mentioned this pull request May 17, 2018

Bump swarmkit to 0fe3f12bdbfee0d47216de0118c569b1042e1588 moby/moby#37059

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runc: be able to get the full ps data (ps -f table) #37

runc: be able to get the full ps data (ps -f table) #37

erick0z commented Feb 27, 2018

crosbymichael commented Feb 27, 2018

erick0z commented Feb 27, 2018

crosbymichael commented Mar 2, 2018

crosbymichael commented Mar 2, 2018

crosbymichael Mar 2, 2018

crosbymichael Mar 2, 2018

crosbymichael Mar 2, 2018

crosbymichael Mar 2, 2018

erick0z Mar 7, 2018 •

edited

Loading

erick0z Mar 7, 2018

crosbymichael Mar 2, 2018

erick0z commented Mar 7, 2018

crosbymichael left a comment

estesp left a comment

runc: be able to get the full ps data (ps -f table) #37

runc: be able to get the full ps data (ps -f table) #37

Conversation

erick0z commented Feb 27, 2018

crosbymichael commented Feb 27, 2018

erick0z commented Feb 27, 2018

crosbymichael commented Mar 2, 2018

crosbymichael commented Mar 2, 2018

crosbymichael Mar 2, 2018

Choose a reason for hiding this comment

crosbymichael Mar 2, 2018

Choose a reason for hiding this comment

crosbymichael Mar 2, 2018

Choose a reason for hiding this comment

crosbymichael Mar 2, 2018

Choose a reason for hiding this comment

erick0z Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

erick0z Mar 7, 2018

Choose a reason for hiding this comment

crosbymichael Mar 2, 2018

Choose a reason for hiding this comment

erick0z commented Mar 7, 2018

crosbymichael left a comment

Choose a reason for hiding this comment

estesp left a comment

Choose a reason for hiding this comment

erick0z Mar 7, 2018 •

edited

Loading