You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@gmail.com> on 2016/04/17 18:10:31 UTC
Review Request 45673: PoC: Docker Volume Isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45673/
-----------------------------------------------------------
Review request for mesos, Gilbert Song and Jie Yu.
Repository: mesos
Description
-------
PoC: Docker Volumt Isolator.
Will split this to several patches after review discussion.
Diffs
-----
src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
src/cli/execute.cpp f70d9e1c4badb7d4342e90ce4d4f8114f27a7dff
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/containerizer/mesos/isolators/docker/volume/state.proto PRE-CREATION
src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728
Diff: https://reviews.apache.org/r/45673/diff/
Testing
-------
Some issues want to discuss:
1) One container can have multiple volumes, and the prepare() will generate all of the mount point for all of the volumes in one container. Each volume driver and volume name need to map to one mount point, so in driver.cpp::mount(), if only adding volume_driver, volume_name, options as paramter, then once the mount point was generated, in prepare(), we will not able to know the relationship of volume_driver, volume_name pair and mount point. So it is better transfer a structure including volume_driver, volume_name and mount point to driver.cpp::mount() so that we can keep the relationship of volume_driver, volume_name and mount point
2) Reference counter when cleanup. My thinking is that we can use volume driver and volume name generate an volume ID, if one volume driver and volume name pair was used by more than one container, then the reference couter of the volume ID will be greater than 1 and we should not unmounts for such case, the unmount was only called when the volume ID reference count was 1.
3) I was using “multihasmap” for infos in volume isolator, the reason is because one container can have multiple volumes. If do not want to use “multihashmap”, then I may need to update Info structure as:
Info (const hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>>& _dockerVolumeMountInfo
const Option<std::string>& _containerPath = None(),
const Option<hashmap<std::string, std::string>>& _driverOptions = None())
: dockerVolumeMountInfo (_dockerVolumeMountInfo),
containerPath(_containerPath),
driverOptions(_driverOptions){}
// The driver.cpp::mount() will fill in the value of mountPoint for each DockerVolumeMount (driver and name)
hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>> dockerVolumeMountInfo;
Option<std::string> containerPath;
Option<hashmap<std::string, std::string>> driverOptions;
};
Thanks,
Guangya Liu
Re: Review Request 45673: PoC: Docker Volume Isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45673/#review129398
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 236)
<https://reviews.apache.org/r/45673/#comment192832>
move the first space to the first line.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 251)
<https://reviews.apache.org/r/45673/#comment192827>
Option<string> rootfs;
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 254 - 255)
<https://reviews.apache.org/r/45673/#comment192835>
do you mind make it:
```
VLOG(1) << "Container '" << containerId << "' "
<< "with rootfs '" << rootfs << "'";
```
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 258)
<https://reviews.apache.org/r/45673/#comment192834>
why not combine the logic above?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 264)
<https://reviews.apache.org/r/45673/#comment192836>
move the `+` above.
src/slave/containerizer/mesos/isolators/docker/volume/paths.hpp (line 32)
<https://reviews.apache.org/r/45673/#comment192840>
Please make comment consistant.
- Gilbert Song
On April 17, 2016, 9:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45673/
> -----------------------------------------------------------
>
> (Updated April 17, 2016, 9:10 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PoC: Docker Volumt Isolator.
>
> Will split this to several patches after review discussion.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/cli/execute.cpp f70d9e1c4badb7d4342e90ce4d4f8114f27a7dff
> 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/containerizer/mesos/isolators/docker/volume/state.proto PRE-CREATION
> src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728
>
> Diff: https://reviews.apache.org/r/45673/diff/
>
>
> Testing
> -------
>
> Some issues want to discuss:
> 1) One container can have multiple volumes, and the prepare() will generate all of the mount point for all of the volumes in one container. Each volume driver and volume name need to map to one mount point, so in driver.cpp::mount(), if only adding volume_driver, volume_name, options as paramter, then once the mount point was generated, in prepare(), we will not able to know the relationship of volume_driver, volume_name pair and mount point. So it is better transfer a structure including volume_driver, volume_name and mount point to driver.cpp::mount() so that we can keep the relationship of volume_driver, volume_name and mount point
> 2) Reference counter when cleanup. My thinking is that we can use volume driver and volume name generate an volume ID, if one volume driver and volume name pair was used by more than one container, then the reference couter of the volume ID will be greater than 1 and we should not unmounts for such case, the unmount was only called when the volume ID reference count was 1.
> 3) I was using “multihasmap” for infos in volume isolator, the reason is because one container can have multiple volumes. If do not want to use “multihashmap”, then I may need to update Info structure as:
>
> Info (const hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>>& _dockerVolumeMountInfo
> const Option<std::string>& _containerPath = None(),
> const Option<hashmap<std::string, std::string>>& _driverOptions = None())
> : dockerVolumeMountInfo (_dockerVolumeMountInfo),
> containerPath(_containerPath),
> driverOptions(_driverOptions){}
>
> // The driver.cpp::mount() will fill in the value of mountPoint for each DockerVolumeMount (driver and name)
> hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>> dockerVolumeMountInfo;
>
> Option<std::string> containerPath;
>
> Option<hashmap<std::string, std::string>> driverOptions;
> };
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45673: PoC: Docker Volume Isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 19, 2016, 11:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, lines 108-109
> > <https://reviews.apache.org/r/45673/diff/3/?file=1350676#file1350676line108>
> >
> > 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?
Yes, as the reference will only be used in `cleanup`, I will move the logic to `cleanup`.
> On 四月 19, 2016, 11:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, line 337
> > <https://reviews.apache.org/r/45673/diff/3/?file=1350677#file1350677line337>
> >
> > 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.
Yes, that's why I was using `mountInfo` in `DockerVolumeClient` `mount` API, as I want to keep the relationship between container path and mount point. ok, will try to see how `Sequence` can help.
> On 四月 19, 2016, 11:40 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/state.hpp, line 34
> > <https://reviews.apache.org/r/45673/diff/3/?file=1350680#file1350680line34>
> >
> > 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.
yes, I was putting it here is because I want to make this shared between `DockerVolumeClient` and `Isolator`, but if I use `Sequence`, then there will be no need to keep this but just move this to `isolator.hpp`
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45673/#review129639
-----------------------------------------------------------
On 四月 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 四月 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
>
>
Re: Review Request 45673: PoC: Docker Volume Isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 45673: PoC: Docker Volume Isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45673/
-----------------------------------------------------------
(Updated 四月 19, 2016, 4:45 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Repository: mesos
Description
-------
PoC: Docker Volume Isolator.
Diffs (updated)
-----
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 (updated)
-------
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
Re: Review Request 45673: PoC: Docker Volume Isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45673/
-----------------------------------------------------------
(Updated 四月 19, 2016, 3:28 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Repository: mesos
Description (updated)
-------
PoC: Docker Volume Isolator.
Diffs (updated)
-----
src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
src/cli/execute.cpp f70d9e1c4badb7d4342e90ce4d4f8114f27a7dff
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/containerizer/mesos/isolators/docker/volume/state.proto PRE-CREATION
src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728
Diff: https://reviews.apache.org/r/45673/diff/
Testing
-------
Some issues want to discuss:
1) One container can have multiple volumes, and the prepare() will generate all of the mount point for all of the volumes in one container. Each volume driver and volume name need to map to one mount point, so in driver.cpp::mount(), if only adding volume_driver, volume_name, options as paramter, then once the mount point was generated, in prepare(), we will not able to know the relationship of volume_driver, volume_name pair and mount point. So it is better transfer a structure including volume_driver, volume_name and mount point to driver.cpp::mount() so that we can keep the relationship of volume_driver, volume_name and mount point
2) Reference counter when cleanup. My thinking is that we can use volume driver and volume name generate an volume ID, if one volume driver and volume name pair was used by more than one container, then the reference couter of the volume ID will be greater than 1 and we should not unmounts for such case, the unmount was only called when the volume ID reference count was 1.
3) I was using “multihasmap” for infos in volume isolator, the reason is because one container can have multiple volumes. If do not want to use “multihashmap”, then I may need to update Info structure as:
Info (const hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>>& _dockerVolumeMountInfo
const Option<std::string>& _containerPath = None(),
const Option<hashmap<std::string, std::string>>& _driverOptions = None())
: dockerVolumeMountInfo (_dockerVolumeMountInfo),
containerPath(_containerPath),
driverOptions(_driverOptions){}
// The driver.cpp::mount() will fill in the value of mountPoint for each DockerVolumeMount (driver and name)
hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>> dockerVolumeMountInfo;
Option<std::string> containerPath;
Option<hashmap<std::string, std::string>> driverOptions;
};
Thanks,
Guangya Liu
Re: Review Request 45673: PoC: Docker Volume Isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45673/#review129416
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 315 - 322)
<https://reviews.apache.org/r/45673/#comment192854>
Why `xxxx`?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 350)
<https://reviews.apache.org/r/45673/#comment192863>
Where do you set the mount point?
- Gilbert Song
On April 17, 2016, 9:10 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45673/
> -----------------------------------------------------------
>
> (Updated April 17, 2016, 9:10 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PoC: Docker Volumt Isolator.
>
> Will split this to several patches after review discussion.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 06f58c4a88e3c527df727df1efe11ed3ab77efa8
> src/Makefile.am ec855263d620e4723c8ba9cd056c40a3a2e9ca99
> src/cli/execute.cpp f70d9e1c4badb7d4342e90ce4d4f8114f27a7dff
> 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/containerizer/mesos/isolators/docker/volume/state.proto PRE-CREATION
> src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728
>
> Diff: https://reviews.apache.org/r/45673/diff/
>
>
> Testing
> -------
>
> Some issues want to discuss:
> 1) One container can have multiple volumes, and the prepare() will generate all of the mount point for all of the volumes in one container. Each volume driver and volume name need to map to one mount point, so in driver.cpp::mount(), if only adding volume_driver, volume_name, options as paramter, then once the mount point was generated, in prepare(), we will not able to know the relationship of volume_driver, volume_name pair and mount point. So it is better transfer a structure including volume_driver, volume_name and mount point to driver.cpp::mount() so that we can keep the relationship of volume_driver, volume_name and mount point
> 2) Reference counter when cleanup. My thinking is that we can use volume driver and volume name generate an volume ID, if one volume driver and volume name pair was used by more than one container, then the reference couter of the volume ID will be greater than 1 and we should not unmounts for such case, the unmount was only called when the volume ID reference count was 1.
> 3) I was using “multihasmap” for infos in volume isolator, the reason is because one container can have multiple volumes. If do not want to use “multihashmap”, then I may need to update Info structure as:
>
> Info (const hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>>& _dockerVolumeMountInfo
> const Option<std::string>& _containerPath = None(),
> const Option<hashmap<std::string, std::string>>& _driverOptions = None())
> : dockerVolumeMountInfo (_dockerVolumeMountInfo),
> containerPath(_containerPath),
> driverOptions(_driverOptions){}
>
> // The driver.cpp::mount() will fill in the value of mountPoint for each DockerVolumeMount (driver and name)
> hashmap<VolumeMountId, tuple<DockerVolumeMount, Option<string> mountPoint>> dockerVolumeMountInfo;
>
> Option<std::string> containerPath;
>
> Option<hashmap<std::string, std::string>> driverOptions;
> };
>
>
> Thanks,
>
> Guangya Liu
>
>