You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/10/16 07:17:28 UTC
Review Request 39386: Fix uncorrect launcher dir in docker executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/
-----------------------------------------------------------
Review request for mesos and Timothy Chen.
Bugs: MESOS-3738
https://issues.apache.org/jira/browse/MESOS-3738
Repository: mesos
Description
-------
Fix uncorrect launcher dir in docker executor.
Diffs
-----
src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
Diff: https://reviews.apache.org/r/39386/diff/
Testing
-------
* make check
* make install and then test with marathon to check if launcher_dir passes correctly.
Thanks,
haosdent huang
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by haosdent huang <ha...@gmail.com>.
> On Oct. 16, 2015, 11:34 p.m., Niklas Nielsen wrote:
> > src/docker/executor.cpp, line 581
> > <https://reviews.apache.org/r/39386/diff/1/?file=1099658#file1099658line581>
> >
> > What happens when you remove this fallback, taken a user don't provide the launcher dir?
Hi, @nnielsen. Do you mean when user don't provide `launcher_dir` here, what it would happend? `launcher_dir` is a require option here, if user don't provide it, the executor would exit.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review103005
-----------------------------------------------------------
On Oct. 19, 2015, 2:59 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 2:59 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
> src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review103005
-----------------------------------------------------------
src/docker/executor.cpp
<https://reviews.apache.org/r/39386/#comment160853>
What happens when you remove this fallback, taken a user don't provide the launcher dir?
- Niklas Nielsen
On Oct. 15, 2015, 10:17 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2015, 10:17 p.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by haosdent huang <ha...@gmail.com>.
> On Oct. 16, 2015, 5:30 p.m., Marco Massenzio wrote:
> > Thanks for doing this.
> >
> > I think you will need to document the flags' usage in `configuration.md` (or wherever appropriate) and state clearly that it's **required** and what it should point to (in other words, what binary(ies) will we be looking for?) - or users will be confused by the error message.
>
> Marco Massenzio wrote:
> Actually - would it be possible to add a unit test to catch the scenario that was reported in the Jira and make sure this fixes it?
> (when it comes to interaction between OS env vars and flags, it's always tricky)
Because we pass the launcher_dir flag directly instead of get it from argv. If the HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange could pass, it show the launcher_dir is correct. So I don't add new test cases here.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review102939
-----------------------------------------------------------
On Oct. 19, 2015, 2:59 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 2:59 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
> src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Oct. 16, 2015, 5:30 p.m., Marco Massenzio wrote:
> > Thanks for doing this.
> >
> > I think you will need to document the flags' usage in `configuration.md` (or wherever appropriate) and state clearly that it's **required** and what it should point to (in other words, what binary(ies) will we be looking for?) - or users will be confused by the error message.
Actually - would it be possible to add a unit test to catch the scenario that was reported in the Jira and make sure this fixes it?
(when it comes to interaction between OS env vars and flags, it's always tricky)
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review102939
-----------------------------------------------------------
On Oct. 16, 2015, 5:17 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 5:17 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review102939
-----------------------------------------------------------
Ship it!
Thanks for doing this.
I think you will need to document the flags' usage in `configuration.md` (or wherever appropriate) and state clearly that it's **required** and what it should point to (in other words, what binary(ies) will we be looking for?) - or users will be confused by the error message.
src/docker/executor.hpp (line 64)
<https://reviews.apache.org/r/39386/#comment160768>
Please note it is a "required" option and what it it is meant to be used for (and, ideally, what it should point to).
- Marco Massenzio
On Oct. 16, 2015, 5:17 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 5:17 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review103090
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39386]
All tests passed.
- Mesos ReviewBot
On Oct. 19, 2015, 2:59 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 2:59 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
> src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review103255
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On Oct. 19, 2015, 2:59 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2015, 2:59 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
> src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/
-----------------------------------------------------------
(Updated Oct. 19, 2015, 2:59 a.m.)
Review request for mesos and Timothy Chen.
Changes
-------
Update configuration document.
Bugs: MESOS-3738
https://issues.apache.org/jira/browse/MESOS-3738
Repository: mesos
Description
-------
Fix uncorrect launcher dir in docker executor.
Diffs (updated)
-----
docs/configuration.md 9443d5fc2de0f46f9e117b08e29b09ff3a4579c6
src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
src/slave/flags.cpp 1bf394ea62fde29caa6705cd5d156eae452adbf2
Diff: https://reviews.apache.org/r/39386/diff/
Testing
-------
* make check
* make install and then test with marathon to check if launcher_dir passes correctly.
Thanks,
haosdent huang
Re: Review Request 39386: Fix uncorrect launcher dir in docker
executor.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39386/#review102901
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39386]
All tests passed.
- Mesos ReviewBot
On Oct. 16, 2015, 5:17 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39386/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2015, 5:17 a.m.)
>
>
> Review request for mesos and Timothy Chen.
>
>
> Bugs: MESOS-3738
> https://issues.apache.org/jira/browse/MESOS-3738
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix uncorrect launcher dir in docker executor.
>
>
> Diffs
> -----
>
> src/docker/executor.hpp 8ab4e98c41017e8420b98c63a59d8e4bc1a99172
> src/docker/executor.cpp 1e4901335854c49e46cd7b132e79ccb11cd72ade
> src/slave/containerizer/docker.cpp 702295808475c092dff66417f42af89b90e6d50d
>
> Diff: https://reviews.apache.org/r/39386/diff/
>
>
> Testing
> -------
>
> * make check
> * make install and then test with marathon to check if launcher_dir passes correctly.
>
>
> Thanks,
>
> haosdent huang
>
>