You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/08/16 01:33:04 UTC
Review Request 24754: Introduced "states" for Docker containers to
transition between.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24754/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Timothy Chen.
Repository: mesos-git
Description
-------
The DockerContainerier needs to be able to properly clean up Docker
containers, regardless of when they are destroyed. For example, if a
container gets destroyed while we are fetching, we need to not keep
running the fetch, nor should we try and start the Docker
container. For this reason, we've split out the states into:
FETCHING
PULLING
RUNNING
DESTROYING
In particular, we made 'PULLING' be it's own state so that we could
easily destroy and cleanup when a user initiated pulling a really big
image but we timeout due to the executor registration timeout. Since
we curently have no way to discard a Docker::run, we needed to
explicitely do the pull (which is the part that takes the longest) so
that we can assume we won't have to wait very long for Docker::run to
complete.
Diffs
-----
src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81
src/docker/docker.cpp 8f04babb9cd06ff5b2a39033664326d7d44cd6c6
src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2
src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280
src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
Diff: https://reviews.apache.org/r/24754/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 24754: Introduced "states" for Docker containers to
transition between.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24754/#review50806
-----------------------------------------------------------
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88675>
kill the extra empty line here.
- Jie Yu
On Aug. 15, 2014, 11:59 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24754/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 11:59 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The DockerContainerier needs to be able to properly clean up Docker
> containers, regardless of when they are destroyed. For example, if a
> container gets destroyed while we are fetching, we need to not keep
> running the fetch, nor should we try and start the Docker
> container. For this reason, we've split out the states into:
>
> FETCHING
> PULLING
> RUNNING
> DESTROYING
>
> In particular, we made 'PULLING' be it's own state so that we could
> easily destroy and cleanup when a user initiated pulling a really big
> image but we timeout due to the executor registration timeout. Since
> we curently have no way to discard a Docker::run, we needed to
> explicitely do the pull (which is the part that takes the longest) so
> that we can assume we won't have to wait very long for Docker::run to
> complete.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81
> src/docker/docker.cpp ebdf22684cb6619a1b603546c2aa25ca4b90afc5
> src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2
> src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 24754: Introduced "states" for Docker containers to
transition between.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24754/#review50802
-----------------------------------------------------------
Ship it!
This is looking really good! Much cleaner!
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88669>
Put 'pull' related function after 'fetch' related function since we fetch first then pull. Same for the implementation.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88667>
'image' is from user, so we need to be careful here. What if the user do:
```
image = '|| rm -rf *'
```
So we need to use the argv version here.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88668>
CHECK_NE(0, status.get());
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88671>
Any reason keeps this temp variable?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88673>
hum, you've transfer the ownership here to the temp variable 'container'. In fact, why keeping a temp variable here?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/24754/#comment88674>
Ditto. Probably you should just store a plain pointer and explicitly delete it later.
- Jie Yu
On Aug. 15, 2014, 11:59 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24754/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2014, 11:59 p.m.)
>
>
> Review request for mesos, Jie Yu and Timothy Chen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The DockerContainerier needs to be able to properly clean up Docker
> containers, regardless of when they are destroyed. For example, if a
> container gets destroyed while we are fetching, we need to not keep
> running the fetch, nor should we try and start the Docker
> container. For this reason, we've split out the states into:
>
> FETCHING
> PULLING
> RUNNING
> DESTROYING
>
> In particular, we made 'PULLING' be it's own state so that we could
> easily destroy and cleanup when a user initiated pulling a really big
> image but we timeout due to the executor registration timeout. Since
> we curently have no way to discard a Docker::run, we needed to
> explicitely do the pull (which is the part that takes the longest) so
> that we can assume we won't have to wait very long for Docker::run to
> complete.
>
>
> Diffs
> -----
>
> src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81
> src/docker/docker.cpp ebdf22684cb6619a1b603546c2aa25ca4b90afc5
> src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2
> src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280
> src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
>
> Diff: https://reviews.apache.org/r/24754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 24754: Introduced "states" for Docker containers to
transition between.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24754/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 11:59 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
The DockerContainerier needs to be able to properly clean up Docker
containers, regardless of when they are destroyed. For example, if a
container gets destroyed while we are fetching, we need to not keep
running the fetch, nor should we try and start the Docker
container. For this reason, we've split out the states into:
FETCHING
PULLING
RUNNING
DESTROYING
In particular, we made 'PULLING' be it's own state so that we could
easily destroy and cleanup when a user initiated pulling a really big
image but we timeout due to the executor registration timeout. Since
we curently have no way to discard a Docker::run, we needed to
explicitely do the pull (which is the part that takes the longest) so
that we can assume we won't have to wait very long for Docker::run to
complete.
Diffs (updated)
-----
src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81
src/docker/docker.cpp ebdf22684cb6619a1b603546c2aa25ca4b90afc5
src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2
src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280
src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
Diff: https://reviews.apache.org/r/24754/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 24754: Introduced "states" for Docker containers to
transition between.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24754/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 11:55 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Repository: mesos-git
Description
-------
The DockerContainerier needs to be able to properly clean up Docker
containers, regardless of when they are destroyed. For example, if a
container gets destroyed while we are fetching, we need to not keep
running the fetch, nor should we try and start the Docker
container. For this reason, we've split out the states into:
FETCHING
PULLING
RUNNING
DESTROYING
In particular, we made 'PULLING' be it's own state so that we could
easily destroy and cleanup when a user initiated pulling a really big
image but we timeout due to the executor registration timeout. Since
we curently have no way to discard a Docker::run, we needed to
explicitely do the pull (which is the part that takes the longest) so
that we can assume we won't have to wait very long for Docker::run to
complete.
Diffs (updated)
-----
src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81
src/docker/docker.cpp 8f04babb9cd06ff5b2a39033664326d7d44cd6c6
src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2
src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280
src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
Diff: https://reviews.apache.org/r/24754/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 24754: Introduced "states" for Docker containers to
transition between.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24754/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 11:43 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Repository: mesos-git
Description
-------
The DockerContainerier needs to be able to properly clean up Docker
containers, regardless of when they are destroyed. For example, if a
container gets destroyed while we are fetching, we need to not keep
running the fetch, nor should we try and start the Docker
container. For this reason, we've split out the states into:
FETCHING
PULLING
RUNNING
DESTROYING
In particular, we made 'PULLING' be it's own state so that we could
easily destroy and cleanup when a user initiated pulling a really big
image but we timeout due to the executor registration timeout. Since
we curently have no way to discard a Docker::run, we needed to
explicitely do the pull (which is the part that takes the longest) so
that we can assume we won't have to wait very long for Docker::run to
complete.
Diffs (updated)
-----
src/docker/docker.hpp 3270c910d8b8d87aaf838f8494ee90ed93322a81
src/docker/docker.cpp 8f04babb9cd06ff5b2a39033664326d7d44cd6c6
src/slave/containerizer/docker.cpp d5292b609a9348e36d1327c9719f347bba84efb2
src/slave/slave.cpp 1eaab04137835d348bac881854d8c81a5f5dd280
src/tests/docker_containerizer_tests.cpp e0fd62f83387635f503817ced7a592cc3ae6e775
Diff: https://reviews.apache.org/r/24754/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman