fix(cmd): add ps command for containerd recovery#601
fix(cmd): add ps command for containerd recovery#601cmainas merged 1 commit intourunc-dev:main-pr601from
Conversation
✅ Deploy Preview for urunc canceled.
|
5e34c4d to
358ca69
Compare
|
Hello @sidneychang , let's discuss a bit about the root cause of the issue over the issue first. |
cmainas
left a comment
There was a problem hiding this comment.
Hello @sidneychang ,
overall the PR looks good. A few comments:
- please use sandbox monitor instead of VMM to also include non-VM based monitors.
- there is the case where we spawn virtiofsd for some containers. This process is tightly connected with the monitor though. There is also the case where a monitor might spawn sub-processes. I think in that case we should also return these PIDs. However, this will not be straightforward without cgroups.
Would you like to work on the extra PIDs task as well in this PR or should we address it in a different iteration?
|
|
||
| var psCommand = &cli.Command{ | ||
| Name: "ps", | ||
| Usage: "displays the host-visible VMM processes associated with a container", |
There was a problem hiding this comment.
VMM -> monitor . There is support for non-VM monitors too in urunc.
| Usage: "displays the host-visible VMM processes associated with a container", | ||
| ArgsUsage: `<container-id>`, | ||
| Description: `The ps command displays the host-visible process IDs associated | ||
| with a urunc container. For VM-based urunc containers this returns the VMM / |
There was a problem hiding this comment.
We can remove this For VM-based urunc containers to not exclude other non-VM based monitors.
| // | ||
| // Keep the return value as []int to match runc's ps implementation | ||
| // and containerd/go-runc's expectation for `ps --format json`. | ||
| pids := []int{unikontainer.State.Pid} |
There was a problem hiding this comment.
There is also the case of virtiofsd, which is another process that starts for some urunc containers. We should also include this Pid here. However, this will require for a better bookkeeping of the Pids.
5e03afe to
3b8d82f
Compare
|
Thanks for the review @cmainas. Regarding whether we should also return virtiofsd and subprocesses spawned by the monitor, I agree this is a valid scenario to consider. However, I would prefer to discuss and handle it separately as a follow-up. My main concern is that urunc currently only persists the main monitor PID. For example, virtiofsd is started before execing the monitor, but its PID is not currently recorded separately, nor is it strongly tied to the monitor through cgroups or a similar mechanism. Therefore, I am not sure whether we can guarantee in all abnormal cases that helper processes will always exit once the main monitor exits. If we include these helper/subprocess PIDs directly in the ps result now, I think we first need to clearly define their semantics relative to the main monitor PID, and decide how to handle the case where the main monitor no longer exists but some helper processes may still be alive. Otherwise, upper layers may see PIDs that do not really represent the liveness of the main monitor/container task. So I think it would be better to treat this as a follow-up item. |
Ok let's do it that way.
This is one of the reasons that in urunc's execution model the monitor process has PID 1 in the PID namespaces and acts as the int. If it exits all other process will exit too. See https://man7.org/linux/man-pages/man7/pid_namespaces.7.html |
|
@cmainas Thanks, that makes sense. |
|
Hello @sidneychang . Could you rebase over main so we can merge this? |
3b8d82f to
6d84825
Compare
|
Hello @cmainas, I’ve rebased it over main. |
Add a runc-compatible `ps --format json` command to urunc.
containerd-shim-urunc-v2 reuses the runc shim manager and task
service. The Pids() path eventually invokes the runtime binary as:
urunc ps --format json <container-id>
Without this command, containerd cannot obtain the host-visible PID
associated with the urunc task.
Return the monitor PID stored in state.json as a []int, matching the
JSON format expected by containerd/go-runc and runc's `ps`
implementation. Keep the result as a slice so it can be extended later
if urunc needs to report additional monitor-related PIDs for a
container.
Signed-off-by: sidneychang <2190206983@qq.com>
cmainas
left a comment
There was a problem hiding this comment.
Thank you @sidneychang for this.
Add a runc-compatible `ps --format json` command to urunc.
containerd-shim-urunc-v2 reuses the runc shim manager and task
service. The Pids() path eventually invokes the runtime binary as:
urunc ps --format json <container-id>
Without this command, containerd cannot obtain the host-visible PID
associated with the urunc task.
Return the monitor PID stored in state.json as a []int, matching the
JSON format expected by containerd/go-runc and runc's `ps`
implementation. Keep the result as a slice so it can be extended later
if urunc needs to report additional monitor-related PIDs for a
container.
PR: #601
Signed-off-by: sidneychang <2190206983@qq.com>
Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk>
Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>
Add a runc-compatible `ps --format json` command to urunc.
containerd-shim-urunc-v2 reuses the runc shim manager and task
service. The Pids() path eventually invokes the runtime binary as:
urunc ps --format json <container-id>
Without this command, containerd cannot obtain the host-visible PID
associated with the urunc task.
Return the monitor PID stored in state.json as a []int, matching the
JSON format expected by containerd/go-runc and runc's `ps`
implementation. Keep the result as a slice so it can be extended later
if urunc needs to report additional monitor-related PIDs for a
container.
PR: #601
Signed-off-by: sidneychang <2190206983@qq.com>
Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk>
Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>
Description
Add a runc-compatible
ps --format jsoncommand to urunc.containerd-shim-urunc-v2 reuses the runc shim manager and task service. During containerd restart, the recovery path calls Pids(), which eventually invokes the runtime binary as:
Without this command, containerd cannot obtain the host-visible PID associated with the urunc task and may treat the shim as leaked.
Return the VMM / sandbox monitor PID stored in state.json as a []int, matching the JSON format expected by containerd/go-runc and runc's ps implementation. Keep the result as a slice so it can be extended later if urunc supports multiple VMM or unikernel processes for one container.
Related issues
How was this tested?
Before the fix
The urunc container was running before restarting containerd:
After restarting containerd:
the same container became Created:
The state stayed Created on the next check:
The urunc shim process was still alive:
After stopping and removing the container:
the shim was still left behind:
At that point, the container metadata had already disappeared:
This shows the failure mode before the fix: containerd restart caused the urunc container to fall from Up to Created, and cleanup left a live containerd-shim-urunc-v2 process behind.
After the fix
After replacing the binaries with the fixed version, a new urunc container was started:
The container was Up before restarting containerd:
After restarting containerd:
the container remained Up:
The container was then stopped and removed normally:
After removal, no urunc shim was left behind; only the grep process appeared:
LLM usage
ChatGPT
Checklist
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).