You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/07/15 20:04:28 UTC

Review Request 23513: Implemented launching executors in DockerContainerizer.

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

Review request for mesos, Benjamin Hindman and Yifan Gu.


Bugs: MESOS-1524
    https://issues.apache.org/jira/browse/MESOS-1524


Repository: mesos-git


Description
-------

This patch allows launching executors in Docker image.

Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.


Diffs
-----

  src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
  src/docker/docker.hpp PRE-CREATION 
  src/docker/docker.cpp PRE-CREATION 
  src/slave/containerizer/docker.cpp PRE-CREATION 
  src/tests/docker_containerizer_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
  src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
  src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/23513/diff/


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by "Timothy St. Clair" <ts...@redhat.com>.

> On July 21, 2014, 1:49 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 150
> > <https://reviews.apache.org/r/23513/diff/1/?file=632624#file632624line150>
> >
> >     I wouldn't default the --net=host, this could have a lot of unintended consequences.
> 
> Timothy Chen wrote:
>     I thought about this as well, but for now it is required to have this flag for launching docker image that is an executor.
>     
>     Technically we can pass another argument in docker run to set this but I wanted to avoid this as this can be one of the options in DockerInfo and change this once we finalize that API.
>     
>     Let me know if you have more thoughts on this or anything else!

"but for now it is required to have this flag for launching docker image that is an executor" - Why?  


- Timothy


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


On July 17, 2014, 6:21 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 6:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by Timothy Chen <tn...@apache.org>.

> On July 21, 2014, 1:49 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 150
> > <https://reviews.apache.org/r/23513/diff/1/?file=632624#file632624line150>
> >
> >     I wouldn't default the --net=host, this could have a lot of unintended consequences.

I thought about this as well, but for now it is required to have this flag for launching docker image that is an executor.

Technically we can pass another argument in docker run to set this but I wanted to avoid this as this can be one of the options in DockerInfo and change this once we finalize that API.

Let me know if you have more thoughts on this or anything else!


- Timothy


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


On July 17, 2014, 6:21 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 6:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by Timothy Chen <tn...@apache.org>.

> On July 21, 2014, 1:49 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 150
> > <https://reviews.apache.org/r/23513/diff/1/?file=632624#file632624line150>
> >
> >     I wouldn't default the --net=host, this could have a lot of unintended consequences.
> 
> Timothy Chen wrote:
>     I thought about this as well, but for now it is required to have this flag for launching docker image that is an executor.
>     
>     Technically we can pass another argument in docker run to set this but I wanted to avoid this as this can be one of the options in DockerInfo and change this once we finalize that API.
>     
>     Let me know if you have more thoughts on this or anything else!
> 
> Timothy St. Clair wrote:
>     "but for now it is required to have this flag for launching docker image that is an executor" - Why?

The executor needs to communicate to the slave and the current driver implementation requires to have the slave PID (IP and port) configured as an environment variable when you launch the executor.

Without host networking there are quite some work to get this to work with Docker and Mesos, as you can imagine it's more than just port mapping but also somehow getting the docker ip and allow us to set it after the docker image already started, etc. Ben and I thought for now let's just use host networking and continue this discussion.


- Timothy


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


On July 17, 2014, 6:21 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 6:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by "Timothy St. Clair" <ts...@redhat.com>.

> On July 21, 2014, 1:49 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 150
> > <https://reviews.apache.org/r/23513/diff/1/?file=632624#file632624line150>
> >
> >     I wouldn't default the --net=host, this could have a lot of unintended consequences.
> 
> Timothy Chen wrote:
>     I thought about this as well, but for now it is required to have this flag for launching docker image that is an executor.
>     
>     Technically we can pass another argument in docker run to set this but I wanted to avoid this as this can be one of the options in DockerInfo and change this once we finalize that API.
>     
>     Let me know if you have more thoughts on this or anything else!
> 
> Timothy St. Clair wrote:
>     "but for now it is required to have this flag for launching docker image that is an executor" - Why?
> 
> Timothy Chen wrote:
>     The executor needs to communicate to the slave and the current driver implementation requires to have the slave PID (IP and port) configured as an environment variable when you launch the executor.
>     
>     Without host networking there are quite some work to get this to work with Docker and Mesos, as you can imagine it's more than just port mapping but also somehow getting the docker ip and allow us to set it after the docker image already started, etc. Ben and I thought for now let's just use host networking and continue this discussion.

We should open a JIRA around this.  


- Timothy


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


On July 17, 2014, 6:21 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 6:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by Timothy Chen <tn...@apache.org>.

> On July 21, 2014, 1:49 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 150
> > <https://reviews.apache.org/r/23513/diff/1/?file=632624#file632624line150>
> >
> >     I wouldn't default the --net=host, this could have a lot of unintended consequences.
> 
> Timothy Chen wrote:
>     I thought about this as well, but for now it is required to have this flag for launching docker image that is an executor.
>     
>     Technically we can pass another argument in docker run to set this but I wanted to avoid this as this can be one of the options in DockerInfo and change this once we finalize that API.
>     
>     Let me know if you have more thoughts on this or anything else!
> 
> Timothy St. Clair wrote:
>     "but for now it is required to have this flag for launching docker image that is an executor" - Why?
> 
> Timothy Chen wrote:
>     The executor needs to communicate to the slave and the current driver implementation requires to have the slave PID (IP and port) configured as an environment variable when you launch the executor.
>     
>     Without host networking there are quite some work to get this to work with Docker and Mesos, as you can imagine it's more than just port mapping but also somehow getting the docker ip and allow us to set it after the docker image already started, etc. Ben and I thought for now let's just use host networking and continue this discussion.
> 
> Timothy St. Clair wrote:
>     We should open a JIRA around this.

Sounds good, I created MESOS-1621 for this.


- Timothy


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


On July 17, 2014, 6:21 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 6:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23513/#review48215
-----------------------------------------------------------



src/docker/docker.cpp
<https://reviews.apache.org/r/23513/#comment84568>

    I wouldn't default the --net=host, this could have a lot of unintended consequences. 


- Timothy St. Clair


On July 17, 2014, 6:21 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 17, 2014, 6:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23513/
-----------------------------------------------------------

(Updated July 17, 2014, 6:21 p.m.)


Review request for mesos, Benjamin Hindman and Yifan Gu.


Bugs: MESOS-1524
    https://issues.apache.org/jira/browse/MESOS-1524


Repository: mesos-git


Description
-------

This patch allows launching executors in Docker image.

Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.


Diffs
-----

  src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
  src/docker/docker.hpp PRE-CREATION 
  src/docker/docker.cpp PRE-CREATION 
  src/slave/containerizer/docker.cpp PRE-CREATION 
  src/tests/docker_containerizer_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
  src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
  src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 

Diff: https://reviews.apache.org/r/23513/diff/


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 23513: Implemented launching executors in DockerContainerizer.

Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23513/#review47911
-----------------------------------------------------------


Bad patch!

Reviews applied: [23513]

Failed command: git apply --index 23513.patch

Error:
 23513.patch:583: trailing whitespace.
	Try<process::Subprocess> install = 
error: src/docker/docker.hpp: does not exist in index
error: src/docker/docker.cpp: does not exist in index
error: src/slave/containerizer/docker.cpp: does not exist in index
error: src/tests/docker_containerizer_tests.cpp: does not exist in index
error: patch failed: src/tests/environment.cpp:141
error: src/tests/environment.cpp: patch does not apply


- Mesos ReviewBot


On July 15, 2014, 6:04 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23513/
> -----------------------------------------------------------
> 
> (Updated July 15, 2014, 6:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Yifan Gu.
> 
> 
> Bugs: MESOS-1524
>     https://issues.apache.org/jira/browse/MESOS-1524
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This patch allows launching executors in Docker image.
> 
> Also added a test Docker image that embeds a test executor and automatically installs if it's not found in local Docker.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a1fe9dc2173d8e23398edfce6ef7dc458a3c81bb 
>   src/docker/docker.hpp PRE-CREATION 
>   src/docker/docker.cpp PRE-CREATION 
>   src/slave/containerizer/docker.cpp PRE-CREATION 
>   src/tests/docker_containerizer_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 434b3f7f890ad05c90c88be796996828e0ac4876 
>   src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION 
>   src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/23513/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>