Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Greg Mann]

> On Oct. 13, 2015, 6:21 p.m., Niklas Nielsen wrote:
> > src/slave/containerizer/containerizer.cpp, line 256
> > 
> >
> > Shouldn't we only set this if it is not present?

Since it gets set before the `foreach()` loop below, if `LIBPROCESS_IP` is 
present in the passed flags, the passed value will overwrite the value that we 
set here.


- Greg


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


On Oct. 9, 2015, 8:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 8:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Niklas Nielsen]

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



src/slave/containerizer/containerizer.cpp (line 256)


Shouldn't we only set this if it is not present?


- Niklas Nielsen


On Oct. 9, 2015, 1:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 1:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39152]

All tests passed.

- Mesos ReviewBot


On Oct. 9, 2015, 8:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 8:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Adam B]

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

Ship it!


Thanks! I'll fix the indent and commit this.

Also, you did more than just run `make check`, you modified 
MesosContainerizerIsolatorPreparationTest.ExecutorEnvironmentVariable to test 
your env-variable setting. Call that out in the "Testing Done" section.


src/tests/containerizer/mesos_containerizer_tests.cpp (lines 334 - 336)


We only indent 2 spaces after a trailing '='.
Try a `grep -rn -B1 "^[^a-zA-Z0-9]*\".*\"$" src/ |grep -A2 "=$"` and you'll 
see.


- Adam B


On Oct. 9, 2015, 1:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 1:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Anand Mazumdar]

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

Ship it!


LGTM


src/tests/containerizer/mesos_containerizer_tests.cpp (line 366)


Not a huge fan of this hackery but this approach is also used by other 
similar tests. I wonder if we should create a separate test fixture for this 
test/such tests in general ?

Also , wondering what do other projects do or what is the recommended way ?


- Anand Mazumdar


On Oct. 9, 2015, 8:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 8:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Greg Mann]

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

(Updated Oct. 9, 2015, 8:33 p.m.)


Review request for mesos and Kapil Arya.


Changes
---

Changed conditional in test.


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


Repository: mesos


Description
---

If DNS is not available on the agent node and a task is launched which 
explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
passed through and the default hostname lookup after spawning the executor 
process will throw an error. This patch alters the agent to always pass 
LIBPROCESS_IP, even when the executor environment is specified.


Diffs (updated)
-

  src/slave/containerizer/containerizer.cpp 
25c87e9f948b7efe8b9a853c403bee69982d6c4c 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
5bc7d408bda0c249e1b66747d8bd87e688362e6c 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Kapil Arya]

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

Ship it!


Ship It!

- Kapil Arya


On Oct. 8, 2015, 8:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 8, 2015, 8:57 p.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Cody Maloney]

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


LGTM!

- Cody Maloney


On Oct. 9, 2015, 12:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 12:57 a.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39152]

All tests passed.

- Mesos ReviewBot


On Oct. 9, 2015, 12:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 12:57 a.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Greg Mann]

> On Oct. 9, 2015, 12:46 a.m., Neil Conway wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 307
> > 
> >
> > Modifying the environment doesn't seem like a great thing for a test to 
> > be doing, since it might impact other tests -- we should at least reset 
> > LIBPROCESS_IP to its previous value at the end of the test. That still 
> > isn't great (e.g., if we want to run multiple tests concurrently in the 
> > same OS process), but I'm not sure there's a better way.

Good call, thanks Neil!


- Greg


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


On Oct. 9, 2015, 12:57 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 12:57 a.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Greg Mann]

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

(Updated Oct. 9, 2015, 12:57 a.m.)


Review request for mesos and Kapil Arya.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

If DNS is not available on the agent node and a task is launched which 
explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
passed through and the default hostname lookup after spawning the executor 
process will throw an error. This patch alters the agent to always pass 
LIBPROCESS_IP, even when the executor environment is specified.


Diffs (updated)
-

  src/slave/containerizer/containerizer.cpp 
25c87e9f948b7efe8b9a853c403bee69982d6c4c 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
5bc7d408bda0c249e1b66747d8bd87e688362e6c 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Neil Conway]

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



src/tests/containerizer/mesos_containerizer_tests.cpp (line 307)


Modifying the environment doesn't seem like a great thing for a test to be 
doing, since it might impact other tests -- we should at least reset 
LIBPROCESS_IP to its previous value at the end of the test. That still isn't 
great (e.g., if we want to run multiple tests concurrently in the same OS 
process), but I'm not sure there's a better way.


- Neil Conway


On Oct. 9, 2015, 12:37 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39152/
> ---
> 
> (Updated Oct. 9, 2015, 12:37 a.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-3553
> https://issues.apache.org/jira/browse/MESOS-3553
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If DNS is not available on the agent node and a task is launched which 
> explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
> passed through and the default hostname lookup after spawning the executor 
> process will throw an error. This patch alters the agent to always pass 
> LIBPROCESS_IP, even when the executor environment is specified.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> 25c87e9f948b7efe8b9a853c403bee69982d6c4c 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 5bc7d408bda0c249e1b66747d8bd87e688362e6c 
> 
> Diff: https://reviews.apache.org/r/39152/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 39152: Pass LIBPROCESS_IP even when executor environment is specified.

[Greg Mann]

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

Review request for mesos and Kapil Arya.


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


Repository: mesos


Description
---

If DNS is not available on the agent node and a task is launched which 
explicitly specifies the executor's environment, LIBPROCESS_IP will not be 
passed through and the default hostname lookup after spawning the executor 
process will throw an error. This patch alters the agent to always pass 
LIBPROCESS_IP, even when the executor environment is specified.


Diffs
-

  src/slave/containerizer/containerizer.cpp 
25c87e9f948b7efe8b9a853c403bee69982d6c4c 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
5bc7d408bda0c249e1b66747d8bd87e688362e6c 

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


Testing
---

`make check`


Thanks,

Greg Mann