runc icon indicating copy to clipboard operation
runc copied to clipboard

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

Open fuweid opened this issue 3 years ago • 14 comments
trafficstars

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:

  • runc exec-create
  • runc exec-start

Looking foraward to the feedback! Thanks!

fuweid avatar Apr 09 '22 11:04 fuweid

ping @AkihiroSuda @kolyshkin @thaJeztah ~

fuweid avatar Apr 12 '22 01:04 fuweid

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

thaJeztah avatar Apr 12 '22 09:04 thaJeztah

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

fuweid avatar Apr 13 '22 01:04 fuweid

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.

cpuguy83 avatar Apr 14 '22 19:04 cpuguy83

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)

thaJeztah avatar Apr 14 '22 20:04 thaJeztah

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 ~

fuweid avatar Apr 19 '22 15:04 fuweid

SGTM

AkihiroSuda avatar Apr 20 '22 06:04 AkihiroSuda

Thanks! I will file pr for this~

fuweid avatar Apr 20 '22 12:04 fuweid

@fuweid Did you have a branch with this work?

cpuguy83 avatar Aug 15 '22 21:08 cpuguy83

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

fuweid avatar Aug 17 '22 14:08 fuweid

Any progress on this issue?

silence-coding avatar Apr 23 '24 04:04 silence-coding