Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

grpc: implement ListProcesses #194

Merged
merged 4 commits into from
Apr 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@
package main

import (
"bytes"
"encoding/json"
"fmt"
"io/ioutil"
"os"
"os/exec"
"path/filepath"
"regexp"
"strconv"
"strings"
"sync"
"syscall"
Expand Down Expand Up @@ -623,6 +627,97 @@ func (a *agentGRPC) WaitProcess(ctx context.Context, req *pb.WaitProcessRequest)
}, nil
}

func getPIDIndex(title string) int {
// looking for PID field in ps title
fields := strings.Fields(title)
for i, f := range fields {
if f == "PID" {
return i
}
}
return -1
}

func (a *agentGRPC) ListProcesses(ctx context.Context, req *pb.ListProcessesRequest) (*pb.ListProcessesResponse, error) {
resp := &pb.ListProcessesResponse{}

c, err := a.sandbox.getContainer(req.ContainerId)
if err != nil {
return resp, err
}

// 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()
if err != nil {
return resp, err
}

switch req.Format {
case "table":
case "json":
resp.ProcessList, err = json.Marshal(pids)
return resp, err
default:
return resp, fmt.Errorf("invalid format option")
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean if the format is not supported ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, table doesn't have a return

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes I was assuming a fallthrough here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

psArgs := req.Args
if len(psArgs) == 0 {
psArgs = []string{"-ef"}
}

// All container's processes are visibles from agent's namespace.
// 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...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

output, err := a.sandbox.subreaper.combinedOutput(cmd)
if err != nil {
return nil, fmt.Errorf("%s: %s", err, output)
}

lines := strings.Split(string(output), "\n")

pidIndex := getPIDIndex(lines[0])

// PID field not found
if pidIndex == -1 {
return nil, fmt.Errorf("failed to find PID field in ps output")
}

// append title
var result bytes.Buffer

result.WriteString(lines[0] + "\n")

for _, line := range lines[1:] {
if len(line) == 0 {
continue
}
fields := strings.Fields(line)
if pidIndex >= len(fields) {
return nil, fmt.Errorf("missing PID field: %s", line)
}

p, err := strconv.Atoi(fields[pidIndex])
if err != nil {
return nil, fmt.Errorf("failed to convert pid to int: %s", fields[pidIndex])
}

// appends pid line
for _, pid := range pids {
if pid == p {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

result.WriteString(line + "\n")
break
}
}
}

resp.ProcessList = result.Bytes()
return resp, nil
}

func (a *agentGRPC) RemoveContainer(ctx context.Context, req *pb.RemoveContainerRequest) (*gpb.Empty, error) {
ctr, err := a.sandbox.getContainer(req.ContainerId)
if err != nil {
Expand Down
71 changes: 71 additions & 0 deletions grpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,3 +155,74 @@ func TestOnlineCPUMem(t *testing.T) {
_, err = a.OnlineCPUMem(context.TODO(), req)
assert.NoError(err)
}

func TestGetPIDIndex(t *testing.T) {
assert := assert.New(t)

title := "UID PID PPID C STIME TTY TIME CMD"
pidIndex := 1
index := getPIDIndex(title)
assert.Equal(pidIndex, index)

title = "PID PPID C STIME TTY TIME CMD"
pidIndex = 0
index = getPIDIndex(title)
assert.Equal(pidIndex, index)

title = "PPID C STIME TTY TIME CMD PID"
pidIndex = 6
index = getPIDIndex(title)
assert.Equal(pidIndex, index)

title = "PPID C STIME TTY TIME CMD"
pidIndex = -1
index = getPIDIndex(title)
assert.Equal(pidIndex, index)
}

func TestListProcesses(t *testing.T) {
containerID := "1"
assert := assert.New(t)
req := &pb.ListProcessesRequest{
ContainerId: containerID,
Format: "table",
Args: []string{"-ef"},
}

a := &agentGRPC{
sandbox: &sandbox{
containers: make(map[string]*container),
subreaper: &mockreaper{},
},
}
// getContainer should fail
r, err := a.ListProcesses(context.TODO(), req)
assert.Error(err)
assert.NotNil(r)

// should fail, unknown format
req.Format = "unknown"
a.sandbox.containers[containerID] = &container{
container: &mockContainer{
id: containerID,
processes: []int{1},
},
}
r, err = a.ListProcesses(context.TODO(), req)
assert.Error(err)
assert.NotNil(r)

// json format
req.Format = "json"
r, err = a.ListProcesses(context.TODO(), req)
assert.NoError(err)
assert.NotNil(r)
assert.NotEmpty(r.ProcessList)

// table format
req.Format = "table"
r, err = a.ListProcesses(context.TODO(), req)
assert.NoError(err)
assert.NotNil(r)
assert.NotEmpty(r.ProcessList)
}
92 changes: 92 additions & 0 deletions mockcontainer.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
//
// Copyright (c) 2018 Intel Corporation
//
// SPDX-License-Identifier: Apache-2.0
//

package main

import (
"os"

"github.com/opencontainers/runc/libcontainer"
"github.com/opencontainers/runc/libcontainer/configs"
)

type mockContainer struct {
id string
status libcontainer.Status
processes []int
}

func (m *mockContainer) ID() string {
return m.id
}

func (m *mockContainer) Status() (libcontainer.Status, error) {
return m.status, nil
}

func (m *mockContainer) State() (*libcontainer.State, error) {
return nil, nil
}

func (m *mockContainer) Config() configs.Config {
return configs.Config{}
}

func (m *mockContainer) Processes() ([]int, error) {
return m.processes, nil
}

func (m *mockContainer) Stats() (*libcontainer.Stats, error) {
return nil, nil
}

func (m *mockContainer) Set(config configs.Config) error {
return nil
}

func (m *mockContainer) Start(process *libcontainer.Process) (err error) {
return nil
}

func (m *mockContainer) Run(process *libcontainer.Process) (err error) {
return nil
}

func (m *mockContainer) Destroy() error {
return nil
}

func (m *mockContainer) Signal(s os.Signal, all bool) error {
return nil
}

func (m *mockContainer) Exec() error {
return nil
}

func (m *mockContainer) Checkpoint(criuOpts *libcontainer.CriuOpts) error {
return nil
}

func (m *mockContainer) Restore(process *libcontainer.Process, criuOpts *libcontainer.CriuOpts) error {
return nil
}

func (m *mockContainer) Pause() error {
return nil
}

func (m *mockContainer) Resume() error {
return nil
}

func (m *mockContainer) NotifyOOM() (<-chan struct{}, error) {
return nil, nil
}

func (m *mockContainer) NotifyMemoryPressure(level libcontainer.PressureLevel) (<-chan struct{}, error) {
return nil, nil
}
Loading