----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46140/#review131378 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp (line 72) <https://reviews.apache.org/r/46140/#comment195316> I don't think you need this change. See my comments below. src/tests/containerizer/docker_volume_isolator_tests.cpp (line 91) <https://reviews.apache.org/r/46140/#comment195314> I would call it: ``` MockDockerVolumeDriverClient ``` Let's make it a pure Mock without a concrete implementation. You can setup Mock expectation in the following tests. src/tests/containerizer/docker_volume_isolator_tests.cpp (line 94) <https://reviews.apache.org/r/46140/#comment195315> Why do you need the additional `_dvdcli` parameter? Is it used? src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 113 - 124) <https://reviews.apache.org/r/46140/#comment195330> This is not needed. Let's make it a pure mock object. src/tests/containerizer/docker_volume_isolator_tests.cpp (line 119) <https://reviews.apache.org/r/46140/#comment195319> No need to do os::exists check here. mkdir will do that anyway. src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 129 - 133) <https://reviews.apache.org/r/46140/#comment195318> Please put MOCKed methods together above 'unmocked' methods. src/tests/containerizer/docker_volume_isolator_tests.cpp (line 149) <https://reviews.apache.org/r/46140/#comment195320> 2 lines apart PLEASE! src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 157 - 166) <https://reviews.apache.org/r/46140/#comment195328> You can try to use `fs::unmountAll` here. src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 172 - 181) <https://reviews.apache.org/r/46140/#comment195321> Why copy the code here? I don't think we need a 'rootfs' to test some basic functionalities. Let's not test the rootfs path yet. src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 269 - 271) <https://reviews.apache.org/r/46140/#comment195322> Hum, please do not blindly copy the comments. Does it apply here?? src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 275 - 279) <https://reviews.apache.org/r/46140/#comment195327> I would suggest that we just create a master, a slave with docker/volume and filesystem/linux isolator turned on. Then, launch a task that uses Volume.Source.DockerVolume and set proper EXPECTATION on the mocked object. See if the task can access the volume by writing some files to the 'container_path'. src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 284 - 286) <https://reviews.apache.org/r/46140/#comment195324> I don't think image is needed for some simple test. - Jie Yu On April 27, 2016, 4:28 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46140/ > ----------------------------------------------------------- > > (Updated April 27, 2016, 4:28 p.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-5266 > https://issues.apache.org/jira/browse/MESOS-5266 > > > Repository: mesos > > > Description > ------- > > WIP: Added dvd isolator test "ROOT_LaunchCommandExecutorWithVolumes". > > > Diffs > ----- > > src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca > src/slave/containerizer/mesos/isolators/docker/volume/driver.hpp > 070d52018e82ed3e46fb1b79714ffc4716f6a306 > src/tests/containerizer/docker_volume_isolator_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/46140/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Guangya Liu > >
