Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

spec: change executable path in App.Exec to be PATH-dependent. #527

Merged
merged 1 commit into from Nov 18, 2015

Conversation

yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented Oct 9, 2015

The searching of the executable path duplicates the shell. Copied from
the description in man exec(3).

@yifan-gu
Copy link
Contributor Author

yifan-gu commented Oct 9, 2015

cc @philips @jonboulle @vbatts @vcaputo

@jonboulle
Copy link
Contributor

Cc @thockin, curious on his take
On Oct 9, 2015 13:47, "Yifan Gu" notifications@github.com wrote:

cc @philips https://github.com/philips @jonboulle
https://github.com/jonboulle @vbatts https://github.com/vbatts
@vcaputo https://github.com/vcaputo


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

@yifan-gu
Copy link
Contributor Author

ping?

@vcaputo
Copy link
Contributor

vcaputo commented Oct 15, 2015

@yifan-gu What is the motivation for supporting this?

@yifan-gu
Copy link
Contributor Author

@vcaputo Mostly because k8s's pod spec allows relative path.

@thockin
Copy link
Contributor

thockin commented Oct 15, 2015

We do? Where do you see that?

On Thu, Oct 15, 2015 at 4:35 PM, Yifan Gu notifications@github.com wrote:

@vcaputo https://github.com/vcaputo Mostly because k8s's pod spec
allows relative path.


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

@yifan-gu
Copy link
Contributor Author

It's not explicitly said that relative path is allowed or not in the spec But I can set something like sh in Container.Command[]. For example in some e2e tests

@yifan-gu
Copy link
Contributor Author

@thockin ^

@thockin
Copy link
Contributor

thockin commented Oct 16, 2015

There's a difference between being a relative name and being a PATH-dependent name. Docker does support PATH lookups on its exec. Does rkt?

@vcaputo
Copy link
Contributor

vcaputo commented Oct 16, 2015

At this time rkt largely reflects the ExecStart rules of systemd as documented in systemd.service(5), so it's an absolute path only.

@yifan-gu
Copy link
Contributor Author

@vcaputo @thockin Actually the systemd will invoke diagexec to wrap the app. and diagexec handles PATH lookup correctly

@thockin
Copy link
Contributor

thockin commented Oct 16, 2015

So other than the title of this PR, the change itself seems to bring the in line with Docker's behavior and kubernetes' expectations. LGTM

@yifan-gu yifan-gu changed the title spec: allow relative executable path in App.Exec. spec: chage executable path in App.Exec to be PATH-dependent. Oct 16, 2015
The searching of the executable path duplicates the shell.
The details are described in the manual page of exec(3).
@yifan-gu yifan-gu changed the title spec: chage executable path in App.Exec to be PATH-dependent. spec: change executable path in App.Exec to be PATH-dependent. Oct 16, 2015
@yifan-gu
Copy link
Contributor Author

Thanks, title changed.

@vcaputo
Copy link
Contributor

vcaputo commented Oct 16, 2015

@yifan-gu I know diagexec supports it, that was done for the sake of rkt enter so users could have PATH lookups succeed in ad-hoc cli invocations. But the current rkt implementation (without your changes) prevent non-absolute paths, and it's worth noting there's already been at least one attempt to remove diagexec in favor of systemd executing the app directly, some food for thought.

jonboulle added a commit that referenced this pull request Nov 18, 2015
spec: change executable path in App.Exec to be PATH-dependent.
@jonboulle jonboulle merged commit e7d1db9 into appc:master Nov 18, 2015
@yifan-gu yifan-gu deleted the relative_exec branch November 18, 2015 02:16
iaguis added a commit to kinvolk-archives/docker2aci that referenced this pull request Jan 25, 2016
Since appc/spec#527 the spec allows relative
paths in App.Exec, so we don't need to prepend "/bin/sh -c".
iaguis added a commit to kinvolk-archives/docker2aci that referenced this pull request Jan 25, 2016
Since appc/spec#527 the spec allows relative
paths in App.Exec, so we don't need to prepend "/bin/sh -c".
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants