----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43015/#review119340 -----------------------------------------------------------
src/slave/containerizer/docker.cpp (lines 161 - 162) <https://reviews.apache.org/r/43015/#comment180653> Can you wrap using 70 char width here? src/slave/containerizer/docker.cpp (lines 413 - 414) <https://reviews.apache.org/r/43015/#comment180687> Should we ignore those volumes that have absolute/nested container path (like we did in linux fs isolator). What's the reason removing it? src/slave/containerizer/docker.cpp (line 419) <https://reviews.apache.org/r/43015/#comment180662> Indentation. src/slave/containerizer/docker.cpp (line 442) <https://reviews.apache.org/r/43015/#comment180688> indentation src/slave/containerizer/docker.cpp (lines 457 - 458) <https://reviews.apache.org/r/43015/#comment180690> Ditto. Why remove the container_path check? src/slave/containerizer/docker.cpp (lines 473 - 488) <https://reviews.apache.org/r/43015/#comment180699> There's a os::exists check in linux fs isolator which you removed. This is fine because we won't update volumes when containerizer->update is called. I think we should at least drop a TODO here so that we don't forget that when we want to support containerizer->update. Alternatively, we can kill updatePersistentVolumes and inline it into mountPersistentVolumes. src/slave/containerizer/docker.cpp (line 881) <https://reviews.apache.org/r/43015/#comment180700> Can you add a TODO stating that this is a hack for now. We need to revisit if docker containers have other mounts that may container 'containerId' src/slave/containerizer/docker.cpp (lines 918 - 921) <https://reviews.apache.org/r/43015/#comment180703> Indentation. ``` futures.push_back( docker->stop( container.id, flags.docker_stop_timeout, true) .then([id]() { return id.get(); })); ``` src/slave/containerizer/docker.cpp (lines 1701 - 1708) <https://reviews.apache.org/r/43015/#comment180694> Why do you need to do this? Looks like we don't mount until pulling is done. Am I missing something? src/slave/containerizer/docker.cpp (lines 1785 - 1790) <https://reviews.apache.org/r/43015/#comment180704> Hum, I think we shouldn't remove the volumes if the container might be still running. So can we move the cleanup logics to `___destroy()`? src/tests/containerizer/docker_containerizer_tests.cpp (lines 1178 - 1217) <https://reviews.apache.org/r/43015/#comment180721> ok, can you instead, launch a slave to do that. This is pretty hacky to me. You can split this test into two: 1) for running containers 2) for orphan containers. Take a look at SlaveRecoveryTest see how to simulate slave failover. src/tests/containerizer/docker_containerizer_tests.cpp (lines 3217 - 3221) <https://reviews.apache.org/r/43015/#comment180708> You can do CREATE+LAUNCH in the same offer cycle, right? src/tests/containerizer/docker_containerizer_tests.cpp (line 3236) <https://reviews.apache.org/r/43015/#comment180713> I might want to write some data to the persistent volume and make sure that file exists in the end. See AccessPersistentVolume in PersistentVolumeTest. src/tests/containerizer/docker_containerizer_tests.cpp (line 3268) <https://reviews.apache.org/r/43015/#comment180715> insert a blank line above. src/tests/containerizer/docker_containerizer_tests.cpp (line 3270) <https://reviews.apache.org/r/43015/#comment180714> insert a blank line above. src/tests/containerizer/docker_containerizer_tests.cpp (line 3285) <https://reviews.apache.org/r/43015/#comment180712> Does this still work? Looks like you're not doing rmdir anymore. Am i missing something? - Jie Yu On Feb. 16, 2016, 3:33 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43015/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2016, 3:33 a.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 > >
