Re: Review Request 26622: Command Executor is broken when used with shell=false

[Timothy Chen]

---
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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Benjamin Hindman]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Benjamin Hindman]

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



src/slave/slave.cpp


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


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

[R.B. Boyer]

> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Jie Yu]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Jie Yu]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[R.B. Boyer]

> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[R.B. Boyer]

> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 24, 2014, 4:40 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Jie Yu]

> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2558-2561
> > 
> >
> > 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

[R.B. Boyer]

> On Oct. 24, 2014, 11:40 a.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 448
> > 
> >
> > 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

[Timothy Chen]

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



src/tests/slave_tests.cpp


I think I'll yank this out since this is already fixed.



src/tests/slave_tests.cpp


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

[R.B. Boyer]

> 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

[Mesos ReviewBot]

---
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

[R.B. Boyer]

---
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

[R.B. Boyer]

> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 402
> > 
> >
> > 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

[R.B. Boyer]

---
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

[Timothy Chen]

> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > 
> >
> > 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 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

[R.B. Boyer]

> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > 
> >
> > 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 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

[R.B. Boyer]

> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > 
> >
> > 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
> > 
> >
> > 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

[Jie Yu]

> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 2558-2561
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > 
> >
> > 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

[R.B. Boyer]

> On Oct. 23, 2014, 5:48 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, lines 431-436
> > 
> >
> > 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
> > 
> >
> > 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

[R.B. Boyer]

> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/slave/slave.cpp, line 2564
> > 
> >
> > 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

[Jie Yu]

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



src/slave/slave.cpp


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


+1



src/tests/slave_tests.cpp


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

[R.B. Boyer]

> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 23, 2014, 9:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > 
> >
> > 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

[Timothy Chen]

> On Oct. 23, 2014, 9:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > 
> >
> > 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

[R.B. Boyer]

> On Oct. 23, 2014, 4:11 p.m., Timothy Chen wrote:
> > src/tests/slave_tests.cpp, line 420
> > 
> >
> > 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

[Timothy Chen]

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



src/slave/slave.cpp


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


Ah I see, sorry I totally missed what's the point of this jira/rb.



src/tests/slave_tests.cpp


delete the detector



src/tests/slave_tests.cpp


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
> 
>

Re: Review Request 26622: Command Executor is broken when used with shell=false

[R.B. Boyer]

> On Oct. 23, 2014, 11:03 a.m., Timothy Chen wrote:
> > src/slave/slave.cpp, line 2567
> > 
> >
> > 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

[Timothy Chen]

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



src/slave/slave.cpp


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

[R.B. Boyer]

---
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