-
Notifications
You must be signed in to change notification settings - Fork 127
interface additions: kill -a flag, ps command, pause & resume #478
Conversation
|
Added a pause & resume implementation. Same limitation as kill -a, I think an interface need to be added to hyperstart to return info about running processes to runv. |
|
ping @tvelocity , I think you need to do a rebase, the client lib has changed. 🐱 |
Adds the -a parameter to the kill runv command, allowing to kill all processes in the container. Signed-off-by: Antonios Motakis <[email protected]>
The PS command is currently limited by the runv-containerd interfacer. Until runv and hyperstart is refactored, print out the information that the current design can support. Signed-off-by: Antonios Motakis <[email protected]>
Implement the pause and resume commands of OCI. According to the spec, pause suspends all processes in the container, and resume resumes them. Signed-off-by: Antonios Motakis <[email protected]>
|
The merge request has been updated with a rebase onto the latest master |
| if err != nil { | ||
| fatal(err) | ||
| } | ||
| os.Stdout.Write(data) |
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.
Should we print more information such as command args?
Never mind, this format is right.
But there's another problem, the pids should be '[]int' not '[]string', and it must be real pid of int type, now I can see pid "init" which makes docker top not work with runv
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.
You should use p.SystemPid instead of p.Pid, it works fine according to my test
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.
SystemPid reports the PID of the qemu process on the host for the container's main process, and AFAIU is invalid for the rest. See my original comment on this topic, in the first comment of this thread
Runv currently implements the OCI interface by wrapping runv-containerd calls, so we are limited by the containerd interface as supported by runv at the moment.
This introduces some limitations on these implementations, which to resolve would require the refactoring or runv, which IMHO should be discussed.First, the processes runv is aware of from each container is limited to only the processes started directly from runv (via run and exec). Children are not being tracked. An extension in hyperstart would be needed to overcome this limitation (to always fetch the process list dynamically).
Additionally, runv-containerd reports only limited information about the processes running inside the container. We can not show the PID for example. I'm not sure whether extending hyperstart to provide this info is enough, as we need to consider how it will affect runv-containerd. Even then, we will be limited by the current fields in the containerd API. In any case runv-containerd and the runv OCI interface need to be considered at the same time to improve upon this point.
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.
@tvelocity SystemPid reports PID of "nslistener" but not the qemu process, this is what we want. Because we need to make the nslistener a pseudo-container for network management. In more detail, we report "nslistener" pid as container init pid so that docker will treat "nslistener" as it's container, when CNI network plugins try to insert a network interface into the "nslistener" namespace, "nslistener" will know it and copy the network configuration into VM. This is how the network part works in runv.
PIDs in guestOS of VM is unmeaning to docker or other processes in host.
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 will merge it although the problems your reported are not addressed... You can fix it later.
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.
@WeiZhang555 ok, thanks for the clarification
|
It seems the pr is ready to be reviewed/merged. I will remove the |
|
Good job. thanks. |
interface additions: kill -a flag, ps command, pause & resume
These two patches add two runc features to runv; the --all flag to kill, and the PS command. Implementing these interfaces, reveals some limitations with the current design.
Runv currently implements the OCI interface by wrapping runv-containerd calls, so we are limited by the containerd interface as supported by runv at the moment.
This introduces some limitations on these implementations, which to resolve would require the refactoring or runv, which IMHO should be discussed.
First, the processes runv is aware of from each container is limited to only the processes started directly from runv (via run and exec). Children are not being tracked. An extension in hyperstart would be needed to overcome this limitation (to always fetch the process list dynamically).
Additionally, runv-containerd reports only limited information about the processes running inside the container. We can not show the PID for example. I'm not sure whether extending hyperstart to provide this info is enough, as we need to consider how it will affect runv-containerd. Even then, we will be limited by the current fields in the containerd API. In any case runv-containerd and the runv OCI interface need to be considered at the same time to improve upon this point.