mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 35721: Set the owner of persistent volumes to frameworkInfo.user .
Date Wed, 24 Jun 2015 08:04:03 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message