You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by "R.B. Boyer" <bo...@nexusvector.net> on 2014/10/18 06:40:03 UTC
Re: Review Request 26622: Command Executor is broken when used with
shell=false
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/
-----------------------------------------------------------
(Updated Oct. 17, 2014, 11:40 p.m.)
Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
Changes
-------
Updated the title to more accurately reflect the severity of the bug. It would be awesome to get this little 3-line changeset merged for the next point-release to fix a feature that has never worked.
Summary (updated)
-----------------
Command Executor is broken when used with shell=false
Bugs: MESOS-1873
https://issues.apache.org/jira/browse/MESOS-1873
Repository: mesos-git
Description (updated)
-------
Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
Don't pass task-related arguments to mesos-executor.
The description on the linked jira ticket goes into more detail.
Diffs
-----
src/slave/slave.cpp 0e342ed
src/tests/slave_tests.cpp f585bdd
Diff: https://reviews.apache.org/r/26622/diff/
Testing
-------
make check
added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
Thanks,
R.B. Boyer
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 11:03 a.m., Timothy Chen wrote:
> > src/slave/slave.cpp, line 2567
> > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2567>
> >
> > Thanks for the fix! I wonder if we should just fix the command executor instead?
> >
> > I usually don't like mingling with user API input as it always confuses whoever using this, and leads to people start asking mailing lists and reading code because their arguemnts are changed.
> >
> > Shell=false was added for a reason and I think we shouldn't just change it for them, did you see why we can't just support this variation in the command executor?
>From a security perspective, shell=true is horrible as it requires the framework user to correctly shell-escape arguments correctly.
I Would hope that the mesos project would push frameworks to deprecate shell=true uses of command executor.
That being said, i don't 100% follow your question, but I'll try to reexplain my motivation here with this bug.
Running a command task on a slave requires the execution of an executor AND the execution of the Task. This is fine. What is busted here is that someone kludged the Executor launch to work by partly copying launch information from the normal Task launch configuration probably because it made sense at the time.
This was fine when the task was totally defined by the single string "value" field ultimately passed to "sh -c". The executor launch simply replaced the "value" and then mesos-executor runs just fine with no arguments.
But when the multivalued task launch (shell=false) was introduced, this launch hack breaks because the Task arguments are incorrectly given to mesos-executor when it launches. This really only breaks if you try to pass an arg with a double-dash, which fast-fails mesos-executor due to how it parses flags.
If you want to revisit how the command executor works in general that's great, but i still think that the current functionality should be repaired first (the purpose of this patch).
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review57997
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review57997
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment98911>
Thanks for the fix! I wonder if we should just fix the command executor instead?
I usually don't like mingling with user API input as it always confuses whoever using this, and leads to people start asking mailing lists and reading code because their arguemnts are changed.
Shell=false was added for a reason and I think we shouldn't just change it for them, did you see why we can't just support this variation in the command executor?
- Timothy Chen
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review59477
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 24, 2014, 3:20 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> >
> > Reviews applied: [26622]
> >
> > Failed command: ./support/apply-review.sh -n -r 26622
> >
> > Error:
> > --2014-10-24 08:20:08-- https://reviews.apache.org/r/26622/diff/raw/
> > Resolving reviews.apache.org (reviews.apache.org)... 140.211.11.74
> > Connecting to reviews.apache.org (reviews.apache.org)|140.211.11.74|:443... connected.
> > HTTP request sent, awaiting response... 200 OK
> > Length: 6363 (6.2K) [text/x-patch]
> > Saving to: '26622.patch'
> >
> > 0K ...... 100% 171M=0s
> >
> > 2014-10-24 08:20:09 (171 MB/s) - '26622.patch' saved [6363/6363]
> >
> > 'fullname' was not found
> > 'email' was not found
> > Successfully applied: Command Executor is broken when used with shell=false
> >
> > Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
> >
> > When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
> >
> > Don't pass task-related arguments to mesos-executor.
> >
> > The description on the linked jira ticket goes into more detail.
> >
> >
> > Review: https://reviews.apache.org/r/26622
> > fatal: empty ident name (for <>) not allowed
> > Failed to commit patch
What does:
'fullname' was not found
'email' was not found
mean in this case? I didn't do anything different to generate the patch file than I did a week ago when I got a "Patch looks great!" reply.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58254
-----------------------------------------------------------
On Oct. 23, 2014, 9:04 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 9:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58254
-----------------------------------------------------------
Bad patch!
Reviews applied: [26622]
Failed command: ./support/apply-review.sh -n -r 26622
Error:
--2014-10-24 08:20:08-- https://reviews.apache.org/r/26622/diff/raw/
Resolving reviews.apache.org (reviews.apache.org)... 140.211.11.74
Connecting to reviews.apache.org (reviews.apache.org)|140.211.11.74|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 6363 (6.2K) [text/x-patch]
Saving to: '26622.patch'
0K ...... 100% 171M=0s
2014-10-24 08:20:09 (171 MB/s) - '26622.patch' saved [6363/6363]
'fullname' was not found
'email' was not found
Successfully applied: Command Executor is broken when used with shell=false
Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
Don't pass task-related arguments to mesos-executor.
The description on the linked jira ticket goes into more detail.
Review: https://reviews.apache.org/r/26622
fatal: empty ident name (for <>) not allowed
Failed to commit patch
- Mesos ReviewBot
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
>
> Timothy Chen wrote:
> Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
> Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
>
> Jie Yu wrote:
> There is no ship-it yet. Please post te latest patch please.
>
> Timothy Chen wrote:
> I think it's because you don't have any email setup under reviewboard too.
>
> Timothy Chen wrote:
> Ok I just merged this with manually putting your name and email in. Please update reviewboard with your email next time.
>
> R.B. Boyer wrote:
> Huh. I swear I've configured my profile in reviewboard. Does the profile have to be public (mine is marked private right now)?
>
> Benjamin Hindman wrote:
> Why has this patch been committed? It does not look like the '/bin/ls -l --author' issue has been resolved. :-(
I fixed it for him since he said he doesn't have time and no access to mac.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
Ya sorry was busy with docker patches, will get to this when they're pushed.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
Tim, are you working on the patch?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
Ok I'll do that.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
>
> Timothy Chen wrote:
> Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
> Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
>
> Jie Yu wrote:
> There is no ship-it yet. Please post te latest patch please.
I think it's because you don't have any email setup under reviewboard too.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
>
> Timothy Chen wrote:
> Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
> Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
>
> Jie Yu wrote:
> There is no ship-it yet. Please post te latest patch please.
>
> Timothy Chen wrote:
> I think it's because you don't have any email setup under reviewboard too.
>
> Timothy Chen wrote:
> Ok I just merged this with manually putting your name and email in. Please update reviewboard with your email next time.
>
> R.B. Boyer wrote:
> Huh. I swear I've configured my profile in reviewboard. Does the profile have to be public (mine is marked private right now)?
Why has this patch been committed? It does not look like the '/bin/ls -l --author' issue has been resolved. :-(
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
>
> Timothy Chen wrote:
> Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
> Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
>
> Jie Yu wrote:
> There is no ship-it yet. Please post te latest patch please.
>
> Timothy Chen wrote:
> I think it's because you don't have any email setup under reviewboard too.
Ok I just merged this with manually putting your name and email in. Please update reviewboard with your email next time.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
>
> Timothy Chen wrote:
> Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
> Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
>
> Jie Yu wrote:
> There is no ship-it yet. Please post te latest patch please.
>
> Timothy Chen wrote:
> I think it's because you don't have any email setup under reviewboard too.
>
> Timothy Chen wrote:
> Ok I just merged this with manually putting your name and email in. Please update reviewboard with your email next time.
Huh. I swear I've configured my profile in reviewboard. Does the profile have to be public (mine is marked private right now)?
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 23, 2014, 9:04 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 9:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
Is there anything else for me to do on this patch?
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 23, 2014, 9:04 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 9:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
I can however do a echo --author, that would have broke earlier right?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
>
> Timothy Chen wrote:
> Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
> Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
There is no ship-it yet. Please post te latest patch please.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
>
> R.B. Boyer wrote:
> I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
>
> If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
>
> GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
>
> To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
>
> Timothy Chen wrote:
> Ok I'll do that.
>
> R.B. Boyer wrote:
> Is there anything else for me to do on this patch?
>
> Jie Yu wrote:
> Tim, are you working on the patch?
>
> Timothy Chen wrote:
> Ya sorry was busy with docker patches, will get to this when they're pushed.
Hi R.B., I can't apply your patch cleanly for some reason it can't find your name/email.
Can you try to reupload it again? If I don't apply your patch I can't commit it with your name/email.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
>
> R.B. Boyer wrote:
> The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
>
> Timothy Chen wrote:
> unfortunately ls doesn't have any double dash options, and I just looked at /usr/bin and none of the non-intrusive binaries has any double dash options too.
> I can however do a echo --author, that would have broke earlier right?
I don't have access to an OSX box, nor will I have time until mid next week to revisit this ticket.
If you can, please try swapping out "[echo, --author]" for my ls example, reverting slave.cpp, and then:
GTEST_FILTER=SlaveTest.MesosExecutorCommandTaskWithArgsList make check
To just double check that "echo --author" killed mesos-executor on OSX with the unpatched protobuf stuff.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 23, 2014, 9:04 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 9:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > <https://reviews.apache.org/r/26622/diff/4/?file=731893#file731893line448>
> >
> > Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
> >
> > It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
The part that actually breaks is any double-dash option arguments. If the OSX version of ls shares any other double-dash arg options with linux then just replace --author with that.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
On Oct. 23, 2014, 9:04 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 9:04 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58287
-----------------------------------------------------------
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/26622/#comment99235>
I think I'll yank this out since this is already fixed.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/26622/#comment99236>
Just tried bin/ls on OSX and realize Mac's ls has no --author flag so this is going to fail on OSX.
It's ok if we don't support it but then we need to wrap the test in #ifdef __linux__
- Timothy Chen
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review59472
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment100755>
This comment is telling me *what* you're doing, but not *why* you're doing that ... what keeps someone in the future from just changing this back to the original because they think they are making a nice simplification?
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment100756>
Again, *why* are we doing this? Especially given this is the default, it's not obvious *why* for another person reading the code.
- Benjamin Hindman
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 9:04 p.m.)
Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
Bugs: MESOS-1873
https://issues.apache.org/jira/browse/MESOS-1873
Repository: mesos-git
Description
-------
Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
Don't pass task-related arguments to mesos-executor.
The description on the linked jira ticket goes into more detail.
Diffs (updated)
-----
src/slave/slave.cpp 55e5264
src/tests/slave_tests.cpp a1bd00c
Diff: https://reviews.apache.org/r/26622/diff/
Testing
-------
make check
added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
Thanks,
R.B. Boyer
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/
-----------------------------------------------------------
(Updated Oct. 23, 2014, 8:30 p.m.)
Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
Changes
-------
Switched CommandInfo to whitelist fields rather than using MergeFrom. Also added an end-to-end test.
Bugs: MESOS-1873
https://issues.apache.org/jira/browse/MESOS-1873
Repository: mesos-git
Description
-------
Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
Don't pass task-related arguments to mesos-executor.
The description on the linked jira ticket goes into more detail.
Diffs (updated)
-----
src/slave/slave.cpp 55e5264
src/tests/slave_tests.cpp a1bd00c
Diff: https://reviews.apache.org/r/26622/diff/
Testing
-------
make check
added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
Thanks,
R.B. Boyer
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2558-2561
> > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2558>
> >
> > The comments here are confusing. We need to explictly call out that the reason we copying uris, environments and user is because we do not perform fetch/setting environments/change user in mesos-executor. We reply on containerizer to do that for mesos-executor so that the task can inherit those settings.
>
> R.B. Boyer wrote:
> Ah so mesos-executor requires only "uris", "environment", AND "user"? If that's the case I'm totally rewriting this to do explicit copying instead of playing protobuf tricks with merging.
>
> Jie Yu wrote:
> Rest of the fields are explicitly set in this function. I am not sure about 'container' because it's being deprecated. To be safe, let's copy 'container' as well.
>
> R.B. Boyer wrote:
> Will do: [uris, environment, user, container]
Can you put the reason why we need to copy these fields (see my above comments) to the comment?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 24, 2014, 2:04 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 2:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line431>
> >
> > It would be great if you can write an end-to-end test so that we can verify that the 'command' actually runs correctly with shell=false and has an argument list (refer to the following test).
>
> R.B. Boyer wrote:
> I would love to try writing an e2e test to test this, but I spent several hours on it before caving and just writing this GetExecutorInfo test. I have about zero experience writing C++ code let alone C++ unit tests so I was fighting with the project more than fighting with the bug.
>
> Timothy Chen wrote:
> You can pretty much copy the MesosExecutorWithOverride test in slave_tests.cpp, and just modify the CommandInfo accordingly. Let us know if you still don't know what to do and spend too much time figuring out.
>
> R.B. Boyer wrote:
> I think what I had trouble with there when last I was working on that was actually asserting that mesos-executor died (exit != 0) when passed double-dash arguments. I could get the test to pass, but it also passed without the fix.
>
> R.B. Boyer wrote:
> So this is the part of that unit test I have no idea how to adapt:
>
> // Expect the launch and just assume it was sucessful since we'll be
> // launching the executor ourselves manually below.
> Future<Nothing> launch;
> EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
> .WillOnce(DoAll(FutureSatisfy(&launch),
> Return(true)));
> ...and more ...
>
> In my case I need to strongly assert that the command launched without failing (which requires a mesos-executor binary to actually run) OR inject a Capture of some sort here and trap the exec to assert it is sane without finishing the e2e part of this.
Ah ok so you should look at this test instead (ROOT_RunTaskWithCommandInfoWithoutUser) which simply launches a real slave and run through the executor launch.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line431>
> >
> > It would be great if you can write an end-to-end test so that we can verify that the 'command' actually runs correctly with shell=false and has an argument list (refer to the following test).
>
> R.B. Boyer wrote:
> I would love to try writing an e2e test to test this, but I spent several hours on it before caving and just writing this GetExecutorInfo test. I have about zero experience writing C++ code let alone C++ unit tests so I was fighting with the project more than fighting with the bug.
You can pretty much copy the MesosExecutorWithOverride test in slave_tests.cpp, and just modify the CommandInfo accordingly. Let us know if you still don't know what to do and spend too much time figuring out.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line431>
> >
> > It would be great if you can write an end-to-end test so that we can verify that the 'command' actually runs correctly with shell=false and has an argument list (refer to the following test).
I would love to try writing an e2e test to test this, but I spent several hours on it before caving and just writing this GetExecutorInfo test. I have about zero experience writing C++ code let alone C++ unit tests so I was fighting with the project more than fighting with the bug.
> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2558-2561
> > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2558>
> >
> > The comments here are confusing. We need to explictly call out that the reason we copying uris, environments and user is because we do not perform fetch/setting environments/change user in mesos-executor. We reply on containerizer to do that for mesos-executor so that the task can inherit those settings.
Ah so mesos-executor requires only "uris", "environment", AND "user"? If that's the case I'm totally rewriting this to do explicit copying instead of playing protobuf tricks with merging.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2558-2561
> > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2558>
> >
> > The comments here are confusing. We need to explictly call out that the reason we copying uris, environments and user is because we do not perform fetch/setting environments/change user in mesos-executor. We reply on containerizer to do that for mesos-executor so that the task can inherit those settings.
>
> R.B. Boyer wrote:
> Ah so mesos-executor requires only "uris", "environment", AND "user"? If that's the case I'm totally rewriting this to do explicit copying instead of playing protobuf tricks with merging.
Rest of the fields are explicitly set in this function. I am not sure about 'container' because it's being deprecated. To be safe, let's copy 'container' as well.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line431>
> >
> > It would be great if you can write an end-to-end test so that we can verify that the 'command' actually runs correctly with shell=false and has an argument list (refer to the following test).
>
> R.B. Boyer wrote:
> I would love to try writing an e2e test to test this, but I spent several hours on it before caving and just writing this GetExecutorInfo test. I have about zero experience writing C++ code let alone C++ unit tests so I was fighting with the project more than fighting with the bug.
>
> Timothy Chen wrote:
> You can pretty much copy the MesosExecutorWithOverride test in slave_tests.cpp, and just modify the CommandInfo accordingly. Let us know if you still don't know what to do and spend too much time figuring out.
I think what I had trouble with there when last I was working on that was actually asserting that mesos-executor died (exit != 0) when passed double-dash arguments. I could get the test to pass, but it also passed without the fix.
> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2558-2561
> > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2558>
> >
> > The comments here are confusing. We need to explictly call out that the reason we copying uris, environments and user is because we do not perform fetch/setting environments/change user in mesos-executor. We reply on containerizer to do that for mesos-executor so that the task can inherit those settings.
>
> R.B. Boyer wrote:
> Ah so mesos-executor requires only "uris", "environment", AND "user"? If that's the case I'm totally rewriting this to do explicit copying instead of playing protobuf tricks with merging.
>
> Jie Yu wrote:
> Rest of the fields are explicitly set in this function. I am not sure about 'container' because it's being deprecated. To be safe, let's copy 'container' as well.
Will do: [uris, environment, user, container]
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line431>
> >
> > It would be great if you can write an end-to-end test so that we can verify that the 'command' actually runs correctly with shell=false and has an argument list (refer to the following test).
>
> R.B. Boyer wrote:
> I would love to try writing an e2e test to test this, but I spent several hours on it before caving and just writing this GetExecutorInfo test. I have about zero experience writing C++ code let alone C++ unit tests so I was fighting with the project more than fighting with the bug.
>
> Timothy Chen wrote:
> You can pretty much copy the MesosExecutorWithOverride test in slave_tests.cpp, and just modify the CommandInfo accordingly. Let us know if you still don't know what to do and spend too much time figuring out.
>
> R.B. Boyer wrote:
> I think what I had trouble with there when last I was working on that was actually asserting that mesos-executor died (exit != 0) when passed double-dash arguments. I could get the test to pass, but it also passed without the fix.
So this is the part of that unit test I have no idea how to adapt:
// Expect the launch and just assume it was sucessful since we'll be
// launching the executor ourselves manually below.
Future<Nothing> launch;
EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _))
.WillOnce(DoAll(FutureSatisfy(&launch),
Return(true)));
...and more ...
In my case I need to strongly assert that the command launched without failing (which requires a mesos-executor binary to actually run) OR inject a Capture of some sort here and trap the exec to assert it is sane without finishing the e2e part of this.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58149
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment99074>
The comments here are confusing. We need to explictly call out that the reason we copying uris, environments and user is because we do not perform fetch/setting environments/change user in mesos-executor. We reply on containerizer to do that for mesos-executor so that the task can inherit those settings.
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment99066>
+1
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/26622/#comment99076>
It would be great if you can write an end-to-end test so that we can verify that the 'command' actually runs correctly with shell=false and has an argument list (refer to the following test).
- Jie Yu
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/slave/slave.cpp, line 2564
> > <https://reviews.apache.org/r/26622/diff/2/?file=719277#file719277line2564>
> >
> > I wonder if we instead should just copy the arguments we explicitly want? If we add another field to commandInfo that is not intended here it will break again.
This would be nicer, if you think it's more appropriate I'll update the patch to ONLY copy "uris" and "environment" as the embedded comment from the original author indicates.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 23, 2014, 9:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line420>
> >
> > bells and whistles doesn't seem to be a good description of what you're trying to do. Perhaps just say you're setting shell and arguments in the original TaskInfo and checking later those are not used to launch the command executor?
>
> R.B. Boyer wrote:
> I totally won't mind if you rewrite the commentary and unit tests here. I've barely read any of the mesos source (only enough to identify this bug) so I was largely submitting the patch to accelerate the actual fix from someone who understands the code and the right way to fix this.
We usually let the submitter finish all the code changes, and commit it with your name/email so you get the credit of doing this.
If you want I can totally take over and rewrite your fix.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 402
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line402>
> >
> > delete the detector
This is my laissez-faire java approach to heap allocation bleeding through.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
On Oct. 23, 2014, 8:30 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2014, 8:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 55e5264
> src/tests/slave_tests.cpp a1bd00c
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line420>
> >
> > bells and whistles doesn't seem to be a good description of what you're trying to do. Perhaps just say you're setting shell and arguments in the original TaskInfo and checking later those are not used to launch the command executor?
I totally won't mind if you rewrite the commentary and unit tests here. I've barely read any of the mesos source (only enough to identify this bug) so I was largely submitting the patch to accelerate the actual fix from someone who understands the code and the right way to fix this.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by "R.B. Boyer" <bo...@nexusvector.net>.
> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line420>
> >
> > bells and whistles doesn't seem to be a good description of what you're trying to do. Perhaps just say you're setting shell and arguments in the original TaskInfo and checking later those are not used to launch the command executor?
>
> R.B. Boyer wrote:
> I totally won't mind if you rewrite the commentary and unit tests here. I've barely read any of the mesos source (only enough to identify this bug) so I was largely submitting the patch to accelerate the actual fix from someone who understands the code and the right way to fix this.
>
> Timothy Chen wrote:
> We usually let the submitter finish all the code changes, and commit it with your name/email so you get the credit of doing this.
> If you want I can totally take over and rewrite your fix.
>
> Timothy Chen wrote:
> Btw I think if you address the issues I mentioned it should be ready.
Ok. Will try to make time tomorrow if possible to clean this up.
- R.B.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
On Oct. 17, 2014, 11:40 p.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2014, 11:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 23, 2014, 9:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line420>
> >
> > bells and whistles doesn't seem to be a good description of what you're trying to do. Perhaps just say you're setting shell and arguments in the original TaskInfo and checking later those are not used to launch the command executor?
>
> R.B. Boyer wrote:
> I totally won't mind if you rewrite the commentary and unit tests here. I've barely read any of the mesos source (only enough to identify this bug) so I was largely submitting the patch to accelerate the actual fix from someone who understands the code and the right way to fix this.
>
> Timothy Chen wrote:
> We usually let the submitter finish all the code changes, and commit it with your name/email so you get the credit of doing this.
> If you want I can totally take over and rewrite your fix.
Btw I think if you address the issues I mentioned it should be ready.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>
Re: Review Request 26622: Command Executor is broken when used with
shell=false
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review58102
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment99004>
I wonder if we instead should just copy the arguments we explicitly want? If we add another field to commandInfo that is not intended here it will break again.
src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment99005>
Ah I see, sorry I totally missed what's the point of this jira/rb.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/26622/#comment99001>
delete the detector
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/26622/#comment99002>
bells and whistles doesn't seem to be a good description of what you're trying to do. Perhaps just say you're setting shell and arguments in the original TaskInfo and checking later those are not used to launch the command executor?
- Timothy Chen
On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2014, 4:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
>
>
> Bugs: MESOS-1873
> https://issues.apache.org/jira/browse/MESOS-1873
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor it is completely broken.
>
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
>
> Don't pass task-related arguments to mesos-executor.
>
> The description on the linked jira ticket goes into more detail.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 0e342ed
> src/tests/slave_tests.cpp f585bdd
>
> Diff: https://reviews.apache.org/r/26622/diff/
>
>
> Testing
> -------
>
> make check
>
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
>
>
> Thanks,
>
> R.B. Boyer
>
>