You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2016/01/04 22:15:27 UTC
Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/
-----------------------------------------------------------
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
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/#review116595
-----------------------------------------------------------
src/docker/executor.cpp (line 136)
<https://reviews.apache.org/r/41892/#comment177639>
@jieyu, this would be a better place to implement the inferring of host paths for persistent volumes, comparing to the current draft.
- Zhitao Li
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/#review113019
-----------------------------------------------------------
src/docker/docker.cpp (lines 410 - 420)
<https://reviews.apache.org/r/41892/#comment173516>
Instead of doing that in this way, I would suggest we modify 'containerInfo' in docker containerizer.
In other words, add additional containerInfo.volumes() in DockerContainerizerProcess::Container::create().
- Jie Yu
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Jie Yu <yu...@gmail.com>.
> 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).
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.
- Jie
-----------------------------------------------------------
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Guangya Liu <gy...@gmail.com>.
> 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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Zhitao Li <zh...@gmail.com>.
> 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.
@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.
- Zhitao
-----------------------------------------------------------
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Guangya Liu <gy...@gmail.com>.
> 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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Zhitao Li <zh...@gmail.com>.
> 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.
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
-----------------------------------------------------------
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/#review114610
-----------------------------------------------------------
src/docker/docker.cpp (lines 410 - 420)
<https://reviews.apache.org/r/41892/#comment175470>
(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).
- Zhitao Li
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/#review112743
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41892]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
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
>
>
Re: Review Request 41892: DockerContinerizer infers hostPath for
persistent volumes.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41892/#review116738
-----------------------------------------------------------
src/docker/docker.cpp (lines 20 - 29)
<https://reviews.apache.org/r/41892/#comment177802>
Not yours but I think that we should also adjust the order here:
#include <process/check.hpp>
#include <process/collect.hpp>
#include <process/io.hpp>
#include <stout/lambda.hpp>
#include <stout/os.hpp>
#include <stout/result.hpp>
#include <stout/strings.hpp>
#include <stout/os/read.hpp>
src/docker/docker.cpp (line 31)
<https://reviews.apache.org/r/41892/#comment177801>
Should put this to line 20
src/docker/executor.hpp (lines 66 - 70)
<https://reviews.apache.org/r/41892/#comment177804>
The configuration.md should also be updated.
- Guangya Liu
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
>
>