-
Notifications
You must be signed in to change notification settings - Fork 114
grpc: implement ListProcesses #194
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,15 @@ | |
package main | ||
|
||
import ( | ||
"bytes" | ||
"encoding/json" | ||
"fmt" | ||
"io/ioutil" | ||
"os" | ||
"os/exec" | ||
"path/filepath" | ||
"regexp" | ||
"strconv" | ||
"strings" | ||
"sync" | ||
"syscall" | ||
|
@@ -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") | ||
} | ||
|
||
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...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is of course assuming that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with gwhaley. Also, we should really avoid execing if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so the user can supply arguments to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, you can specify ps options using
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with runc
with kata-runtime
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docker exec with runc
docker exec with kata-runtime
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh. I see. That is interesting - that it seems There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
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 | ||
} |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 arereturn
for each statement, includingdefault
. How do you expect the code further down to be reached ?There was a problem hiding this comment.
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 areturn
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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 fortable
is below theswitch
.