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