----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45963/#review151802 -----------------------------------------------------------
- Can we move this patch to before the example framework so we can commit this one first and consolidate the example framework into one patch? My apologies if it was the case before but now it seems this patch has higher priority. :) - The same logic exists in src/slave/containerizer/docker.cpp which we need to take care together. - Unit test (at least for the linux filesystem isolator)? src/master/validation.hpp (lines 72 - 74) <https://reviews.apache.org/r/45963/#comment220341> Not quite sure what `isTaskContext` is? src/master/validation.cpp (lines 426 - 428) <https://reviews.apache.org/r/45963/#comment220350> Not sure about the meaning of `isTaskContext` but for simplicty I think we can just remove this check. Previously it didn't make sense to ever allow the volume to be RO but now the situation is changed with shared persistent volumes. Even when the persistent volume is not shared, the task is just going to fail if it attempts to write to a read-only volume so it seems no harm to me that we remove this check. src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 635 - 643) <https://reviews.apache.org/r/45963/#comment220351> But the group id doesn't always need to match, and the volume is created with a `0755` so "same group" is as good as "world". It may be worthwhile to do something more comprehensive to improve filesystem security of sandboxes and volumes but at the moment I'd say we don't go beyond the current expectations: use some sensible defaults without guessing too hard in the absence of explict user input what "should work a little better". In light of MESOS-4324 and considering the fact that users may opt not to set explicit perms when creating persistent volumes, here I think we just need to: - When the volume is not in use, we change the owner to the task user. - When the volume is in use, do nothing. If the task user cannot write to the volume, it'll fail. Note that it's possible that the volume was first used by task user1 (with files written as user1), then unused, then used by task user2. In this case we still change the ownership of the volume to user2 but not the files in it. This is by design. src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 659 - 660) <https://reviews.apache.org/r/45963/#comment220353> What's the problem with the previous formatting? src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 698 - 701) <https://reviews.apache.org/r/45963/#comment220357> This may not work, IIRC I needed to mount twice previously: https://github.com/apache/mesos/blob/4ea9651aabd01f623f2578d2823271488d924c5b/src/slave/containerizer/mesos/provisioner/backends/bind.cpp#L133-L158 But could you verify? i.e., a unit test? src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 779 - 783) <https://reviews.apache.org/r/45963/#comment220345> The caller already guarantees that it's a persistent volume so this is unnecessary. src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 785 - 789) <https://reviews.apache.org/r/45963/#comment220347> We are then left with just these lines in this method with a single caller. Doesn't appear that we need it as a method. Can we kill it and move these lines to the call-site? - Jiang Yan Xu On Oct. 4, 2016, 5:04 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45963/ > ----------------------------------------------------------- > > (Updated Oct. 4, 2016, 5:04 p.m.) > > > Review request for mesos, Greg Mann and Jiang Yan Xu. > > > Bugs: MESOS-4324 > https://issues.apache.org/jira/browse/MESOS-4324 > > > Repository: mesos > > > Description > ------- > > Allow the task to specify the persistent volume access to be read-only > or read-write. Note that the persistent volume is always created as > read-write. > If the task is the first consumer of the shared persistent volume, then > set the ownership of the persistent volume to match that of the task. > Otherwise, allow the task to be executed only if the ownership of the > persistent volume matches that of the task. > Added an option to run the test in mixed (default) mode or shared-only > mode. In mixed mode, multiple shards alternate between shared and > unshared persistent volumes for the tasks. In shared-only mode, all > shards use shared persistent volumes for the tasks. > > > Diffs > ----- > > src/examples/persistent_shared_volume_framework.cpp PRE-CREATION > src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 > src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d > src/slave/containerizer/mesos/isolators/filesystem/linux.hpp > 0a85935550e36c9142d845465cfa70a1634a647a > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > ea418252956c8089acc5a491888ed7f6df6cafcd > src/slave/containerizer/mesos/isolators/filesystem/posix.hpp > 794b6e5990db5f8eb21a6535872f284ca02e0553 > src/slave/containerizer/mesos/isolators/filesystem/posix.cpp > af427c6e5691f1770ab3ebef79502eb2c2176c4a > > Diff: https://reviews.apache.org/r/45963/diff/ > > > Testing > ------- > > Tests successful. > > > Thanks, > > Anindya Sinha > >
