You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/04/07 02:40:36 UTC
Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ian Downes.
Bugs: MESOS-2592
https://issues.apache.org/jira/browse/MESOS-2592
Repository: mesos
Description
-------
During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
Diffs
-----
src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
Diff: https://reviews.apache.org/r/32911/diff/
Testing
-------
Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
Thanks,
Niklas Nielsen
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79104
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32911]
All tests passed.
- Mesos ReviewBot
On April 7, 2015, 12:40 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 12:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 7, 2015, 10:13 a.m., Ian Downes wrote:
> > src/slave/slave.cpp, line 4166
> > <https://reviews.apache.org/r/32911/diff/1/?file=918555#file918555line4166>
> >
> > getExecutorInfo() will return the ExecutorInfo if the TaskInfo includes it, otherwise it will construct one (for the command executor), taking the user from the CommandInfo, if present, otherwise this code will fallback to the user from the FrameworkInfo.
What is the issue here?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79198
-----------------------------------------------------------
On April 6, 2015, 5:40 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 6, 2015, 5:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79198
-----------------------------------------------------------
Ship it!
src/slave/slave.cpp
<https://reviews.apache.org/r/32911/#comment128406>
getExecutorInfo() will return the ExecutorInfo if the TaskInfo includes it, otherwise it will construct one (for the command executor), taking the user from the CommandInfo, if present, otherwise this code will fallback to the user from the FrameworkInfo.
- Ian Downes
On April 6, 2015, 5:40 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 6, 2015, 5:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 6, 2015, 7:22 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 4164
> > <https://reviews.apache.org/r/32911/diff/1/?file=918555#file918555line4164>
> >
> > Great comment! Can we also add something to the end of the comment that says that the user is validated when the task goes through the master? Thanks!
Are you referring to when authentication and ACL's are enabled?
> On April 6, 2015, 7:22 p.m., Benjamin Hindman wrote:
> > src/slave/slave.cpp, line 4166
> > <https://reviews.apache.org/r/32911/diff/1/?file=918555#file918555line4166>
> >
> > Why don't you need to check the taskInfo? Is it because we should have set the executorInfo's 'user' appropriately? If so, let's comment as much, even going as far as introducing a CHECK!
I don't follow; the user may not be set on the task.command.user or task.executor.command.user? It isn't ensured to be set by the master?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79113
-----------------------------------------------------------
On April 6, 2015, 5:40 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 6, 2015, 5:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79113
-----------------------------------------------------------
Ship it!
src/slave/paths.cpp
<https://reviews.apache.org/r/32911/#comment128225>
Let's move the TODO into the body of the 'if' (closer to where the error checking is) and then add a comment above this 'if' that explains that we need to alays 'chown' the directory so that the permissions are correct. Feel free to go as far as noting the JIRA issue and that this had worked in the past because the fetcher was doing it but sometimes we won't be fetching anything hence the need to always do it here.
Also, I'm guessing all containerizers call 'createExecutorPath' except the ExternalContainerizer?
src/slave/slave.cpp
<https://reviews.apache.org/r/32911/#comment128223>
Great comment! Can we also add something to the end of the comment that says that the user is validated when the task goes through the master? Thanks!
src/slave/slave.cpp
<https://reviews.apache.org/r/32911/#comment128224>
Why don't you need to check the taskInfo? Is it because we should have set the executorInfo's 'user' appropriately? If so, let's comment as much, even going as far as introducing a CHECK!
- Benjamin Hindman
On April 7, 2015, 12:40 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 12:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 7, 2015, 1:17 a.m., Joris Van Remoortere wrote:
> > src/slave/containerizer/external_containerizer.cpp, line 374
> > <https://reviews.apache.org/r/32911/diff/1/?file=918552#file918552line374>
> >
> > Can we remove the capture by reference here? I know it's not in the style guide yet, but it will likely be accepted.
+1
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79103
-----------------------------------------------------------
On April 7, 2015, 12:40 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 12:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79103
-----------------------------------------------------------
src/slave/containerizer/external_containerizer.cpp
<https://reviews.apache.org/r/32911/#comment128199>
Can we remove the capture by reference here? I know it's not in the style guide yet, but it will likely be accepted.
- Joris Van Remoortere
On April 7, 2015, 12:40 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 12:40 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61cb096771b7e4a1350079f79a20102e78f9
> src/slave/paths.hpp 1618439d728ded347ec75317ce8dd998acd7ee94
> src/slave/paths.cpp 01ea856aa2e628d4aee5fd31f7e49d147f740e8f
> src/slave/slave.cpp 521624c335b9110e12ee1ff21c3918e5af6a2bde
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/#review79231
-----------------------------------------------------------
Patch looks great!
Reviews applied: [32911]
All tests passed.
- Mesos ReviewBot
On April 7, 2015, 6:10 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32911/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 6:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ian Downes.
>
>
> Bugs: MESOS-2592
> https://issues.apache.org/jira/browse/MESOS-2592
>
>
> Repository: mesos
>
>
> Description
> -------
>
> During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
>
>
> Diffs
> -----
>
> src/slave/containerizer/external_containerizer.cpp 1bbd61c
> src/slave/paths.hpp 1618439
> src/slave/paths.cpp 01ea856
> src/slave/slave.cpp 521624c
>
> Diff: https://reviews.apache.org/r/32911/diff/
>
>
> Testing
> -------
>
> Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 32911: Fixed sandbox ownership bug for executors
without URIs.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32911/
-----------------------------------------------------------
(Updated April 7, 2015, 11:10 a.m.)
Review request for mesos, Benjamin Hindman and Ian Downes.
Bugs: MESOS-2592
https://issues.apache.org/jira/browse/MESOS-2592
Repository: mesos
Description
-------
During recent refactorings, executor directory ownership was delegated to the fetcher. However, the fetcher is not invoked if no URIs are present in the executor or task command. This left some of these tasks broken as the directory ownership defaulted to the mesos-slave's (root).
Diffs (updated)
-----
src/slave/containerizer/external_containerizer.cpp 1bbd61c
src/slave/paths.hpp 1618439
src/slave/paths.cpp 01ea856
src/slave/slave.cpp 521624c
Diff: https://reviews.apache.org/r/32911/diff/
Testing
-------
Functional tests with mesos-execute and make check. Have created JIRA's for introduction of more permission/user tests.
Thanks,
Niklas Nielsen