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