Add a 'start' command #210

duglin · 2015-08-18T01:34:50Z

When any non-global-flag parameter appears on the command line make sure
there's a "command" even in the 'start' (run) case to ensure its not
ambiguous as to what the arg is. For example, w/o this fix its not
clear if

   runc foo

means 'foo' is the name of a config file or an unknown command. Or worse,
you can't name a config file the same as ANY command, even future (yet to
be created) commands.

We should fix this now before we ship 1.0 and are forced to support this
ambiguous case for a long time.

Signed-off-by: Doug Davis [email protected]

dqminh · 2015-08-18T10:09:08Z

code LGTM

I'm not sure about exec name though, I would prefer start as that's the word we use in the specs.

wking · 2015-08-18T15:49:25Z

On Mon, Aug 17, 2015 at 06:34:52PM -0700, Doug Davis wrote:

For example, w/o this fix its not
clear if
   runc foo
means 'foo' is the name of a config file or an unknown command. Or
worse, you can't name a config file the same a ANY command, even
future (yet to be created) commands.

In #200, I've been arguing that we should handle this sort of thing by
adjusting the config file, and possibly feeding the adjusted config in on
stdin (#202). That's all we need for this use case, and adding
additional syntactic sugar for “common” replacements just makes it
harder for folks to write alternative opencontainers/specs
implementations that you can drop in as runC replacements. Since the
intention for runC and other opencontainer/specs implementations is to
be low-level tooling 1, I don't think the alter-and-pipe approach is
too awkward.

For an example with a hard-coded command replacement, see config.json
and config-sh.json in 2.

wking · 2015-08-18T15:54:30Z

And I haven't compared the backing code to see if the intended
semantics are the same, but #205 is also talking about adding an
‘exec’ command.

duglin · 2015-08-18T16:01:21Z

@wking oops, I didn't notice #205 but he did it slightly differently because it still keeps the existing code, which means there's a bit of duplication. @tonistiigi am I reading your PR correctly? If so, I'm ok with closing this PR if you want to remove the dup code in ours. Thoughts?

duglin · 2015-08-18T16:04:29Z

Also, @tonistiigi I think you'll need to update the help/docs too if we go with your PR.

w.r.t #200 I think that's a slightly different usecase. I view that as trying to manipulate, or inject, data into the processing of a new container (which I like the idea of), but this PR is more at a higher level because its about just asking for runc to even start a container, not what happens after that.

wking · 2015-08-18T16:21:37Z

On Tue, Aug 18, 2015 at 09:04:31AM -0700, Doug Davis wrote:

w.r.t #200 I think that's a slightly different usecase.

Ah, I'd misread your initial post's “means 'foo' is the name of … an
unknown command” to mean “a command executed in the container” instead
of “a runC action”. So I think this PR is an improvement on what we
do now, but I'd be happier moving the config override to an option
that we can share between commands, instead of using a positional
argument 1:
$ runc --config config.json exec

which would give us a uniform syntax for overriding the config file
that we could also use with the other commands (checkpoint, restore,
events, …), while leaving stdin free (for example, if we wanted to
launch an container with an interactive terminal).

duglin · 2015-08-18T16:24:03Z

@wking totally agree and was going to issue a PR for that later on - but one step at a time :-)

tonistiigi · 2015-08-18T16:31:53Z

@duglin we seem to be implementing different functionality with the same name. My PR is for docker exec kind of functionality for running new processes inside a container that already exists.

duglin · 2015-08-18T16:33:31Z

@tonistiigi ah totally missed that. And 'exec' is better for yours than mine. So let me rename mine to 'start' per @dqminh's comment. Thanks for the clarification.

dqminh · 2015-08-18T16:35:41Z

In context of runc, start and exec do not differ too much I think exec allows you to specify a process and that's it. However separate those for clarification purpose makes sense to me.

wking · 2015-08-18T16:37:01Z

On Tue, Aug 18, 2015 at 09:31:56AM -0700, Tõnis Tiigi wrote:

@duglin we seem to implementing different functionality with the
same name. My PR is for docker exec kind of functionality for
running new processes inside a container that already exists.

Maybe distinguish by using ‘start’ here 1 and ‘enter’ (echoing
util-linux's nsenter) for #205?

duglin · 2015-08-18T16:38:14Z

made the 'exec' -> 'start' change.

wking · 2015-08-18T16:42:56Z

On Tue, Aug 18, 2015 at 09:35:43AM -0700, Daniel, Dao Quang Minh wrote:

In context of runc, start and exec do not differ too much…

No? One means “create new namespaces, cgroups, mounts, etc. and
launch a new process in that environment”, while the other means
“launch a new process in an existing environment”. While thinking
about naming precedents, iproute2's ‘ip netns’ uses ‘add’ for “create”
and ‘exec’ for “enter and launch a process”. ‘ip netns’ doesn't have
a single command for “create, enter, and launch a process”, do we
have to have a single command for that (maybe cgroups or some such
evaporate if they have no processes using them)?

duglin · 2015-08-18T16:55:48Z

well, if we think we may want to have a 'create' command at some point, which does seem to make sense, then 'start' for create+start doesn't seem right since 'start' should be to start an existing container.
I want to use 'run' but of course "runc run" seems a bit funky. But I'm also having trouble thinking of a better word. Someone mentioned using "enter" instead of "exec" in the other PR but I think "exec" is a better UX choice. Open to suggestions....

wking · 2015-08-18T17:02:48Z

On Tue, Aug 18, 2015 at 09:55:49AM -0700, Doug Davis wrote:

well, if we think we may want to have a 'create' command at some
point, which does seem to make sense, then 'start' for create+start
doesn't seem right since 'start' should be to start an existing
container.

If we get a ‘create’ command, I'd just drop the “create and exec”
helper and have folks start a container with:

$ runc create && runc exec

or:

$ runc --config config.json create && runc --config config.json exec

Someone mentioned using "enter" instead of "exec" in the other PR
but I think "exec" is a better UX choice.

I floated ‘enter’ earlier too 1, but you're probably right about
‘exec’ being more appriate for “launch a process in an existing
environment”.

duglin · 2015-08-18T17:05:03Z

I was afraid you'd suggest that :-) While from a dev perspective having it be two commands works, I think dropping the UX benefit of merging the two would probably be a mistake.

wking · 2015-08-18T17:29:49Z

On Tue, Aug 18, 2015 at 10:05:06AM -0700, Doug Davis wrote:

I think dropping the UX benefit of merging the two would probably be
a mistake.

This is probably food for the mailing list. This PR looks good to me
;).

wking · 2015-08-18T18:01:06Z

On Tue, Aug 18, 2015 at 10:27:47AM -0700, W. Trevor King wrote:

This is probably food for the mailing list.

I spun off the larger “do we want a runtime-user API specification,
and what should it focus on?” question into 1.

 Message-ID: <[email protected]>

laijs · 2015-08-19T01:43:52Z

@codegangsta From codegangsta/cli.git
CMD --global-options -- FOO other-argument
The FOO is tried to be parsed as a subcommand at first, I don't think it is a good choice since FOO is after the "--".

In the runc side, should we treat FOO as non-subcommand in this case. thus we can use
runc --global-options -- FOO to start the container, even the config name(FOO) is a special name such as kill, init or events.

duglin · 2015-08-19T03:54:18Z

@laijs yea but that's kind of funky.

duglin · 2015-08-19T19:24:39Z

I'd like to have this considered as is, with "start". We can always rename it later on if we decide we want to change it.
ping @crosbymichael

crosbymichael · 2015-08-21T21:47:23Z

main.go

why did you still leave this here?

I didn't want to break existing functionality - meaning "runc" by itself. I believe this line makes it default to "start" if no command is given. Is that not correct?

yep, just checking. we could probably remove it soon right? maybe in the docs not have the runc example without using runc start

yep we could if the long-term direction is to always require an 'action' verb. Want me to update the docs to always show an action?

wking · 2015-08-21T22:15:43Z

README.md

This could maybe be “To run a container, execute runc start in the bundle's root directory:”, since we don't want just runc.

works for me. done

wking · 2015-08-21T22:25:33Z

README.md

“diectory” → “directory”.

geez I need spell check - thanks.

When any non-global-flag parameter appears on the command line make sure there's a "command" even in the 'start' (run) case to ensure its not ambiguous as to what the arg is. For example, w/o this fix its not clear if runc foo means 'foo' is the name of a config file or an unknown command. Or worse, you can't name a config file the same a ANY command, even future (yet to be created) commands. We should fix this now before we ship 1.0 and are forced to support this ambiguous case for a long time. Signed-off-by: Doug Davis <[email protected]>

crosbymichael · 2015-08-21T22:58:04Z

LGTM

mrunalp · 2015-08-25T15:21:20Z

LGTM

Add a 'start' command

Cleanup bundle.md

Because: 1. This section defines the configuration format, and doesn't need to be tied to a particular filename. 2. The bundle spec (in bundle.md) already has: This REQUIRED file MUST reside in the root of the bundle directory and MUST be named `config.json`. The config.md line may have been useful when it was added (77d44b1, Update runtime.md, 2015-07-16). But since the bundle.md line landed in 106ec2d (Cleanup bundle.md, 2015-10-02, opencontainers#210), I think it's been redundant. Signed-off-by: W. Trevor King <[email protected]>

GordonTheTurtle added the status/0-triage label Aug 18, 2015

duglin force-pushed the AddExecCmd branch from ae510c2 to e63238f Compare August 18, 2015 16:36

duglin changed the title ~~Add an 'exec' command~~ Add a 'start' command Aug 18, 2015

This was referenced Aug 20, 2015

Add exec command #205

Merged

Can runc take its configuration on stdin? #202

Closed

crosbymichael reviewed Aug 21, 2015
View reviewed changes

duglin force-pushed the AddExecCmd branch from e63238f to 6db9bb2 Compare August 21, 2015 22:06

wking reviewed Aug 21, 2015
View reviewed changes

duglin force-pushed the AddExecCmd branch from 6db9bb2 to 6dec851 Compare August 21, 2015 22:21

wking reviewed Aug 21, 2015
View reviewed changes

duglin force-pushed the AddExecCmd branch from 6dec851 to 714ae2a Compare August 21, 2015 22:26

mrunalp pushed a commit that referenced this pull request Aug 25, 2015

Merge pull request #210 from duglin/AddExecCmd

7291a52

Add a 'start' command

mrunalp merged commit 7291a52 into opencontainers:master Aug 25, 2015

wking mentioned this pull request Sep 14, 2015

Adjust runc to new opencontainers/specs version #242

Merged

wking mentioned this pull request Jan 13, 2016

Separate container sandbox lifecycle from that of the processes inside it opencontainers/runtime-spec#299

Open

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#210 from duglin/modBundle

cf8dd12

Cleanup bundle.md

Add a 'start' command #210

Add a 'start' command #210

Uh oh!

Conversation

duglin commented Aug 18, 2015

Uh oh!

dqminh commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

tonistiigi commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

dqminh commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

duglin commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

wking commented Aug 18, 2015

Uh oh!

laijs commented Aug 19, 2015

Uh oh!

duglin commented Aug 19, 2015

Uh oh!

duglin commented Aug 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented Aug 21, 2015

Uh oh!

mrunalp commented Aug 25, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants