This is an automatically generated e-mail. To reply, visit:

src/docker/docker.hpp (line 45)

    This doesn't need to be a public static method since it's only used in 
    Matter of fact just inline the prefixing in the create method, no need for 
a small prefix method.

src/docker/docker.hpp (line 161)

    Alignment is off:
     Docker(const std::string& _path,
            const std::string& _socket)
          : path(_path), socket(_socket) {};

src/docker/docker.hpp (line 242)

    Get rid of these if you follow my comment.

src/docker/docker.cpp (line 57)

    You can get rid of all these if you follow my comment below.

src/docker/docker.cpp (line 102)

    So if you look at the definition of docker_socket, it's explicitly saying 
we need the file path to the docker_socket (e.g: /var/run/docker.sock)
    I think we shouldn't even expect users passing in tcp:// or 
unix:///var/run/docker.sock as well.
    What I would do instead here, is actually check the socket path is valid by 
just making sure it's a absolute path: 
    if (!strings::startsWith(socket, "/")) {
      return Error("Invalid Docker socket path: " + socket);
    And then just prefix unix:// before passing it to Docker constructor.

src/docker/docker.cpp (line 129)

    Fix the style:
    Try<Docker*> Docker::create(
        const string& path,
        const string& socket,
        bool validate)

- Timothy Chen

On Aug. 26, 2015, 2:15 a.m., Vaibhav Khanduja wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37114/
> -----------------------------------------------------------
> (Updated Aug. 26, 2015, 2:15 a.m.)
> Review request for mesos, Benjamin Hindman, Ben Mahler, Timothy Chen, and 
> Vinod Kone.
> Bugs: MESOS-3187
>     https://issues.apache.org/jira/browse/MESOS-3187
> Repository: mesos
> Description
> -------
> MESOS-3187, support docker host command line option.
> Docker daemon supports starting on a non-default port. Such scenarios would 
> needed when starting Docker daemon on TCP or non-default unix port. Mesos 
> slave does not work if Docker daemon is started on any of such non-default 
> port. The code change is needed in Mesos slave to accept this parameter so as 
> connect for its operations to the right Docker daemon. The change is made in 
> Mesos slave, so as it is available to any framework making using Docker 
> executor. 
> The code is added to start slave binary with --docker_host, instructing it to 
> connect on port as specified in the parameter. The default value of 
> --default_host is "unix:///var/run/docker.sock, which is default port for 
> Docker daemon.
> The main class src/docker.cpp/.hpp is kept backward compartible to make 
> Docker cli execute on default Docker port.
> Diffs
> -----
>   src/docker/docker.hpp 38e5299ad38b9e20501387f2193b0fa448e49e3e 
>   src/docker/docker.cpp 1367de8a7bbbda6348a30e4ef4c616378e450250 
>   src/docker/executor.hpp fa13b6e9905051eef27d3a51b75a5c86fdad0dd7 
>   src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
>   src/slave/containerizer/docker.cpp 8f5d302477b216df9ac2f59156304bbc4a96f24b 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 80ed60e2b0fa39e8302867a7cb6a7388c25f9a40 
>   src/tests/containerizer/docker_tests.cpp 
> a4a2725c05ae0cb88426c587f7ded0da77154edc 
>   src/tests/environment.cpp 525347090f38b61f2085a2b2a6002d28d11b222f 
>   src/tests/flags.hpp 364495695c5915e54257014aeebb1e212d3da6fc 
> Diff: https://reviews.apache.org/r/37114/diff/
> Testing
> -------
> Following scenarios were executed to test the code changes. Kindly suggest if 
> more test-cases are required:
> a) Mesos slave with unix port : unix:///var/run/docker_myport.sock
>    i) Start slave with --docker_host parameter 
> "unix:///var/run/docker_myport.sock
>    ii) Using a framework, in my case Marathon, post a Docker job
>    iii) The docker job does get started on the slave, confirmed with docker 
> ps command output 
> docker -H unix:///var/run/docker_myport.sock ps
> CONTAINER ID        IMAGE               COMMAND                CREATED        
>      STATUS              PORTS               NAMES
> 07fc4ec86bac        mygoserver          "/bin/sh -c /mygoser   19 minutes ago 
>      Up 19 minutes       */tcp, */udp        
> mesos-20150731-104052-1051068938-5050-7913-S33.17b355cd-2754-4fb2-a558-66820dff033c
>     iv) Stop or destroy the job from Marathon GUI
> b) Two mesos slave with non-default docker port
>     i) On two different hosts, start slave, with one running on default port 
> and other non-default. The start slaves with attributes - default and or 
> non-default.
>     ii) Give jobs to these slaves, using Marathon UNIQUE attribute, selecting 
> slave - non-default & default
>     iii) Stop/destroy the jobs
> d) Modified unit test-case taking docker port value - make check
> Thanks,
> Vaibhav Khanduja

Reply via email to