Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
fuweid opened this issue Apr 9, 2022 · 14 comments
Open

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

fuweid opened this issue Apr 9, 2022 · 14 comments

Comments

@fuweid
Copy link
Member

fuweid commented Apr 9, 2022

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
Copy link
Member Author

fuweid commented Apr 12, 2022

ping @AkihiroSuda @kolyshkin @thaJeztah ~

@thaJeztah
Copy link
Member

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
Copy link
Member Author

fuweid commented Apr 13, 2022

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 opencontainers/runtime-spec#345 said the exec should be handled by implementation. 😂 2016

@cpuguy83
Copy link
Contributor

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
Copy link
Member

I found that the issue opencontainers/runtime-spec#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
Copy link
Member Author

fuweid commented Apr 19, 2022

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
Copy link
Member

SGTM

@fuweid
Copy link
Member Author

fuweid commented Apr 20, 2022

Thanks! I will file pr for this~

@cpuguy83
Copy link
Contributor

@fuweid Did you have a branch with this work?

@fuweid
Copy link
Member Author

fuweid commented Aug 17, 2022

@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
Copy link

Any progress on this issue?

@pmengelbert
Copy link

@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
Copy link
Member Author

fuweid commented Jun 10, 2024

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

@silence-coding
Copy link

@pmengelbert Excuse me, Any progress on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants
@cpuguy83 @thaJeztah @AkihiroSuda @fuweid @silence-coding @pmengelbert and others