----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46140/#review131457 -----------------------------------------------------------
src/tests/containerizer/docker_volume_isolator_tests.cpp (line 112) <https://reviews.apache.org/r/46140/#comment195447> `sandbox->c_str()` Can you also log an ERROR `LOG(ERROR)` if unmountAll returns an Error? src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 132 - 133) <https://reviews.apache.org/r/46140/#comment195448> This format looks weird to me. I think you can save a temporary 'Volume::Source' above: ``` Volume volume volume.set_mode(Volume::RW); volume.set_container_path(...); Volume::Source* source = volume.mutable_source(); source->set_type(Volume::Source::DOCKER_VOLUME); Volume::Source::DockerVolume* docker = source->mutable_docker_volume(); docker->set_driver(driver); docker->set_name(name); if (parameters.isSome()) { ... } ``` ALso, i think we should make 'parameters' of this helper to be a hashmap. It's easier for the callsite. src/tests/containerizer/docker_volume_isolator_tests.cpp (line 143) <https://reviews.apache.org/r/46140/#comment195449> Can you pass in an Owned<DriverClient>& instead? src/tests/containerizer/docker_volume_isolator_tests.cpp (line 164) <https://reviews.apache.org/r/46140/#comment195450> Kill this line. src/tests/containerizer/docker_volume_isolator_tests.cpp (line 204) <https://reviews.apache.org/r/46140/#comment195453> `s/ROOT_LaunchCommandExecutorNoRootfsWithSingleVolume/ROOT_CommandTaskNoRootfs/` src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 214 - 227) <https://reviews.apache.org/r/46140/#comment195452> I think you should move this down right before calling `launchTasks`. Also, please capture the parameter to the 'mount' and 'unmount' here to verify that it's the same as specified in ContainerInfo. src/tests/containerizer/docker_volume_isolator_tests.cpp (lines 214 - 215) <https://reviews.apache.org/r/46140/#comment195455> Is it used? src/tests/containerizer/docker_volume_isolator_tests.cpp (line 223) <https://reviews.apache.org/r/46140/#comment195454> is it used? src/tests/containerizer/docker_volume_isolator_tests.cpp (line 267) <https://reviews.apache.org/r/46140/#comment195451> What do you test here? I think you should pre-create some file in that volume and do `test -f ...` in the command. - Jie Yu On May 3, 2016, 5:12 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46140/ > ----------------------------------------------------------- > > (Updated May 3, 2016, 5:12 a.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-5266 > https://issues.apache.org/jira/browse/MESOS-5266 > > > Repository: mesos > > > Description > ------- > > Added test "ROOT_LaunchCommandExecutorNoRootfsWithSingleVolume". > > > Diffs > ----- > > src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 > 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 > >
