mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 53500: Used an environment variable to pass command environment.
Date Sat, 05 Nov 2016 23:38:21 GMT


> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 78
> > <https://reviews.apache.org/r/53500/diff/1/?file=1554709#file1554709line78>
> >
> >     why the renaming here. All the parameters here are for 'command'. I suggest
we stick to the previous name.

The rename is because just so that we don't use a environment variable name "MESOS_ENVIRONMENT"
which is not very descriptive. The suggest below would solve this problem so yeah I'll change
it back.


> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1405
> > <https://reviews.apache.org/r/53500/diff/1/?file=1554707#file1554707line1405>
> >
> >     I'll be nice to use environment variable for all flags here. I think we probably
need a helper in FlagsBase to return the corresponding environment variables for the flags:
> >     ```
> >     // Export flags as environment variables.
> >     map<string, string> FlagsBase::environment(
> >         const Option<string>& prefix);
> >     ```
> >     
> >     And then, we can just merge with os::environment and pass it to Launcher::fork

This helper looks like handy. 

I am not necessarily against this but wanted us to give it more thought. 

1. Should all Mesos libexec binaries work this way?
2. Removing all of the flags makes all the helper binary instances look the same through `ps`.
Yes privileged users can still get the info in /proc for individual processes but we loose
 we lose easy "grep-pability" for debugging. 

To me the line to draw is between user-supplied info (prefer env vars) and Mesos generated
info (e.g., runtime dir which encodes contianerId) which is not sensitive. Thoughts?


> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/main.cpp, line 43
> > <https://reviews.apache.org/r/53500/diff/1/?file=1554710#file1554710line43>
> >
> >     I'd prefer using a different prefix here because `MESOS_` prefix is used by
agent as well. It will be very hard to debug if there are conflicting flags.
> >     
> >     I'd suggest we use `MESOS_CONTAINERIZER_` here

+1.


- Jiang Yan


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


On Nov. 4, 2016, 2:29 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2016, 2:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
>     https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/containerizer.cpp 67cc595278f124cdf518d2f4fcfb257439f067e2

>   src/slave/containerizer/mesos/launch.hpp 8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
>   src/slave/containerizer/mesos/launch.cpp 377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
>   src/slave/containerizer/mesos/main.cpp 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors depends on
this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message