Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Joseph Wu]

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

(Updated Feb. 7, 2017, 2:17 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Moved the command executor changes into another review, to keep those logically 
separate.  See https://reviews.apache.org/r/56410/


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


Repository: mesos


Description
---

This commit adds more special-casing in the `docker/runtime` isolator
for the command executor.  The command executor will generally break
when the `docker/runtime` isolator provides environment variables
directly to the executor.  This is because the environment variables
are provided in the context of the container image, rather than the
host.

For example, a container image may provide an environment variable
like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
executor expects to find libraries in the host's environment.  If the
image's environment end up in the command executor at launch time, 
the command executor may simply fail to launch.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
9bc3fd8309435846c17944e74f611212069dbd76 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
2d816e512c95ed2922c9578ba796908c5ce23da4 

Diff: https://reviews.apache.org/r/56341/diff/


Testing
---

make check

sudo src/mesos-tests --gtest_filter="*ROOT*"

All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
chain) are now passing.


Thanks,

Joseph Wu

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Joseph Wu]

> On Feb. 7, 2017, 11:17 a.m., Jie Yu wrote:
> > src/launcher/executor.cpp, line 939
> > 
> >
> > I'd do that in `launch`.
> > 
> > I'll also add a TODO. Ideally, we should use execvpe, rather than 
> > relying on inheritence.
> > 
> > Are you sure it won't affect command health check if you do that in 
> > this way?

Health checks will fail too (particularly the type that spawns a 
`mesos-tcp-connect` helper), unless we do the execvpe solution.  So I should 
update this code path in its entirety.

Side note: in CMake, we plan to make `mesos-tcp-connect` a static binary.  So 
it would not be affected by the environment.


- Joseph


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


On Feb. 6, 2017, 5:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56341/
> ---
> 
> (Updated Feb. 6, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds more special-casing in the `docker/runtime` isolator
> for the command executor.  The command executor will generally break
> when the `docker/runtime` isolator provides environment variables
> directly to the executor.  This is because the environment variables
> are provided in the context of the container image, rather than the
> host.
> 
> For example, a container image may provide an environment variable
> like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
> executor expects to find libraries in the host's environment.  If the
> image's environment end up in the command executor at launch time, 
> the command executor may simply fail to launch.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2d816e512c95ed2922c9578ba796908c5ce23da4 
> 
> Diff: https://reviews.apache.org/r/56341/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*"
> 
> All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
> chain) are now passing.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Jie Yu]

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




src/launcher/executor.cpp (line 860)


Similar to capabilities, can we add a parse function for Environment and 
use `Option` here.



src/launcher/executor.cpp (line 928)


Let's avoid the `mesos::` prefix here by using a `using` clause at the 
begining of the file.



src/launcher/executor.cpp (lines 937 - 940)


Let's just pass `environment` to `CommandExecutor` below.



src/launcher/executor.cpp (line 939)


I'd do that in `launch`.

I'll also add a TODO. Ideally, we should use execvpe, rather than relying 
on inheritence.

Are you sure it won't affect command health check if you do that in this 
way?



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 140)


Let's update AppcRuntimeIsolator as well.


- Jie Yu


On Feb. 7, 2017, 1:19 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56341/
> ---
> 
> (Updated Feb. 7, 2017, 1:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds more special-casing in the `docker/runtime` isolator
> for the command executor.  The command executor will generally break
> when the `docker/runtime` isolator provides environment variables
> directly to the executor.  This is because the environment variables
> are provided in the context of the container image, rather than the
> host.
> 
> For example, a container image may provide an environment variable
> like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
> executor expects to find libraries in the host's environment.  If the
> image's environment end up in the command executor at launch time, 
> the command executor may simply fail to launch.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2d816e512c95ed2922c9578ba796908c5ce23da4 
> 
> Diff: https://reviews.apache.org/r/56341/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*"
> 
> All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
> chain) are now passing.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Joseph Wu]

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

(Updated Feb. 6, 2017, 5:19 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This commit adds more special-casing in the `docker/runtime` isolator
for the command executor.  The command executor will generally break
when the `docker/runtime` isolator provides environment variables
directly to the executor.  This is because the environment variables
are provided in the context of the container image, rather than the
host.

For example, a container image may provide an environment variable
like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
executor expects to find libraries in the host's environment.  If the
image's environment end up in the command executor at launch time, 
the command executor may simply fail to launch.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
2d816e512c95ed2922c9578ba796908c5ce23da4 

Diff: https://reviews.apache.org/r/56341/diff/


Testing (updated)
---

make check

sudo src/mesos-tests --gtest_filter="*ROOT*"

All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
chain) are now passing.


Thanks,

Joseph Wu

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Joseph Wu]

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

(Updated Feb. 6, 2017, 3:19 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Changed approach, as the docker/runtime isolator is already full of 
command-executor special-casing.  This also doesn't break other isolators that 
need to affect the command executor rather than the command task.


Summary (updated)
-

Changed docker/runtime isolator's handling of Environment.


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


Repository: mesos


Description (updated)
---

This commit adds more special-casing in the `docker/runtime` isolator
for the command executor.  The command executor will generally break
when the `docker/runtime` isolator provides environment variables
directly to the executor.  This is because the environment variables
are provided in the context of the container image, rather than the
host.

For example, a container image may provide an environment variable
like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
executor expects to find libraries in the host's environment.  If the
image's environment end up in the command executor at launch time, 
the command executor may simply fail to launch.


Diffs (updated)
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
2d816e512c95ed2922c9578ba796908c5ce23da4 

Diff: https://reviews.apache.org/r/56341/diff/


Testing (updated)
---

In progress...


Thanks,

Joseph Wu