agent: implement ps command by devimc · Pull Request #269 · containers/virtcontainers

devimc · 2017-05-23T18:51:05Z

ps command is used to list the processes running inside
the container, this command is called by docker top

partially fixes clearcontainers/runtime#95

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

coveralls · 2017-05-23T18:57:31Z

Coverage decreased (-1.3%) to 65.069% when pulling b31c842 on devimc:topic/psCommand into 522a9e0 on containers:master.

sboeuf · 2017-05-23T21:42:02Z

api.go

+
+// PsContainer is the virtcontainers entry point to list
+// processes running inside a container
+func PsContainer(podID, containerID string, psArgs []string) error {


Maybe I am missing something here but you never return anything, right ? How do you plan to get the data back ?

Also I think it could be part of StatusContainer() instead of adding a new function to the exported API. @sameo any thoughts on this one ?

probably I have to return a string

I would say that you want to return a new dedicated structure. Something like:

type ProcessInfo struct { Name string PID string PPID string ... } type PsInfo struct { ID string ProcessesList []ProcessInfo }

jodh-intel · 2017-05-24T08:49:01Z

Related to clearcontainers/runtime#95.

sameo · 2017-05-29T16:36:11Z

pkg/hyperstart/hyperstart.go

 	SetupInterface:  SetupInterfaceCode,
 	SetupRoute:      SetupRouteCode,
 	RemoveContainer: RemoveContainerCode,
+	PsContainer:     PsContainerCode,


Where in hyperstart is this defined ?

It is not defined, not yet I'm working on that clearcontainers/hyperstart#30

https://github.com/clearcontainers/hyperstart/pull/30/files#diff-b94e58fcb93efa0dafb0674fda336bacR31

sameo · 2017-05-30T13:12:09Z

Depends on an hyperstart PR

jodh-intel · 2017-07-05T08:20:17Z

Related: clearcontainers/proxy#83

devimc · 2017-08-22T16:52:22Z

@sameo @sboeuf @jodh-intel or somebody with superpowers , please remove do-not-merge tag

coveralls · 2017-08-22T17:11:08Z

Coverage decreased (-0.9%) to 65.787% when pulling 82477b6 on devimc:topic/psCommand into 6abb9c6 on containers:master.

coveralls · 2017-08-22T19:29:25Z

Coverage decreased (-0.9%) to 65.772% when pulling 911e469 on devimc:topic/psCommand into 6abb9c6 on containers:master.

sboeuf · 2017-08-23T13:07:32Z

agent.go

 	killContainer(pod Pod, c Container, signal syscall.Signal, all bool) error
+
+	// psContainer will list the processes running inside the container
+	psContainer(pod Pod, c Container, format string, psArgs []string) ([]byte, error)


processListContainer seems like a more generic name IMO.
And also I would prefer a function prototype a bit different:

processListContainer(pod Pod, c Container, processListOptions []Param) (ProcessList, error)

The reason here is that I would like to get the same processList structure from any agent implementation and get this structure formatted as expected from the runtime implementation.

sboeuf · 2017-08-23T13:13:03Z

api.go

+
+// PsContainer is the virtcontainers entry point to list
+// processes running inside a container
+func PsContainer(podID, containerID string, format string, psArgs []string) ([]byte, error) {


s/PsContainer/ProcessListContainer/
Prototype

ProcessListContainer(podID, containerID string, processListOptions []Param) (ProcessList, error)

sboeuf · 2017-08-23T13:16:42Z

container.go

 	return nil
 }

+func (c *Container) ps(format string, psArgs []string) ([]byte, error) {


Replace

ps(format string, psArgs []string) ([]byte, error)

with

processList(processListOptions []Param) (ProcessList, error)

devimc · 2017-08-28T16:54:59Z

@sboeuf changes applied

coveralls · 2017-08-28T17:03:57Z

Coverage decreased (-0.9%) to 65.057% when pulling 3f520fc on devimc:topic/psCommand into e2c760d on containers:master.

sboeuf · 2017-09-01T06:19:24Z

@devimc Thanks but you've addressed only half of the comments. Here is what you missed:

And also I would prefer a function prototype a bit different:
processListContainer(pod Pod, c Container, processListOptions []Param) (ProcessList, error)
The reason here is that I would like to get the same processList structure from any agent implementation and get this structure formatted as expected from the runtime implementation.

And

Replace

ps(format string, psArgs []string) ([]byte, error)

with

processList(processListOptions []Param) (ProcessList, error)

The important point here is that I think it would be better to return a generic structure that we define inside virtcontainers so that it can be reused by any runtime implementation. That means that I would expect to see the runtime doing the conversion between this structure and the expected format.
Does that make sense ?
@sameo any thought on that ?

devimc · 2017-09-18T16:08:32Z

@sboeuf imo does not make sense to create a struct just for 1 element, I agree with you I'd expect the conversion of types in the runtime, for this reason I defined the type ProcessList as []byte

coveralls · 2017-09-18T16:43:33Z

Coverage decreased (-0.8%) to 65.104% when pulling 270f7e2 on devimc:topic/psCommand into 6b4015e on containers:master.

jodh-intel

Just a minor nit.

@sboeuf - what do you think?

jodh-intel · 2017-10-09T11:25:52Z

hyperstart.go

+
+	response, err := h.proxy.sendCmd(proxyCmd)
+	if msg, ok := response.([]byte); ok {
+		return msg, err


This would be clearer as follows (currently it is unclear if it's an error or not :):

return msg, nil

yes I think this should return return msg, nil since this is the right case.

@sboeuf - Once that's done, is this good to merge?

@jodh-intel @devimc Yes after this is cleared up, I think we're good to merge this PR. Please could you implement like this in order to get all this clearer:

response, err := h.proxy.sendCmd(proxyCmd) if err != nil { return nil, err } if msg, ok := response.([]byte); !ok { return nil, fmt.Errorf("failed to get response message") } return msg, nil

@devimc any progress on this one ?

devimc · 2017-10-25T14:08:40Z

@jodh-intel @sboeuf changes applied , thanks

coveralls · 2017-10-25T14:14:38Z

Coverage decreased (-0.3%) to 65.658% when pulling b398ccc on devimc:topic/psCommand into 981b575 on containers:master.

sboeuf · 2017-10-25T14:38:40Z

LGTM

jodh-intel · 2017-10-25T14:44:42Z

agent.go

+	Args []string
+}
+
+// ProcessList represents the list of running process inside the container


s/process/processes/

jodh-intel · 2017-10-25T14:47:11Z

agent.go

+// ProcessListOptions contains the options used to list running
+// processes inside the container
+type ProcessListOptions struct {
+	// Format describes the output format to list the running processes


I originally thought this might be for ps(1) -o formats, so I think it would be helpful to state somehow that this is unrelated to ps(1) formats (and maybe list some). Looking at clearcontainers/agent#90, I'm wondering if it would be useful if the agent would default to table if Format="".

sure, lemme update the comment to specify format is unrelated to ps.
default value is set in the runtime [1]

[1] - https://github.com/clearcontainers/runtime/pull/444/files#diff-b985e723bb136b291af1a569f76616d4R32

jodh-intel · 2017-10-25T15:03:31Z

agent.go

+	// Format describes the output format to list the running processes
+	Format string
+
+	// Args contains the list of arguments to run ps command


Again, the agent PR shows that if this isn't set, it default to sane values so I wonder if it would be worth adding "(optional)" at the end of the comment here?

default value is set in the runtime https://github.com/clearcontainers/runtime/pull/444/files#diff-b985e723bb136b291af1a569f76616d4R67

jodh-intel · 2017-10-25T15:05:11Z

api.go

+
+// ProcessListContainer is the virtcontainers entry point to list
+// processes running inside a container
+func ProcessListContainer(podID, containerID string, options ProcessListOptions) (ProcessList, error) {


Since this changes the API, you'll need to also do the following:

interfaces.go: Add this function to the VC interface).

implementation.go: Add a simple wrapper like the one for SetLogger()).

pkg/vcMock/types.go: Again, add a function similar to SetLoggerFunc).

pkg/vcMock/mock.go: Add a simple wrapper like the one for SetLogger()).

pkg/vcMock/mock_test.go: Add a simple TestProcessListContainer().

done, thanks

Wow that was fast! 😄

jodh-intel · 2017-10-25T15:06:29Z

container.go

+	}
+
+	if state.State != StateRunning {
+		return nil, fmt.Errorf("Container not running, impossible to list processes")


Can you add c.id to the error here? Also, this is racy as the container might be running here, but by the time you call processListContainer() below it might have stopped.

jodh-intel · 2017-10-25T15:08:21Z

hyperstart.go

+
+	msg, ok := response.([]byte)
+	if !ok {
+		return nil, fmt.Errorf("failed to get response message")


Would be useful to have podID and cID in the error here.

jodh-intel · 2017-10-25T15:15:07Z

I should have said that above are addressed, I think this is good to land [1].

[1] - I suppose this could land before clearcontainers/agent#90 as the agent will just reject the unknown command.

devimc · 2017-10-25T16:05:36Z

@jodh-intel changes applied, thanks

ps command is used to list the processes running inside the container, this command is called by ```docker top``` partially fixes clearcontainers/runtime#95 Signed-off-by: Julio Montes <julio.montes@intel.com>

implement mock functions to test processListContainer Signed-off-by: Julio Montes <julio.montes@intel.com>

coveralls · 2017-10-25T16:52:43Z

Coverage decreased (-0.4%) to 65.619% when pulling ff99e51 on devimc:topic/psCommand into 981b575 on containers:master.

sboeuf reviewed May 23, 2017

View reviewed changes

sameo reviewed May 29, 2017

View reviewed changes

sameo added the do-not-merge label May 30, 2017

devimc force-pushed the topic/psCommand branch 2 times, most recently from 60c3834 to 01c1fee Compare August 22, 2017 16:50

devimc changed the title ~~[WIP] agent: implements ps command~~ agent: implement ps command Aug 22, 2017

devimc force-pushed the topic/psCommand branch from 01c1fee to 82477b6 Compare August 22, 2017 17:04

mcastelino removed the do-not-merge label Aug 22, 2017

devimc force-pushed the topic/psCommand branch from 82477b6 to 911e469 Compare August 22, 2017 19:22

devimc mentioned this pull request Aug 22, 2017

cli: implement ps command clearcontainers/runtime#444

Merged

sboeuf suggested changes Aug 23, 2017

View reviewed changes

devimc force-pushed the topic/psCommand branch from 911e469 to 3f520fc Compare August 28, 2017 16:54

devimc force-pushed the topic/psCommand branch from 3f520fc to 270f7e2 Compare September 18, 2017 16:06

jodh-intel reviewed Oct 9, 2017

View reviewed changes

devimc force-pushed the topic/psCommand branch from 270f7e2 to b398ccc Compare October 25, 2017 14:07

jodh-intel suggested changes Oct 25, 2017

View reviewed changes

devimc force-pushed the topic/psCommand branch from b398ccc to 9ac9ad4 Compare October 25, 2017 16:00

sboeuf approved these changes Oct 25, 2017

View reviewed changes

devimc force-pushed the topic/psCommand branch from 9ac9ad4 to 670000b Compare October 25, 2017 16:05

jodh-intel approved these changes Oct 25, 2017

View reviewed changes

agent: implement ps command

48667e6

ps command is used to list the processes running inside the container, this command is called by ```docker top``` partially fixes clearcontainers/runtime#95 Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the topic/psCommand branch 2 times, most recently from 72c9de6 to fe38e5a Compare October 25, 2017 16:43

mock: update mock API

ff99e51

implement mock functions to test processListContainer Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the topic/psCommand branch from fe38e5a to ff99e51 Compare October 25, 2017 16:47

sboeuf merged commit 20deadb into containers:master Oct 25, 2017

devimc deleted the topic/psCommand branch March 6, 2018 14:40

Conversation

devimc commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodh-intel commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sameo commented May 30, 2017

Uh oh!

jodh-intel commented Jul 5, 2017

Uh oh!

devimc commented Aug 22, 2017

Uh oh!

coveralls commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devimc commented Aug 28, 2017

Uh oh!

coveralls commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf commented Sep 1, 2017

Uh oh!

devimc commented Sep 18, 2017

Uh oh!

coveralls commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jodh-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devimc commented Oct 25, 2017

Uh oh!

coveralls commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf commented Oct 25, 2017 • edited by dlespiau Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devimc Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devimc commented May 23, 2017 •

edited

Loading

coveralls commented May 23, 2017 •

edited

Loading

coveralls commented Aug 22, 2017 •

edited

Loading

coveralls commented Aug 22, 2017 •

edited

Loading

coveralls commented Aug 28, 2017 •

edited

Loading

coveralls commented Sep 18, 2017 •

edited

Loading

coveralls commented Oct 25, 2017 •

edited

Loading

sboeuf commented Oct 25, 2017 •

edited by dlespiau

Loading

devimc Oct 25, 2017 •

edited

Loading

coveralls commented Oct 25, 2017 •

edited

Loading