> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/master/validation.hpp, lines 72-74
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511369#file1511369line72>
> >
> >     Not quite sure what `isTaskContext` is?

We want to make sure persistent volumes when CREATEd have a mode as `RW`, but 
for tasks it can be either `RO` or `RW`. As discussed, we shall not check for 
mode in this function. Instead, that check shall be contained within 
`Option<Error> validatePersistentVolume(const RepeatedPtrField<Resource>& 
volumes)` which is only called when validating volumes in `CREATE` or `DESTROY` 
calls.


> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 426-428
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511370#file1511370line426>
> >
> >     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.

See previous response.


> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp, lines 659-660
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511372#file1511372line659>
> >
> >     What's the problem with the previous formatting?

It crosses 80 chars mainly because it is not in a nested block.


> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp, lines 698-701
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511372#file1511372line698>
> >
> >     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?

Great catch. Apparently, this inconsistency has been fixed in util-linux v2.27. 
Prior to this version, we need to do a bind mount followed by a remount as 
read-only. Post util-linux v2.27, a single bind mount as read-only should 
suffice.
So, I moved this to a 2-stage mount.


- Anindya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45963/#review151802
-----------------------------------------------------------


On Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45963/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2016, 5:24 a.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, launch the task but if the task is unable to read/write the
> persistent volume, it would fail at that point of time.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8f62162519f12a157c28ca5f2a76502e466c1481 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> af427c6e5691f1770ab3ebef79502eb2c2176c4a 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> eb191a32381f9d1ca84ec29adf352dde375c2f2d 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/45963/diff/
> 
> 
> Testing
> -------
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Reply via email to