opencontainers / runc

CLI tool for spawning and running containers according to the OCI specification
https://www.opencontainers.org/
Apache License 2.0
11.77k stars 2.09k forks source link

Feature Request: Support Two Phases to Start Exec Process like Init #3453

Open fuweid opened 2 years ago

fuweid commented 2 years ago

Currently, the exec-process is created by runc-exec one command. The common container engine layer will care about the exit code of exec-process, which required that the runc-exec's parent process must be the subreaper.

Since pidfd_open(2) is available, we can watch the exit event by pidfd and retrieve the exit code provided bpf sched_process_exit tracepoint. The PID=1, like systemd, will be reaper of exec-process. So, the common container engine is not required to be subreaper of exec-process, like what embedshim containerd plugin does. However, non-subreaper mode requires that exec-process starts in two phases.

  1. Fork: Setup and waiting for the exec.fifo event

    • container engine opens pidfd and trace it by eBPF
  2. Exec: Signal the init and start to exec

Currently, embedshim uses runc-exec wrapper command to be temporary subreaper to sync the status, like:

[ runc-exec-ext(child, after finish runc-exec)]                 [     embedshim(parent)    ] 

    SyncExecPid                           -->              Read exec-process pid

                                                      <--                SyncExecPidDone

    SyncExecPidStatus                             -->            Get exec-process current status

                                 <--                SyncExecPidStatusDone

It is heavy mode to start exec-process for non-subreaper, so I file this issue to request the feature for two phases to exec-process:

Looking foraward to the feedback! Thanks!

fuweid commented 2 years ago

ping @AkihiroSuda @kolyshkin @thaJeztah ~

thaJeztah commented 2 years ago

These commands would come in addition to the existing command?

My first thought was "should this be defined in the runtime spec?", but it looks like exec is not described in the spec (unless I missed it 🤔); https://github.com/opencontainers/runtime-spec/blob/v1.0.2/runtime.md#lifecycle

fuweid commented 2 years ago

These commands would come in addition to the existing command?

I didn't find out a good way to work with current commandline 😂 I would like to use new sub-commands for this.

My first thought was "should this be defined in the runtime spec?", but it looks like exec is not described in the spec (unless I missed it 🤔); https://github.com/opencontainers/runtime-spec/blob/v1.0.2/runtime.md#lifecycle

I found that the issue https://github.com/opencontainers/runtime-spec/issues/345 said the exec should be handled by implementation. 😂 2016

cpuguy83 commented 2 years ago

I would also like to see this. One thing I found when working on https://github.com/cpuguy83/containerd-shim-systemd-v1 is if a command exits quickly enough this can cause issues with systemd being able to attach to the process at all because by the time it can get the pid the process has already exited.

thaJeztah commented 2 years ago

I found that the issue https://github.com/opencontainers/runtime-spec/issues/345 said the exec should be handled by implementation. 😂 2016

Thanks for digging that up (I could've sworn exec was described, but from that link I see it was removed from the spec)

So, yes, I'm not against adding this

Let me also /cc @mrunalp (I see he was on that original discussion)

fuweid commented 2 years ago

if a command exits quickly enough this can cause issues with systemd being able to attach to the process at all because by the time it can get the pid the process has already exited.

Yes. That is why I want it to be like init process.

So, yes, I'm not against adding this

@thaJeztah Thanks!

ping @AkihiroSuda @kolyshkin and @mrunalp ~

AkihiroSuda commented 2 years ago

SGTM

fuweid commented 2 years ago

Thanks! I will file pr for this~

cpuguy83 commented 2 years ago

@fuweid Did you have a branch with this work?

fuweid commented 2 years ago

@fuweid Did you have a branch with this work?

Sorry for late reply. I was distracted to handle other things. Sorry for that. I will file pr soon. If you already have one, please feel free to open it. Thanks.

silence-coding commented 5 months ago

Any progress on this issue?

pmengelbert commented 3 months ago

@fuweid it looks like progress on this is stalled. I am considering taking it on -- do you still need this feature? If so, we should coordinate on requirements. Otherwise we'll just move forward if that's alright with you

@cpuguy83

fuweid commented 3 months ago

@pmengelbert sorry for that. please feel free to carry this. Thanks

silence-coding commented 3 weeks ago

@pmengelbert Excuse me, Any progress on this issue?