You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/02/06 23:19:11 UTC
Re: Review Request 56341: Changed docker/runtime isolator's handling
of Environment.
-----------------------------------------------------------
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
Re: Review Request 56341: Changed docker/runtime isolator's handling
of Environment.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Feb. 7, 2017, 11:17 a.m., Jie Yu wrote:
> > src/launcher/executor.cpp, line 939
> > <https://reviews.apache.org/r/56341/diff/2/?file=1625716#file1625716line939>
> >
> > 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.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56341/#review164541
-----------------------------------------------------------
src/launcher/executor.cpp (line 860)
<https://reviews.apache.org/r/56341/#comment236297>
Similar to capabilities, can we add a parse function for Environment and use `Option<Environment>` here.
src/launcher/executor.cpp (line 928)
<https://reviews.apache.org/r/56341/#comment236298>
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)
<https://reviews.apache.org/r/56341/#comment236299>
Let's just pass `environment` to `CommandExecutor` below.
src/launcher/executor.cpp (line 939)
<https://reviews.apache.org/r/56341/#comment236300>
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)
<https://reviews.apache.org/r/56341/#comment236303>
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: Updated AppC & Docker runtime isolators'
handling of Environment.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56341/#review164583
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Feb. 7, 2017, 10:26 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56341/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 10:26 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 builds upon the command executor's new `--task_environment`
> flag, which allows isolators to specify environment variables meant
> for the task, without affecting the executor's environment.
>
> This is important as the command executor is both an executor and
> a task. Some environment variables from isolators are intended
> for the executor, while others are intended for the task (such as
> from the two runtime isolators).
>
> 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/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: Updated AppC & Docker runtime isolators'
handling of Environment.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56341/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 2:26 p.m.)
Review request for mesos, Jie Yu and Kevin Klues.
Changes
-------
Oops, forgot to update the summary/description.
Summary (updated)
-----------------
Updated AppC & Docker runtime isolators' handling of Environment.
Bugs: MESOS-7027
https://issues.apache.org/jira/browse/MESOS-7027
Repository: mesos
Description (updated)
-------
This commit builds upon the command executor's new `--task_environment`
flag, which allows isolators to specify environment variables meant
for the task, without affecting the executor's environment.
This is important as the command executor is both an executor and
a task. Some environment variables from isolators are intended
for the executor, while others are intended for the task (such as
from the two runtime isolators).
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/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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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