Re: Review Request 38900: Update command executor to support rootfs.

2015-11-04 Thread Jie Yu 

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

Ship it!



src/slave/slave.cpp (line 3343)


Can you add a NOTE here saying that if switch_user flag is false and the 
slave runs under a non-root user, the task will be rejected by the Posix 
filesystem isolator. Linux filesystem isolator requires slave to have root 
permission.


- Jie Yu


On Nov. 4, 2015, 8:59 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Nov. 4, 2015, 8:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
>   src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
>   src/slave/slave.hpp e6fa66b40c7f17c500056b7d6f95d7e795a16ca0 
>   src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 
>   src/tests/slave_tests.cpp 91dbdba56c7d3a374e56be92d88c0b367c7a2e1c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38900: Update command executor to support rootfs.

2015-11-04 Thread Timothy Chen 

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

(Updated Nov. 4, 2015, 8:59 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
  src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
  src/slave/slave.hpp e6fa66b40c7f17c500056b7d6f95d7e795a16ca0 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 
  src/tests/slave_tests.cpp 91dbdba56c7d3a374e56be92d88c0b367c7a2e1c 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38900: Update command executor to support rootfs.

2015-11-02 Thread Timothy Chen 

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

(Updated Nov. 2, 2015, 7:02 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
  src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-31 Thread Timothy Chen 

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

(Updated Oct. 31, 2015, 5:57 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
  src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
  src/slave/containerizer/mesos/containerizer.hpp 
4aad8a3be43b331efc6b8157b2fae090df16c1b4 
  src/slave/containerizer/mesos/containerizer.cpp 
91e4ea3a907ad165c359e7422135138737e14085 
  src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-31 Thread Timothy Chen 


> On Oct. 14, 2015, 7:23 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 3390-3393
> > 
> >
> > Hum... Does that mean that if the user task does not specify a user 
> > (e.g., rely on framework.user), we are going to launch the task under root?

That's right, otherwise I don't think we could have done all other operations 
for them in the first place if it's non-root right?
Wouldn't this be the same for custom executor as well?


- Timothy


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


On Oct. 31, 2015, 5:57 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Oct. 31, 2015, 5:57 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp de6b58a93346c618a9214032d891c1004203ca56 
>   src/slave/constants.cpp b69471b2d57aad0c254ef3bb7dce9405abeab93a 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4aad8a3be43b331efc6b8157b2fae090df16c1b4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 91e4ea3a907ad165c359e7422135138737e14085 
>   src/slave/slave.cpp 5f9b52b41eaab0c24965f28e192074340e00bde5 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-14 Thread Jie Yu 

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


Only one issue (regarding user). See my detailed comments below.


src/launcher/executor.cpp (line 187)


Could you please add some comments about what we are doing here:

```
// If 'sandbox_diretory' is specified, that means the user
// task specifies a root filesystem, and that root filesystem has
// already been prepared at COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH.
// The command executor is reponsible for mounting the sandbox
// into the root filesystem, chrooting into it and changing the
// user before exec-ing the user process.
```



src/launcher/executor.cpp (lines 203 - 212)


This loop looks like unnecessary. Can you simply do:
```
rootfs = path::join(os::getcwd(), COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH);
```



src/launcher/executor.cpp (line 218)


Failed to create sandbox mount point at '...': 



src/launcher/executor.cpp (line 224)


Please add a NOTE here saying that this is a non-recursive bind mount. You 
don't want to recursively mount root filesystem into root filesystem since the 
root filesystem is under sandbox as well.



src/launcher/executor.cpp (line 227)


this should be 'sandbox'?



src/launcher/executor.cpp (line 306)


Hum, looks like no one will use 'rootfs' variable on non linux platforms. 
WIll that trigger compiler warning/error?

So should you put #ifdef within the if block and abort if it's not linux?



src/slave/containerizer/mesos/containerizer.cpp (lines 592 - 597)


WHy the change here? Can you do that in a separate patch?



src/slave/containerizer/mesos/containerizer.cpp 


Ditto here.



src/slave/slave.cpp (lines 3390 - 3393)


Hum... Does that mean that if the user task does not specify a user (e.g., 
rely on framework.user), we are going to launch the task under root?


- Jie Yu


On Oct. 13, 2015, 3:39 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Oct. 13, 2015, 3:39 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp 1197268576ee2ec37601db75ea9536ec09882886 
>   src/slave/constants.cpp 96dadce6d28aa585410f50a2509a34445f8dcf82 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4aad8a3be43b331efc6b8157b2fae090df16c1b4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d1fc5a460e7313828014eea999cf4e63dde01921 
>   src/slave/slave.cpp 6b25b49458c163097a5292843134363c4d0f5e6f 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-04 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [38900]

All tests passed.

- Mesos ReviewBot


On Oct. 4, 2015, 5:35 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Oct. 4, 2015, 5:35 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-03 Thread Timothy Chen 

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

(Updated Oct. 4, 2015, 5:35 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
  src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
  src/slave/containerizer/mesos/containerizer.hpp 
4c1419290645ad4c44360a81618a6cea7ad190df 
  src/slave/containerizer/mesos/containerizer.cpp 
b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
  src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-03 Thread Timothy Chen 

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



src/slave/slave.cpp (line 3312)


Jie we explicitly set shell to true above, do you remember why?

This works locally.


- Timothy Chen


On Oct. 4, 2015, 5:35 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Oct. 4, 2015, 5:35 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-02 Thread Jie Yu 

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



src/launcher/executor.cpp (line 97)


Not yours, but can you use `_override` for the parameter?



src/launcher/executor.cpp (lines 186 - 195)


First, I would like all the preparation to be done before the fork (because 
after the fork, we technically cannot do any async signal unsafe work).

Second, I don't think you need to search for the `rootVolume`. You can just 
simply assume it exists and fail if it doesn't. So the logic should be:
```
Option rootfs;

if (sandboxDirectory is some) {
  // This is the case where the user specifies a
  // rootfs for the command.
  if (current user is not root) {
error message
abort();
  }
  
  if (.rootfs does not exist) {
error message
abort();
  }
  
  rootfs = path::join(...);
  sandbox = path::join(...);
  
  if (sandbox does not exist) {
mkdir sandbox
  }
  
  mount the sandbox
}

...
fork()
...

// In the child.
if (rootfs.isSome()) {
  chroot(rootfs)
  chdir(sandboxDirectory)
  su(user)
}
```



src/launcher/executor.cpp (lines 709 - 710)


Please mention that these flags are only meaningful if rootfs is used for 
the user command.



src/slave/slave.cpp (lines 3227 - 3234)


I don't think you need to do this check for mac (it's a silent ignore 
anyway). The filesystem isolator is going to reject the task during launch on 
Mac.



src/slave/slave.cpp (lines 3316 - 3325)


Is it possible to use the non-shell version so that you don't need to worry 
about escaping?


- Jie Yu


On Oct. 2, 2015, 12:16 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Oct. 2, 2015, 12:16 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
> https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
>   src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-01 Thread Timothy Chen 

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

(Updated Oct. 2, 2015, 12:16 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs (updated)
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
  src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
  src/slave/containerizer/mesos/containerizer.hpp 
4c1419290645ad4c44360a81618a6cea7ad190df 
  src/slave/containerizer/mesos/containerizer.cpp 
b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
  src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38900: Update command executor to support rootfs.

2015-10-01 Thread Timothy Chen 

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

(Updated Oct. 2, 2015, 12:16 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341 
  src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55 
  src/slave/containerizer/mesos/containerizer.hpp 
4c1419290645ad4c44360a81618a6cea7ad190df 
  src/slave/containerizer/mesos/containerizer.cpp 
b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
  src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38900: Update command executor to support rootfs.

2015-09-30 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [38900]

All tests passed.

- Mesos ReviewBot


On Sept. 30, 2015, 6:09 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> ---
> 
> (Updated Sept. 30, 2015, 6:09 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 4c1419290645ad4c44360a81618a6cea7ad190df 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
>   src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Review Request 38900: Update command executor to support rootfs.

2015-09-30 Thread Timothy Chen 

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

Review request for mesos, Jie Yu and Jiang Yan Xu.


Repository: mesos


Description
---

Update command executor to support rootfs.


Diffs
-

  src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
  src/slave/containerizer/mesos/containerizer.hpp 
4c1419290645ad4c44360a81618a6cea7ad190df 
  src/slave/containerizer/mesos/containerizer.cpp 
b904b2d88e9b62fa4ba312c4569a4d89b0dc6052 
  src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c 

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


Testing
---

make check


Thanks,

Timothy Chen