----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42278/#review114820 -----------------------------------------------------------
Fix it, then Ship it! Sorry for being late on this review. Overall looks good to me. Some style nits. src/launcher/executor.cpp (lines 236 - 244) <https://reviews.apache.org/r/42278/#comment175668> Can you add some comments here explaining why we need to do MS_REC bind mount and why we need to do the unmountAll? src/slave/slave.cpp (line 3491) <https://reviews.apache.org/r/42278/#comment176513> Please use path::absolute here. src/launcher/executor.cpp (lines 236 - 237) <https://reviews.apache.org/r/42278/#comment178270> Can you combine this with umountAll below: ``` Try<Nothing> umountAll = fs::unmountAll(path:join( sandbox, COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH)); ``` Also, add some comments about why we need to do the umount here. It'll be very valuable for readers as the logics here is pretty subtle. src/slave/slave.cpp (line 3558) <https://reviews.apache.org/r/42278/#comment178271> container->volumes_size() src/slave/slave.cpp (lines 3561 - 3564) <https://reviews.apache.org/r/42278/#comment178272> ``` volume->set_container_path(path::join( COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH, volume->container_path())); ``` src/tests/containerizer/filesystem_isolator_tests.cpp (line 376) <https://reviews.apache.org/r/42278/#comment178281> I would also mention Command Executor in the test name. src/tests/containerizer/filesystem_isolator_tests.cpp (line 420) <https://reviews.apache.org/r/42278/#comment178279> It's better to add a comment here explaining the mapping: ``` host_path: dir1, container_path: /tmp host_path: dir2, container_path: relative_dir ``` src/tests/containerizer/filesystem_isolator_tests.cpp (line 422) <https://reviews.apache.org/r/42278/#comment178274> Nits, I would introduce a blank line above this line here. src/tests/containerizer/filesystem_isolator_tests.cpp (line 424) <https://reviews.apache.org/r/42278/#comment178273> Nits, I would introduce a blank line above this line here. src/tests/containerizer/filesystem_isolator_tests.cpp (lines 438 - 443) <https://reviews.apache.org/r/42278/#comment178277> I would use createVolumeFromHostPath here. src/tests/containerizer/filesystem_isolator_tests.cpp (line 439) <https://reviews.apache.org/r/42278/#comment178276> Add a blank line above this. src/tests/containerizer/filesystem_isolator_tests.cpp (lines 444 - 447) <https://reviews.apache.org/r/42278/#comment178278> Ditto. src/tests/containerizer/filesystem_isolator_tests.cpp (line 473) <https://reviews.apache.org/r/42278/#comment178282> I would also mention Command Executor in test name. src/tests/containerizer/filesystem_isolator_tests.cpp (lines 547 - 555) <https://reviews.apache.org/r/42278/#comment178283> Why two offers? Can you perform CREATE and LAUNCH in the same acceptOffer cycle? src/tests/containerizer/filesystem_isolator_tests.cpp (lines 564 - 567) <https://reviews.apache.org/r/42278/#comment178284> Ditto on using createVolumeFromHostPath src/tests/containerizer/filesystem_isolator_tests.cpp (line 580) <https://reviews.apache.org/r/42278/#comment178285> Remove this? - Jie Yu On Jan. 26, 2016, 6:37 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42278/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2016, 6:37 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese. > > > Bugs: MESOS-4285 > https://issues.apache.org/jira/browse/MESOS-4285 > > > Repository: mesos > > > Description > ------- > > Fixed volume paths for command tasks with image. > > > Diffs > ----- > > src/launcher/executor.cpp 356d311fdf97b2c4663c60e13ede7cdb71a264c7 > src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f > src/tests/containerizer/filesystem_isolator_tests.cpp > 496275a73601664b51155ef1373d8d46b9069613 > > Diff: https://reviews.apache.org/r/42278/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
