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

Exec semantics #345

Closed
vishh opened this issue Mar 16, 2016 · 18 comments
Closed

Exec semantics #345

vishh opened this issue Mar 16, 2016 · 18 comments
Assignees

Comments

@vishh
Copy link
Contributor

vishh commented Mar 16, 2016

The current Spec attempts to express exec functionality by splitting the container into a sandbox and a process. This split fails to address some of the use cases like as follows -

  1. Imposing additional resource restrictions on the exec process.
  2. Imposing alternate/additional security policy on the exec process.

It is proving to be difficult to represent all the use cases and also preserve the semantics around the sandbox and the process.
Have we considered representing exec as a separate container with a process? That separate container can share some parts of the existing container's sandbox.
IIRC, the Spec tried to represent exec as a separate container in the beginning, but I cannot recall when it diverged from the original plan.

cc @crosbymichael @mrunalp

@wking
Copy link
Contributor

wking commented Mar 16, 2016

On Wed, Mar 16, 2016 at 12:37:22PM -0700, Vish Kannan wrote:

IIRC, the Spec tried to represent exec as a separate container in
the beginning, but I cannot recall when it diverged from the
original plan.

‘exec’ only landed three weeks ago with #225, so there's not a lot of
history in the spec itself. There has been wording that looks sort of
like separate create / exec since 77d44b1 (Update runtime.md,
2015-06-16) 1, but that's from the ancient past before changes were
publically motivated and the wording went away with the initial
lifecycle docs in #231.

Public out-of-spec discussion around separating sandbox creation from
process execution goes back a lot further (e.g. see [2,3,4,5]). I
think the main point in favor of distinguishing between sandboxes
(create) and processes (exec) is that it makes it easy to launch new
processes in an existing container 6.

Have we considered representing exec as a separate container with
a process?

I think the namespace creation and cgroup joining in the current spec
should allow you to:

  1. Join an existing container (or not).
  2. Create a new container (or not).
  3. Execute a new process inside (1) and/or (2).

As far as cgroups are concerned, join-and-create-a-subgroup is easy
with cgroupsPath (just use ‘{existing-parent-path}/{new-child-path}’,
see #68). Namespaces are a bit trickier, because of the very-narrow
visibility scoping in the state registry [7,8]. It's possible that:

$ nsenter funC start .

would create a new container whose state was only accessible via:

$ nsenter funC state

and not directly via:

$ funC state

An extra nsenter call doesn't seem like that big a deal though.

And this sort of nested-namespace approach will also make clarifying
PID namespaces more important 9.

 Subject: Re: Specifying the runtime's command-line interface
 Date: Wed, 26 Aug 2015 00:01:48 -0700 (PDT)
 Message-Id: <[email protected]>

 Date: Wed, 26 Aug 2015 13:40:47 -0700
 Subject: Splitting container.json from application.json
 Message-ID: <[email protected]>

 Subject: Documenting container lifecycles
 Date: Mon, 14 Sep 2015 20:47:17 -0700
 Message-ID: <[email protected]>





 Subject: Linux: Adding the PID namespace inode to state JSON
 Date: Wed, 30 Dec 2015 21:34:57 -0800
 Message-ID: <[email protected]>

@vishh
Copy link
Contributor Author

vishh commented Mar 16, 2016

Thanks for the detailed response @wking!
To rephrase, I guess my proposal is to not include exec as a first class entity in the Spec and instead define a standard way for implementing it using the Spec.

@crosbymichael
Copy link
Member

@vishh i have always said i don't think exec should be in the spec because ppl want too many different things for it. This would be a much better solution instead of trying to code in every use case into this one feature.

@vishh
Copy link
Contributor Author

vishh commented Mar 16, 2016

@crosbymichael: Yeah. Agreed. It's one thing to come up with a suggestion for implementing exec, but making it a standardized feature doesn't make sense.

ping @mrunalp @duglin @julz for input here.

@crosbymichael
Copy link
Member

@vishh same. just because we choose not to standardize on exec because of the variations does not mean runtimes cannot implement it, it means they have more freedom to implement it to their specific requirements.

@wking
Copy link
Contributor

wking commented Mar 16, 2016

On Wed, Mar 16, 2016 at 04:02:51PM -0700, Vish Kannan wrote:

To rephrase, I guess my proposal is to not include exec as a first
class entity in the Spec and instead define a standard way for
implementing it using the Spec.

This is fine with me, but see also [1,2,3]. ‘exec’ didn't get much
airtime in #225 (I don't see a reply to 4). Whichever way we go, it
would be nice if changes to operations landed with clear motivation in
the associated commit and/or PR, so we don't remove operations within
a month of adding them as frequently ;).

@mrunalp
Copy link
Contributor

mrunalp commented Mar 17, 2016

@vishh I agree with @crosbymichael. I think there are too many variations and it might make sense to leave it up to the runtimes to implement it as required.

@julz
Copy link
Contributor

julz commented Mar 17, 2016

yeah I'm slowly changing my mind on this :).

I was initially in favour of standardising exec because it makes certain common things super easy (and as a higher-level system that wants the freedom to easily swap out runtimes, I really only want to rely on standardised bits). But I can't think of a case where there's something I can achieve with exec that I can't achieve by creating a second container that shares all the namespaces/cgroups/etc of the main container -- and with more power especially in the case of cgroups. So maybe a high level system that wants to support the maximum number of backends can/should just rely on create and ignore exec entirely. And @crosbymichael / @vishh are clearly right that the further we push on exec the closer we get to a bad version of just doing that in the first place :).

So +1 from me, as long as we agree (I think everyone does anyway?) that anything possible with exec should always also be possible via create/start with an appropriate config.

@vishh
Copy link
Contributor Author

vishh commented Mar 17, 2016

@julz @mrunalp: Appreciate the feedback. If @duglin also doesn't have any objections to this change, I can post a patch to make exec not part of the UX and add documentation explaining how it can be achieved using the Spec.

@duglin
Copy link
Contributor

duglin commented Mar 17, 2016

@vishh as @julz said, as long as we can get the same net result with a secondary create/start then I'm ok with it. After reading your proposed doc changes for how all of this will be expressed in the config.json I'd like to explore some runc command line niceties to make it easier for people - or at least try, but that's secondary.

@mrunalp
Copy link
Contributor

mrunalp commented Mar 17, 2016

@duglin The way I see it, I think runc can continue doing what it is doing today and then add additional features as necessary.

@duglin
Copy link
Contributor

duglin commented Mar 17, 2016

@mrunalp do you mean expose it as "runc exec .." but under the covers do some kind of "join" to the existing namespaces/etc... ?

@mrunalp
Copy link
Contributor

mrunalp commented Mar 17, 2016

@duglin Yes.

@duglin
Copy link
Contributor

duglin commented Mar 17, 2016

@mrunalp that would be excellent! Does this mean we're closer to adopting the create/start split? :-)

@mrunalp
Copy link
Contributor

mrunalp commented Mar 17, 2016

@duglin Still needs to be reviewed :D However, exec can be achieved either ways :)

@julz
Copy link
Contributor

julz commented Mar 17, 2016

Yeah to be clear I think runC exec should stay, just I agree we don't need to specify it. Imo instead we should doc (in the spec) how to specify a config to exec a process in an existing container. Also seems potentially useful either in ocitools or runC to have a command to generate an exec config for a running container (but this isn't super hard to do manually anyway).

@wking
Copy link
Contributor

wking commented Apr 20, 2016

After #388, is there anything left to do here?

@hqhq
Copy link
Contributor

hqhq commented Aug 7, 2016

@wking It should be safe to close now.

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