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)#37estesp merged 1 commit intocontainerd:masterfrom erick0z:ps_full
Conversation
|
How are consumers supposed to consume this programmatically? |
|
@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. |
|
Wouldn't it be better to have these runtimes output structured data? This is a hard one since the spec does not define a https://github.com/opencontainers/runtime-spec/blob/master/runtime.md#query-state |
|
Ok, I see your latest revision. I think this is more inline and consistent with the rest of the package. lets review from there. |
runc.go
Outdated
| } | ||
|
|
||
| // 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) { |
There was a problem hiding this comment.
Can you change the function name to Top?
runc.go
Outdated
|
|
||
| // 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 |
There was a problem hiding this comment.
comments include the field name first, just like function comments
runc.go
Outdated
| type Format string | ||
|
|
||
| // TopBody represents the structured data of the full ps output | ||
| type TopBody struct { |
There was a problem hiding this comment.
Body is a little off to me, maybe something about TopResults or something better?
utils.go
Outdated
| topBody.Titles = fieldsASCII(lines[0]) | ||
|
|
||
| pidIndex := -1 | ||
| for i, name := range topBody.Titles { |
There was a problem hiding this comment.
What is this validation for? Does it really matter if Pid is not in the output?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
One purpose is to ensure the struct will contain the PIDs and another is to handle the output when the m option is passed.
runc.go
Outdated
| Processes [][]string `json:"Processes"` | ||
|
|
||
| // The ps column titles | ||
| Titles []string `json:"Titles"` |
There was a problem hiding this comment.
I think this is commonly named Headers or Columns
|
@crosbymichael I honestly would prefer to have the |
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>
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>
This is the first commit of an attempt to change the current state of the
docker topbehaviour. Sincedocker topusesps -efto parse the hostprocesses 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.