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/22 20:12:55 UTC
Re: Review Request 23771: Added a Docker containerizer.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 22, 2014, 6:12 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
Added a bit more detail to the 'Summary'.
Summary (updated)
-----------------
Added a Docker containerizer.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/docker/docker.cpp, line 149
> > <https://reviews.apache.org/r/23771/diff/1/?file=637643#file637643line149>
> >
> > Any specific reason why you use --net=host here? Should this be exposed so bridge can be used too?
This will be addressed in the future, left a TODO
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/docker/docker.cpp, line 319
> > <https://reviews.apache.org/r/23771/diff/1/?file=637643#file637643line319>
> >
> > If it doesn't exist then docker inspect should exit 1 so it won't even reach here.
Fixed the comment.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/docker/docker.hpp, lines 105-112
> > <https://reviews.apache.org/r/23771/diff/1/?file=637642#file637642line105>
> >
> > Can you make this into a libprocess object and then defer to these non-static methods? The lifecycle of the object you pass in the lambda::binds is not clear. Any you can make delayed calls later if necessary.
I chatted with Ben and we think we'll punt on this for now.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/slave/containerizer/docker.hpp, line 50
> > <https://reviews.apache.org/r/23771/diff/2/?file=638778#file638778line50>
> >
> > This can be private.
We actually want to maintain it open so we can inherit this class in tests. I can leave a comment.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/linux/cgroups.cpp, line 1902
> > <https://reviews.apache.org/r/23771/diff/1/?file=637648#file637648line1902>
> >
> > Don't you mean memory?
I believe you're looking at a old revision? Ben's last commit changed this already.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/docker/docker.cpp, lines 141-147
> > <https://reviews.apache.org/r/23771/diff/1/?file=637643#file637643line141>
> >
> > Instead of escaping character(s) you should use the execve-style version of subprocess which takes a vector<string> for argv.
I just tried to do this change and I think it ends up to be as troublesome, so I'm going to skip passing in argv.
The main reason is that we're taking a arbitrary command string for Docker, which the string itself has a number of args but it's all contained in one string.
To pass in a vector<string> I need to split the command string manually with spaces, which then I need to handle quotes and spaces, which is more complicated than this.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/docker/docker.cpp, line 134
> > <https://reviews.apache.org/r/23771/diff/1/?file=637643#file637643line134>
> >
> > You could clean this up a little with the min/max using Option(s) provided by stout, e.g.:
> >
> > template <typename T>
> > Option<T> max(const T& left, const Option<T>& right)
I don't think it helps much actually, since I need to only get max when memory/cpu is defined and using stout max I will always return a defined value since the right hand side is a constant
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 11:26 p.m., Ian Downes wrote:
> > src/docker/docker.hpp, line 36
> > <https://reviews.apache.org/r/23771/diff/1/?file=637642#file637642line36>
> >
> > Is there any check on compatibility with the Docker version?
> >
> > What about having a generic Docker abstraction with DockerCLI as one implementation?
There isn't, we plan to do that once we have another implementation but will punt on this.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48287
-----------------------------------------------------------
This is the first half of the review from Jie and Ian. We will continue tomorrow.
src/common/status_utils.hpp
<https://reviews.apache.org/r/23771/#comment85012>
Fix include <>
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84732>
Is there any check on compatibility with the Docker version?
What about having a generic Docker abstraction with DockerCLI as one implementation?
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85014>
Better if this was:
Try<Docker*> DockerCLI::create(const string& path)
Try<Docker*> DockerREST::create(const string& sockFile)
Because you've created the instance and then later you validate it. The create function can do whatever validation is necessary.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85015>
For consistency the argument should be _json.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84727>
s/Pid/pid/
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85016>
Make private once using ::create.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85017>
Remove const for the general Docker interface (and for the DockerCLI too). Other implementors may want to change the object.
Return the Subprocess status is only really for the DockerCLI and it isn't actually used. Better to return a Future<bool> or some Docker specific error type. The error structure could have the error code and error string.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85018>
Ditto remove const for this and other methods.
Ditto for return value of other methods. Check with the REST API and return something relevant to both CLI and REST.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84781>
no need for const on basic types.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85021>
s/Depreciate/Deprecate/
s/the//
If you want to support "rm --kill" then just support this as a flag to kill now and clean up comments.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84782>
ditto
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84783>
separate with newlines.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85032>
Can you make this into a libprocess object and then defer to these non-static methods? The lifecycle of the object you pass in the lambda::binds is not clear. Any you can make delayed calls later if necessary.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84730>
kill newline and re-order.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84731>
s/Docker &docker/Docker& docker/
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84758>
Why is this necessary. Shouldn't we let Docker decide if it can run containers?
All validation should be moved into the Docker::create function.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85038>
Docker::info can be moved into Docker::create since it's only used here.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85039>
Can the json parsing be done in a single place, say into a Try<Docker::Container> Docker::Container::create(json) rather than parsing every time.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84761>
Should these actually be CHECKs - can you not make these Try<>s and return Errors?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84777>
consistency, newline before return?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84778>
Two lines between functions.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84779>
ditto
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84733>
s/yifan)/yifan):/
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84776>
ditto, newline.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84785>
ditto
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84768>
kill newline
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84770>
why the leading whitespace here? what about starting with path + " run -d" here and appending the arguments?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84772>
You could clean this up a little with the min/max using Option(s) provided by stout, e.g.:
template <typename T>
Option<T> max(const T& left, const Option<T>& right)
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85044>
Instead of escaping character(s) you should use the execve-style version of subprocess which takes a vector<string> for argv.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85042>
Any specific reason why you use --net=host here? Should this be exposed so bridge can be used too?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84773>
Why are you using pipes for std* here? You don't read/write to them and there's the possibility of blocking the subprocess if you fill a buffer. If they aren't needed then redirect to /dev/null.
Subprocess::PATH("/dev/null")
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84775>
ditto newline.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84774>
ditto.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85046>
ditto
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85047>
This should be moved into kill and enabled with optional flag.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84786>
Do you want to log this or is it not unexpected?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84780>
newline
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84787>
Ditto, does anyone do anything with these pipes?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84798>
Can this replace the existing stout os::read()? It should go into stout regardless.
If you're reading asynchronously using io::read(fd) then you don't need this function.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84799>
This should definitely be done, otherwise the subprocess could be blocked!
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85050>
kill newline
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85051>
If it doesn't exist then docker inspect should exit 1 so it won't even reach here.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85052>
Redundant because this is from .then()
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85054>
This seems very fragile.... Perhaps add a comment warning about this!
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85055>
This should get moved to Docker::create()
src/linux/cgroups.hpp
<https://reviews.apache.org/r/23771/#comment84725>
Every pid is at least a member of the root cgroup for any attached subsystem - drop the last part of this sentence.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/23771/#comment84724>
ditto
src/linux/cgroups.cpp
<https://reviews.apache.org/r/23771/#comment84722>
Default constructor is none.
src/linux/cgroups.cpp
<https://reviews.apache.org/r/23771/#comment84691>
Don't you mean memory?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/23771/#comment84692>
Ditto - you should be finding the memory cgroup.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment84800>
Clean up parts of the commenting that don't apply now, e.g., this code doesn't use the launcher.
src/slave/flags.hpp
<https://reviews.apache.org/r/23771/#comment84726>
I assume that this must be the absolute path? is $PATH searched? Can I specify a relative path...?
src/linux/cgroups.hpp
<https://reviews.apache.org/r/23771/#comment85060>
Add test for this and other new cgroups functions.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/23771/#comment85059>
s/subsytem/subsystem/
src/linux/cgroups.hpp
<https://reviews.apache.org/r/23771/#comment85062>
if the subsystem is mounted, every pid is in either the root cgroup or another cgroup.
src/linux/cgroups.hpp
<https://reviews.apache.org/r/23771/#comment85061>
s/subsytem/subsystem/
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/23771/#comment85064>
Fits on a single line?
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/23771/#comment85065>
This can be private.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85066>
Consistency, name arguments _flags and _docker.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85068>
kill extra newline
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85069>
no need for const reference
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85070>
ditto
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85071>
consistency, just call variable validate
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85072>
pull this out of the ifdef so the limits are always known?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85073>
write a helper function which returns the docker container name.
- Ian Downes
On July 22, 2014, 11:12 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 11:12 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.hpp, line 74
> > <https://reviews.apache.org/r/23771/diff/2/?file=638769#file638769line74>
> >
> > Graceful termination? How does signal escalation get handled?
This is just an abstraction for Docker kill, in the containerizer we actually do a rm and kill if not removed.
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.hpp, line 37
> > <https://reviews.apache.org/r/23771/diff/2/?file=638769#file638769line37>
> >
> > So imho we should try to match parity with the API where possible, to provide an abstraction which is consistent.
> >
> > https://docs.docker.com/reference/api/docker_remote_api_v1.13/
> >
> > IMHO - It looks more like a cli wrap vs. docker API abstraction.
> >
> > Perhaps we could open another JIRA after this?
Besides the naming of POST/GET containers, I think at least the actions are an interface that we swap out. If we move to the REST api, we are performing the same actions but just callign the REST apis.
Ideally we want to use libcontainer though, which if we model after the REST api it's going to look a lot of different in the end IMO.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48402
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.hpp, line 74
> > <https://reviews.apache.org/r/23771/diff/2/?file=638769#file638769line74>
> >
> > Graceful termination? How does signal escalation get handled?
>
> Timothy Chen wrote:
> This is just an abstraction for Docker kill, in the containerizer we actually do a rm and kill if not removed.
Docker kill via api enables signal to be passed.
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.hpp, line 37
> > <https://reviews.apache.org/r/23771/diff/2/?file=638769#file638769line37>
> >
> > So imho we should try to match parity with the API where possible, to provide an abstraction which is consistent.
> >
> > https://docs.docker.com/reference/api/docker_remote_api_v1.13/
> >
> > IMHO - It looks more like a cli wrap vs. docker API abstraction.
> >
> > Perhaps we could open another JIRA after this?
>
> Timothy Chen wrote:
> Besides the naming of POST/GET containers, I think at least the actions are an interface that we swap out. If we move to the REST api, we are performing the same actions but just callign the REST apis.
>
> Ideally we want to use libcontainer though, which if we model after the REST api it's going to look a lot of different in the end IMO.
>
The api has a bit more breadth and depth from what I've read, I was combing through while doing the review.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48402
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/slave/containerizer/isolators/cgroups/cpushare.cpp, line 59
> > <https://reviews.apache.org/r/23771/diff/2/?file=638782#file638782line59>
> >
> > Umm this is not always true, it's a weight based on the total across all groups. In systemd land this means promoting docker to a top level group and assigning a weight that has meaning.
> >
> > ref - http://timothysc.github.io/blog/2013/06/14/systemd-cgroup-sla/
I think this is another JIRA ticket then, as this is shared implementation with our isloators
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 149
> > <https://reviews.apache.org/r/23771/diff/2/?file=638770#file638770line149>
> >
> > Input args will need to override defaults re: net, there is a separate JIRA on *this.
This won't be addressed in this patch, DockerInfo (or similiar construct) is our next iteration
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 78
> > <https://reviews.apache.org/r/23771/diff/2/?file=638770#file638770line78>
> >
> > Might want to consider placing into a serializable struct.
We'll do that when we see a need for complex interactions. Nice suggestion though!
> On July 22, 2014, 8:38 p.m., Timothy St. Clair wrote:
> > src/docker/docker.cpp, line 25
> > <https://reviews.apache.org/r/23771/diff/2/?file=638770#file638770line25>
> >
> > code standard - header ordering.
Sounds good.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48402
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48402
-----------------------------------------------------------
only 1/2 way through, this may take a bit.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84964>
So imho we should try to match parity with the API where possible, to provide an abstraction which is consistent.
https://docs.docker.com/reference/api/docker_remote_api_v1.13/
IMHO - It looks more like a cli wrap vs. docker API abstraction.
Perhaps we could open another JIRA after this?
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment84976>
Graceful termination? How does signal escalation get handled?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84965>
code standard - header ordering.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84968>
This is pretty weird to have here and would only exist on a pre-EL7. All relatively newer distro(s) it would be guaranteed true.
Do folks want to support the pre-EL7 route? Because it may kneecap docker.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84969>
Might want to consider placing into a serializable struct.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84971>
See other comment re: shares and groups.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84972>
Input args will need to override defaults re: net, there is a separate JIRA on *this.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84979>
Should this be here?
Also manifest constant for the chunck is probably wise for page size.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment84982>
It almost seems like at certain points the REST API would be simpler/cleaner.
e.g. GET /containers/(id)/json
Perhaps we should open a JIRA?
src/slave/containerizer/isolators/cgroups/cpushare.cpp
<https://reviews.apache.org/r/23771/#comment84970>
Umm this is not always true, it's a weight based on the total across all groups. In systemd land this means promoting docker to a top level group and assigning a weight that has meaning.
ref - http://timothysc.github.io/blog/2013/06/14/systemd-cgroup-sla/
- Timothy St. Clair
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48469
-----------------------------------------------------------
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85098>
We don't, but this class you see is suppose to be the abstraction although it's not abstract.
I think we can do this when we want to switch over to some other implementation later though.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85128>
Docker although can run containers, we won't be able to update or set the initial resources for docker via cgroups. We choose to to minimum validation upfront for that
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85130>
This is actually parsed JSON Objects already, so it's not really parsing it every time but just traversing the object tree.
- Timothy Chen
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48796
-----------------------------------------------------------
Bad patch!
Reviews applied: [23771]
Failed command: git apply --index 23771.patch
Error:
23771.patch:3554: trailing whitespace.
Try<process::Subprocess> install =
error: patch failed: src/Makefile.am:223
error: src/Makefile.am: patch does not apply
error: patch failed: src/common/status_utils.hpp:24
error: src/common/status_utils.hpp: patch does not apply
error: patch failed: src/slave/containerizer/containerizer.cpp:36
error: src/slave/containerizer/containerizer.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:339
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:2426
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by "Timothy St. Clair" <ts...@redhat.com>.
> On July 22, 2014, 8:45 p.m., Timothy Chen wrote:
> > src/docker/docker.cpp, line 53
> > <https://reviews.apache.org/r/23771/diff/2/?file=638770#file638770line53>
> >
> > It is at least manifested when this is launched on GCE, so we want to make sure the minimum cgroup support is there.
GCE offers their "Container Optimized Image", did you see it there? If so, I would be shocked.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48426
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 22, 2014, 8:45 p.m., Timothy Chen wrote:
> > src/docker/docker.cpp, line 53
> > <https://reviews.apache.org/r/23771/diff/2/?file=638770#file638770line53>
> >
> > It is at least manifested when this is launched on GCE, so we want to make sure the minimum cgroup support is there.
>
> Timothy St. Clair wrote:
> GCE offers their "Container Optimized Image", did you see it there? If so, I would be shocked.
Ben actually reported this, but I think we want to at least support the standard image.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48426
-----------------------------------------------------------
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48426
-----------------------------------------------------------
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85013>
It is at least manifested when this is launched on GCE, so we want to make sure the minimum cgroup support is there.
- Timothy Chen
On July 22, 2014, 6:12 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 22, 2014, 6:12 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/mesos_test_executor_docker_image/Dockerfile PRE-CREATION
> src/tests/mesos_test_executor_docker_image/install.sh PRE-CREATION
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49040
-----------------------------------------------------------
Bad patch!
Reviews applied: [23771]
Failed command: git apply --index 23771.patch
Error:
error: patch failed: src/Makefile.am:223
error: src/Makefile.am: patch does not apply
error: patch failed: src/common/status_utils.hpp:22
error: src/common/status_utils.hpp: patch does not apply
error: patch failed: src/slave/containerizer/containerizer.cpp:36
error: src/slave/containerizer/containerizer.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:339
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:2426
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On July 29, 2014, 1:04 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 1:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 30, 2014, 1:47 a.m., Jie Yu wrote:
> > Have you updated the diff? I don't see any difference between r3 and r4.
Hi Jie, you're right for some reason my patch didn't have my changes! Please take a look now.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49075
-----------------------------------------------------------
On July 30, 2014, 5:08 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 5:08 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49075
-----------------------------------------------------------
Have you updated the diff? I don't see any difference between r3 and r4.
- Jie Yu
On July 29, 2014, 1:04 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 1:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49142
-----------------------------------------------------------
Bad patch!
Reviews applied: [23771]
Failed command: git apply --index 23771.patch
Error:
error: patch failed: src/Makefile.am:223
error: src/Makefile.am: patch does not apply
error: patch failed: src/common/status_utils.hpp:22
error: src/common/status_utils.hpp: patch does not apply
error: patch failed: src/slave/containerizer/containerizer.cpp:36
error: src/slave/containerizer/containerizer.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:339
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:2426
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On July 30, 2014, 5:08 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 5:08 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 31, 2014, 3:14 a.m., Jie Yu wrote:
> > src/docker/docker.cpp, line 260
> > <https://reviews.apache.org/r/23771/diff/7/?file=646245#file646245line260>
> >
> > I think making 'success' a static function and get rid of the docker::internal namespace is better.
I remember you wanted me earlier to make it only defined in the cpp file :)
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49117
-----------------------------------------------------------
On July 30, 2014, 8:42 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 8:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49117
-----------------------------------------------------------
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85959>
Please move the right parenthesis up right next to '_pid':
Container(
const std::string& _id,
const std::string& _name,
const Option<pid_t>& _pid)
: id(_id), name(_name), pid(_pid) {}
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86069>
This is a continuation, renamed it to '_failure'.
Also, make them 'static' since it only used in this cpp file. Same for the following functions as well.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86068>
s/failedMessage/failure/
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86070>
What if errFd is closed because the corresponding Subprocess goes out of scope? You may want to hold a reference to the Subprocess by passing it in?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86071>
We typically do not use else in this case:
if (...) {
}
io::read(...)...
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86073>
You probably want to pass Subprocess in here.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86072>
I think making 'success' a static function and get rid of the docker::internal namespace is better.
- Jie Yu
On July 30, 2014, 8:42 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 8:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49252
-----------------------------------------------------------
Bad patch!
Reviews applied: [23771]
Failed command: git apply --index 23771.patch
Error:
error: patch failed: src/Makefile.am:223
error: src/Makefile.am: patch does not apply
error: patch failed: src/common/status_utils.hpp:22
error: src/common/status_utils.hpp: patch does not apply
error: patch failed: src/slave/containerizer/containerizer.cpp:36
error: src/slave/containerizer/containerizer.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:339
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:2426
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On July 31, 2014, 8:06 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 8:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 31, 2014, 6:39 p.m., Jie Yu wrote:
> > src/tests/environment.cpp, lines 147-151
> > <https://reviews.apache.org/r/23771/diff/8/?file=646729#file646729line147>
> >
> > Hum, you switch the order. But that doesn't solve the problem. My point here is that:
> >
> > This function should never return true early, otherwise, you'll miss the following checks. So it should be
> >
> > if (docker.isError()) {
> > return false;
> > }
> >
> > #ifdef __linux__
> > if (user.get() != "root") {
> > return false;
> > }
> > #endif
ah ok I see now!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49240
-----------------------------------------------------------
On July 31, 2014, 8:06 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 8:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 31, 2014, 6:39 p.m., Jie Yu wrote:
> > src/docker/docker.cpp, line 351
> > <https://reviews.apache.org/r/23771/diff/8/?file=646708#file646708line351>
> >
> > if (status.get() != 0) {
> > return err(s).then(lambda::bind(&__check, cmd, status.get(), lambda::_1));
> > }
I actually can't do this since the return type doesn't make __check.
What I ended up doing is another static method with a template.
> On July 31, 2014, 6:39 p.m., Jie Yu wrote:
> > src/docker/docker.cpp, line 80
> > <https://reviews.apache.org/r/23771/diff/8/?file=646708#file646708line80>
> >
> > This has been gone through a few iterations already. Sorry, this is my fault that I haven't thought about it in deep. But I hope we can resolve this before shipping this.
> >
> > A few issues:
> >
> > 1) This is a library, we usually don't have LOG() in the library (otherwise, very likely we'll have double logging). Instead, we usually put error messages in the return value (either Future or Try).
> >
> > 2) Returning 'bool' doesn't seem to be necessary? We always return false on error and return true on success, right? In that case, we can just use Future<Nothing>?
> >
> > So here is what I would suggest (s/success/check/ maybe):
> >
> > Future<Nothing> check(const string& cmd, const Subprocess& s)
> > {
> > return s.status().then(lambda::bind(&_success, cmd, s);
> > }
> >
> > Future<Nothing> _check(const string& cmd, const Subprocess& s)
> > {
> > Option<int> status = s.status().get();
> > if (status.isNone()) {
> > return Failure("No status found for '" + cmd + "'");
> > }
> >
> > if (status.get() != 0) {
> > return err(s).then(lambda::bind(&__check, cmd, status, lambda::_1));
> > }
> >
> > return Nothing();
> > }
> >
> > Future<Nothing> __check(const string& cmd, int status, const string& err)
> > {
> > return Failure(
> > "Failed to '" + cmd + "': exit status = " +
> > WSTRINGIFY(status) + " stderr = " + err);
> > }
> >
> > // Retrieve the standard error of 's'.
> > Future<string> err(const Subprocess& s)
> > {
> > CHECK_SOME(s.err());
> >
> > Try<Nothing> nonblock = os::nonblock(s.err().get());
> > if (nonblock.isError()) {
> > return Failure("Cannot set nonblock for stderr");
> > }
> >
> > return io::read(s.err().get());
> > }
I had an compile error where _check conflicts with the _check method from process/check.hpp. I renamed all to [_|__]checkStatus
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49240
-----------------------------------------------------------
On July 31, 2014, 8:06 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 8:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 31, 2014, 6:39 p.m., Jie Yu wrote:
> > src/docker/docker.cpp, lines 288-294
> > <https://reviews.apache.org/r/23771/diff/8/?file=646708#file646708line288>
> >
> > You should be able to chain remove with check:
> >
> > return check(cmd, s).then(lambda::bind(&__kill, *this, container, lambda::_1));
> >
> > __kill(docker, container, force)
> > {
> > docker.rm(container, force);
> > }
I actually can't since I want to call remove regardless of status.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49240
-----------------------------------------------------------
On July 31, 2014, 8:06 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 8:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49240
-----------------------------------------------------------
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86123>
CHECK_READY(s.status());
CHECK_SOME(s.status().get());
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86128>
See my comments below.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86146>
This has been gone through a few iterations already. Sorry, this is my fault that I haven't thought about it in deep. But I hope we can resolve this before shipping this.
A few issues:
1) This is a library, we usually don't have LOG() in the library (otherwise, very likely we'll have double logging). Instead, we usually put error messages in the return value (either Future or Try).
2) Returning 'bool' doesn't seem to be necessary? We always return false on error and return true on success, right? In that case, we can just use Future<Nothing>?
So here is what I would suggest (s/success/check/ maybe):
Future<Nothing> check(const string& cmd, const Subprocess& s)
{
return s.status().then(lambda::bind(&_success, cmd, s);
}
Future<Nothing> _check(const string& cmd, const Subprocess& s)
{
Option<int> status = s.status().get();
if (status.isNone()) {
return Failure("No status found for '" + cmd + "'");
}
if (status.get() != 0) {
return err(s).then(lambda::bind(&__check, cmd, status, lambda::_1));
}
return Nothing();
}
Future<Nothing> __check(const string& cmd, int status, const string& err)
{
return Failure(
"Failed to '" + cmd + "': exit status = " +
WSTRINGIFY(status) + " stderr = " + err);
}
// Retrieve the standard error of 's'.
Future<string> err(const Subprocess& s)
{
CHECK_SOME(s.err());
Try<Nothing> nonblock = os::nonblock(s.err().get());
if (nonblock.isError()) {
return Failure("Cannot set nonblock for stderr");
}
return io::read(s.err().get());
}
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86122>
Add a CHECK_READY(s.status());
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86147>
Here, you can just do:
return check(cmd, s);
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86166>
You should be able to chain remove with check:
return check(cmd, s).then(lambda::bind(&__kill, *this, container, lambda::_1));
__kill(docker, container, force)
{
docker.rm(container, force);
}
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86121>
This should fit in the above line?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86168>
pass in cmd here
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86167>
if (status.isNone()) {
return Failure(...);
}
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86169>
if (status.get() != 0) {
return err(s).then(lambda::bind(&__check, cmd, status.get(), lambda::_1));
}
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86182>
Ditto. Passing cmd in.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86181>
Ditto.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment86185>
If 'docker.run' returns Nothing, you won't have a chance to execute this code, but I think it's ok because Self::destroy will do 'docker.kill' in anyway.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment86187>
Logging format please.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment86189>
Logging format.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment86190>
Ditto.
src/tests/environment.cpp
<https://reviews.apache.org/r/23771/#comment86199>
I think this has been fixed. YOu need to do a rebase.
src/tests/environment.cpp
<https://reviews.apache.org/r/23771/#comment86195>
Hum, you switch the order. But that doesn't solve the problem. My point here is that:
This function should never return true early, otherwise, you'll miss the following checks. So it should be
if (docker.isError()) {
return false;
}
#ifdef __linux__
if (user.get() != "root") {
return false;
}
#endif
src/tests/environment.cpp
<https://reviews.apache.org/r/23771/#comment86198>
This is buggy too (not your fault). Do a rebase, it should be fixed. If not, fix it.
- Jie Yu
On July 31, 2014, 8:06 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 8:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49313
-----------------------------------------------------------
src/health-check/main.cpp
<https://reviews.apache.org/r/23771/#comment86274>
These are changes that got in from rebase. This should be all in master already.
- Timothy Chen
On Aug. 1, 2014, 12:46 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2014, 12:46 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp c51a8c6
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 10d57a0
> src/launcher/executor.cpp 948673e
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp af6be22
> src/master/master.cpp 77f2536
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 551698f
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 0d29c6d
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On Aug. 1, 2014, 11:53 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 2429
> > <https://reviews.apache.org/r/23771/diff/10/?file=647996#file647996line2429>
> >
> > Is this an existing non-docker-related bug in the slave code?
> > How about renaming 'future' to make it clear what the bool true/false actually represents, like "Future<bool>& launched" so "if (!launched.isReady())" and "else if (!launched.get())" read clearly.
> > Maybe this (and removing all the "using namespace mesos::internal::status;") should go in a separate review.
It's unfortunately fixing the composing containerizer that is what this patch is depended upon.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49338
-----------------------------------------------------------
On Aug. 1, 2014, 8:11 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2014, 8:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp c51a8c6
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 10d57a0
> src/launcher/executor.cpp 948673e
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp af6be22
> src/master/master.cpp 77f2536
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 551698f
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 0d29c6d
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49338
-----------------------------------------------------------
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86334>
alphabetize?
src/slave/slave.cpp
<https://reviews.apache.org/r/23771/#comment86335>
Is this an existing non-docker-related bug in the slave code?
How about renaming 'future' to make it clear what the bool true/false actually represents, like "Future<bool>& launched" so "if (!launched.isReady())" and "else if (!launched.get())" read clearly.
Maybe this (and removing all the "using namespace mesos::internal::status;") should go in a separate review.
- Adam B
On Aug. 1, 2014, 1:11 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2014, 1:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp c51a8c6
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 10d57a0
> src/launcher/executor.cpp 948673e
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp af6be22
> src/master/master.cpp 77f2536
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 551698f
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 0d29c6d
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49418
-----------------------------------------------------------
Bad patch!
Reviews applied: [23771]
Failed command: git apply --index 23771.patch
Error:
error: patch failed: src/Makefile.am:263
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/containerizer.cpp:36
error: src/slave/containerizer/containerizer.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:340
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:2429
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On Aug. 1, 2014, 11:31 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2014, 11:31 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp c51a8c6
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 10d57a0
> src/launcher/executor.cpp 948673e
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp af6be22
> src/master/master.cpp 77f2536
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 551698f
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 0d29c6d
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated Aug. 1, 2014, 11:31 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
Added comments on the disabled test.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp c51a8c6
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 10d57a0
src/launcher/executor.cpp 948673e
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp af6be22
src/master/master.cpp 77f2536
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 551698f
src/tests/flags.hpp a003e7f
src/tests/script.cpp 0d29c6d
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated Aug. 1, 2014, 4:36 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp c51a8c6
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 10d57a0
src/launcher/executor.cpp 948673e
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp af6be22
src/master/master.cpp 77f2536
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 551698f
src/tests/flags.hpp a003e7f
src/tests/script.cpp 0d29c6d
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49328
-----------------------------------------------------------
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/23771/#comment86310>
I am disabling this test for the reasons I have in my comments.
- Timothy Chen
On Aug. 1, 2014, 8:11 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2014, 8:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp c51a8c6
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 10d57a0
> src/launcher/executor.cpp 948673e
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp af6be22
> src/master/master.cpp 77f2536
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 551698f
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 0d29c6d
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated Aug. 1, 2014, 8:11 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp c51a8c6
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 10d57a0
src/launcher/executor.cpp 948673e
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp af6be22
src/master/master.cpp 77f2536
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 551698f
src/tests/flags.hpp a003e7f
src/tests/script.cpp 0d29c6d
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49320
-----------------------------------------------------------
Ship it!
LGTM! Please rebase to the master and I'll get it committed tomorrow! Thanks Tim!
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment86289>
I guess we have some mis-communication last time. I want these function to be static to the cpp file (not static to this class) so that you don't need to declare them in the header file.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86290>
Add a comment about this helper:
// Asynchronously read stderr from subprocess.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86293>
Add a comment about this helper. Say 'Return a failure if error occurs'.
Also, checkError seems to be a more appropriate name
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86296>
Add a TODO here saying:
// TODO: Consider returning stdout as well.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment86297>
The semantics here is that you'll return the result of 'docker.rm' if 'kill' failed and 'remove=true', right? If yes, please document this somewhere.
- Jie Yu
On Aug. 1, 2014, 12:46 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2014, 12:46 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp c51a8c6
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 10d57a0
> src/launcher/executor.cpp 948673e
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp af6be22
> src/master/master.cpp 77f2536
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 551698f
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 0d29c6d
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated Aug. 1, 2014, 12:46 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp c51a8c6
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 10d57a0
src/launcher/executor.cpp 948673e
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp af6be22
src/master/master.cpp 77f2536
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 551698f
src/tests/flags.hpp a003e7f
src/tests/script.cpp 0d29c6d
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 31, 2014, 8:06 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 30, 2014, 8:42 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
Added some logging of when docker container skips launching.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 30, 2014, 8:12 p.m., Jay Buffington wrote:
> > src/slave/containerizer/docker.cpp, line 525
> > <https://reviews.apache.org/r/23771/diff/5/?file=645526#file645526line525>
> >
> > What does returning false here *mean*? I think it is supposed to mean "the docker containerizer is passing on this task, the next containerizer in the list should make an attempt"
> >
> > But that's not what's happening here. If the
> > scheduler doesn't prefix ContainerInfo's image with docker:/// "failed to start: TaskInfo/ExecutorInfo not supported" shows up in the log and the task fails to start. See slave/slave.cpp line 2443.
I think the place you see in slave.cpp is the output of the ComposingContainerizer, which it internally traverses all the containerizers that is configured and tries to launch each one. When it finally returns false, it means it has tried all configured containerizers not just docker. You can look at the containerizer/composing.cpp to see more details.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49152
-----------------------------------------------------------
On July 30, 2014, 7:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jay Buffington <me...@jaybuff.com>.
> On July 30, 2014, 8:12 p.m., Jay Buffington wrote:
> > src/slave/containerizer/docker.cpp, line 523
> > <https://reviews.apache.org/r/23771/diff/5/?file=645526#file645526line523>
> >
> > This should be docker:// (two slashes, not three). With a private registry I'll set image to docker://docker-registry.example.com/jaybuff/centos:6u5
> >
> > Same goes for line 537.
>
> Timothy Chen wrote:
> There is no standard of how a docker image should be prefixed unfortunately, and this is how docker images are configured in Deimos as the argument goes that it conforms more towards to the standards with three slashes (https://github.com/mesosphere/deimos/issues/7)
My reading of that deimos issue is that burke said "it should be two slashes, not three" and then solidsnack agreed and made it two slashes with this commit: https://github.com/mesosphere/deimos/commit/7b9fd0d2d335edca75379880df5252f646d51994
> On July 30, 2014, 8:12 p.m., Jay Buffington wrote:
> > src/slave/containerizer/docker.cpp, line 525
> > <https://reviews.apache.org/r/23771/diff/5/?file=645526#file645526line525>
> >
> > What does returning false here *mean*? I think it is supposed to mean "the docker containerizer is passing on this task, the next containerizer in the list should make an attempt"
> >
> > But that's not what's happening here. If the
> > scheduler doesn't prefix ContainerInfo's image with docker:/// "failed to start: TaskInfo/ExecutorInfo not supported" shows up in the log and the task fails to start. See slave/slave.cpp line 2443.
>
> Timothy Chen wrote:
> I think the place you see in slave.cpp is the output of the ComposingContainerizer, which it internally traverses all the containerizers that is configured and tries to launch each one. When it finally returns false, it means it has tried all configured containerizers not just docker. You can look at the containerizer/composing.cpp to see more details.
Okay, but the error message is still bogus, right? TaskInfo/ExecutorInfo is supported with this patch now. The error message should more accurately read "No containerizer can launch task <task_id>".
When a containerize passes on a task, it should log the reason why. In my above example, it should log an info message such as "ContainerInfo image is not a docker image (it is not prefixed with docker:///)"
That will make debugging when writing a scheduler that uses the docker containerizer much easier.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49152
-----------------------------------------------------------
On July 30, 2014, 8:42 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 8:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 30, 2014, 8:12 p.m., Jay Buffington wrote:
> > src/slave/containerizer/docker.cpp, line 523
> > <https://reviews.apache.org/r/23771/diff/5/?file=645526#file645526line523>
> >
> > This should be docker:// (two slashes, not three). With a private registry I'll set image to docker://docker-registry.example.com/jaybuff/centos:6u5
> >
> > Same goes for line 537.
There is no standard of how a docker image should be prefixed unfortunately, and this is how docker images are configured in Deimos as the argument goes that it conforms more towards to the standards with three slashes (https://github.com/mesosphere/deimos/issues/7)
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49152
-----------------------------------------------------------
On July 30, 2014, 7:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 30, 2014, 8:12 p.m., Jay Buffington wrote:
> > src/slave/containerizer/docker.cpp, line 525
> > <https://reviews.apache.org/r/23771/diff/5/?file=645526#file645526line525>
> >
> > What does returning false here *mean*? I think it is supposed to mean "the docker containerizer is passing on this task, the next containerizer in the list should make an attempt"
> >
> > But that's not what's happening here. If the
> > scheduler doesn't prefix ContainerInfo's image with docker:/// "failed to start: TaskInfo/ExecutorInfo not supported" shows up in the log and the task fails to start. See slave/slave.cpp line 2443.
>
> Timothy Chen wrote:
> I think the place you see in slave.cpp is the output of the ComposingContainerizer, which it internally traverses all the containerizers that is configured and tries to launch each one. When it finally returns false, it means it has tried all configured containerizers not just docker. You can look at the containerizer/composing.cpp to see more details.
>
> Jay Buffington wrote:
> Okay, but the error message is still bogus, right? TaskInfo/ExecutorInfo is supported with this patch now. The error message should more accurately read "No containerizer can launch task <task_id>".
>
> When a containerize passes on a task, it should log the reason why. In my above example, it should log an info message such as "ContainerInfo image is not a docker image (it is not prefixed with docker:///)"
>
> That will make debugging when writing a scheduler that uses the docker containerizer much easier.
I added debug lines when docker containerizer skips launching. The composing containerizer is in another reviewboard patch that Ben is putting up, so will be best to leave comments on that.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49152
-----------------------------------------------------------
On July 30, 2014, 8:42 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 8:42 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jay Buffington <me...@jaybuff.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review49152
-----------------------------------------------------------
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85999>
This should be docker:// (two slashes, not three). With a private registry I'll set image to docker://docker-registry.example.com/jaybuff/centos:6u5
Same goes for line 537.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85998>
What does returning false here *mean*? I think it is supposed to mean "the docker containerizer is passing on this task, the next containerizer in the list should make an attempt"
But that's not what's happening here. If the
scheduler doesn't prefix ContainerInfo's image with docker:/// "failed to start: TaskInfo/ExecutorInfo not supported" shows up in the log and the task fails to start. See slave/slave.cpp line 2443.
- Jay Buffington
On July 30, 2014, 7:54 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 30, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 30, 2014, 7:54 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 30, 2014, 5:08 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 29, 2014, 1:04 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.hpp, line 48
> > <https://reviews.apache.org/r/23771/diff/2/?file=638778#file638778line48>
> >
> > I don't see the implementation of this function?
Sorry it was used but then removed later, forgot to delete the def.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/docker/docker.cpp, lines 212-216
> > <https://reviews.apache.org/r/23771/diff/3/?file=643404#file643404line212>
> >
> > Would you please add a TODO here. Ideally, we should use the argv version to avoid escaping and use something similar to 'shlex.split' to split the 'command' below.
Sounds good, I think we should include a split utils or change our command inputs to a list of args instead.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 839-840
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line839>
> >
> > Hum, this looks problematic to me. These two static variables will be initialized even before 'main' is called, at which time, the mesos containerizer (potentially) may not have initialized the cgroups mounts yet.
I don't think we're relying on the Mesos Containerizer to initialize the cgroups mount though, as we call this when _update is called it should already been mounted.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, line 915
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line915>
> >
> > Probably we should comment about how OOM is handled. Does docker use the kernel oom killer? Do we need to check oom_control to verify that? Should we listen for oom events?
I'm going to leave a TODO for OOM.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, line 1038
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line1038>
> >
> > Seems that we don't trigger any Limitation. I am curious when killed=true will be used? In other words, why do we need this additional parameter? Worth commenting it.
This is set when the process is killed instead of being exited on itself, we keep this flag so in the end when we mark the termination status we can use it to set the killed field in containerizer::Termination
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/tests/docker_containerizer_tests.cpp, line 190
> > <https://reviews.apache.org/r/23771/diff/3/?file=643423#file643423line190>
> >
> > Is this the only place you want to use validate = false for Docker::create? If yes, I would suggest remove this parameter and you can do
> >
> > Docker::create(tests::flags.docker).get()
> >
> > here, and the internal check in 'get()' will make sure docker is available.
Not sure how that change can skip validation, but I was trying to make the test run a bit faster by not validating on each Docker create since it's already been checked before.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/tests/docker_containerizer_tests.cpp, lines 229-232
> > <https://reviews.apache.org/r/23771/diff/3/?file=643423#file643423line229>
> >
> > Probably add a comment here saying that test-executor will be downloaded. Have you consider making this test self contained?
Thought about it, but the binary image itself isn't small and even including the DockerFile since incurrs a download no matter what.
So in the end we end up having the smallest executor that we have atm which is a native executor written in go, that is only about 15 mb.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
On July 28, 2014, 5:41 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 5:41 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 521-523
> > <https://reviews.apache.org/r/23771/diff/2/?file=638779#file638779line521>
> >
> > Use a helper function to construct the docker container name.
Ignore this. This is for version2.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
On July 28, 2014, 5:41 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 5:41 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 839-840
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line839>
> >
> > Hum, this looks problematic to me. These two static variables will be initialized even before 'main' is called, at which time, the mesos containerizer (potentially) may not have initialized the cgroups mounts yet.
>
> Timothy Chen wrote:
> I don't think we're relying on the Mesos Containerizer to initialize the cgroups mount though, as we call this when _update is called it should already been mounted.
>
> Jie Yu wrote:
> I am referring to the cases where --containerizers=docker,mesos
>
> MesosContainerizer is gonna call cgroups::prepare for cpu/memory which will mount cpu/memory subsystems if they are not yet mounted.
>
> But seems that according to C++ standard, local static variables will be initialized the first time its translation unit is entered. At which time, we should already mount the cgroups. So your code is fine, but I would rather either store it as a field member, or call cgroups::hierarchy each time 'update' is called.
But if it becomes a field member it will get evaluated before cgroups::prepare is called then right?
Ben intentionally put it as a static variable to avoid re-doing the hierarchy lookup since it doesn't change, this is pretty standard C++ and don't really see what are the good reasons to call this each time.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
On July 29, 2014, 1:04 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 1:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 839-840
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line839>
> >
> > Hum, this looks problematic to me. These two static variables will be initialized even before 'main' is called, at which time, the mesos containerizer (potentially) may not have initialized the cgroups mounts yet.
>
> Timothy Chen wrote:
> I don't think we're relying on the Mesos Containerizer to initialize the cgroups mount though, as we call this when _update is called it should already been mounted.
>
> Jie Yu wrote:
> I am referring to the cases where --containerizers=docker,mesos
>
> MesosContainerizer is gonna call cgroups::prepare for cpu/memory which will mount cpu/memory subsystems if they are not yet mounted.
>
> But seems that according to C++ standard, local static variables will be initialized the first time its translation unit is entered. At which time, we should already mount the cgroups. So your code is fine, but I would rather either store it as a field member, or call cgroups::hierarchy each time 'update' is called.
>
> Timothy Chen wrote:
> But if it becomes a field member it will get evaluated before cgroups::prepare is called then right?
> Ben intentionally put it as a static variable to avoid re-doing the hierarchy lookup since it doesn't change, this is pretty standard C++ and don't really see what are the good reasons to call this each time.
You are right. Ignore my comments. Sorry about the confusion.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
On July 29, 2014, 1:04 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 1:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 839-840
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line839>
> >
> > Hum, this looks problematic to me. These two static variables will be initialized even before 'main' is called, at which time, the mesos containerizer (potentially) may not have initialized the cgroups mounts yet.
>
> Timothy Chen wrote:
> I don't think we're relying on the Mesos Containerizer to initialize the cgroups mount though, as we call this when _update is called it should already been mounted.
>
> Jie Yu wrote:
> I am referring to the cases where --containerizers=docker,mesos
>
> MesosContainerizer is gonna call cgroups::prepare for cpu/memory which will mount cpu/memory subsystems if they are not yet mounted.
>
> But seems that according to C++ standard, local static variables will be initialized the first time its translation unit is entered. At which time, we should already mount the cgroups. So your code is fine, but I would rather either store it as a field member, or call cgroups::hierarchy each time 'update' is called.
>
> Timothy Chen wrote:
> But if it becomes a field member it will get evaluated before cgroups::prepare is called then right?
> Ben intentionally put it as a static variable to avoid re-doing the hierarchy lookup since it doesn't change, this is pretty standard C++ and don't really see what are the good reasons to call this each time.
>
> Jie Yu wrote:
> You are right. Ignore my comments. Sorry about the confusion.
Sounds good! Please let me know if you have any other comments, I believe I have addressed all of them except two that Ben is working on.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
On July 29, 2014, 1:04 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 1:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
> On July 28, 2014, 9:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/docker.cpp, lines 839-840
> > <https://reviews.apache.org/r/23771/diff/3/?file=643413#file643413line839>
> >
> > Hum, this looks problematic to me. These two static variables will be initialized even before 'main' is called, at which time, the mesos containerizer (potentially) may not have initialized the cgroups mounts yet.
>
> Timothy Chen wrote:
> I don't think we're relying on the Mesos Containerizer to initialize the cgroups mount though, as we call this when _update is called it should already been mounted.
I am referring to the cases where --containerizers=docker,mesos
MesosContainerizer is gonna call cgroups::prepare for cpu/memory which will mount cpu/memory subsystems if they are not yet mounted.
But seems that according to C++ standard, local static variables will be initialized the first time its translation unit is entered. At which time, we should already mount the cgroups. So your code is fine, but I would rather either store it as a field member, or call cgroups::hierarchy each time 'update' is called.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
On July 29, 2014, 1:04 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 1:04 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48738
-----------------------------------------------------------
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/23771/#comment85484>
I don't see the implementation of this function?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85486>
This CHECK seems to be redundant as you just have 'if (!run.get().forkedPid.isSome())' above.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85487>
This log seems to be redundant as the caller should print the error.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85488>
Use a helper function to construct the docker container name.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85489>
Ditto. Remove this redundant log line.
src/docker/docker.hpp
<https://reviews.apache.org/r/23771/#comment85600>
Move ')' above. Also, use const ref for parameters. Finally, remove ';' at the end.
Container(
const std::string& _id,
const std::string& _name,
const Option<pid_t> _pid)
: id(_id), name(_name), pid(_pid) {}
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85601>
Add a new line above. We add a new line if two headers are not in the same directory.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85602>
Instead of putting all the validation code in to a giant if block, how about this:
if (!validate) {
return Docker(path);
}
// Validation code.
return Docker(path);
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85604>
// TODO(yifan): Reload ... above.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85607>
Kill this empty line.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85610>
Would you please add a TODO here. Ideally, we should use the argv version to avoid escaping and use something similar to 'shlex.split' to split the 'command' below.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85615>
why not use 'cmd' here?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85619>
Remove const for primitive types.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85623>
Move this to the bottom (or top) so that inspect and _inspect are close together for better readability.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85612>
if (status.isNone()) {
return false;
}
if (status.get() != 0) {
LOG(WARNING) << cmd << " failed (" << WSTRINGIFY(status.get()) << ")";
return false;
}
return true;
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85626>
Should we also have a warning or error message if status is None?
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85625>
Kill the empty line here.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85620>
Remove const.
src/docker/docker.cpp
<https://reviews.apache.org/r/23771/#comment85627>
Ditto. Should we handle status.isNone()?
src/examples/docker_no_executor_framework.cpp
<https://reviews.apache.org/r/23771/#comment85628>
Docker No Executor Framework (C++) ?
src/linux/cgroups.cpp
<https://reviews.apache.org/r/23771/#comment85630>
Kill the empty line here.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85632>
Does 'parse' need to be a member function? What about making them a static function in the cpp file, and do not declare them in the header? Also, consider renaming them:
static Option<ContainerID> containerID(...);
static string containerName(..);
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85635>
This check seems to be redundant because you just have a if check above.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85646>
Remove this log line as it should be logged by the caller.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85647>
Ditto. Remove this redundant log line.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85656>
IIUC, !taskInfo.has_command() shouldn't happen, because otherwise it would use the other version of the launch, right?
If that's the case, could you please add a warning here. If that's not the case, it would be better to add a comment to explain in what situation this could have happened.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85654>
Any reason to use bash here. Seems that we use /bin/sh everywhere else (e.g., in subprocess).
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85657>
The errno might be reset by os::close. You probably want to save it before calling os::close.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85658>
Wrap the comments with 70 char width.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85660>
No need to call 'value()':
LOG(WARNING) << "Ignore..." << containerId;
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85662>
Hum, this looks problematic to me. These two static variables will be initialized even before 'main' is called, at which time, the mesos containerizer (potentially) may not have initialized the cgroups mounts yet.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85663>
We usually have the following format for logging:
LOG(WARNING) << "..."
<< "...";
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85664>
Ditto.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85665>
Ditto.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85668>
Probably we should comment about how OOM is handled. Does docker use the kernel oom killer? Do we need to check oom_control to verify that? Should we listen for oom events?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85666>
Ditto.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85667>
Copy the TODO from cgroups/mem.cpp
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85671>
Seems that we don't trigger any Limitation. I am curious when killed=true will be used? In other words, why do we need this additional parameter? Worth commenting it.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/23771/#comment85672>
Should we set message for the Termination as well?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/23771/#comment85673>
Rename it to 'exists'?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/23771/#comment85661>
Use stringify(containerId) instead.
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/23771/#comment85684>
Is this the only place you want to use validate = false for Docker::create? If yes, I would suggest remove this parameter and you can do
Docker::create(tests::flags.docker).get()
here, and the internal check in 'get()' will make sure docker is available.
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/23771/#comment85688>
Probably add a comment here saying that test-executor will be downloaded. Have you consider making this test self contained?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/23771/#comment85690>
Hum, in this test, you constructed the RunState manually. Is it possible to use docker containerizer to launch tasks and then recover it? Similar to what we did in MesosContainerizerSlaveRecoveryTest?
src/tests/docker_tests.cpp
<https://reviews.apache.org/r/23771/#comment85681>
We usually use:
using namespace process;
for tests.
src/tests/docker_tests.cpp
<https://reviews.apache.org/r/23771/#comment85682>
Wrap the comments using 70 char width. Here and everywhere else.
src/tests/environment.cpp
<https://reviews.apache.org/r/23771/#comment85680>
Kill this include?
src/tests/environment.cpp
<https://reviews.apache.org/r/23771/#comment85679>
Hum, it's not your fault. What if the user define a test: DOCKER_BENCHMARK_...
Are we gonna run the test even if flags.benchmark = false?
- Jie Yu
On July 28, 2014, 5:41 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 5:41 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/#review48827
-----------------------------------------------------------
Bad patch!
Reviews applied: [23771]
Failed command: git apply --index 23771.patch
Error:
error: patch failed: src/Makefile.am:223
error: src/Makefile.am: patch does not apply
error: patch failed: src/common/status_utils.hpp:22
error: src/common/status_utils.hpp: patch does not apply
error: patch failed: src/slave/containerizer/containerizer.cpp:36
error: src/slave/containerizer/containerizer.cpp: patch does not apply
error: patch failed: src/slave/flags.hpp:339
error: src/slave/flags.hpp: patch does not apply
error: patch failed: src/slave/slave.cpp:2426
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On July 28, 2014, 5:41 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23771/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 5:41 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Docker implementation.
> This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
>
>
> Diffs
> -----
>
> src/Makefile.am 45afcd1
> src/common/status_utils.hpp 1980551
> src/docker/docker.hpp PRE-CREATION
> src/docker/docker.cpp PRE-CREATION
> src/examples/docker_no_executor_framework.cpp PRE-CREATION
> src/health-check/main.cpp 707810a
> src/launcher/executor.cpp 9c80848
> src/linux/cgroups.hpp decad9d
> src/linux/cgroups.cpp 6a73dd7
> src/master/master.cpp 251b699
> src/slave/containerizer/containerizer.cpp 1b71f33
> src/slave/containerizer/docker.hpp PRE-CREATION
> src/slave/containerizer/docker.cpp PRE-CREATION
> src/slave/containerizer/external_containerizer.cpp 3f28d85
> src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
> src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
> src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
> src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
> src/slave/containerizer/isolators/posix.hpp 17bbd10
> src/slave/flags.hpp 1fe7b7d
> src/slave/slave.cpp f42ab60
> src/tests/cgroups_tests.cpp 01cf498
> src/tests/docker_containerizer_tests.cpp PRE-CREATION
> src/tests/docker_tests.cpp PRE-CREATION
> src/tests/environment.cpp 434b3f7
> src/tests/flags.hpp a003e7f
> src/tests/script.cpp 15a6542
> src/usage/usage.hpp 5a76746
> src/usage/usage.cpp 29014d1
>
> Diff: https://reviews.apache.org/r/23771/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23771: Added a Docker containerizer.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23771/
-----------------------------------------------------------
(Updated July 28, 2014, 5:41 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu.
Changes
-------
Addressed review comments
Repository: mesos-git
Description
-------
Docker implementation.
This is all the docker code Ben, I and Yifan worked on excluding the composing containerizer patches.
Diffs (updated)
-----
src/Makefile.am 45afcd1
src/common/status_utils.hpp 1980551
src/docker/docker.hpp PRE-CREATION
src/docker/docker.cpp PRE-CREATION
src/examples/docker_no_executor_framework.cpp PRE-CREATION
src/health-check/main.cpp 707810a
src/launcher/executor.cpp 9c80848
src/linux/cgroups.hpp decad9d
src/linux/cgroups.cpp 6a73dd7
src/master/master.cpp 251b699
src/slave/containerizer/containerizer.cpp 1b71f33
src/slave/containerizer/docker.hpp PRE-CREATION
src/slave/containerizer/docker.cpp PRE-CREATION
src/slave/containerizer/external_containerizer.cpp 3f28d85
src/slave/containerizer/isolators/cgroups/cpushare.hpp 780037b
src/slave/containerizer/isolators/cgroups/cpushare.cpp 3265a80
src/slave/containerizer/isolators/cgroups/mem.hpp 8c476c7
src/slave/containerizer/isolators/cgroups/mem.cpp e8d1e35
src/slave/containerizer/isolators/posix.hpp 17bbd10
src/slave/flags.hpp 1fe7b7d
src/slave/slave.cpp f42ab60
src/tests/cgroups_tests.cpp 01cf498
src/tests/docker_containerizer_tests.cpp PRE-CREATION
src/tests/docker_tests.cpp PRE-CREATION
src/tests/environment.cpp 434b3f7
src/tests/flags.hpp a003e7f
src/tests/script.cpp 15a6542
src/usage/usage.hpp 5a76746
src/usage/usage.cpp 29014d1
Diff: https://reviews.apache.org/r/23771/diff/
Testing
-------
make check
Thanks,
Timothy Chen