runtime: Document args[0] and the no-usable-args fallback

[wking]

Leaning on Go's docs:

Args holds command line arguments, including the command as
Args[0]. If the Args field is empty or nil, Run uses
{Path}.

for the fallback wording.

This restores the ability to explicitly set args[0] independent of the
path, which was requested in #34 and ack-ed by Micheal and
Mrunal, but didn't match the examples that landed with #34.

This new wording matches the implementation that's currently in flight
as opencontainers/runc#160. I also raised this issue on the mailing
list, but didn't have any discussion there before we reached a
consensus in the runC PR.

[laijs]

I think we should use [] instead of null
null is confusing/misleading here, and "args" : null is invalided in the config.

[wking]

On Wed, Aug 26, 2015 at 06:16:33PM -0700, Lai Jiangshan wrote:

+If the args field is missing, empty, or null, the runtime will use [path].

I think we should use [] instead of null. null is
confusing/misleading here, and "args" : null is invalided in the
config.

The docs are about the JSON, where ‘null’ should be a non-confusing
value. I'd intended “empty” to mean ‘[]’ and “missing” to mean “no
‘args’ key in the object.”. I think all of those should be mapped to
‘[path]’, although if someone else felt very strongly about it I'd be
ok with ‘[]’ being mapped to ‘[]’, and just letting folks who specify
empty values deal with the fallout for programs that assume argv[0]
exists.

[philips]

We really need to clarify this. Re-reading this section it is unclear what path actually means and I don't like having two different ways to specify the same thing. cc @mrunalp

[philips]

The comments on #34 clarify the use case of path to some degree but it still doesn't make sense to me.

[wking]

On Wed, Aug 26, 2015 at 10:19:35PM -0700, Brandon Philips wrote:

Re-reading this section it is unclear what path actually means…

If we want a template for a more specific description, execve(2)'s
seems pretty clear 1.

… and I don't like having two different ways to specify the same
thing.

The path and args[0] are not the same thing. For example, busybox
looks at argv[0] when deciding which applet to load:

{
"path": "/bin/busybox",
"args": ["sh"],
}

[mrunalp]

@philips Setting the args is useful for implementing busybox-like programs as @wking pointed out in previous comment.

[philips]

Ehhh, I don't think we need this level of flexibility. If a plugin intends to be invoked with a different arg0 it should symlink itself to that path.

[mrunalp]

@philips I am okay either ways. @LK4D4 @crosbymichael any thoughts?

[wking]

On Thu, Aug 27, 2015 at 01:50:11PM -0700, Brandon Philips wrote:

Ehhh, I don't think we need this level of flexibility. If a plugin
intends to be invoked with a different arg0 it should symlink itself
to that path.

That's what BusyBox usually does, but thte #34 comments were for
‘setup-namespace’ as args[0] for /usr/bin/docker 1. I'm not sure if
it's traditional to setup that symlink, but I don't have it with
Gentoo's default app-emulation/docker-1.7.1.

[philips]

I really don't think we should bend over backwards for this arg0 use case which I would argue breaks good practice and convention.

[wking]

On Sat, Aug 29, 2015 at 08:30:18AM -0700, Brandon Philips wrote:

I really don't think we should bend over backwards for this arg0 use
case…

I don't think it's bending all that far backward to support the
semantics used by the execv family [1,2] and Go's Cmd.Args 3.
Changing to the semantics recommended by this PR made
opencontainers/runc#160 marginally cleaner (now it just
initializes the Cmd struct instead of using Command 4 and then
configuring the attributes not handled by Command). I agree that
many use cases will be handled by the Command-style approach, but
to me the benefit of conforming to established execv/Cmd
standards and supporting folks that do need this
functionality (without making them jump through symlink hoops)
outweighs a few characters of duplication due to using:

{
"path": "/usr/sbin/cleanup.sh",
"args": ["cleanup.sh", "-f"]
}

instead of:

{
"path": "/usr/sbin/cleanup.sh",
"args": ["-f"]
}

… which I would argue breaks good practice and convention.

I don't see any problem with “good practice” here. Switching on
args[0] is what allows BusyBox to work with symlinks. For
convention, see my execv* and Go comments above, but I agree that when
you're launching things from the shell you can't set argv[0] and the
executable path independently.

If we're looking for other possible conventions, Python requires args
and makes the path an optional override (‘args’ and ‘executable’ for
Popen 5). So how about:

{
"args": ["/usr/sbin/cleanup.sh", "-f"]
}

and:

{
"args": ["sh"],
"path": "/bin/busybox"
}

That way only folks who need to override args[0] would need to set
‘path’. It would take a tiny bit more work to map that to Go's Cmd,
but it seems to address your concerns about keeping most user's JSON
DRY.

[mrunalp]

@crosbymichael @LK4D4 Are you okay going back to a simple args array for this?

[wking]

On Tue, Sep 08, 2015 at 10:32:30AM -0700, Mrunal Patel wrote:

@crosbymichael @LK4D4 Are you okay going back to a simple args array
for this?

What's the simple args array? Possible APIs that I'm aware of are:

a. ‘path’ is required, and ‘[path] + args…’ is the executed argv (this
is where opencontainers/runc#160 started)
b. ‘path’ is required, and ‘args…’ is the executed argv (this is where
opencontainers/runc#160 and this PR are now).
c. ‘path’ is optional, ‘args…’ is the executed argv, and ‘path’ takes
precedence (when it's set) over ‘args[0]’ when searching for the
executable. This is how Python does it 1, and seems to address
@philips' concerns while still allowing argv[0] to differ from the
executed path.
d. ‘args…’ is thet executed argv, and ‘args[0]’ is used when searching
for the executable.

Are you suggesting (d)? Why prefer that over (c)?

[dqminh]

I'm in favor of c. ‘path’ is optional, ‘args…’ is the executed argv, and ‘path’ takes precedence (when it's set) over ‘args[0]’ when searching for the executable.

[wking]

On Wed, Sep 09, 2015 at 11:27:53AM -0700, Daniel, Dao Quang Minh wrote:

I'm in favor of c. ‘path’ is optional, ‘args…’ is the executed argv, and ‘path’ takes precedence (when it's set) over ‘args[0]’ when searching for the executable.

Just in case it's hidden too deeply in 1, that's my current favorite
too. I haven't heard anyone push back against it (but I haven't heard
anyone other than you mention it at all ;).

[crosbymichael]

Lets just leave it the way it is now because we already had this discussion in the original PR and decided on this.

[wking]

On Wed, Sep 09, 2015 at 03:03:27PM -0700, Michael Crosby wrote:

Lets just leave it the way it is now because we already had this
discussion in the original PR and decided on this.

(c) wasn't discussed in opencontainers/runc#160 and @philips (who's
pushing back against (b) 1) wasn't involved in that discussion.
Switching to (c) seems like an easy change, both for the spec and
runC, so don't think Grandfathering-in (b) because previous discussion
exists is a good call ;).

[vbatts]

Is it broken staying the way it is?
On Sep 9, 2015 6:12 PM, "W. Trevor King" notifications@github.com wrote:

On Wed, Sep 09, 2015 at 03:03:27PM -0700, Michael Crosby wrote:

Lets just leave it the way it is now because we already had this
discussion in the original PR and decided on this.

(c) wasn't discussed in opencontainers/runc#160 and @philips (who's
pushing back against (b) 1) wasn't involved in that discussion.
Switching to (c) seems like an easy change, both for the spec and
runC, so don't think Grandfathering-in (b) because previous discussion
exists is a good call ;).

—
Reply to this email directly or view it on GitHub
#118 (comment).

[crosbymichael]

@wking i cannot understand your footnotes and (a,b,c) points so let others speak for themselves because its hard to follow along and have a conversation with another person on this repo when u have a comment inbetween everyone else during a discussion.

[wking]

On Wed, Sep 09, 2015 at 03:20:49PM -0700, Vincent Batts wrote:

Is it broken staying the way it is?

It's workable but redundant with the current required path approach.

[crosbymichael]

I'm fine with any changes, keeping it the same, just args, whatever, we just need to be able to have a constructive discussion and let everyone voice their concerns then think about a way to resolve them without alot of noise on the issue while people consider the approaches of the proposed solutions.

[wking]

On Wed, Sep 09, 2015 at 03:21:00PM -0700, Michael Crosby wrote:

@wking i cannot understand your footnotes and (a,b,c)…

Those are references to my summary of possible APIs 1

… so let others speak for themselves…

I try to @mention folks and link to my sources when I paraphrase
others. In this case, my push back has two parts:

• I wasn't thinking about the (c) API 1 when I participated in the
  Add prestart/poststop hooks to runc runc#160 discussion, so I'd rather not be bound by
  that if we're going to land the API that that discussion settled on
  in the spec.
• @philips (who wasn't a part of the Add prestart/poststop hooks to runc runc#160
  discussion) pushed back against the API it settled on 2, which
  seems like a valid reason to not Grandfather in that API. But I'm
  fine not mentioning stuff like this in the future if you find it
  distracting, and would rather read his comments directly.

[vbatts]

let's not do this for now. I realize there is a gap in C vs golang approach to this, but args is clearly args presently.

[wking]

On Fri, Sep 11, 2015 at 10:19:25AM -0700, Vincent Batts wrote:

let's not do this for now. I realize there is a gap in C vs golang
approach to this, but args is clearly args presently.

The examples as they're currently written suggest the (a) API 1. I
guess that's ok for now, but if we all agree that (c) is the cleanest
API, I don't see why we shouldn't switch to it now. It seems like
punting will just make for more bundle-migrating pain down the line.

[wking]

I just pushed 64b170c → eab2bf1, rebasing onto master and switching
from API (b) to API (c) 1.

[wking]

On Fri, Sep 11, 2015 at 10:37:29AM -0700, W. Trevor King wrote:

I just pushed 64b170c → eab2bf1, rebasing onto master and switching
from API (b) to API (c) [1].

And I just pushed eab2bf1 → 4e5c9d4, fixing a ‘net.eth’ → ‘net.eth1’
typo.

[vbatts]

but wait. this new commit is more variadic.. :-\

[crosbymichael]

I see three maintainers say no to this including myself. Closing so we can move on.

[wking]

On Fri, Sep 11, 2015 at 12:01:11PM -0700, Michael Crosby wrote:

I see three maintainers say no to this including myself.

Just to summarize pushback against the current proposal (4e5c9d4),
since my summary of potential APIs 1, we have:

• @dqminh 2 and me 3 in favor.
• @crosbymichael 4 and @vbatts 5 saying “leave it alone”.
• @vbatts asked if the current approach was broken 6. I interpreted
  that as “this PR's current approach (64b170c)” in my response 7,
  but maybe he meant “master's current approach”. I think master's
  current approach is unclear, but it sounds like (d) (from my API
  listing 1), which is unambiguously broken (you can't distinguish
  between args[0] and the executable's path) as I lay out in the
  commit message and my initial post 8.
• @vbatts saying 4e5c9d4 was variadic [9](and elaborating on IRC with
  “11:57 < vbatts> wking: args is now needed processing, it's not just
  appended to the exec'd path”).

This last point seems to be the only technical pushback that the
approach in 4e5c9d4 has received, and I think it is unfounded. Go's
Cmd has 10:

• Args, which is exactly the same as the args I'm proposing in
  4e5c9d4.
• Path, which is (with 4e5c9d4), ‘path’ if ‘path’ is set, and
  ‘args[0]’ otherwise.

So I may be just misunderstanding either the “variadic” phrasing or
the “needs processing” phrasing, but it seems like the approach taken
in 4e5c9d4 is unambiguous to me.

I'm fine postponing this discussion until some future date 11, if
that's easier for the maintainers to handle, but I think handling it
now would be easier for everybody. I think that the technical merits
of the 4e5c9d4 are clear (you can distinguish between args[0] and the
path if you need to, but don't need a WET ‘path’ declaration
otherwise), and am happy to field technical pushback if others don't
agree. But closing a PR without providing a channel to revisit the
issue or technical reasons for the closure seems… hasty.