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 22:04:50 UTC

Review Request 56341: Added special case for command executor's environment.

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

Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
-------

The environment variables given to the command executor will sometimes
modify the behavior of the command executor, rather than the command
task (which is often the intended target of the environment variables).
For example, specifying `LIBPROCESS_IP=invalid` will cause the command 
executor to terminate itself as soon as it starts.  We do not filter 
out environment variables because these types of errors generally fall
under "user error" or "operator error".

The command executor inherits environment variables from:
  * (User) Variables inside a `CommandInfo`;
  * (Operator) Hooks and isolators installed/enabled on the agent;
  * (Operator) The agent flag `--executor_environment_variables`;
  * And Mesos-specific variables provided by the agent.

This commit adds a special case for the command executor.  Isolator
environment variables should be passed into the command executor
separately, as the command executor explicitly depends on environment
variables from the agent.  In some cases, environment variables from
the isolator will cause the command executor to fail; while some 
variables are necessary for the command executor to function.

For example, when running a container image with the unified 
containerizer, if the container image contains the environment variable
`LIBPROCESS_IP=invalid`, the command executor will fail.

For a counter example, when SSL is enabled on the agent, without
downgrade support, you may need to add hooks/isolators to supply the
relevant `LIBPROCESS_SSL_*` environment variables.  Otherwise, the
command executor will not be able to register.


Diffs
-----

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
  src/slave/containerizer/mesos/containerizer.cpp d2b4f75a55dbe4746bc2dfc180335fa831a554ef 

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


Testing
-------

In progress...

NOTE: As mentioned in the commit description, this patch addresses one problem (unified containerizer env vars + command task), while exacerbating another (SSL + command task).


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


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, 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