> On Jan. 15, 2016, 12:03 a.m., Zhitao Li wrote:
> > src/docker/docker.cpp, lines 410-420
> > <https://reviews.apache.org/r/41892/diff/1/?file=1181052#file1181052line410>
> >
> >     (Sorry I just got time to come back to this).
> >     
> >     I don't exactly understand your suggestion about "add additional containerInfo.volumes() in DockerContainerizerProcess::Container::create()", because the latter function actually does not pass a `ContainerInfo`.
> >     
> >     Because the volume is actually included in `TaskInfo::ContainerInfo::volumes`, we would need to mutate the `TaskInfo` before passing it to the `DockerExecutorProcess::launchTask()` function, which I don't really know how to do it, and I even doubt whether it's a good idea for logging/clarity purpose.
> >     
> >     Please let me know if I didn't understand your suggestion, or if you think we should explore the other alternative (passing `hostPath` earlier in resource offer).
> 
> Jie Yu wrote:
>     Sorry for being late on this.
>     
>     My sugguestion is that we add some logics in DockerContainerProcess::Container::create(), the 'create' function has both TaskINfo and ExecutorInfo. So, you should be able to get the ContainerInfo from them. When you generate DockerContainerizerProcess::Container, you should be able to generate a new ContainerInfo that has persistent volumes in it. Let me know if you still don't understand.
>     
>     FYI, docker just merged its mount propagation support for volumes. That means we can have full persistent volume support. We need to mount the sandbox as a shared mount into the docker container. ANy mounts under the sandbox mount will be automatically propergated to the docker container.
> 
> Zhitao Li wrote:
>     @jieyu, I tried your recommendation and managed to generate a different `ContainerInfo` object inside `DockerContainerProcess::Container::create()`. However, I don't think it could work either because the actual docker container is created by the call sequence `DockerExecutor::launchTask()` -> `Docker::run()` in the forked executor subprocess, and it seems impossible to properly pass the modified `ContainerInfo` to that.
>     
>     My proposal is:
>     - 1) keep the `persistent_volumes_root` flag to the executor;
>     - 2) implement this logic inside `DockerExecutor::launchTask()` (see my other inline comment for exact location) so we don't need to change `Docker` class;
>     - 3) for testing, create a test case in docker_containerizer_tests.cpp which creates persistent volume and launch an executor.
>     
>     I'll rebase and update this diff with the above proposal.

The `DockerContainerProcess::Container::create()` only include `TaskInfo` when using a `command executor`, otherwise, the `DockerContainerProcess::Container::create()` will not include task info. For this case it is not `command executor`, so I think that @Zhitao's solution should be the right choice. Please correct me if there's sth wrong, thanks.


- Guangya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/#review114610
-----------------------------------------------------------


On Jan. 4, 2016, 9:15 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41892/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2016, 9:15 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Jie Yu.
> 
> 
> Bugs: MESOS-3413
>     https://issues.apache.org/jira/browse/MESOS-3413
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This diff makes DockerContainerizer infers hostPath for persistent volumes from resources passed-in.
> 
> This allows current DockerContainerizer to use persistent volumes w/o hard code slave's work_dir. I also checked that the inferred directory actually exists to avoid framework messing up role or persistent id.
> 
> Note that some validation on the resource part should be done before we start the container, which seems to belong to slave.
> 
> 
> Diffs
> -----
> 
>   src/docker/docker.hpp dde2b29deda7e40929e0169935a5bafdd43136b1 
>   src/docker/docker.cpp e93280735f1c5f66c765fcbabfd3e50b46c024e8 
>   src/docker/executor.hpp abbc419533ab40312e917931a2fc2ce78b38da41 
>   src/docker/executor.cpp 7512d07de6c8324340e6b5f3e5162ef00efc47fc 
>   src/slave/containerizer/docker.cpp aacf90f2cb6c08f94340936d29b2df513ac9825a 
>   src/tests/containerizer/docker_containerizer_tests.cpp cb58b7183c36d96b9ac4803c63980c278a50c97b 
>   src/tests/containerizer/docker_tests.cpp 83eceacaddc38d0ccfc42e65e700a09406d8df36 
>   src/tests/mesos.hpp a4811b0d8dee33ff2ca4968f532ce64b7ea95249 
>   src/tests/mesos.cpp f4b0f82449c4b6a2b4b7b7f14518714485d5a13a 
> 
> Diff: https://reviews.apache.org/r/41892/diff/
> 
> 
> Testing
> -------
> 
> New unit test.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>