----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43015/#review119250 -----------------------------------------------------------
src/slave/containerizer/docker.cpp (line 396) <https://reviews.apache.org/r/43015/#comment180541> Looks like we use 'directory' consistently in this file for container working directory. So please s/sandboxDirectory/directory/ src/slave/containerizer/docker.cpp (line 397) <https://reviews.apache.org/r/43015/#comment180542> s/currentResources/current/ src/slave/containerizer/docker.cpp (line 398) <https://reviews.apache.org/r/43015/#comment180543> s/newResources/updated/ src/slave/containerizer/docker.cpp (lines 420 - 424) <https://reviews.apache.org/r/43015/#comment180547> You may want to make sure `slave.work_dir` is a shared mount in its own peer group (see code in `LinuxFilesystemIsolatorProcess::create` and also ticket https://issues.apache.org/jira/browse/MESOS-3483). The reason we need to do that is because when we fork an executor process (in Mesos containerizer) with a new mount namespace, if the mount is a private mount, the mount in the new mount namespace will be a private mount as well, holding an extra reference to the mount. That mount can be for other containers. When the slave remove the mount followed by an rmdir (removing the mount point), the rmdir could fail due to the extra references to the mount. Although we did some best effort umount in the new mount namespace (trying to umount any irrelevant mounts), but race still exists. For instance, while we're umounting, the slave tries to umount+rmdir for some mounts. src/slave/containerizer/docker.cpp (lines 512 - 513) <https://reviews.apache.org/r/43015/#comment180538> Returning a failure doesn't match the existing semantics. Could you instead use a LOG(ERROR) here? src/slave/containerizer/docker.cpp (line 518) <https://reviews.apache.org/r/43015/#comment180540> Hum, can you make 'updatePersistentVolumes' a member function so that you have access to 'flags' and do not need to pass in this parameter. src/slave/containerizer/docker.cpp (line 522) <https://reviews.apache.org/r/43015/#comment180539> Insert a blank line above. src/slave/containerizer/docker.cpp (lines 1281 - 1289) <https://reviews.apache.org/r/43015/#comment180544> This is useless right now, isn't it? Can you remove it and instead, drop a TODO here. To fully support updating volumes, we need to have docker's mount propagation support. src/slave/containerizer/docker.cpp (lines 1707 - 1719) <https://reviews.apache.org/r/43015/#comment180545> Do you need this? Looks like 'unmountPersistentVolumes' will take care of the umount. - Jie Yu On Feb. 15, 2016, 3:39 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43015/ > ----------------------------------------------------------- > > (Updated Feb. 15, 2016, 3:39 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-3413 > https://issues.apache.org/jira/browse/MESOS-3413 > > > Repository: mesos > > > Description > ------- > > Fixed persistent volumes with docker tasks. > > > Diffs > ----- > > src/slave/containerizer/docker.hpp 77a50d80179672cf3c270cbdd7fa003c7d9ad6f3 > src/slave/containerizer/docker.cpp ed1c9a551f03a37d572470e4c495f5df834198cc > src/tests/containerizer/docker_containerizer_tests.cpp > 645bdcf095145097d8b8c65d592c787417883145 > > Diff: https://reviews.apache.org/r/43015/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
