mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 45673: PoC: Docker Volume Isolator.
Date Tue, 19 Apr 2016 23:40:26 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message