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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (lines 89 - 
90)
<https://reviews.apache.org/r/45673/#comment193126>

    I would swap the order of these two.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (lines 108 - 
109)
<https://reviews.apache.org/r/45673/#comment193117>

    To me, this is just an optimization, isn't it? We can still go through the 
'infos' list to see a volume is still begin used, right?
    
    Can we not introduce this optimizaiton yet?



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 282)
<https://reviews.apache.org/r/45673/#comment193136>

    This can just be a nested struct in IsolatorProcess (no need to be a 
protobuf):
    ```
    struct MountInfo
    {
      DockerVolume volume;
      string containerPath;
    };
    ```



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 316 - 
317)
<https://reviews.apache.org/r/45673/#comment193134>

    Let's try to write JSON files instead of protobuf files directly. It's 
better to be human readable.



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 332)
<https://reviews.apache.org/r/45673/#comment193135>

    We need to serialize calls to 'client' for a given driver and a given 
volume name. You can use `Sequence` for each driver+name pair in 
DockerVolumeDriverClient to achieve that.



src/slave/containerizer/mesos/isolators/docker/volume/state.hpp (line 29)
<https://reviews.apache.org/r/45673/#comment193127>

    Should this be nested in IsolatorProcess?



src/slave/containerizer/mesos/isolators/docker/volume/state.hpp (line 34)
<https://reviews.apache.org/r/45673/#comment193130>

    I suggest that we only put informations that we can recover in 'Info' 
struct. For things like container_path, mount_point that we cannot recover, try 
not to put them in 'Info' struct. You can always create a parallel vector with 
those additional information, and pass down using parameters in the lambda.


- Jie Yu


On April 19, 2016, 4:45 p.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45673/
> -----------------------------------------------------------
> 
> (Updated April 19, 2016, 4:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> PoC: Docker Volume Isolator.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt aabb33dfd4b985fe5774f87bb63b4ec1ec853962 
>   src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac 
>   src/cli/execute.cpp 087a73143d739f41638b8335c7e24dfcd14bc7fb 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1e1a36903f4377497bb72b69e4ead63675d453c0 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 786f917c0ec04b6839bbd524fa7c8de3729f9bdb 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 0ad473dc3bd45e122fba55a670e1a893e61c977a 
>   src/slave/containerizer/mesos/isolators/docker/volume/paths.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/paths.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/state.hpp 
> PRE-CREATION 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
> 
> Diff: https://reviews.apache.org/r/45673/diff/
> 
> 
> Testing
> -------
> 
> state.proto
> 
> message DockerVolumeMount {
>   required string volume_driver = 1;
>   required string volume_name = 2;
> }
> 
> 
> message DockerVolumeMountList {
>   repeated DockerVolumeMount mounts = 1;
> }
> 
> 
> message DockerVolumeInfo {
>   // Docker volume mount info for volume driver and volume name.
>   required DockerVolumeMount dvm = 1;
> 
>   // Container path, this is optional as this is not needed by recover()
>   // but only for prepare() .
>   optional string container_path = 2;
> 
>   // Mount point for the container, this is optiona as this is only needed
>   // by prepare().
>   optional string mount_point = 3;
>   
>   // Volume driver mount options.
>   optional Parameters driver_options = 4;
> }
> 
> DockerVolumeMount will be checkpointed.
> 
> 
> //   /var/run/mesos/isolators/docker/volume
> //    |- <ID of Container1>/
> //    |  |-- DockerVolumeMountList
> //    |-- <ID of Container2>/
> //    |  |-- DockerVolumeMountList
> //    |-<ID of Container3>/
> //    |- ...
> 
> Info structure
> We assume that one container do not use multiple same volume driver and 
> volume name pair.
> struct Info
> {
>   Info (const list<DockerVolumeInfo>& _volumeInfos)
>     : volumeInfos(_volumeInfos) {}
> 
>   list<DockerVolumeInfo> volumeInfos;
> };
> 
> Hashmap in isolator:
> hashmap<ContainerID, process::Owned<Info>> infos;
> hashmap<string, int> volumeReference; // string is 
> volume_driver::volume_name, e.g. convoy:dvd1
> 
> Cleanup()
> When cleanup, check the container’s mount point reference counter, if it is 
> greater than 1, do not remove the mount point; If it is 1, unmount the mount 
> point.
> driver.cpp::mount
> We need to transfer the whole mount info to mount() as parameter to make sure 
> that the container path can get its own related mount point. If the mount() 
> only return a string, then we cannot get the relationship of container path 
> and mount point.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Reply via email to