mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 29889: Recover Docker containers when mesos slave is in a container
Date Tue, 28 Apr 2015 22:11:09 GMT

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



Dockerfile
<https://reviews.apache.org/r/29889/#comment132377>

    It seems that docker.io could be on its own line, since it is a kinda separate topic,
which seems to be the grouping criterium here?



docs/configuration.md
<https://reviews.apache.org/r/29889/#comment132378>

    s/CLI/Docker client CLI/



docs/configuration.md
<https://reviews.apache.org/r/29889/#comment132385>

    Suggestion how to phrase all this:
    
    The UNIX socket path to be mounted into the docker executor container to provide docker
CLI access to the docker daemon. This must be the path used by the slave's docker image.



docs/configuration.md
<https://reviews.apache.org/r/29889/#comment132387>

    s/a/an



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132389>

    s/!isSome/isNone



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132390>

    s/> >/>>



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132391>

    s/contianer/container



src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment132394>

    Unfortunately, checkError() and _run() are so similar, but so differently named. Maybe
a comment explaining that error checking is different when not detached in that stderr does
not get read?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132400>

    Suggestion to replace the last sentence with: 
    
    The executor is set up in a way that ensures its continued execution if the slave exits.
If the slave is running in a docker container this is inherently the case. Otherwise the executor
creates a new process group for itself.



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132402>

    Shouldn't we leave a trace of what happened in the output?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132403>

    Given rephrasing the class comment above, this comment may get removed.



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132406>

    Do we really need this variable? Could we just use task.container()?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132410>

    Please comment why we are adding the executor resources here: for reporting total usage
of task + executor instead of just task. However, isn't it the case that we are now actually
using "task.resources() + executor.resources() + executor.resources()", since the xecutor
is already running?



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132412>

    Out of the 3 vars "launched", "dockerRun", "killed" you only need 2.



src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment132413>

    This is also virtual in the CommandExecutor, but why?


- Bernd Mathiske


On April 22, 2015, 3:50 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 3:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This is a one mega patch and contains many reviews that's already on rb.
> 
> This review is not meant to be merged, only provided for easier review.
> 
> 
> Diffs
> -----
> 
>   Dockerfile 35abf25 
>   docs/configuration.md 54c4e31 
>   docs/docker-containerizer.md a5438b7 
>   src/Makefile.am 93c7c8a 
>   src/docker/docker.hpp 3ebbc1f 
>   src/docker/docker.cpp 3a485a2 
>   src/docker/executor.cpp PRE-CREATION 
>   src/slave/containerizer/docker.hpp 0d5289c 
>   src/slave/containerizer/docker.cpp f9fb078 
>   src/slave/flags.hpp d3b1ce1 
>   src/slave/flags.cpp d0932b0 
>   src/tests/docker_containerizer_tests.cpp b119a17 
> 
> Diff: https://reviews.apache.org/r/29889/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message