You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/06/22 03:56:21 UTC
Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/
-----------------------------------------------------------
Review request for mesos.
Repository: mesos
Description
-------
Set the owner of persistent volumes to frameworkInfo.user .
Diffs
-----
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
Diff: https://reviews.apache.org/r/35721/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/
-----------------------------------------------------------
(Updated June 28, 2015, 5:46 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2603
https://issues.apache.org/jira/browse/MESOS-2603
Repository: mesos
Description
-------
Set the owner of persistent volumes to frameworkInfo.user .
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 313e9b74d3a0157609226041246575d2c4a503f8
Diff: https://reviews.apache.org/r/35721/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/
-----------------------------------------------------------
(Updated June 28, 2015, 5:42 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2603
https://issues.apache.org/jira/browse/MESOS-2603
Repository: mesos
Description
-------
Set the owner of persistent volumes to frameworkInfo.user .
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 313e9b74d3a0157609226041246575d2c4a503f8
Diff: https://reviews.apache.org/r/35721/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review88717
-----------------------------------------------------------
Patch looks great!
Reviews applied: [35721]
All tests passed.
- Mesos ReviewBot
On June 22, 2015, 1:56 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 1:56 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89430
-----------------------------------------------------------
src/messages/messages.proto (line 336)
<https://reviews.apache.org/r/35721/#comment142037>
Why do you need to pass the user through CheckpointResourcesMessage. Slave should already keep track of FrameworkInfo for each live framework.
src/slave/slave.cpp (lines 2153 - 2157)
<https://reviews.apache.org/r/35721/#comment142039>
Ditto Adam's comment on switch_user.
Also, what if the user specifies a user in CommandInfo. The current semantics is that the executor will be launched under that user. See https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L4633
I think the owner/group of a persistent volume should be the same as the sandbox directory. So probably you can chown whenever you link the volumes (see updateVolumes in MesosContainerizer, which soon will be moved to a posix filesystem isolator https://reviews.apache.org/r/34135/) and sync that with the sandbox owner/group.
- Jie Yu
On June 22, 2015, 1:56 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 1:56 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89247
-----------------------------------------------------------
This is good, see my only comment, not even sure that's an issue here.
Can we please however test this (also with NFS mounts - they do behave sometimes weirdly when it comes to chown on mounts)
src/slave/slave.cpp (line 2154)
<https://reviews.apache.org/r/35721/#comment141836>
you don't actually need the `true` (that is the default value).
But I do wonder whether the recursive behavior is the desired one here?
it risks making data/resources (in subdirs, created by different users) unavailable for processes/tasks that are run.
We should probably also LOG(INFO) here as it may help when debugging "permission denied" issues.
- Marco Massenzio
On June 22, 2015, 1:56 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 1:56 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by haosdent huang <ha...@gmail.com>.
> On June 24, 2015, 8:04 a.m., Adam B wrote:
> > This seems like a positive step forward, since all volumes were previously owned by root (or whatever user the slave was run as). However, if a persistent volume is passed from one framework to another within the same role, they might have different users, so the second framework would still be unable to access it. Moreover, different tasks within the same framework might run as different users, so if two subsequent tasks try to mount the same persistent volume as different users, they won't both be able to access it.
> > So, in addition to initially chowning the volume to the frameworkInfo.user, we should also consider chowning the volume to the commandInfo.user within `_runTask` so that the volume ownership is changed to the user running each task before the task begins executing, so that each task mounting the volume is guaranteed access to the volume.
>
> haosdent huang wrote:
> Thank you very much for your review. If we change the persistent volume in _runTask, does it cause the scenario like this:
>
> task_A use user_A, task_B use user_B. And then they running at the same time. And the persistent volume could not belong to two different users at the same time. So one of these two tasks would be failed. Is it acceptable?
>
> Adam B wrote:
> Currently we don't allow two concurrent tasks to use the same persistent volume. I suppose this could change in the future, but for now we should be safe chowning the volume before each task takes ownership of the volume.
> In a future with shared persistent volumes, perhaps we could create a linux group for the role, make the volume rw for the group, and add each task user to the group. This is probably overkill for the moment.
>
> haosdent huang wrote:
> Got it. Let me remove it and chown user in runTask
@adam-mesos, @jieyu, @macro Thank you very much for your review. And now I change the implementation to sync the ownership from sandbox directory to persistent volume in "MesosContainerizerProcess::updateVolumes". Could you help me review it again? Thank you very much.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89138
-----------------------------------------------------------
On June 28, 2015, 5:46 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 28, 2015, 5:46 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, and Marco Massenzio.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 313e9b74d3a0157609226041246575d2c4a503f8
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by Adam B <ad...@mesosphere.io>.
> On June 24, 2015, 1:04 a.m., Adam B wrote:
> > This seems like a positive step forward, since all volumes were previously owned by root (or whatever user the slave was run as). However, if a persistent volume is passed from one framework to another within the same role, they might have different users, so the second framework would still be unable to access it. Moreover, different tasks within the same framework might run as different users, so if two subsequent tasks try to mount the same persistent volume as different users, they won't both be able to access it.
> > So, in addition to initially chowning the volume to the frameworkInfo.user, we should also consider chowning the volume to the commandInfo.user within `_runTask` so that the volume ownership is changed to the user running each task before the task begins executing, so that each task mounting the volume is guaranteed access to the volume.
>
> haosdent huang wrote:
> Thank you very much for your review. If we change the persistent volume in _runTask, does it cause the scenario like this:
>
> task_A use user_A, task_B use user_B. And then they running at the same time. And the persistent volume could not belong to two different users at the same time. So one of these two tasks would be failed. Is it acceptable?
Currently we don't allow two concurrent tasks to use the same persistent volume. I suppose this could change in the future, but for now we should be safe chowning the volume before each task takes ownership of the volume.
In a future with shared persistent volumes, perhaps we could create a linux group for the role, make the volume rw for the group, and add each task user to the group. This is probably overkill for the moment.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89138
-----------------------------------------------------------
On June 21, 2015, 6:56 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 21, 2015, 6:56 p.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by haosdent huang <ha...@gmail.com>.
> On June 24, 2015, 8:04 a.m., Adam B wrote:
> > This seems like a positive step forward, since all volumes were previously owned by root (or whatever user the slave was run as). However, if a persistent volume is passed from one framework to another within the same role, they might have different users, so the second framework would still be unable to access it. Moreover, different tasks within the same framework might run as different users, so if two subsequent tasks try to mount the same persistent volume as different users, they won't both be able to access it.
> > So, in addition to initially chowning the volume to the frameworkInfo.user, we should also consider chowning the volume to the commandInfo.user within `_runTask` so that the volume ownership is changed to the user running each task before the task begins executing, so that each task mounting the volume is guaranteed access to the volume.
>
> haosdent huang wrote:
> Thank you very much for your review. If we change the persistent volume in _runTask, does it cause the scenario like this:
>
> task_A use user_A, task_B use user_B. And then they running at the same time. And the persistent volume could not belong to two different users at the same time. So one of these two tasks would be failed. Is it acceptable?
>
> Adam B wrote:
> Currently we don't allow two concurrent tasks to use the same persistent volume. I suppose this could change in the future, but for now we should be safe chowning the volume before each task takes ownership of the volume.
> In a future with shared persistent volumes, perhaps we could create a linux group for the role, make the volume rw for the group, and add each task user to the group. This is probably overkill for the moment.
Got it. Let me remove it and chown user in runTask
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89138
-----------------------------------------------------------
On June 22, 2015, 1:56 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 1:56 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by haosdent huang <ha...@gmail.com>.
> On June 24, 2015, 8:04 a.m., Adam B wrote:
> > This seems like a positive step forward, since all volumes were previously owned by root (or whatever user the slave was run as). However, if a persistent volume is passed from one framework to another within the same role, they might have different users, so the second framework would still be unable to access it. Moreover, different tasks within the same framework might run as different users, so if two subsequent tasks try to mount the same persistent volume as different users, they won't both be able to access it.
> > So, in addition to initially chowning the volume to the frameworkInfo.user, we should also consider chowning the volume to the commandInfo.user within `_runTask` so that the volume ownership is changed to the user running each task before the task begins executing, so that each task mounting the volume is guaranteed access to the volume.
Thank you very much for your review. If we change the persistent volume in _runTask, does it cause the scenario like this:
task_A use user_A, task_B use user_B. And then they running at the same time. And the persistent volume could not belong to two different users at the same time. So one of these two tasks would be failed. Is it acceptable?
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89138
-----------------------------------------------------------
On June 22, 2015, 1:56 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 1:56 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/#review89138
-----------------------------------------------------------
This seems like a positive step forward, since all volumes were previously owned by root (or whatever user the slave was run as). However, if a persistent volume is passed from one framework to another within the same role, they might have different users, so the second framework would still be unable to access it. Moreover, different tasks within the same framework might run as different users, so if two subsequent tasks try to mount the same persistent volume as different users, they won't both be able to access it.
So, in addition to initially chowning the volume to the frameworkInfo.user, we should also consider chowning the volume to the commandInfo.user within `_runTask` so that the volume ownership is changed to the user running each task before the task begins executing, so that each task mounting the volume is guaranteed access to the volume.
src/master/master.cpp (line 5108)
<https://reviews.apache.org/r/35721/#comment141733>
Would we need to re-chown the volume directory if the frameworkinfo.user changes? Not an issue now, but something to consider as we begin to allow updating FrameworkInfo fields.
src/messages/messages.proto (lines 333 - 334)
<https://reviews.apache.org/r/35721/#comment141735>
This is specifically about the user that owns the persistent volumes. It has no impact on dynamic reservations, since they are tied to the role/framework, not a particular linux user. Please be specific about the relationship with persistent volumes. If we add more operations for which the user field is relevant, we can update the comment to reference them as well.
src/messages/messages.proto (line 335)
<https://reviews.apache.org/r/35721/#comment141734>
s/launch/launched the/
src/slave/slave.hpp (lines 152 - 153)
<https://reviews.apache.org/r/35721/#comment141732>
We usually like to keep the order here matching the order of the fields in the protobuf message. Makes it easier to manage if/when we add more fields.
src/slave/slave.cpp (lines 2150 - 2153)
<https://reviews.apache.org/r/35721/#comment141736>
Does this need to be within the !exists check? Would we ever want to chown a persistent volume that already exists? Maybe only if the frameworkInfo.user changes (not possible yet).
src/slave/slave.cpp (line 2154)
<https://reviews.apache.org/r/35721/#comment141737>
We should only chown this if `flags.switch_user` is true (default). If it is false, all tasks will run as the slave user, and volumes will need to stay as that user in order to be accessible by the tasks.
- Adam B
On June 21, 2015, 6:56 p.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35721/
> -----------------------------------------------------------
>
> (Updated June 21, 2015, 6:56 p.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Bugs: MESOS-2603
> https://issues.apache.org/jira/browse/MESOS-2603
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Set the owner of persistent volumes to frameworkInfo.user .
>
>
> Diffs
> -----
>
> src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
> src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
> src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
> src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
>
> Diff: https://reviews.apache.org/r/35721/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 35721: Set the owner of persistent volumes to
frameworkInfo.user .
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35721/
-----------------------------------------------------------
(Updated June 22, 2015, 1:56 a.m.)
Review request for mesos, Adam B and Jie Yu.
Bugs: MESOS-2603
https://issues.apache.org/jira/browse/MESOS-2603
Repository: mesos
Description
-------
Set the owner of persistent volumes to frameworkInfo.user .
Diffs
-----
src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d
src/messages/messages.proto 1c8d79e3fca365520cdd67051f8730593955cab6
src/slave/slave.hpp f1cf3b85ccb3eaf614fe844c830f7cc44f7916fe
src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
Diff: https://reviews.apache.org/r/35721/diff/
Testing
-------
Thanks,
haosdent huang