> 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.
> 
> Guangya Liu wrote:
>     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.
> 
> Zhitao Li wrote:
>     Hi Guangya,
>     
>     I had some offline chat with Jie about this issue and agreed that this 
> should instead be fixed with a more involved solution on a different 
> direction (mounting persistent volume inside sandbox as a "shared volume"). 
> The bug has been reassigned to Timothy Chen who's implementing on that.
>     
>     I'll abandon this review for now.

@Zhitao, can you please append some of your dicussion here? Want to get some 
detail for this, 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
> 
>

Reply via email to