You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/02/05 00:48:37 UTC
Review Request 30643: Optionally specify executor for "mesos execute".
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/
-----------------------------------------------------------
Review request for mesos, Timothy Chen and Vinod Kone.
Repository: mesos
Description
-------
Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
+tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
Diffs
-----
src/cli/execute.cpp 77deec91177c90a8a20ccd4fe063b82ca08cefbd
src/common/parse.hpp ae581e5168606bf7ea7e092c5095ddf9d321c792
src/launcher/executor.cpp 1cf28f168cac6e8c7e98686a35509c2b4e052504
Diff: https://reviews.apache.org/r/30643/diff/
Testing
-------
make check
manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
Thanks,
Ian Downes
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Ian Downes <ia...@gmail.com>.
> On Feb. 4, 2015, 5:44 p.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 137
> > <https://reviews.apache.org/r/30643/diff/1/?file=849116#file849116line137>
> >
> > I think it's worth commenting somewhere high level that the command executor is expecting executorInfo's data to be an commandInfo.
>
> Timothy Chen wrote:
> also i dont know if this is useful at all besides the mesos-execute case? who else can leverage this change?
This is documented in the executor_info flag help.
> On Feb. 4, 2015, 5:44 p.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 130
> > <https://reviews.apache.org/r/30643/diff/1/?file=849116#file849116line130>
> >
> > They shouldn't both be set as well, I know we check this in the slave, but I think it's worth mentinoing that we're doing that check else where.
Code removed after rebasing.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review71122
-----------------------------------------------------------
On May 12, 2015, 5:22 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:22 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Timothy Chen <tn...@apache.org>.
> On Feb. 5, 2015, 1:44 a.m., Timothy Chen wrote:
> > src/launcher/executor.cpp, line 137
> > <https://reviews.apache.org/r/30643/diff/1/?file=849116#file849116line137>
> >
> > I think it's worth commenting somewhere high level that the command executor is expecting executorInfo's data to be an commandInfo.
also i dont know if this is useful at all besides the mesos-execute case? who else can leverage this change?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review71122
-----------------------------------------------------------
On Feb. 4, 2015, 11:48 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 11:48 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp 77deec91177c90a8a20ccd4fe063b82ca08cefbd
> src/common/parse.hpp ae581e5168606bf7ea7e092c5095ddf9d321c792
> src/launcher/executor.cpp 1cf28f168cac6e8c7e98686a35509c2b4e052504
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review71122
-----------------------------------------------------------
src/cli/execute.cpp
<https://reviews.apache.org/r/30643/#comment116731>
Seems like in the custom executorInfo case, dockerImage will simply be ignored as it's looking at the executor's containerInfo which might not be docker.
I think it's probably simpler if we check that either dockerImage is set or executorInfo and print out that we only support one of them.
src/launcher/executor.cpp
<https://reviews.apache.org/r/30643/#comment116733>
They shouldn't both be set as well, I know we check this in the slave, but I think it's worth mentinoing that we're doing that check else where.
src/launcher/executor.cpp
<https://reviews.apache.org/r/30643/#comment116732>
I think it's worth commenting somewhere high level that the command executor is expecting executorInfo's data to be an commandInfo.
- Timothy Chen
On Feb. 4, 2015, 11:48 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 11:48 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp 77deec91177c90a8a20ccd4fe063b82ca08cefbd
> src/common/parse.hpp ae581e5168606bf7ea7e092c5095ddf9d321c792
> src/launcher/executor.cpp 1cf28f168cac6e8c7e98686a35509c2b4e052504
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review82830
-----------------------------------------------------------
Are you still planning to merge this?
- Timothy Chen
On Feb. 4, 2015, 11:48 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 11:48 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp 77deec91177c90a8a20ccd4fe063b82ca08cefbd
> src/common/parse.hpp ae581e5168606bf7ea7e092c5095ddf9d321c792
> src/launcher/executor.cpp 1cf28f168cac6e8c7e98686a35509c2b4e052504
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review83550
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30643]
All tests passed.
- Mesos ReviewBot
On May 13, 2015, 12:22 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:22 a.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review88151
-----------------------------------------------------------
Can you split this review into 1) adding support for a custom executor to mesos-execute and 2) updating command executor to be run as a custom executor. It's clear that the former is useful and general. It's not clear if the latter is strictly needed.
src/cli/execute.cpp (lines 243 - 248)
<https://reviews.apache.org/r/30643/#comment140559>
This check is happening way late. Push this down to #444.
- Vinod Kone
On May 26, 2015, 10:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:49 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review88140
-----------------------------------------------------------
This is pretty hacky. Can you elaborate why this is beneficial for testing filesystem isolation. Can we instead allow users to specify TaskInfo.container, or it's not sufficient?
- Jie Yu
On May 26, 2015, 10:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:49 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review85332
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30643]
All tests passed.
- Mesos ReviewBot
On May 26, 2015, 10:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:49 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review89976
-----------------------------------------------------------
Ship it!
Ship It!
src/cli/execute.cpp (line 101)
<https://reviews.apache.org/r/30643/#comment142884>
I think we should remove '/path/to/file' since parsing throws a warning that it's expecting a file:// prefix.
- Timothy Chen
On May 26, 2015, 10:49 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 10:49 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/
-----------------------------------------------------------
(Updated May 26, 2015, 3:49 p.m.)
Review request for mesos, Timothy Chen and Vinod Kone.
Changes
-------
@tnachen could you take another look at this when you get some time, I'd like to commit it soon.
Repository: mesos
Description
-------
Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
+tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
Diffs (updated)
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
Diff: https://reviews.apache.org/r/30643/diff/
Testing
-------
make check
manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
Thanks,
Ian Downes
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/
-----------------------------------------------------------
(Updated May 12, 2015, 5:22 p.m.)
Review request for mesos, Timothy Chen and Vinod Kone.
Repository: mesos
Description
-------
Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
+tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
Diffs (updated)
-----
src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
Diff: https://reviews.apache.org/r/30643/diff/
Testing
-------
make check
manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
Thanks,
Ian Downes
Re: Review Request 30643: Optionally specify executor for "mesos
execute".
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30643/#review71120
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30643]
All tests passed.
- Mesos ReviewBot
On Feb. 4, 2015, 11:48 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30643/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 11:48 p.m.)
>
>
> Review request for mesos, Timothy Chen and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Provided executor will be used to run the task rather than deferring to the slave to determine an executor to use (currently the local included command executor).
>
> This is helpful for testing containers with filesystem isolation as you can fetch an executor into the work directory rather than using the command executor on the host filesystem. You can, of course, still use the command executor, but it will be copied/fetched.
>
> +tnachen - can you please verify I've preserved the override functionality in the command executor? Is that still required?
>
>
> Diffs
> -----
>
> src/cli/execute.cpp 77deec91177c90a8a20ccd4fe063b82ca08cefbd
> src/common/parse.hpp ae581e5168606bf7ea7e092c5095ddf9d321c792
> src/launcher/executor.cpp 1cf28f168cac6e8c7e98686a35509c2b4e052504
>
> Diff: https://reviews.apache.org/r/30643/diff/
>
>
> Testing
> -------
>
> make check
>
> manually used 'mesos execute' and specified command executor, both directly as the value and fetching from file:///. Tested enviroment variables too.
>
>
> Thanks,
>
> Ian Downes
>
>