grpc: implement ListProcesses #194

devimc · 2018-03-28T20:34:24Z

ListProcesses returns a list of running processes inside
the container, this function should be called by the runtime
in the ps command implementation.

fixes #193

Signed-off-by: Julio Montes julio.montes@intel.com

codecov · 2018-03-28T22:22:53Z

Codecov Report

Merging #194 into master will increase coverage by 1.97%.
The diff coverage is 70.43%.

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
+ Coverage   35.37%   37.34%   +1.97%     
==========================================
  Files          11       12       +1     
  Lines        1631     1652      +21     
==========================================
+ Hits          577      617      +40     
+ Misses        969      953      -16     
+ Partials       85       82       -3

Impacted Files	Coverage Δ
reaper.go	`0% <0%> (ø)`	⬆️
mockcontainer.go	`100% <100%> (ø)`
mockreaper.go	`77.77% <57.14%> (-22.23%)`	⬇️
grpc.go	`20.68% <75.51%> (+6.47%)`	⬆️
network.go	`47.79% <0%> (+1.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6166b7...ff7eaa1. Read the comment docs.

sboeuf · 2018-03-28T23:46:55Z

grpc.go

+	}
+
+	cmd := exec.Command("ps", psArgs...)
+	output, err := cmd.CombinedOutput()


Have you tried to run this intensively ? I ask because I am afraid this command might error sometimes, due to the subreaper. Let's say you run your ps process from the agent, the subreaper might be the one reaping the process when it terminates, meaning the cmd.Process.Wait() called from cmd.CombinedOutput() might return an error because it didn't properly got the exit code from the command.
@laijs @bergwolf any input on that ?

Ah, nasty... the subreaper is collecting all child exit status so we cannot use cmd.CombinedOutput() directly. Maybe we can provide a s.subreaper.combindedoutput() that buffers cmd stdout/stderr in a separate buffer.

you're right, fixed

bergwolf · 2018-03-29T02:38:31Z

grpc.go

+		psArgs = []string{"-ef"}
+	}
+
+	cmd := exec.Command("ps", psArgs...)


IIUC we need to enter container's pidns to call ps, which may or may not be the global sandbox pidns.

nop, c.container.Processes() returns the list of process

The ps command is going to run in agent's pidns, which is not the container's pidns AFAICT.

yes, and that's ok, or I'm missing something.. ?

@devimc If we want to return a list of processes running inside a container, should'nt we be running the ps command inside container's PID namespace.

@amshinde nop, c.container.Processes() already does that

I see. c.container.Processes() returns pids in the caller's namespace. So it works if the caller is in the init_ns, which is true for the agent. Thanks for the explanation @devimc !

@devimc Can you add explain/ add comments why you need to run the ps command in the agent namespace when you already have all the pids from the container?
Cant you use a predefined ps table format?

bergwolf · 2018-03-29T02:54:02Z

grpc.go

+	}
+
+	cmd := exec.Command("ps", psArgs...)
+	output, err := cmd.CombinedOutput()


Ah, nasty... the subreaper is collecting all child exit status so we cannot use cmd.CombinedOutput() directly. Maybe we can provide a s.subreaper.combindedoutput() that buffers cmd stdout/stderr in a separate buffer.

bergwolf · 2018-03-29T03:05:49Z

protocols/grpc/agent.proto

+
+// ListProcessesResponse represents the list of running processes inside the container
+message ListProcessesResponse {
+	bytes processList = 1;


process_list

sboeuf · 2018-04-03T14:50:22Z

grpc.go

+	default:
+		return resp, fmt.Errorf("invalid format option")
+	}
+


@devimc everything after this looks like it is not reachable. Am I missing something ?

do you mean if the format is not supported ?

I mean that if you look at this switch statement, there are return for each statement, including default. How do you expect the code further down to be reached ?

no, table doesn't have a return

...... case "table": case "json": resp.ProcessList, err = json.Marshal(pids) return resp, err .......

Oh yes I was assuming a fallthrough here...

@sboeuf - I think the code is validating table as a legitimate format type (but with no associated statement). However, the actual handling for table is below the switch.

devimc · 2018-04-03T15:38:45Z

@bergwolf @sboeuf changes applied, thanks

devimc · 2018-04-04T14:57:41Z

@kata-containers/agent I'll address code coverage in this pr #207

devimc · 2018-04-05T18:50:33Z

can we merge this?, I'll fix the code coverage in #207

devimc · 2018-04-06T12:36:45Z

rebased 😄

bergwolf · 2018-04-09T13:59:24Z

lgtm

amshinde · 2018-04-09T19:06:04Z

grpc.go

+
+		// appends pid line
+		for _, pid := range pids {
+			if pid == p {


You are comparing the pid from the container namespace to the one in the agent pid namespace. Can you explain why you do this? I would imagine not processes from the container pid namespace would not be visible in the agent namespace.

I added more comments in the code, like this

// Get the list of processes that are running inside the containers. // the PIDs match with the system PIDs, not with container's namespace pids, err := c.container.Processes()

just fmi then, which type of pid do we return - I'd hope a container pid namespace pid, not a system pid?

The fact that you are able to match the pids from the agent pid namespace makes me think that this is returning the system pids.

sorry I don't understand your concerns , is the agent running inside a namespace? because I tested this patch and works, ps returns all the processes, even I see the PID 1 (systemd)

I think that is sort of the crux - when you do a 'ps' of a container, does it show you the systemd process '1' of the (for want of a better phrase..) mini-OS that is in the VM. If I am understanding correctly, a ps of a container should only show you the processes in that container - in that workload - we should not see anything, such as systemd pid1, from the mini-OS.

Maybe you can post an example output of what happens to for a 'busybox sh' container or similar to clarify what we get? Also, maybe, a ps from inside the container (docker exec ps) so we can compare?

with runc

$ docker run --runtime=runc -dti debian bash d7d8f6ed73faef744b3961efdef780a4b94642b6d396bd73afa5699c32ee7d01 $ sudo docker-runc ps d7d8f6ed73faef744b3961efdef780a4b94642b6d396bd73afa5699c32ee7d01 UID PID PPID C STIME TTY TIME CMD root 4337 4320 0 08:31 pts/0 00:00:00 bash

with kata-runtime

$ docker run --runtime=kata-runtime -dti debian bash 32b6d8eb33c750c8ebf960464a0a40860e138d24bfd13a45ee45704dbd1462bc $ sudo kata-runtime ps 32b6d8eb33c750c8ebf960464a0a40860e138d24bfd13a45ee45704dbd1462bc UID PID PPID C STIME TTY TIME CMD root 147 115 0 13:44 ? 00:00:00 bash

docker exec with runc

$ docker run --runtime=runc -dti busybox sh 5e93234a965d7e64d7a2ab918b9eaace72712137f08b6a43ce61162f45752ad0 $ docker exec 5e93234a965d7e64d7a2ab918b9eaace72712137f08b6a43ce61162f45752ad0 ps PID USER TIME COMMAND 1 root 0:00 sh 6 root 0:00 ps

docker exec with kata-runtime

$ docker run --runtime=kata-runtime -dti busybox sh 9e1070e509f5574117e86319aff9c1c2d64a00f2d27a40b974c46f59971266c3 $ docker exec 9e1070e509f5574117e86319aff9c1c2d64a00f2d27a40b974c46f59971266c3 ps PID USER TIME COMMAND 1 root 0:00 sh 5 root 0:00 ps

Ooh. I see. That is interesting - that it seems docker ps does not use the PIDS from inside the container user namepace, but indeed shows the host space PIDS. Not really what I expected at all (but maybe there are sound reasons for runc - that maybe you can then manipulate those processes from the guest).
Things are a little different for kata, as even with that 'guest PID', that is really the VM root PID, so there is not really much you can do with that from the guest - but, showing that is probably as near to runc behaviour as we can get. We could debate if that is meaningful for VM containers though.
@amshinde , you agree here that this is fine then?

Yeah, I was not expecting to see the system processes here. Looks fine to me then.

devimc · 2018-04-10T15:18:21Z

@amshinde changes applied, thanks

devimc · 2018-04-10T15:19:17Z

fixed code coverage 😄

devimc · 2018-04-11T14:29:08Z

almost done @kata-containers/agent one more review please 😃

devimc · 2018-04-12T14:35:33Z

lgtm 😆

sboeuf · 2018-04-12T14:37:08Z

@grahamwhaley @egernst @jcvenegas ping ! PTAL

sboeuf · 2018-04-12T14:39:13Z

@devimc @chavafg any idea why centos build is stuck here ?

chavafg · 2018-04-12T15:17:10Z

I see the centos job working correctly, maybe we hit the limit of concurrent jobs and this one was waiting.

grahamwhaley

A query or two.

grahamwhaley · 2018-04-12T17:20:11Z

grpc.go

+	// pids already contains the list of processes that are running
+	// inside a container, now we have to use that list to filter
+	// ps output and return just container's processes
+	cmd := exec.Command("ps", psArgs...)


This is of course assuming that ps is available in the VM rootfs is it not? I'm not sure we have a 100% guarantee, or a documented 100% requirement, for that do we?
The alternative is to peruse the /proc (and/or the process namespace related) files, which would require no extra helper apps? That may also mean we don't have to try and parse the string output, which might be easier as well?

Agree with gwhaley. Also, we should really avoid execing if possible.

see https://github.com/opencontainers/runc/blob/master/ps.go#L63

the idea behind runtime's ps sub-command is to list the processes using linux ps command, "peruse the /proc" is not an option since the user decides the output format and the information to show. Would be nice to have an implementation of ps in go to include it here, but as far as I know there is no. If you concern is that we don't include ps command in the image, don't worries I already filed an issue for that

Ah, so the user can supply arguments to ps from the docker command line? In which case, yes, it makes more sense to use ps directly.
I will note, you might find that some implementations of ps (notably busybox, which then also implies Alpine I believe), may act differently and/or not support all options that you get with the normal 'full ps'. So, you may wish to beware and test that out.

yes, you can specify ps options using docker top

$ docker top --help Usage: docker top CONTAINER [ps OPTIONS]

sure thing, I'll consider alpine case

grahamwhaley · 2018-04-12T17:22:20Z

grpc.go

+
+		// appends pid line
+		for _, pid := range pids {
+			if pid == p {


just fmi then, which type of pid do we return - I'd hope a container pid namespace pid, not a system pid?

ListProcesses returns a list of running processes inside the container, this function should be called by the runtime in the ps command implementation. fixes kata-containers#193 Signed-off-by: Julio Montes <julio.montes@intel.com>

add unit tests for mockreaper run and combinedOutput functions Signed-off-by: Julio Montes <julio.montes@intel.com>

mockContainer is a mock struct to write unit tests Signed-off-by: Julio Montes <julio.montes@intel.com>

mockreaper and mockcontainer are used to implement TestListProcesses. This patch increases a little the code coverage. Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc · 2018-04-13T13:46:34Z

rebased

amshinde

lgtm

sboeuf · 2018-04-13T16:31:02Z

@grahamwhaley I think you can merge this now (after you've ack'ed it !)

grahamwhaley

lgtm

devimc added the review label Mar 28, 2018

devimc mentioned this pull request Mar 28, 2018

virtcontainers: agent: implement kata agent processListContainer kata-containers/runtime#130

Merged

devimc force-pushed the cli/fixPs branch from d31aedf to 2aeb949 Compare March 28, 2018 21:35

sboeuf reviewed Mar 28, 2018

View reviewed changes

bergwolf reviewed Mar 29, 2018

View reviewed changes

devimc force-pushed the cli/fixPs branch 2 times, most recently from c9fcb94 to 41cb076 Compare April 3, 2018 14:41

sboeuf reviewed Apr 3, 2018

View reviewed changes

devimc force-pushed the cli/fixPs branch from 41cb076 to dd113d1 Compare April 3, 2018 15:36

devimc force-pushed the cli/fixPs branch from dd113d1 to 1cc71ea Compare April 3, 2018 20:39

devimc force-pushed the cli/fixPs branch from 1cc71ea to 625603e Compare April 6, 2018 12:36

devimc added the do-not-merge label Apr 9, 2018

amshinde reviewed Apr 9, 2018

View reviewed changes

devimc force-pushed the cli/fixPs branch 2 times, most recently from a2a806f to f97757b Compare April 10, 2018 15:17

devimc removed the do-not-merge label Apr 10, 2018

devimc force-pushed the cli/fixPs branch from f97757b to f6bb055 Compare April 10, 2018 18:53

sboeuf approved these changes Apr 11, 2018

View reviewed changes

grahamwhaley reviewed Apr 12, 2018

View reviewed changes

Julio Montes added 4 commits April 13, 2018 08:43

grpc: implement ListProcesses

01bc953

ListProcesses returns a list of running processes inside the container, this function should be called by the runtime in the ps command implementation. fixes kata-containers#193 Signed-off-by: Julio Montes <julio.montes@intel.com>

mockreaper: add unit tests

d208929

add unit tests for mockreaper run and combinedOutput functions Signed-off-by: Julio Montes <julio.montes@intel.com>

mockcontainer: implement mockContainer

c394b63

mockContainer is a mock struct to write unit tests Signed-off-by: Julio Montes <julio.montes@intel.com>

grpc: add unit test for ListProcesses

ff7eaa1

mockreaper and mockcontainer are used to implement TestListProcesses. This patch increases a little the code coverage. Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the cli/fixPs branch from f6bb055 to ff7eaa1 Compare April 13, 2018 13:44

amshinde approved these changes Apr 13, 2018

View reviewed changes

grahamwhaley approved these changes Apr 13, 2018

View reviewed changes

grahamwhaley merged commit 7aa11d8 into kata-containers:master Apr 13, 2018

grahamwhaley removed the review label Apr 13, 2018

grahamwhaley mentioned this pull request Apr 18, 2018

Add limitations doc kata-containers/documentation#48

Merged

grpc: implement ListProcesses #194

grpc: implement ListProcesses #194

Conversation

devimc commented Mar 28, 2018

codecov bot commented Mar 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc commented Apr 3, 2018

devimc commented Apr 4, 2018

devimc commented Apr 5, 2018

devimc commented Apr 6, 2018

bergwolf commented Apr 9, 2018 • edited by amshinde Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc commented Apr 10, 2018

devimc commented Apr 10, 2018

devimc commented Apr 11, 2018

devimc commented Apr 12, 2018 • edited by amshinde Loading

sboeuf commented Apr 12, 2018

sboeuf commented Apr 12, 2018

chavafg commented Apr 12, 2018

grahamwhaley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devimc commented Apr 13, 2018

amshinde left a comment

Choose a reason for hiding this comment

sboeuf commented Apr 13, 2018

grahamwhaley left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 28, 2018 •

edited

Loading

bergwolf commented Apr 9, 2018 •

edited by amshinde

Loading

devimc commented Apr 12, 2018 •

edited by amshinde

Loading