Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Anand Mazumdar]

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


Fix it, then Ship it!




Minor comments. I would fix them while committing!


src/launcher/default_executor.cpp
Lines 379-381 (patched)


```cpp
// TODO(asridharan): This won't work when the framework sets the ...
```



src/launcher/default_executor.cpp
Lines 384 (patched)


s/here//



src/launcher/default_executor.cpp
Lines 385 (patched)


s/from/from the


- Anand Mazumdar


On June 15, 2017, 12:38 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 15, 2017, 12:38 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 91a4e1ebed3ff979bd3c0f7f46559cec24f61c2c 
> 
> 
> Diff: https://reviews.apache.org/r/59949/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

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

(Updated June 15, 2017, 12:38 a.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.


Diffs (updated)
-

  src/launcher/default_executor.cpp 91a4e1ebed3ff979bd3c0f7f46559cec24f61c2c 


Diff: https://reviews.apache.org/r/59949/diff/5/

Changes: https://reviews.apache.org/r/59949/diff/4-5/


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

> On June 13, 2017, 6:42 p.m., Jie Yu wrote:
> > src/launcher/default_executor.cpp
> > Lines 371 (patched)
> > 
> >
> > I'd add a TODO here. Because for bridge mode, `self().address.ip` might 
> > be the advertise IP (i.e., agent IP), which is not what we've wanted here.
> 
> Avinash sridharan wrote:
> From a CNI perspective the BRIDGE mode is just another CNI network, so 
> the self().address.ip will be the IP on the BRIDGE network which is what is 
> expected. So for the time being dropping this comment. Please open if you 
> still feel this is an issue.
> 
> Jie Yu wrote:
> i am just giving an example here. Any container that set 
> LIBPROCESS_ADVERTISE_IP env var will change `self().address.ip`. And this is 
> not the IP we want to set for `MESOS_CONTAINER_IP` env var.

Re-opening this issue based on Jie's comments above.


- Avinash


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


On June 14, 2017, 1:57 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 14, 2017, 1:57 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 91a4e1ebed3ff979bd3c0f7f46559cec24f61c2c 
> 
> 
> Diff: https://reviews.apache.org/r/59949/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Jie Yu]

> On June 13, 2017, 6:42 p.m., Jie Yu wrote:
> > src/launcher/default_executor.cpp
> > Lines 371 (patched)
> > 
> >
> > I'd add a TODO here. Because for bridge mode, `self().address.ip` might 
> > be the advertise IP (i.e., agent IP), which is not what we've wanted here.
> 
> Avinash sridharan wrote:
> From a CNI perspective the BRIDGE mode is just another CNI network, so 
> the self().address.ip will be the IP on the BRIDGE network which is what is 
> expected. So for the time being dropping this comment. Please open if you 
> still feel this is an issue.

i am just giving an example here. Any container that set 
LIBPROCESS_ADVERTISE_IP env var will change `self().address.ip`. And this is 
not the IP we want to set for `MESOS_CONTAINER_IP` env var.


- Jie


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


On June 14, 2017, 1:57 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 14, 2017, 1:57 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 91a4e1ebed3ff979bd3c0f7f46559cec24f61c2c 
> 
> 
> Diff: https://reviews.apache.org/r/59949/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

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

(Updated June 14, 2017, 1:57 p.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Addressed Jie's comments on adding a TODO.


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


Repository: mesos


Description
---

Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.


Diffs (updated)
-

  src/launcher/default_executor.cpp 91a4e1ebed3ff979bd3c0f7f46559cec24f61c2c 


Diff: https://reviews.apache.org/r/59949/diff/4/

Changes: https://reviews.apache.org/r/59949/diff/3-4/


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

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

(Updated June 14, 2017, 1:53 p.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Addressed Anand's comments.


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


Repository: mesos


Description
---

Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.


Diffs (updated)
-

  src/launcher/default_executor.cpp 91a4e1ebed3ff979bd3c0f7f46559cec24f61c2c 


Diff: https://reviews.apache.org/r/59949/diff/3/

Changes: https://reviews.apache.org/r/59949/diff/2-3/


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

> On June 13, 2017, 6:42 p.m., Jie Yu wrote:
> > src/launcher/default_executor.cpp
> > Lines 371 (patched)
> > 
> >
> > I'd add a TODO here. Because for bridge mode, `self().address.ip` might 
> > be the advertise IP (i.e., agent IP), which is not what we've wanted here.

>From a CNI perspective the BRIDGE mode is just another CNI network, so the 
>self().address.ip will be the IP on the BRIDGE network which is what is 
>expected. So for the time being dropping this comment. Please open if you 
>still feel this is an issue.


> On June 13, 2017, 6:42 p.m., Jie Yu wrote:
> > src/launcher/default_executor.cpp
> > Lines 438 (patched)
> > 
> >
> > Let's add a TODO to document this Downward API.

Instead of a TODO, should we just create a JIRA and add it to the CNI 
documentation and the executor documentation?


- Avinash


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


On June 9, 2017, 6:39 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 9, 2017, 6:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
> 
> 
> Diff: https://reviews.apache.org/r/59949/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Anand Mazumdar]

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



Looks good modulo comments from Jie. Mostly minor style nits.


src/launcher/default_executor.cpp
Lines 83 (patched)


One more new line after this.



src/launcher/default_executor.cpp
Lines 367 (patched)


s/env/environment



src/launcher/default_executor.cpp
Lines 438 (patched)


```cpp
// Set the `MESOS_CONTAINER_IP` environment for the task.
```



src/launcher/default_executor.cpp
Lines 441-445 (patched)


Can we do this after L372 to make them aligned with the code comment 
earlier? It looks more readable that way.

Here, we can just copy the environment into the `CommandInfo` once you make 
the earlier change.



src/launcher/default_executor.cpp
Lines 447 (patched)


Also, in addition to the verbosity, can we do it only once since it's the 
same for all the tasks in the task group?

See my earlier comments around moving this block outside.

Also, use `'MESOS_CONTAINER_IP'` in quotes inside the logline.


- Anand Mazumdar


On June 9, 2017, 6:39 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 9, 2017, 6:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
> 
> 
> Diff: https://reviews.apache.org/r/59949/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Jie Yu]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp
Lines 371 (patched)


I'd add a TODO here. Because for bridge mode, `self().address.ip` might be 
the advertise IP (i.e., agent IP), which is not what we've wanted here.



src/launcher/default_executor.cpp
Lines 438 (patched)


Let's add a TODO to document this Downward API.



src/launcher/default_executor.cpp
Lines 447 (patched)


Why WARNING here? use INFO instead?


- Jie Yu


On June 9, 2017, 6:39 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 9, 2017, 6:39 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
> 
> 
> Diff: https://reviews.apache.org/r/59949/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

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

Review request for mesos, Anand Mazumdar and Jie Yu.


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


Repository: mesos


Description
---

Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.


Diffs
-

  src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 


Diff: https://reviews.apache.org/r/59949/diff/1/


Testing
---

make check


Thanks,

Avinash sridharan