You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/09/23 23:51:28 UTC
Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/
-----------------------------------------------------------
Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-3504
https://issues.apache.org/jira/browse/MESOS-3504
Repository: mesos
Description
-------
Added MESOS_SANDBOX environment variable in Mesos containerizer.
Diffs
-----
src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
Diff: https://reviews.apache.org/r/38693/diff/
Testing
-------
sudo make check
Thanks,
Jie Yu
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100328
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38693]
All tests passed.
- Mesos ReviewBot
On Sept. 23, 2015, 9:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 23, 2015, 10:14 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 767
> > <https://reviews.apache.org/r/38693/diff/1/?file=1083526#file1083526line767>
> >
> > I think we should consolidate it, but we need to allow it to be passed in I think as I don't think the logic will be the same for both.
>
> Jie Yu wrote:
> Can you elaborate? What do you mean by "allow it to be passed in"?
>
> Jiang Yan Xu wrote:
> Tim,
>
> If it is added as a paraemter of `executorEnvironment()`, can DockerContainerizer take advantage of it?
>
> ```
> map<string, string> executorEnvironment(
> const ExecutorInfo& executorInfo,
> const string& hostDirectory,
> const string& containerDirectory,
> const SlaveID& slaveId,
> const PID<Slave>& slavePid,
> bool checkpoint,
> const Flags& flags,
> bool includeOsEnvironment)
> ```
>
> If names such as MESOS_DIRECTORY and MESOS_SANDBOX have to be used for backwards compatibility reason, I think inside the code we can name things better to make it easier to differentiate the two (there may be better names than `hostDirectory` and `containerDirectory` and I am just suggesting them).
>
> It totally could be a TODO but it would be nice to see them being consolidated.
I'll do that in a separate patch to consolidate them. I am wondering if docker run has two `-e` of the same environment variable is OK or not?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100301
-----------------------------------------------------------
On Sept. 23, 2015, 9:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Sept. 23, 2015, 3:14 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 767
> > <https://reviews.apache.org/r/38693/diff/1/?file=1083526#file1083526line767>
> >
> > I think we should consolidate it, but we need to allow it to be passed in I think as I don't think the logic will be the same for both.
>
> Jie Yu wrote:
> Can you elaborate? What do you mean by "allow it to be passed in"?
Tim,
If it is added as a paraemter of `executorEnvironment()`, can DockerContainerizer take advantage of it?
```
map<string, string> executorEnvironment(
const ExecutorInfo& executorInfo,
const string& hostDirectory,
const string& containerDirectory,
const SlaveID& slaveId,
const PID<Slave>& slavePid,
bool checkpoint,
const Flags& flags,
bool includeOsEnvironment)
```
If names such as MESOS_DIRECTORY and MESOS_SANDBOX have to be used for backwards compatibility reason, I think inside the code we can name things better to make it easier to differentiate the two (there may be better names than `hostDirectory` and `containerDirectory` and I am just suggesting them).
It totally could be a TODO but it would be nice to see them being consolidated.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100301
-----------------------------------------------------------
On Sept. 23, 2015, 2:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 2:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 23, 2015, 10:14 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 767
> > <https://reviews.apache.org/r/38693/diff/1/?file=1083526#file1083526line767>
> >
> > I think we should consolidate it, but we need to allow it to be passed in I think as I don't think the logic will be the same for both.
Can you elaborate? What do you mean by "allow it to be passed in"?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100301
-----------------------------------------------------------
On Sept. 23, 2015, 9:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100301
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp (line 767)
<https://reviews.apache.org/r/38693/#comment157473>
I think we should consolidate it, but we need to allow it to be passed in I think as I don't think the logic will be the same for both.
- Timothy Chen
On Sept. 23, 2015, 9:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 23, 2015, 11:50 p.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/filesystem_isolator_tests.cpp, lines 872-873
> > <https://reviews.apache.org/r/38693/diff/1/?file=1083528#file1083528line872>
> >
> > I think this works too:
> >
> > ```
> > if [[ \"$MESOS_DIRECTORY\" != \"%s\" || \"$MESOS_SANDBOX\" != \"%s\" ]]; then exit 1; fi
> > ```
I prefer my way because it's more easy to add more checks:)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100319
-----------------------------------------------------------
On Sept. 23, 2015, 9:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 9:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38693: Added MESOS_SANDBOX environment variable in
Mesos containerizer.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38693/#review100319
-----------------------------------------------------------
Ship it!
src/tests/containerizer/filesystem_isolator_tests.cpp (lines 872 - 873)
<https://reviews.apache.org/r/38693/#comment157497>
I think this works too:
```
if [[ \"$MESOS_DIRECTORY\" != \"%s\" || \"$MESOS_SANDBOX\" != \"%s\" ]]; then exit 1; fi
```
- Jiang Yan Xu
On Sept. 23, 2015, 2:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38693/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 2:51 p.m.)
>
>
> Review request for mesos, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3504
> https://issues.apache.org/jira/browse/MESOS-3504
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added MESOS_SANDBOX environment variable in Mesos containerizer.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp b4a77e734706f40dc1d2110939d4f33fcbd1fd8c
> src/slave/containerizer/mesos/launch.cpp be600e32115d2f30446f68fb80849c6eaf77afc4
> src/tests/containerizer/filesystem_isolator_tests.cpp 1478531d3036ed567f14cba58fb3f67fa0b60552
>
> Diff: https://reviews.apache.org/r/38693/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>