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




src/master/validation.cpp (lines 500 - 501)
<https://reviews.apache.org/r/45963/#comment221562>

    MESOS-6374 is where we want eventually but for now the same check is in 
`validateDiskInfo()` as well so we don't have to duplicate it here?



src/master/validation.cpp (lines 502 - 503)
<https://reviews.apache.org/r/45963/#comment221560>

    This is called when you destroy a volume as well. So if the framework is 
has changed the volume to RO after creating it then it cannot destroy it (at 
least in the form it was created)...
    
    I think we should just validate this in the following method?
    
    ```
    Option<Error> validate(
        const Offer::Operation::Create& create,
        const Resources& checkpointedResources,
        const Option<string>& principal) {}
    ```



src/slave/containerizer/docker.cpp (line 469)
<https://reviews.apache.org/r/45963/#comment221572>

    s/container directory/sandbox directory/ because the latter term is used 
widely and explained in documentation. "container directory" is not as clear. 
People may wonder "which contianer directory?".



src/slave/containerizer/docker.cpp (line 470)
<https://reviews.apache.org/r/45963/#comment221591>

    We shouldn't use snake casing in this case. Admittedly a single letter `s` 
is usually discourged. So how about
    
    ```
    struct stat s;
    
    ...
    
    const uid_t uid = s.st_uid;
    const gid_t gid = s.st_gid;
    ```
    
    And we use `uid`, `gid` in the code that follows?



src/slave/containerizer/docker.cpp (lines 498 - 499)
<https://reviews.apache.org/r/45963/#comment221570>

    If in use, we do nothing right? 
    
    You already have a comment above `if (!isResourceInUse) {` that explains 
what's going to happen if this variable is true so here perhaps kill the 
comment? The variable name is pretty self-describing to me.



src/slave/containerizer/docker.cpp (line 500)
<https://reviews.apache.org/r/45963/#comment221577>

    Here we are exclusively talking about persistent volumes so I think 
`isVolumeInUse` or even simpler `isInUse` is a better name.



src/slave/containerizer/docker.cpp (lines 509 - 514)
<https://reviews.apache.org/r/45963/#comment221583>

    Consolidate the comments into one paragraph because they are tightly 
related? 
    
    ```
    Set the ownership of the persistent volume to match that of the sandbox 
directory if the volume is not already in use. If the volume is currently in 
use by other containers, tasks in this container may fail to read from or write 
to the persistent volume due to incompatible ownership and file system 
permissions.
    ```



src/slave/containerizer/docker.cpp (lines 516 - 521)
<https://reviews.apache.org/r/45963/#comment221585>

    This is no longer true, and the comment above already serves as the caution 
note so we can kill this.



src/slave/containerizer/docker.cpp (line 552)
<https://reviews.apache.org/r/45963/#comment221568>

    What info in flags is relevant here?



src/slave/containerizer/docker.cpp (line 555)
<https://reviews.apache.org/r/45963/#comment221565>

    From the persistent volume to the container?



src/slave/containerizer/docker.cpp (line 570)
<https://reviews.apache.org/r/45963/#comment221567>

    s/(as read-only)/as read-only/ since the read-only part is the key info 
here and not "an aside"?



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 648)
<https://reviews.apache.org/r/45963/#comment221592>

    Comments for linux.cpp are the same as those for docker.cpp.



src/slave/containerizer/mesos/isolators/filesystem/posix.cpp (lines 153 - 158)
<https://reviews.apache.org/r/45963/#comment221593>

    Comments for posix.cpp are the same as those for docker.cpp.



src/slave/containerizer/mesos/isolators/filesystem/posix.cpp (lines 266 - 268)
<https://reviews.apache.org/r/45963/#comment221638>

    Log a warning if the volume is RO?



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1427)
<https://reviews.apache.org/r/45963/#comment221655>

    Add a summary that mentions "task attempts to write to the volume fails" 
and also "we use a shared persistent volume here but its sharedness is not 
important here. Regular persistent volumes works no differently".



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1428)
<https://reviews.apache.org/r/45963/#comment221654>

    
s/ROOT_SharedVolumeUsageReadOnlyMode/ROOT_WriteAccessSharedPersistentVolumeReadOnlyMode/
    
    The name is slightly longer but the full spelling `SharedPersistentVolume` 
helps with code grepping.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1438)
<https://reviews.apache.org/r/45963/#comment221641>

    We don't need "disk/du" for the purpose of this test.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (lines 1443 - 1446)
<https://reviews.apache.org/r/45963/#comment221642>

    We don't need this since we don't need to do "disk/du".



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1455)
<https://reviews.apache.org/r/45963/#comment221644>

    Nit: use `frameworkInfo.set_role(DEFAULT_TEST_ROLE);`?



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1486)
<https://reviews.apache.org/r/45963/#comment221645>

    Use a period in the comment.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1493)
<https://reviews.apache.org/r/45963/#comment221646>

    Indentation.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1495)
<https://reviews.apache.org/r/45963/#comment221647>

    Kill this.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1499)
<https://reviews.apache.org/r/45963/#comment221649>

    Let's use a simple command such as "echo hello > volume_path/file" without 
the sleep.


- Jiang Yan Xu


On Oct. 12, 2016, 10:24 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45963/
> -----------------------------------------------------------
> 
> (Updated Oct. 12, 2016, 10:24 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, 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