Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. Additionally, the name of the binary is never passed 
as an argument so executors making use of argv[0] will fail. This provides the 
full path to the executor so that the `execvp` or `execvpe` is successful. The 
name of the binary is also passed as the first argument for cases where there 
is no shell used.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/1/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178534
---



Someone correct me if I'm wrong, but I don't think we need this anymore:
```
  if (launchInfo.has_working_directory()) {
Try chdir = os::chdir(launchInfo.working_directory());
if (chdir.isError()) {
  cerr << "Failed to chdir into current working directory "
   << "'" << launchInfo.working_directory() << "': "
   << chdir.error() << endl;
  exitWithStatus(EXIT_FAILURE);
}
  }
```
Everyone okay with me removing this?

- Aaron Wood


On June 21, 2017, 7:15 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/1/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Jie Yu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178562
---




src/slave/containerizer/mesos/launch.cpp
Lines 671-674 (patched)


I would do the following to make it more symmetric:
```
vector args;
if (launchInfo.command().shell()) {
  args = {
  os::Shell::arg0,
  os::Shell::arg1,
  launchInfo.command().value()
  };
} else {
  args = { launchInfo.command().value().c_str()) };
  
  args.insert(
  args.end(),
  launchInfo.command().arguments().begin(),
  launchInfo.command().arguments().end());
}

os::raw::Argv argv(args);
```



src/slave/containerizer/mesos/launch.cpp
Lines 801 (patched)


Why this is necessary? I think execvp will look into the current working 
directory?


- Jie Yu


On June 21, 2017, 7:15 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/1/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Jie Yu 


> On June 21, 2017, 7:28 p.m., Aaron Wood wrote:
> > Someone correct me if I'm wrong, but I don't think we need this anymore:
> > ```
> >   if (launchInfo.has_working_directory()) {
> > Try chdir = os::chdir(launchInfo.working_directory());
> > if (chdir.isError()) {
> >   cerr << "Failed to chdir into current working directory "
> ><< "'" << launchInfo.working_directory() << "': "
> ><< chdir.error() << endl;
> >   exitWithStatus(EXIT_FAILURE);
> > }
> >   }
> > ```
> > Everyone okay with me removing this?

No, we cannot. We do want the process's cwd to be inside the sandbox.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178534
---


On June 21, 2017, 7:15 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/1/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 


> On June 21, 2017, 9:54 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 801 (patched)
> > 
> >
> > Why this is necessary? I think execvp will look into the current 
> > working directory?

I found that this was not true. This is the real core of the fix. It seems that 
exec will not look in the current directory, only in PATH.


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178562
---


On June 21, 2017, 7:15 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/1/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 


> On June 21, 2017, 7:28 p.m., Aaron Wood wrote:
> > Someone correct me if I'm wrong, but I don't think we need this anymore:
> > ```
> >   if (launchInfo.has_working_directory()) {
> > Try chdir = os::chdir(launchInfo.working_directory());
> > if (chdir.isError()) {
> >   cerr << "Failed to chdir into current working directory "
> ><< "'" << launchInfo.working_directory() << "': "
> ><< chdir.error() << endl;
> >   exitWithStatus(EXIT_FAILURE);
> > }
> >   }
> > ```
> > Everyone okay with me removing this?
> 
> Jie Yu wrote:
> No, we cannot. We do want the process's cwd to be inside the sandbox.

Okay, will keep it as is then.


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178534
---


On June 21, 2017, 7:15 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/1/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated June 21, 2017, 10:26 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. Additionally, the name of the binary is never passed 
as an argument so executors making use of argv[0] will fail. This provides the 
full path to the executor so that the `execvp` or `execvpe` is successful. The 
name of the binary is also passed as the first argument for cases where there 
is no shell used.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/2/

Changes: https://reviews.apache.org/r/60280/diff/1-2/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 


> On June 21, 2017, 9:54 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 801 (patched)
> > 
> >
> > Why this is necessary? I think execvp will look into the current 
> > working directory?
> 
> Aaron Wood wrote:
> I found that this was not true. This is the real core of the fix. It 
> seems that exec will not look in the current directory, only in PATH.

`The file is sought in the colon-separated list of directory pathnames 
specified in the PATH environment variable. If this variable isn't defined, the 
path list defaults to the current directory followed by the list of directories 
returned by confstr(_CS_PATH). (This confstr(3) call typically returns the 
value "/bin:/usr/bin".)`


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178562
---


On June 21, 2017, 10:26 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/2/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Jie Yu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178586
---




src/slave/containerizer/mesos/launch.cpp
Lines 671-674 (patched)


Sorry, I got confused. User should be the one setting argv[0]. So we don't 
need to change the code here.



src/slave/containerizer/mesos/launch.cpp
Lines 801 (patched)


OK, what if 'executable' has an absolute path?


- Jie Yu


On June 21, 2017, 10:26 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/2/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178612
---



Patch looks great!

Reviews applied: [60280]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 21, 2017, 10:26 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/2/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-21 Thread Aaron Wood via Review Board 


> On June 21, 2017, 11:07 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 671-674 (patched)
> > 
> >
> > Sorry, I got confused. User should be the one setting argv[0]. So we 
> > don't need to change the code here.

No problem.
So every time someone wants to make use of `argv[0]` in their executor 
somewhere they'll have to construct that argument and send it along in the 
protos. Why not take care of it here so that it's less to do (and less going 
over the wire) from the scheduler's side?


> On June 21, 2017, 11:07 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 801 (patched)
> > 
> >
> > OK, what if 'executable' has an absolute path?

I guess this would break in that case. My thought was that no one would know 
the absolute path ahead of time. For example, how would a user know the 
container ID (not even generated yet since the executor/task has not been sent 
to Mesos) to be used in the full absolute path to their executor binary in the 
sandbox?


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178586
---


On June 21, 2017, 10:26 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/2/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-22 Thread Jie Yu 


> On June 21, 2017, 11:07 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 671-674 (patched)
> > 
> >
> > Sorry, I got confused. User should be the one setting argv[0]. So we 
> > don't need to change the code here.
> 
> Aaron Wood wrote:
> No problem.
> So every time someone wants to make use of `argv[0]` in their executor 
> somewhere they'll have to construct that argument and send it along in the 
> protos. Why not take care of it here so that it's less to do (and less going 
> over the wire) from the scheduler's side?

Some user might want to customize argv[0] (which will be displayed by ps).


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178586
---


On June 21, 2017, 10:26 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 21, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. Additionally, the name of the binary is never passed 
> as an argument so executors making use of argv[0] will fail. This provides 
> the full path to the executor so that the `execvp` or `execvpe` is 
> successful. The name of the binary is also passed as the first argument for 
> cases where there is no shell used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/2/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-22 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated June 22, 2017, 3:19 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. Additionally, the name of the binary is never passed 
as an argument so executors making use of argv[0] will fail. This provides the 
full path to the executor so that the `execvp` or `execvpe` is successful. The 
name of the binary is also passed as the first argument for cases where there 
is no shell used.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/3/

Changes: https://reviews.apache.org/r/60280/diff/2-3/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-22 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated June 22, 2017, 3:20 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description (updated)
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/3/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-22 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review178695
---



Patch looks great!

Reviews applied: [60280]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 22, 2017, 5:20 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 22, 2017, 5:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/3/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-06-30 Thread Jie Yu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179433
---




src/slave/containerizer/mesos/launch.cpp
Lines 799 (patched)


What if `launchInfo.working_directory()` is not set? Maybe use os::realpath 
here to get the absolute path?


- Jie Yu


On June 22, 2017, 3:20 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 22, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/3/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-03 Thread Aaron Wood via Review Board 


> On June 30, 2017, 11:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 799 (patched)
> > 
> >
> > What if `launchInfo.working_directory()` is not set? Maybe use 
> > os::realpath here to get the absolute path?

Sure, I'll alter that. Is it wrong to assume that the working directory will 
always be set at this point? I thought that without the sandbox work dir no 
container would be able to move forward.


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179433
---


On June 22, 2017, 3:20 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated June 22, 2017, 3:20 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/3/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-03 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 3, 2017, 4 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/4/

Changes: https://reviews.apache.org/r/60280/diff/3-4/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-03 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 3, 2017, 4:18 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Changes
---

Fix comment.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/5/

Changes: https://reviews.apache.org/r/60280/diff/4-5/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-03 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 3, 2017, 6:35 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Changes
---

Exit with failure status if we can't get the absolute path.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/6/

Changes: https://reviews.apache.org/r/60280/diff/5-6/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Jie Yu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179650
---




src/slave/containerizer/mesos/launch.cpp
Lines 799 (patched)


This is still based on the assumption that `launchInfo` has 
`working_directory()` set. Can you do the following instead:
```
Result realpath = os::realpath(executable);
if (!realpath.isSome()) {
  ...
}
```


- Jie Yu


On July 3, 2017, 6:35 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 3, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/6/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Jie Yu 


> On June 30, 2017, 11:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 799 (patched)
> > 
> >
> > What if `launchInfo.working_directory()` is not set? Maybe use 
> > os::realpath here to get the absolute path?
> 
> Aaron Wood wrote:
> Sure, I'll alter that. Is it wrong to assume that the working directory 
> will always be set at this point? I thought that without the sandbox work dir 
> no container would be able to move forward.

I think there are some cases at the moment where the working_directory is not 
set (DEBUG containers):
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1540

But I think that's a tech debt itself.


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179433
---


On July 3, 2017, 6:35 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 3, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/6/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Jie Yu 


> On July 5, 2017, 4:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 799 (patched)
> > 
> >
> > This is still based on the assumption that `launchInfo` has 
> > `working_directory()` set. Can you do the following instead:
> > ```
> > Result realpath = os::realpath(executable);
> > if (!realpath.isSome()) {
> >   ...
> > }
> > ```

I also realized that using realpath will make the search for PATH not possible. 
This issue is indeed not trivial. The best way I can think of is to test if 
executable is relative and see if it exists in the sandbox or not and test if 
it is executable or not. Alternatively, we can add cwd to PATH, but this will 
polute PATH i would rather avoid.

Do you have other thoughts?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179650
---


On July 3, 2017, 6:35 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 3, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/6/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Aaron Wood via Review Board 


> On July 5, 2017, 4:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 799 (patched)
> > 
> >
> > This is still based on the assumption that `launchInfo` has 
> > `working_directory()` set. Can you do the following instead:
> > ```
> > Result realpath = os::realpath(executable);
> > if (!realpath.isSome()) {
> >   ...
> > }
> > ```
> 
> Jie Yu wrote:
> I also realized that using realpath will make the search for PATH not 
> possible. This issue is indeed not trivial. The best way I can think of is to 
> test if executable is relative and see if it exists in the sandbox or not and 
> test if it is executable or not. Alternatively, we can add cwd to PATH, but 
> this will polute PATH i would rather avoid.
> 
> Do you have other thoughts?

Is it possible to assume that any custom executor would always be in the 
sandbox? E.g. assume that a framework will always provide a URI to fetch the 
executor and have Mesos execute it in the usual way. If that's the case then 
would we need to search `PATH` at all?


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179650
---


On July 3, 2017, 6:35 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 3, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/6/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Aaron Wood via Review Board 


> On June 30, 2017, 11:48 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 799 (patched)
> > 
> >
> > What if `launchInfo.working_directory()` is not set? Maybe use 
> > os::realpath here to get the absolute path?
> 
> Aaron Wood wrote:
> Sure, I'll alter that. Is it wrong to assume that the working directory 
> will always be set at this point? I thought that without the sandbox work dir 
> no container would be able to move forward.
> 
> Jie Yu wrote:
> I think there are some cases at the moment where the working_directory is 
> not set (DEBUG containers):
> 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L1540
> 
> But I think that's a tech debt itself.

Thanks, I didn't realize this existed.


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179433
---


On July 3, 2017, 6:35 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 3, 2017, 6:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/6/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 5, 2017, 9:03 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/7/

Changes: https://reviews.apache.org/r/60280/diff/6-7/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179735
---



Bad patch!

Reviews applied: [60280]

Failed command: python support/apply-reviews.py -n -r 60280

Error:
Traceback (most recent call last):
  File "support/apply-reviews.py", line 417, in 
main()
  File "support/apply-reviews.py", line 412, in main
reviewboard(options)
  File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
  File "support/apply-reviews.py", line 158, in apply_review
fetch_patch(options)
  File "support/apply-reviews.py", line 215, in fetch_patch
shell(cmd, options['dry_run'])
UnboundLocalError: local variable 'cmd' referenced before assignment

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/125/console

- Mesos Reviewbot Windows


On July 5, 2017, 9:03 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 5, 2017, 9:03 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/7/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-05 Thread Jie Yu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179745
---



This will break the windows build. os::which is only in posix header.

I would ask in the #windows channel see if we can port os::which to be on both 
windows and posix (sounds very doable).


src/slave/containerizer/mesos/launch.cpp
Lines 799-814 (patched)


I will do the following:
```
// Search executable in the current working directory as well.
// `execvpe` and `execvp` will only search executable from the
// current working directory if environment variable `PATH` is
// not set.
if (!path::absolute(executable) &&
launchInfo.has_working_directory()) {
  Option which = os::which(
  executable,
  launchInfo.working_directory());

  if (which.isSome()) {
executable = which.get();
  }
}
```



src/slave/containerizer/mesos/launch.cpp
Lines 808 (patched)


This line is longger than 80 chars:

```
Jies-MacBook-Pro:mesos jie$ support/apply-review.sh -r 60280
2017-07-05 19:39:04 URL:https://reviews.apache.org/r/60280/diff/raw/ 
[1397/1397] -> "60280.patch" [1]
Checking 1 C++ file
src/slave/containerizer/mesos/launch.cpp:803:  Lines should be <= 80 
characters long  [whitespace/line_length] [2]
src/slave/containerizer/mesos/launch.cpp:808:  Lines should be <= 80 
characters long  [whitespace/line_length] [2]
Total errors found: 1
No Python files to lint
```

ALso, we need to test if working_directory is set or not.



src/slave/containerizer/mesos/launch.cpp
Lines 810 (patched)


indentation here should be 2


- Jie Yu


On July 5, 2017, 9:03 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 5, 2017, 9:03 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/7/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-06 Thread Aaron Wood via Review Board 


> On July 6, 2017, 2:59 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 808 (patched)
> > 
> >
> > This line is longger than 80 chars:
> > 
> > ```
> > Jies-MacBook-Pro:mesos jie$ support/apply-review.sh -r 60280
> > 2017-07-05 19:39:04 URL:https://reviews.apache.org/r/60280/diff/raw/ 
> > [1397/1397] -> "60280.patch" [1]
> > Checking 1 C++ file
> > src/slave/containerizer/mesos/launch.cpp:803:  Lines should be <= 80 
> > characters long  [whitespace/line_length] [2]
> > src/slave/containerizer/mesos/launch.cpp:808:  Lines should be <= 80 
> > characters long  [whitespace/line_length] [2]
> > Total errors found: 1
> > No Python files to lint
> > ```
> > 
> > ALso, we need to test if working_directory is set or not.

Can't we just rely on `os::which` checking if the working_directory is set or 
not?


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179745
---


On July 5, 2017, 9:03 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 5, 2017, 9:03 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/7/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-06 Thread Aaron Wood via Review Board 


> On July 6, 2017, 2:59 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 808 (patched)
> > 
> >
> > This line is longger than 80 chars:
> > 
> > ```
> > Jies-MacBook-Pro:mesos jie$ support/apply-review.sh -r 60280
> > 2017-07-05 19:39:04 URL:https://reviews.apache.org/r/60280/diff/raw/ 
> > [1397/1397] -> "60280.patch" [1]
> > Checking 1 C++ file
> > src/slave/containerizer/mesos/launch.cpp:803:  Lines should be <= 80 
> > characters long  [whitespace/line_length] [2]
> > src/slave/containerizer/mesos/launch.cpp:808:  Lines should be <= 80 
> > characters long  [whitespace/line_length] [2]
> > Total errors found: 1
> > No Python files to lint
> > ```
> > 
> > ALso, we need to test if working_directory is set or not.
> 
> Aaron Wood wrote:
> Can't we just rely on `os::which` checking if the working_directory is 
> set or not?

Nevermind, just noticed that `os::which` will grab PATH if `_path` is not set.


- Aaron


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/#review179745
---


On July 5, 2017, 9:03 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60280/
> ---
> 
> (Updated July 5, 2017, 9:03 p.m.)
> 
> 
> Review request for mesos, Jie Yu, James Peach, and Zhitao Li.
> 
> 
> Bugs: MESOS-7703
> https://issues.apache.org/jira/browse/MESOS-7703
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework specifies use of its own executor and sets shell to false the 
> executor is never found. This provides the full path to the executor so that 
> the `execvp` or `execvpe` is successful.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 162ca1c2e 
> 
> 
> Diff: https://reviews.apache.org/r/60280/diff/7/
> 
> 
> Testing
> ---
> 
> `cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
> Also spun up a master and agent, connected and sent a task using the UCR 
> (both with and without the use of an OCI image) via our own framework, and 
> checked the sandbox to verify that things went accordingly.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-06 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 6, 2017, 3:27 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/8/

Changes: https://reviews.apache.org/r/60280/diff/7-8/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-06 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 6, 2017, 3:32 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/9/

Changes: https://reviews.apache.org/r/60280/diff/8-9/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood

Re: Review Request 60280: Provide full path to the custom executor.

2017-07-06 Thread Aaron Wood via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60280/
---

(Updated July 6, 2017, 10:44 p.m.)


Review request for mesos, Jie Yu, James Peach, and Zhitao Li.


Bugs: MESOS-7703
https://issues.apache.org/jira/browse/MESOS-7703


Repository: mesos


Description
---

If a framework specifies use of its own executor and sets shell to false the 
executor is never found. This provides the full path to the executor so that 
the `execvp` or `execvpe` is successful.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 162ca1c2e 


Diff: https://reviews.apache.org/r/60280/diff/9/


Testing
---

`cd build && cmake .. -DCMAKE_BUILD_TYPE=Release && make -j4`
Also spun up a master and agent, connected and sent a task using the UCR (both 
with and without the use of an OCI image) via our own framework, and checked 
the sandbox to verify that things went accordingly.


Thanks,

Aaron Wood