You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/10/16 12:11:09 UTC
Review Request 39388: Explicitly set the `LIBPROCESS_IP` env variable
for docker containers.
-----------------------------------------------------------
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.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > 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.
Posted by haosdent huang <ha...@gmail.com>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > 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!
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
-----------------------------------------------------------
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.
Posted by haosdent huang <ha...@gmail.com>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > 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.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > 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<pid_t> 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(&setup, 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.
Posted by Klaus Ma <kl...@cguru.net>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > Seems we already have this env in https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
I think those environment are used for `docker run` instead of passing them to the container.
- Klaus
-----------------------------------------------------------
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.
Posted by haosdent huang <ha...@gmail.com>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > 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<pid_t> 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(&setup, 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.
Posted by haosdent huang <ha...@gmail.com>.
> On Oct. 18, 2015, 4:10 a.m., haosdent huang wrote:
> > src/docker/docker.cpp, line 430
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line430>
> >
> > 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<pid_t> 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(&setup, 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.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39388/#review103050
-----------------------------------------------------------
src/docker/docker.cpp (line 430)
<https://reviews.apache.org/r/39388/#comment160886>
Seems we already have this env in https://github.com/apache/mesos/blob/master/src/slave/containerizer/containerizer.cpp#L254
- haosdent huang
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.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 19, 2015, 11:22 p.m., Niklas Nielsen wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 504
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100524#file1100524line504>
> >
> > 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
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100524#file1100524line509>
> >
> > 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
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100524#file1100524line518>
> >
> > 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.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/39388/#comment161157>
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)
<https://reviews.apache.org/r/39388/#comment161155>
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)
<https://reviews.apache.org/r/39388/#comment161156>
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.
Posted by Cody Maloney <co...@mesosphere.io>.
> On Nov. 2, 2015, 5:43 a.m., Timothy Chen wrote:
> > src/docker/docker.cpp, line 433
> > <https://reviews.apache.org/r/39388/diff/2/?file=1100523#file1100523line433>
> >
> > 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.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39388/#review104679
-----------------------------------------------------------
src/docker/docker.cpp (line 433)
<https://reviews.apache.org/r/39388/#comment162930>
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.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Klaus Ma <kl...@cguru.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39388/#review103055
-----------------------------------------------------------
Ship it!
Ship It!
- Klaus Ma
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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39388/#review103045
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39388]
All tests passed.
- Mesos ReviewBot
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.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Niklas Nielsen <ni...@qni.dk>.
> 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
>
>
Re: Review Request 39388: Explicitly set the `LIBPROCESS_IP` env
variable for docker containers.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39388/#review102956
-----------------------------------------------------------
src/docker/docker.cpp (line 427)
<https://reviews.apache.org/r/39388/#comment160790>
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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
>
>