Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Oct. 19, 2016, 5:49 p.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 608
> > 
> >
> > THis test will certainly break if nobody does not have access to my 
> > home dir where I run my tests.
> 
> Jiang Yan Xu wrote:
> How will it break? I vaguely remeber seeing this concern somewhere. Any 
> pointers?
> 
> Jiang Yan Xu wrote:
> OK dug this up: https://reviews.apache.org/r/51783
> 
> You mean we should do it the way in https://reviews.apache.org/r/51783?
> 
> Jiang Yan Xu wrote:
> Wait... I think I am OK? Here I have an ExecutorInfo which doesn't have 
> any URIs and directly "exit 0". It doesn't access anything. I think we are 
> talking about the case where a command task is launched but the command 
> executor is not accessible?
> 
> Jiang Yan Xu wrote:
> s/we are/you are/

Ran the test with a home dir with 700 permission successfully.


- Jiang Yan


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


On Oct. 14, 2016, 11:14 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 11:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Oct. 19, 2016, 5:49 p.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 608
> > 
> >
> > THis test will certainly break if nobody does not have access to my 
> > home dir where I run my tests.
> 
> Jiang Yan Xu wrote:
> How will it break? I vaguely remeber seeing this concern somewhere. Any 
> pointers?
> 
> Jiang Yan Xu wrote:
> OK dug this up: https://reviews.apache.org/r/51783
> 
> You mean we should do it the way in https://reviews.apache.org/r/51783?
> 
> Jiang Yan Xu wrote:
> Wait... I think I am OK? Here I have an ExecutorInfo which doesn't have 
> any URIs and directly "exit 0". It doesn't access anything. I think we are 
> talking about the case where a command task is launched but the command 
> executor is not accessible?

s/we are/you are/


- Jiang Yan


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


On Oct. 14, 2016, 11:14 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 11:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Oct. 19, 2016, 5:49 p.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 608
> > 
> >
> > THis test will certainly break if nobody does not have access to my 
> > home dir where I run my tests.
> 
> Jiang Yan Xu wrote:
> How will it break? I vaguely remeber seeing this concern somewhere. Any 
> pointers?
> 
> Jiang Yan Xu wrote:
> OK dug this up: https://reviews.apache.org/r/51783
> 
> You mean we should do it the way in https://reviews.apache.org/r/51783?

Wait... I think I am OK? Here I have an ExecutorInfo which doesn't have any 
URIs and directly "exit 0". It doesn't access anything. I think we are talking 
about the case where a command task is launched but the command executor is not 
accessible?


- Jiang Yan


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


On Oct. 14, 2016, 11:14 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 11:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Oct. 19, 2016, 5:49 p.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 608
> > 
> >
> > THis test will certainly break if nobody does not have access to my 
> > home dir where I run my tests.
> 
> Jiang Yan Xu wrote:
> How will it break? I vaguely remeber seeing this concern somewhere. Any 
> pointers?

OK dug this up: https://reviews.apache.org/r/51783

You mean we should do it the way in https://reviews.apache.org/r/51783?


- Jiang Yan


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


On Oct. 14, 2016, 11:14 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 11:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Oct. 19, 2016, 5:49 p.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 608
> > 
> >
> > THis test will certainly break if nobody does not have access to my 
> > home dir where I run my tests.

How will it break? I vaguely remeber seeing this concern somewhere. Any 
pointers?


- Jiang Yan


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


On Oct. 14, 2016, 11:14 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 11:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jie Yu]

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




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


THis test will certainly break if nobody does not have access to my home 
dir where I run my tests.


- Jie Yu


On Oct. 14, 2016, 6:14 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52828, 52058]

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 Oct. 14, 2016, 6:14 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 14, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 14, 2016, 6:14 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/slave/containerizer/fetcher.cpp (line 732)


We are constructing the path at least 3 times. 

How about 

```
const string stdoutPath = path::join(info.sandbox_directory(), "stdout");
```

and use the variable below?

Same for stderr.



src/slave/containerizer/fetcher.cpp (line 740)


Same deal. Use `stderrPath`.



src/slave/containerizer/fetcher.cpp (line 754)


Below the line we should add a TODO

```
TODO(megha.sharma): Fetcher should not create seperate stdout/stderr files 
but rather use FDs prepared by the container logger. See MESOS-6271 for more 
details.
```



src/slave/containerizer/fetcher.cpp (line 764)


Missing space before `+`.



src/slave/containerizer/fetcher.cpp (line 766)


s/ with error//

The ":" is enough to indicate the error message that follows.



src/slave/containerizer/fetcher.cpp (line 781)


s/ with error//

The ":" is enough to indicate the error message that follows.



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


"This test verified that" or "Tests that".



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


"exit 0" may be a better (more clear w.r.t intention) dummy executor 
command.



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


`os::getuid(user)` didn't work?



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


s/std::string/string/

s/stdOutPath/stdoutPath/

stdout is common enough that we should just treat it as one word.



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


s/std::string/string/

s/stdErrPath/stderrPath/



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


2 space indentation.


- Jiang Yan Xu


On Oct. 13, 2016, 7:33 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 13, 2016, 7:33 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52828, 52058]

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 Oct. 13, 2016, 2:33 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 13, 2016, 2:33 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 13, 2016, 2:33 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 13, 2016, 2:30 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 12, 2016, 9:38 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 6, 2016, 6:28 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 5, 2016, 5:40 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Oct. 4, 2016, 5:54 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing (updated)
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

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




src/slave/containerizer/fetcher.cpp (lines 755 - 756)


'check if it's a valid user' is not accurate, here we have past the point 
that the user can still be invalid. It's obvious that we are chowning the two 
files here so I'd say we don't need to comment on it anymore.



src/slave/containerizer/fetcher.cpp (lines 757 - 783)


Let's reorder a bit.

```
Try chownOut = os::chown(
user.get(),
path::join(info.sandbox_directory(), "stdout"),
false);

if (chownOut.isError()) {
  os::close(out.get());
  os::close(err.get());
  
  return Failure(...);
}

Try chownErr = os::chown(
user.get(),
path::join(info.sandbox_directory(), "stderr"),
false);

if (chownErr.isError()) {
  os::close(out.get());
  os::close(err.get());

  return Failure(...);
}
```

Even though we are calling 

```
  os::close(out.get());
  os::close(err.get());
```

in two places but with a little bit of redundancy we would not be in the 
dilemma of choosing which error to propagate and "what if they both failed?" If 
chownOut failed, we return without chowning the other file, which is pretty 
reasonable.



src/slave/containerizer/fetcher.cpp (line 764)


No space between function name and open paren.



src/slave/containerizer/fetcher.cpp (lines 770 - 773)


Let's remove a few redundant words. The following output should be clear 
enough:

"Failed to chown '/path/to/sandbox/stdout' to user foo: "



src/slave/containerizer/fetcher.cpp (lines 771 - 773)


Wrap operator `+` at the end of lines (for consistency).



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


We probably don't need such a constraint. macOS should be fine.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 506 - 524)


Can we just use the user `nobody`? See the comments in the test.



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


s/SandboxOwnershipTest/MesosContainerizerExecuteTest/ would probably make 
sense.



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


Can we just use 

```
const string user = "nobody";
Result uid = os::getuid(user);
ASSERT_SOME(uid);
```
?



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


This is probably not necessary. I understand that you are simulating the 
agent's chowning of the sandbox but here what matters is that the test runs 
under root but the files are still owned by `nobody`, then we have verified 
that it's working.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 538 - 541)


We don't need these?



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 551 - 552)


This is OK but we can directly use `DEFAULT_CONTAINER_ID` inline below.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 554 - 561)


Simplify:

```
ExecutorInfo executorInfo = CREATE_EXECUTOR_INFO("executor", "echo hello");
executorInfo.mutable_command()->set_value(user);
```



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


DEFAULT_CONTAINER_ID



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


Capitalization and punctuation.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 581 - 582)


Inline it?

```
::stat(path::join(sandbox, "stdout").c_str(), );
```



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


Capitalization and punctuation.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 586 - 587)


Same as above.


- Jiang Yan Xu


On Sept. 28, 2016, 10:17 a.m., Megha Sharma wrote:
> 
> 

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...
> 
> Megha Sharma wrote:
> Since now the fetcher is being run as task user as a result of jira 
> https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
> chowned to the same user for the fetcher to be able to write to them.
> 
> Jiang Yan Xu wrote:
> @Joseph: Given the way it works today, is the fetcher stdout/stderr 
> handled by the container logger at all? It doesn't look so but should it?
> 
> The chown (recursive or not) does look out of place in the fetcher. Since 
> we always have a container logger now (default to SandboxContainerLogger 
> which has no runtime component but only exists during container setup) it 
> seems appropriate for the logger to assume the responsibility to set up 
> stderr and stdout FDs (backed by files or not) and make sure the contained 
> processes (fetcher and executor both now run under the task user) **can 
> write** to them, right?
> 
> Joseph Wu wrote:
> It does make sense to pass responsibility for the fetcher's stdout/stderr 
> into the ContainerLogger too.  This will give a unified logging interface, in 
> case anyone wants to pipe their logs somewhere else.
> 
> In order to do this, looks like there are a couple changes needed to the 
> fetcher interface and how we pass along file descriptors from the 
> ContainerLogger.
> 
> (1) The fetcher will need to take some additional arguments of the form:
> ```
> const ContainerLogger::SubprocessInfo& subprocessInfo
> ```
> Here:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/fetcher.hpp#L95-L106
> 
> ---
> 
> (2) The containerizers need to pass this `SubprocessInfo` from the 
> ContainerLogger to Fetcher.  We can accomplish this fairly trivially in the 
> Mesos Containerizer:
> 
> The logger is called here:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1188-L1190
> 
> And the fetcher is called inside the logger's continuation:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1358-L1361
> 
> Unfortunately, the Docker Containerizer has it backwards:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L-L1123
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L1143-L1155
> 
> ---
> 
> (3) We will need to tweak the way `SubprocessInfo::FD` is passed around.  
> Currently, as soon as the FD is sent into `subprocess`, the ownership of the 
> FD is transfered into the subprocess.  If we plan to send the same FD to the 
> Executor and Fetcher, the fetcher cannot be allowed to take ownership.
> 
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/include/mesos/slave/container_logger.hpp#L79-L86
> 
> Jiang Yan Xu wrote:
> Thanks for the reply. In addition to passing SubprocessInfo to the 
> fetcher, we have to set the perms for the files. Since it's only relavent to 
> the sandbox logger, I guess we have to handle it in the sandbox logger 
> itself? This would probably 

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 28, 2016, 5:17 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
8b5a19e121ef74eaf99b39682f8fd170605bf56d 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

> On Sept. 26, 2016, 11:41 p.m., Anindya Sinha wrote:
> > src/slave/containerizer/fetcher.cpp, line 784
> > 
> >
> > I think this should be more like:
> > ```
> > if (chownOut.isError() || chownErr.isError()) {
> >   os::close(out.get());
> >   os::close(err.get());
> >   return Failure(...);
> > }
> > ```
> > 
> > And the `Failure` message can indicate which one failed, ie. `stdout` 
> > or `stderr` or both (if you want to expose that bit of information).

Moved the error checks for stdout and stderr to different if blocks so we can 
have custom failure messages for stdout and stderr.


- Megha


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


On Sept. 28, 2016, 5:10 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 28, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 8b5a19e121ef74eaf99b39682f8fd170605bf56d 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 28, 2016, 5:10 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
8b5a19e121ef74eaf99b39682f8fd170605bf56d 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Joseph Wu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...
> 
> Megha Sharma wrote:
> Since now the fetcher is being run as task user as a result of jira 
> https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
> chowned to the same user for the fetcher to be able to write to them.
> 
> Jiang Yan Xu wrote:
> @Joseph: Given the way it works today, is the fetcher stdout/stderr 
> handled by the container logger at all? It doesn't look so but should it?
> 
> The chown (recursive or not) does look out of place in the fetcher. Since 
> we always have a container logger now (default to SandboxContainerLogger 
> which has no runtime component but only exists during container setup) it 
> seems appropriate for the logger to assume the responsibility to set up 
> stderr and stdout FDs (backed by files or not) and make sure the contained 
> processes (fetcher and executor both now run under the task user) **can 
> write** to them, right?
> 
> Joseph Wu wrote:
> It does make sense to pass responsibility for the fetcher's stdout/stderr 
> into the ContainerLogger too.  This will give a unified logging interface, in 
> case anyone wants to pipe their logs somewhere else.
> 
> In order to do this, looks like there are a couple changes needed to the 
> fetcher interface and how we pass along file descriptors from the 
> ContainerLogger.
> 
> (1) The fetcher will need to take some additional arguments of the form:
> ```
> const ContainerLogger::SubprocessInfo& subprocessInfo
> ```
> Here:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/fetcher.hpp#L95-L106
> 
> ---
> 
> (2) The containerizers need to pass this `SubprocessInfo` from the 
> ContainerLogger to Fetcher.  We can accomplish this fairly trivially in the 
> Mesos Containerizer:
> 
> The logger is called here:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1188-L1190
> 
> And the fetcher is called inside the logger's continuation:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1358-L1361
> 
> Unfortunately, the Docker Containerizer has it backwards:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L-L1123
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L1143-L1155
> 
> ---
> 
> (3) We will need to tweak the way `SubprocessInfo::FD` is passed around.  
> Currently, as soon as the FD is sent into `subprocess`, the ownership of the 
> FD is transfered into the subprocess.  If we plan to send the same FD to the 
> Executor and Fetcher, the fetcher cannot be allowed to take ownership.
> 
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/include/mesos/slave/container_logger.hpp#L79-L86
> 
> Jiang Yan Xu wrote:
> Thanks for the reply. In addition to passing SubprocessInfo to the 
> fetcher, we have to set the perms for the files. Since it's only relavent to 
> the sandbox logger, I guess we have to handle it in the sandbox logger 
> itself? This would probably 

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Anindya Sinha]

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




src/slave/containerizer/fetcher.cpp (line 765)


Add a line here.



src/slave/containerizer/fetcher.cpp (line 770)


I think this should be more like:
```
if (chownOut.isError() || chownErr.isError()) {
  os::close(out.get());
  os::close(err.get());
  return Failure(...);
}
```

And the `Failure` message can indicate which one failed, ie. `stdout` or 
`stderr` or both (if you want to expose that bit of information).



src/slave/containerizer/fetcher.cpp (line 772)


If there is an error in either `chownOut` or `chownErr`, there is a 
`os::close(out.get())` and then we return `Failure`... which means we never get 
to do a `os::close(err.get())`. So, a potential leak in file descriptor!



src/slave/containerizer/fetcher.cpp (line 774)


Also, the `Failure` message indicates the failure is with `stdout` which 
may not be true if only `chownErr.isError()` is true.



src/slave/containerizer/fetcher.cpp (line 779)


Kill this block if you consolidate the error handling for `chownOut` and 
`chownErr`.


- Anindya Sinha


On Sept. 20, 2016, 11:32 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 20, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...
> 
> Megha Sharma wrote:
> Since now the fetcher is being run as task user as a result of jira 
> https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
> chowned to the same user for the fetcher to be able to write to them.
> 
> Jiang Yan Xu wrote:
> @Joseph: Given the way it works today, is the fetcher stdout/stderr 
> handled by the container logger at all? It doesn't look so but should it?
> 
> The chown (recursive or not) does look out of place in the fetcher. Since 
> we always have a container logger now (default to SandboxContainerLogger 
> which has no runtime component but only exists during container setup) it 
> seems appropriate for the logger to assume the responsibility to set up 
> stderr and stdout FDs (backed by files or not) and make sure the contained 
> processes (fetcher and executor both now run under the task user) **can 
> write** to them, right?
> 
> Joseph Wu wrote:
> It does make sense to pass responsibility for the fetcher's stdout/stderr 
> into the ContainerLogger too.  This will give a unified logging interface, in 
> case anyone wants to pipe their logs somewhere else.
> 
> In order to do this, looks like there are a couple changes needed to the 
> fetcher interface and how we pass along file descriptors from the 
> ContainerLogger.
> 
> (1) The fetcher will need to take some additional arguments of the form:
> ```
> const ContainerLogger::SubprocessInfo& subprocessInfo
> ```
> Here:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/fetcher.hpp#L95-L106
> 
> ---
> 
> (2) The containerizers need to pass this `SubprocessInfo` from the 
> ContainerLogger to Fetcher.  We can accomplish this fairly trivially in the 
> Mesos Containerizer:
> 
> The logger is called here:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1188-L1190
> 
> And the fetcher is called inside the logger's continuation:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1358-L1361
> 
> Unfortunately, the Docker Containerizer has it backwards:
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L-L1123
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L1143-L1155
> 
> ---
> 
> (3) We will need to tweak the way `SubprocessInfo::FD` is passed around.  
> Currently, as soon as the FD is sent into `subprocess`, the ownership of the 
> FD is transfered into the subprocess.  If we plan to send the same FD to the 
> Executor and Fetcher, the fetcher cannot be allowed to take ownership.
> 
> 
> https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/include/mesos/slave/container_logger.hpp#L79-L86

Thanks for the reply. In addition to passing SubprocessInfo to the fetcher, we 
have to set the perms for the files. Since it's only relavent to the sandbox 
logger, I guess we have to handle it in the sandbox logger itself? This would 
probably require the 'user' (which can't be 

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Joseph Wu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...
> 
> Megha Sharma wrote:
> Since now the fetcher is being run as task user as a result of jira 
> https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
> chowned to the same user for the fetcher to be able to write to them.
> 
> Jiang Yan Xu wrote:
> @Joseph: Given the way it works today, is the fetcher stdout/stderr 
> handled by the container logger at all? It doesn't look so but should it?
> 
> The chown (recursive or not) does look out of place in the fetcher. Since 
> we always have a container logger now (default to SandboxContainerLogger 
> which has no runtime component but only exists during container setup) it 
> seems appropriate for the logger to assume the responsibility to set up 
> stderr and stdout FDs (backed by files or not) and make sure the contained 
> processes (fetcher and executor both now run under the task user) **can 
> write** to them, right?

It does make sense to pass responsibility for the fetcher's stdout/stderr into 
the ContainerLogger too.  This will give a unified logging interface, in case 
anyone wants to pipe their logs somewhere else.

In order to do this, looks like there are a couple changes needed to the 
fetcher interface and how we pass along file descriptors from the 
ContainerLogger.

(1) The fetcher will need to take some additional arguments of the form:
```
const ContainerLogger::SubprocessInfo& subprocessInfo
```
Here:
https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/fetcher.hpp#L95-L106

---

(2) The containerizers need to pass this `SubprocessInfo` from the 
ContainerLogger to Fetcher.  We can accomplish this fairly trivially in the 
Mesos Containerizer:

The logger is called here:
https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1188-L1190

And the fetcher is called inside the logger's continuation:
https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/mesos/containerizer.cpp#L1358-L1361

Unfortunately, the Docker Containerizer has it backwards:
https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L-L1123
https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/src/slave/containerizer/docker.cpp#L1143-L1155

---

(3) We will need to tweak the way `SubprocessInfo::FD` is passed around.  
Currently, as soon as the FD is sent into `subprocess`, the ownership of the FD 
is transfered into the subprocess.  If we plan to send the same FD to the 
Executor and Fetcher, the fetcher cannot be allowed to take ownership.

https://github.com/apache/mesos/blob/86c9f063b8d94a5e1bd611db5850d9d31dbb30b8/include/mesos/slave/container_logger.hpp#L79-L86


- Joseph


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


On Sept. 20, 2016, 4:32 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Jiang Yan Xu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...
> 
> Megha Sharma wrote:
> Since now the fetcher is being run as task user as a result of jira 
> https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
> chowned to the same user for the fetcher to be able to write to them.

@Joseph: Given the way it works today, is the fetcher stdout/stderr handled by 
the container logger at all? It doesn't look so but should it?

The chown (recursive or not) does look out of place in the fetcher. Since we 
always have a container logger now (default to SandboxContainerLogger which has 
no runtime component but only exists during container setup) it seems 
appropriate for the logger to assume the responsibility to set up stderr and 
stdout FDs (backed by files or not) and make sure the contained processes 
(fetcher and executor both now run under the task user) **can write** to them, 
right?


- Jiang Yan


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


On Sept. 20, 2016, 4:32 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 20, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52058]

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 Sept. 20, 2016, 11:32 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 20, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 20, 2016, 11:32 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

> On Sept. 19, 2016, 8:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...

Since now the fetcher is being run as task user as a result of jira 
https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
chowned to the same user for the fetcher to be able to write to them.


- Megha


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


On Sept. 19, 2016, 11:08 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 19, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Joseph Wu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.

Right, forgot about the fetcher's own stdout/stderr :)

I'd argue it makes sense to not do any chown-ing at all.  The stdout/stderr 
files are technically created by other entities than the executor (i.e. the 
fetcher, agent, and logger module).  Not sure if this breaks any expectations 
in the fetcher though...


- Joseph


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


On Sept. 19, 2016, 4:08 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 19, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 19, 2016, 11:08 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 19, 2016, 11:04 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

Review request for mesos.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma