Re: Review Request 39837: Passing os environment variables when start docker executor.

[Jojy Varghese]

> On Nov. 1, 2015, 5:56 p.m., Jojy Varghese wrote:
> > How did you test this? Could you please elaborate the test steps? Or even 
> > better - add a test case?
> 
> haosdent huang wrote:
> Need add a test case.

Thanks haosdent. Shout if you need help.


- Jojy


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


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39837: Passing os environment variables when start docker executor.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39837]

All tests passed.

- Mesos ReviewBot


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 39837: Passing os environment variables when start docker executor.

[haosdent huang]

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

Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.


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


Repository: mesos


Description
---

Passing os environment variables when start docker executor.


Diffs
-

  src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 

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


Testing
---

manually test.


Thanks,

haosdent huang

Re: Review Request 39837: Passing os environment variables when start docker executor.

[Timothy Chen]

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



src/slave/containerizer/docker.cpp 


We intentionally exclude os enviornments as you can see here for a reason.

There are a lot of environment variables that are default to the OS, that 
will break when docker containers run and we need to not include them unless 
it's specifically specified by the user with executor environement variables or 
taskinfo.

The fix should be carefully picking the ones we need only.


- Timothy Chen


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39837: Passing os environment variables when start docker executor.

[Jojy Varghese]

> On Nov. 1, 2015, 4:29 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 302
> > 
> >
> > We intentionally exclude os enviornments as you can see here for a 
> > reason.
> > 
> > There are a lot of environment variables that are default to the OS, 
> > that will break when docker containers run and we need to not include them 
> > unless it's specifically specified by the user with executor environement 
> > variables or taskinfo.
> > 
> > The fix should be carefully picking the ones we need only.
> 
> haosdent huang wrote:
> How about only include `MESOS_` and `LIBPROCESS_` prefix environment 
> variables default?

In this particular case, it looks like we DONT include the SSL environment 
variables in the executor process which breaks it. So maybe we can address this 
particular issue for now by adding SSL env variables. Also we need a better way 
to express the idea of adding specific environment variables in the executor 
process(maybe something like a "include" and "exclude" filters)


- Jojy


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


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39837: Passing os environment variables when start docker executor.

[haosdent huang]

> On Nov. 1, 2015, 4:29 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 302
> > 
> >
> > We intentionally exclude os enviornments as you can see here for a 
> > reason.
> > 
> > There are a lot of environment variables that are default to the OS, 
> > that will break when docker containers run and we need to not include them 
> > unless it's specifically specified by the user with executor environement 
> > variables or taskinfo.
> > 
> > The fix should be carefully picking the ones we need only.

How about only include `MESOS_` and `LIBPROCESS_` prefix environment variables 
default?


- haosdent


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


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39837: Passing os environment variables when start docker executor.

[Jojy Varghese]

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


How did you test this? Could you please elaborate the test steps? Or even 
better - add a test case?

- Jojy Varghese


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39837: Passing os environment variables when start docker executor.

[haosdent huang]

> On Nov. 1, 2015, 5:56 p.m., Jojy Varghese wrote:
> > How did you test this? Could you please elaborate the test steps? Or even 
> > better - add a test case?

Need add a test case.


- haosdent


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


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39837: Passing os environment variables when start docker executor.

[haosdent huang]

> On Nov. 1, 2015, 4:29 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 302
> > 
> >
> > We intentionally exclude os enviornments as you can see here for a 
> > reason.
> > 
> > There are a lot of environment variables that are default to the OS, 
> > that will break when docker containers run and we need to not include them 
> > unless it's specifically specified by the user with executor environement 
> > variables or taskinfo.
> > 
> > The fix should be carefully picking the ones we need only.
> 
> haosdent huang wrote:
> How about only include `MESOS_` and `LIBPROCESS_` prefix environment 
> variables default?
> 
> Jojy Varghese wrote:
> In this particular case, it looks like we DONT include the SSL 
> environment variables in the executor process which breaks it. So maybe we 
> can address this particular issue for now by adding SSL env variables. Also 
> we need a better way to express the idea of adding specific environment 
> variables in the executor process(maybe something like a "include" and 
> "exclude" filters)

Agree. So let me change to only include SSL environment variables.


- haosdent


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


On Nov. 1, 2015, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39837/
> ---
> 
> (Updated Nov. 1, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Passing os environment variables when start docker executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 5e5e0f93123b7f0bda6c8afeb4df271e796e4637 
> 
> Diff: https://reviews.apache.org/r/39837/diff/
> 
> 
> Testing
> ---
> 
> manually test.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>