Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Cody Maloney]

> On Nov. 2, 2015, 5:43 a.m., Timothy Chen wrote:
> > src/docker/docker.cpp, line 433
> > 
> >
> > I think there are two problems here to address:
> > - --net is not always set to host, and if it's not it's causing problem 
> > as LIBPROCESS_IP takes precedence and also causes framework to not be able 
> > to connect
> > - docker code here in src/docker/docker.cpp is meant to be an 
> > abstraction to run docker, which we use to run executors and docker tasks. 
> > It shouldn't really have logic like this embedded especialy when this is 
> > not the desire effect for all docker containers we ever launch from Mesos. 
> > And really this problem and setting has to be two cases 1) We run a 
> > executor in the docker container 2) --net is host. We need to specifically 
> > solve this outside of the docker abstraction.

libprocess processes running with non-host networking (--net=bridge) won't work 
as libprocess processes doesn't like crossing NAT boundaries (which bridge 
networking is).

That we set it to the host IP might actually make it work better than if we 
don't (The libprocess inside the docker container announces the public host IP, 
which makes it so if it is talking to libprocess processes on other remote 
hosts, the remote hosts will get the IP which actually works to connect back, 
rather than the docker internal IP which doesn't. If you're running a bunch of 
things on the same host and connecting the docker networks together then that 
wouldn't work. The way libprocess works I think making both those work at once 
isn't feasible, and the --net=host working at all is generally preferrable (We 
launch mesos frameworks inside of docker containers, they need to talk to the 
Mesos Master using libmesos)


- Cody


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Timothy Chen]

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



src/docker/docker.cpp (line 433)


I think there are two problems here to address:
- --net is not always set to host, and if it's not it's causing problem as 
LIBPROCESS_IP takes precedence and also causes framework to not be able to 
connect
- docker code here in src/docker/docker.cpp is meant to be an abstraction 
to run docker, which we use to run executors and docker tasks. It shouldn't 
really have logic like this embedded especialy when this is not the desire 
effect for all docker containers we ever launch from Mesos. And really this 
problem and setting has to be two cases 1) We run a executor in the docker 
container 2) --net is host. We need to specifically solve this outside of the 
docker abstraction.


- Timothy Chen


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Timothy Chen]

> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > 
> >
> > Seems we already have this env in 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
> 
> Klaus Ma wrote:
> I think those environment are used for `docker run` instead of passing 
> them to the container.
> 
> Michael Park wrote:
> That one is used to pass the `LIBPROCES_IP` to the `subprocess` call when 
> we're launching a process, whereas this is used to pass the `LIBPROCESS_IP` 
> to `docker->run` when we're launching a container via `-e`.
> 
> haosdent huang wrote:
> Thank you very much for explanation, got it!
> 
> haosdent huang wrote:
> Doe we change the line in docker executor could solve this problem?
> ```
> diff --git a/src/docker/executor.cpp b/src/docker/executor.cpp
> index 1e49013..a125078 100644
> --- a/src/docker/executor.cpp
> +++ b/src/docker/executor.cpp
> @@ -147,7 +147,7 @@ public:
>  sandboxDirectory,
>  mappedDirectory,
>  task.resources() + task.executor().resources(),
> -None(),
> +os::environment(),
>  path::join(sandboxDirectory, "stdout"),
>  path::join(sandboxDirectory, "stderr"))
>.onAny(defer(
> ```
> 
> haosdent huang wrote:
> Or how about serialize the environment variables to docker flag and pass 
> it to `docker->run` if `os::environment` noisy?
> ```
> diff --git a/src/slave/containerizer/docker.cpp 
> b/src/slave/containerizer/docker.cpp
> index 7022958..d9b23d2 100644
> --- a/src/slave/containerizer/docker.cpp
> +++ b/src/slave/containerizer/docker.cpp
> @@ -911,7 +911,7 @@ Future 
> DockerContainerizerProcess::launchExecutorProcess(
>Subprocess::PIPE(),
>Subprocess::PATH(path::join(container->directory, "stdout")),
>Subprocess::PATH(path::join(container->directory, "stderr")),
> -  dockerFlags(flags, container->name(), container->directory),
> +  dockerFlags(flags, container->name(), container->directory, 
> environment),
>environment,
>lambda::bind(, container->directory));
> ```

We should just solve the cases we know required and avoid passing environment 
variables if don't need to. We have found various problems passing down env 
variables and doing os::environment or environment will cause more pain in the 
future.


- Timothy


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[haosdent huang]

> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > 
> >
> > Seems we already have this env in 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
> 
> Klaus Ma wrote:
> I think those environment are used for `docker run` instead of passing 
> them to the container.
> 
> Michael Park wrote:
> That one is used to pass the `LIBPROCES_IP` to the `subprocess` call when 
> we're launching a process, whereas this is used to pass the `LIBPROCESS_IP` 
> to `docker->run` when we're launching a container via `-e`.
> 
> haosdent huang wrote:
> Thank you very much for explanation, got it!
> 
> haosdent huang wrote:
> Doe we change the line in docker executor could solve this problem?
> ```
> diff --git a/src/docker/executor.cpp b/src/docker/executor.cpp
> index 1e49013..a125078 100644
> --- a/src/docker/executor.cpp
> +++ b/src/docker/executor.cpp
> @@ -147,7 +147,7 @@ public:
>  sandboxDirectory,
>  mappedDirectory,
>  task.resources() + task.executor().resources(),
> -None(),
> +os::environment(),
>  path::join(sandboxDirectory, "stdout"),
>  path::join(sandboxDirectory, "stderr"))
>.onAny(defer(
> ```
> 
> haosdent huang wrote:
> Or how about serialize the environment variables to docker flag and pass 
> it to `docker->run` if `os::environment` noisy?
> ```
> diff --git a/src/slave/containerizer/docker.cpp 
> b/src/slave/containerizer/docker.cpp
> index 7022958..d9b23d2 100644
> --- a/src/slave/containerizer/docker.cpp
> +++ b/src/slave/containerizer/docker.cpp
> @@ -911,7 +911,7 @@ Future 
> DockerContainerizerProcess::launchExecutorProcess(
>Subprocess::PIPE(),
>Subprocess::PATH(path::join(container->directory, "stdout")),
>Subprocess::PATH(path::join(container->directory, "stderr")),
> -  dockerFlags(flags, container->name(), container->directory),
> +  dockerFlags(flags, container->name(), container->directory, 
> environment),
>environment,
>lambda::bind(, container->directory));
> ```
> 
> Timothy Chen wrote:
> We should just solve the cases we know required and avoid passing 
> environment variables if don't need to. We have found various problems 
> passing down env variables and doing os::environment or environment will 
> cause more pain in the future.

Got it. Thank you very much.


- haosdent


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[haosdent huang]

> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > 
> >
> > Seems we already have this env in 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
> 
> Klaus Ma wrote:
> I think those environment are used for `docker run` instead of passing 
> them to the container.
> 
> Michael Park wrote:
> That one is used to pass the `LIBPROCES_IP` to the `subprocess` call when 
> we're launching a process, whereas this is used to pass the `LIBPROCESS_IP` 
> to `docker->run` when we're launching a container via `-e`.
> 
> haosdent huang wrote:
> Thank you very much for explanation, got it!
> 
> haosdent huang wrote:
> Doe we change the line in docker executor could solve this problem?
> ```
> diff --git a/src/docker/executor.cpp b/src/docker/executor.cpp
> index 1e49013..a125078 100644
> --- a/src/docker/executor.cpp
> +++ b/src/docker/executor.cpp
> @@ -147,7 +147,7 @@ public:
>  sandboxDirectory,
>  mappedDirectory,
>  task.resources() + task.executor().resources(),
> -None(),
> +os::environment(),
>  path::join(sandboxDirectory, "stdout"),
>  path::join(sandboxDirectory, "stderr"))
>.onAny(defer(
> ```

Or how about serialize the environment variables to docker flag and pass it to 
`docker->run` if `os::environment` noisy?
```
diff --git a/src/slave/containerizer/docker.cpp 
b/src/slave/containerizer/docker.cpp
index 7022958..d9b23d2 100644
--- a/src/slave/containerizer/docker.cpp
+++ b/src/slave/containerizer/docker.cpp
@@ -911,7 +911,7 @@ Future 
DockerContainerizerProcess::launchExecutorProcess(
   Subprocess::PIPE(),
   Subprocess::PATH(path::join(container->directory, "stdout")),
   Subprocess::PATH(path::join(container->directory, "stderr")),
-  dockerFlags(flags, container->name(), container->directory),
+  dockerFlags(flags, container->name(), container->directory, environment),
   environment,
   lambda::bind(, container->directory));
```


- haosdent


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Niklas Nielsen]

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

Ship it!


See comments below :)


src/tests/containerizer/docker_containerizer_tests.cpp (line 504)


Mind adding a comment about the test sequence? Why does the 'test 
$LIBPROCESS_IP = \"foobar"\' work, etc.



src/tests/containerizer/docker_containerizer_tests.cpp (lines 509 - 510)


No biggie; but could we have done the creation of the mock docker inline in 
line 512? The first thing that came to my mind was that we were leaking the 
object.



src/tests/containerizer/docker_containerizer_tests.cpp (line 518)


Can we make this assumption (do we have other places where we assume 
localhost is always 127.0.0.1)?


- Niklas Nielsen


On Oct. 17, 2015, 4:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 4:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Michael Park]

> On Oct. 19, 2015, 11:22 p.m., Niklas Nielsen wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 504
> > 
> >
> > Mind adding a comment about the test sequence? Why does the 'test 
> > $LIBPROCESS_IP = \"foobar"\' work, etc.

Yep, will do.


> On Oct. 19, 2015, 11:22 p.m., Niklas Nielsen wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, lines 509-510
> > 
> >
> > No biggie; but could we have done the creation of the mock docker 
> > inline in line 512? The first thing that came to my mind was that we were 
> > leaking the object.

Probably, I'll look into it a bit more. I followed the pattern of existing 
tests so I'll perhaps update those correspondingly.


> On Oct. 19, 2015, 11:22 p.m., Niklas Nielsen wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 518
> > 
> >
> > Can we make this assumption (do we have other places where we assume 
> > localhost is always 127.0.0.1)?

The choice of `127.0.0.1` is arbitrary here. We could've assigned it to `x`, 
and performed `test $LIBPROCESS_IP = x` instead.
Actually, I'll pull it out to a variable and leave a comment in line with what 
I described here.


- Michael


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十月 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated 十月 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Michael Park]

> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > 
> >
> > Seems we already have this env in 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
> 
> Klaus Ma wrote:
> I think those environment are used for `docker run` instead of passing 
> them to the container.

That one is used to pass the `LIBPROCES_IP` to the `subprocess` call when we're 
launching a process, whereas this is used to pass the `LIBPROCESS_IP` to 
`docker->run` when we're launching a container via `-e`.


- Michael


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[haosdent huang]

> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > 
> >
> > Seems we already have this env in 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
> 
> Klaus Ma wrote:
> I think those environment are used for `docker run` instead of passing 
> them to the container.
> 
> Michael Park wrote:
> That one is used to pass the `LIBPROCES_IP` to the `subprocess` call when 
> we're launching a process, whereas this is used to pass the `LIBPROCESS_IP` 
> to `docker->run` when we're launching a container via `-e`.

Thank you very much for explanation, got it!


- haosdent


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


On Oct. 17, 2015, 11:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 17, 2015, 11:18 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 4bb65afd0ee61cafef68e064a697fdce65d60058 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
> fails without the changes made to `src/docker/docker.cpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Michael Park]

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

(Updated Oct. 17, 2015, 11:18 p.m.)


Review request for mesos and Niklas Nielsen.


Changes
---

Added a test, expanded upon the comment


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
  src/tests/containerizer/docker_containerizer_tests.cpp 
4bb65afd0ee61cafef68e064a697fdce65d60058 

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


Testing (updated)
---

Added `DockerContainerizerTest.ROOT_DOCKER_LaunchWithLibprocessIP` test which 
fails without the changes made to `src/docker/docker.cpp`.


Thanks,

Michael Park

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Niklas Nielsen]

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



src/docker/docker.cpp (line 427)


Maybe be a bit more explicit: "... in cases where LIBPROCESS_IP has been 
set explicitly in host environment. Launching executors within docker 
containers in cases where libprocess cannot resolve the IP to bind to, will 
otherwise fail."


- Niklas Nielsen


On Oct. 16, 2015, 3:11 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 16, 2015, 3:11 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Niklas Nielsen]

> On Oct. 16, 2015, 11:18 a.m., Niklas Nielsen wrote:
> >

Also, let's get a test wired up to verify that this works :)


- Niklas


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


On Oct. 16, 2015, 3:11 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 16, 2015, 3:11 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Michael Park]

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

Review request for mesos and Niklas Nielsen.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 

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


Testing
---


Thanks,

Michael Park

Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable for docker containers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39388]

All tests passed.

- Mesos ReviewBot


On Oct. 16, 2015, 10:11 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39388/
> ---
> 
> (Updated Oct. 16, 2015, 10:11 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3740
> https://issues.apache.org/jira/browse/MESOS-3740
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 56d63dc75637c9f89a239af371f476a85a570696 
> 
> Diff: https://reviews.apache.org/r/39388/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>