Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1405
> > 
> >
> > I'll be nice to use environment variable for all flags here. I think we 
> > probably need a helper in FlagsBase to return the corresponding environment 
> > variables for the flags:
> > ```
> > // Export flags as environment variables.
> > map FlagsBase::environment(
> > const Option& prefix);
> > ```
> > 
> > And then, we can just merge with os::environment and pass it to 
> > Launcher::fork
> 
> Jiang Yan Xu wrote:
> This helper looks like handy. 
> 
> I am not necessarily against this but wanted us to give it more thought. 
> 
> 1. Should all Mesos libexec binaries work this way?
> 2. Removing all of the flags makes all the helper binary instances look 
> the same through `ps`. Yes privileged users can still get the info in /proc 
> for individual processes but we loose  we lose easy "grep-pability" for 
> debugging. 
> 
> To me the line to draw is between user-supplied info (prefer env vars) 
> and Mesos generated info (e.g., runtime dir which encodes contianerId) which 
> is not sensitive. Thoughts?
> 
> Jie Yu wrote:
> This is just a suggestion. That's the reason I didn't put an issue there.
> 
> For 1, yes, I would say so because libexec binaries are Mesos details, 
> which should not be exposed to users
> For 2, one can still log that in the agent log.
> 
> But we can do that later, we don't have to do it now.

I captured it here: https://issues.apache.org/jira/browse/MESOS-6573


- Jiang Yan


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


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 9, 2016, 2:37 a.m., Gastón Kleiman wrote:
> > The Docker containerizer still passes the env variables to the executor 
> > through cmd line flags, we might want to fix that as well:
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L244
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L351-L372
> 
> Gastón Kleiman wrote:
> I just checked and the env variables are not visible in the 
> `DockerExecutor` cmd line, but the executor passes them to the `docker` 
> binary using the cmd line.
> 
> See: https://issues.apache.org/jira/browse/MESOS-6566

Thanks Gaston for filing the ticket. :)


- Jiang Yan


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


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Gastón Kleiman]

> On Nov. 9, 2016, 10:37 a.m., Gastón Kleiman wrote:
> > The Docker containerizer still passes the env variables to the executor 
> > through cmd line flags, we might want to fix that as well:
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L244
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L351-L372

I just checked and the env variables are not visible in the `DockerExecutor` 
cmd line, but the executor passes them to the `docker` binary using the cmd 
line.

See: https://issues.apache.org/jira/browse/MESOS-6566


- Gastón


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


On Nov. 8, 2016, 4:46 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Gastón Kleiman]

> On Nov. 9, 2016, 10:37 a.m., Gastón Kleiman wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1401
> > 
> >
> > There's a typo here, it should be `launch` instead of `lauch.`

https://reviews.apache.org/r/53601/


- Gastón


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


On Nov. 8, 2016, 4:46 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Gastón Kleiman]

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



The Docker containerizer still passes the env variables to the executor through 
cmd line flags, we might want to fix that as well:

https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L244
https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L351-L372


src/slave/containerizer/mesos/containerizer.cpp (line 1400)


There's a typo here, it should be `launch` instead of `lauch.`


- Gastón Kleiman


On Nov. 8, 2016, 4:46 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Joseph Wu]

> On Nov. 8, 2016, 11:56 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1499
> > 
> >
> > This appears to break every single test that uses the Mesos 
> > Containerizer.
> > 
> > Every task immediately exits with:
> > ```
> > Expecting 'MESOS_FRAMEWORK_ID' to be set in the environment
> > ```
> 
> Jiang Yan Xu wrote:
> Where did you see this failure?

On OSX, with a plain `make check`.

The first one that fails is: `bin/mesos-tests.sh 
--gtest_filter="HTTPCommandExecutorTest.TerminateWithACK"`


- Joseph


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


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 8, 2016, 11:56 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1499
> > 
> >
> > This appears to break every single test that uses the Mesos 
> > Containerizer.
> > 
> > Every task immediately exits with:
> > ```
> > Expecting 'MESOS_FRAMEWORK_ID' to be set in the environment
> > ```

Where did you see this failure?


- Jiang Yan


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


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Joseph Wu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1498)


This appears to break every single test that uses the Mesos Containerizer.

Every task immediately exits with:
```
Expecting 'MESOS_FRAMEWORK_ID' to be set in the environment
```


- Joseph Wu


On Nov. 8, 2016, 8:46 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 8:46 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 8, 2016, 4:46 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 8, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 32fba7697b7488b63192c4e9630ff348c572e424 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

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

(Updated Nov. 8, 2016, 8:46 a.m.)


Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.


Changes
---

I screwed up the env var name.


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


Repository: mesos


Description
---

Used an environment variable to pass command environment.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
32fba7697b7488b63192c4e9630ff348c572e424 
  src/slave/containerizer/mesos/main.cpp 
1a0e765ddb6d8519426b8d47067efdfa3432e07a 

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


Testing
---

make check.

No new tests are written but the fact the existing tests that run exectuors 
depends on this to work correctly is a proof.


Thanks,

Jiang Yan Xu

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 7, 2016, 10:51 p.m., Jie Yu wrote:
> > Looks like the review bot has a segfault?

The failed test 'HTTPCommandExecutorTest.TerminateWithACK' is broken on master 
head so it's not it. I saw Anand pinging Qian on the build mailing list. We can 
have a JIRA to track it.


- Jiang Yan


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


On Nov. 7, 2016, 12:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 7, 2016, 12:13 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jie Yu]

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



Looks like the review bot has a segfault?

- Jie Yu


On Nov. 7, 2016, 8:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 7, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jie Yu]

> On Nov. 5, 2016, 9:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1405
> > 
> >
> > I'll be nice to use environment variable for all flags here. I think we 
> > probably need a helper in FlagsBase to return the corresponding environment 
> > variables for the flags:
> > ```
> > // Export flags as environment variables.
> > map FlagsBase::environment(
> > const Option& prefix);
> > ```
> > 
> > And then, we can just merge with os::environment and pass it to 
> > Launcher::fork
> 
> Jiang Yan Xu wrote:
> This helper looks like handy. 
> 
> I am not necessarily against this but wanted us to give it more thought. 
> 
> 1. Should all Mesos libexec binaries work this way?
> 2. Removing all of the flags makes all the helper binary instances look 
> the same through `ps`. Yes privileged users can still get the info in /proc 
> for individual processes but we loose  we lose easy "grep-pability" for 
> debugging. 
> 
> To me the line to draw is between user-supplied info (prefer env vars) 
> and Mesos generated info (e.g., runtime dir which encodes contianerId) which 
> is not sensitive. Thoughts?

This is just a suggestion. That's the reason I didn't put an issue there.

For 1, yes, I would say so because libexec binaries are Mesos details, which 
should not be exposed to users
For 2, one can still log that in the agent log.

But we can do that later, we don't have to do it now.


- Jie


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


On Nov. 7, 2016, 8:13 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 7, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

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

(Updated Nov. 7, 2016, 12:13 p.m.)


Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.


Changes
---

Addressed comments other than to pass any all configs through environment 
variables. I suggest we do it systematically based on the disussion conclusion 
instead of having it lumped together with this fix.


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


Repository: mesos


Description
---

Used an environment variable to pass command environment.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/main.cpp 
1a0e765ddb6d8519426b8d47067efdfa3432e07a 

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


Testing
---

make check.

No new tests are written but the fact the existing tests that run exectuors 
depends on this to work correctly is a proof.


Thanks,

Jiang Yan Xu

Re: Review Request 53500: Used an environment variable to pass command environment.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53500]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Nov. 4, 2016, 9:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 4, 2016, 9:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/launch.hpp 
> 8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
>   src/slave/containerizer/mesos/launch.cpp 
> 377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 78
> > 
> >
> > why the renaming here. All the parameters here are for 'command'. I 
> > suggest we stick to the previous name.

The rename is because just so that we don't use a environment variable name 
"MESOS_ENVIRONMENT" which is not very descriptive. The suggest below would 
solve this problem so yeah I'll change it back.


> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1405
> > 
> >
> > I'll be nice to use environment variable for all flags here. I think we 
> > probably need a helper in FlagsBase to return the corresponding environment 
> > variables for the flags:
> > ```
> > // Export flags as environment variables.
> > map FlagsBase::environment(
> > const Option& prefix);
> > ```
> > 
> > And then, we can just merge with os::environment and pass it to 
> > Launcher::fork

This helper looks like handy. 

I am not necessarily against this but wanted us to give it more thought. 

1. Should all Mesos libexec binaries work this way?
2. Removing all of the flags makes all the helper binary instances look the 
same through `ps`. Yes privileged users can still get the info in /proc for 
individual processes but we loose  we lose easy "grep-pability" for debugging. 

To me the line to draw is between user-supplied info (prefer env vars) and 
Mesos generated info (e.g., runtime dir which encodes contianerId) which is not 
sensitive. Thoughts?


> On Nov. 5, 2016, 2:38 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/main.cpp, line 43
> > 
> >
> > I'd prefer using a different prefix here because `MESOS_` prefix is 
> > used by agent as well. It will be very hard to debug if there are 
> > conflicting flags.
> > 
> > I'd suggest we use `MESOS_CONTAINERIZER_` here

+1.


- Jiang Yan


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


On Nov. 4, 2016, 2:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 4, 2016, 2:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/launch.hpp 
> 8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
>   src/slave/containerizer/mesos/launch.cpp 
> 377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53500: Used an environment variable to pass command environment.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1404)


I'll be nice to use environment variable for all flags here. I think we 
probably need a helper in FlagsBase to return the corresponding environment 
variables for the flags:
```
// Export flags as environment variables.
map FlagsBase::environment(
const Option& prefix);
```

And then, we can just merge with os::environment and pass it to 
Launcher::fork



src/slave/containerizer/mesos/launch.cpp (line 78)


why the renaming here. All the parameters here are for 'command'. I suggest 
we stick to the previous name.



src/slave/containerizer/mesos/main.cpp (line 43)


I'd prefer using a different prefix here because `MESOS_` prefix is used by 
agent as well. It will be very hard to debug if there are conflicting flags.

I'd suggest we use `MESOS_CONTAINERIZER_` here


- Jie Yu


On Nov. 4, 2016, 9:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53500/
> ---
> 
> (Updated Nov. 4, 2016, 9:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6526
> https://issues.apache.org/jira/browse/MESOS-6526
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an environment variable to pass command environment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 67cc595278f124cdf518d2f4fcfb257439f067e2 
>   src/slave/containerizer/mesos/launch.hpp 
> 8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
>   src/slave/containerizer/mesos/launch.cpp 
> 377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
>   src/slave/containerizer/mesos/main.cpp 
> 1a0e765ddb6d8519426b8d47067efdfa3432e07a 
> 
> Diff: https://reviews.apache.org/r/53500/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> No new tests are written but the fact the existing tests that run exectuors 
> depends on this to work correctly is a proof.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 53500: Used an environment variable to pass command environment.

[Jiang Yan Xu]

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

Review request for mesos, Anindya Sinha, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Used an environment variable to pass command environment.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
67cc595278f124cdf518d2f4fcfb257439f067e2 
  src/slave/containerizer/mesos/launch.hpp 
8b23c1b6df6bc1fdd987af5a4469664356e7f27a 
  src/slave/containerizer/mesos/launch.cpp 
377a9d94aa780ab598b1c2034c10ce25a4e02cbe 
  src/slave/containerizer/mesos/main.cpp 
1a0e765ddb6d8519426b8d47067efdfa3432e07a 

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


Testing
---

make check.

No new tests are written but the fact the existing tests that run exectuors 
depends on this to work correctly is a proof.


Thanks,

Jiang Yan Xu