Re: Review Request 51277: Added an 'ns::enter' helper.

[Benjamin Hindman]

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

(Updated Sept. 25, 2016, 4:04 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues.


Repository: mesos


Description
---

Added an 'ns::enter' helper.


Diffs (updated)
-

  src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 
  src/tests/containerizer/ns_tests.cpp 6aa5ade9a25356057ace609fb875f070aca1ec0e 

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


Testing
---


Thanks,

Benjamin Hindman

Re: Review Request 51277: Added an 'ns::enter' helper.

[haosdent huang]

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




src/linux/ns.hpp (lines 79 - 130)


It would be better that we move this to ``, so that we could 
reuse these in ``.



src/linux/ns.hpp (lines 230 - 282)


Move this to `ns::enter` in ``, than subprocess could reuse 
this part as well.


- haosdent huang


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51277/
> ---
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an 'ns::enter' helper.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 
> 
> Diff: https://reviews.apache.org/r/51277/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 51277: Added an 'ns::enter' helper.

[Jie Yu]

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




src/linux/ns.hpp (line 230)


One suggestion on naming of this function. Looks like this is more like a 
`spawn` as it does actually clone the namespaces. So maybe we rename this to 
`spawn`?


- Jie Yu


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51277/
> ---
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an 'ns::enter' helper.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 
> 
> Diff: https://reviews.apache.org/r/51277/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 51277: Added an 'ns::enter' helper.

[Jie Yu]

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


Fix it, then Ship it!




LGTM!


src/linux/ns.hpp (line 283)


I'd add a comment saying that `sockets[0]` is the read end and `sockets[1]` 
is the write end.

Unlike `pipe`, socketpair is bidirectional. Therefore, comment is important.



src/linux/ns.hpp (line 397)


Use assert instead. CHECK will initialize glog, which is not async safe.



src/linux/ns.hpp (line 399)


streaming functions are definitey not async safe.

Also `strerror` is not safe as well

http://austin-group-l.opengroup.narkive.com/jBp07fPN/adding-simple-string-functions-to-async-signal-safe-list

You forgot to remove it after debugging?

We should not add side effect to this helper function. I'd suggest we 
simply remove it.

i think the only reason setns can fail here is EPERM. Maybe you should add 
a comment saything that this function should only be invoked when the caller is 
privileged.



src/linux/ns.hpp (line 402)


`exit` is not async safe because it'll invoke destructors. use `_exit` 
instead



src/linux/ns.hpp (line 411)


Can you add some optimizaiton here to avoid the extra fork if 
`fds.get(CLONE_NEWPID).isNone() || fds.get(CLONE_NEWUSER).isNone()`? At least 
add a TODO?



src/linux/ns.hpp (line 422)


Ditto on using `_exit`



src/linux/ns.hpp (line 430)


os::clone is not async signal safe. It does an dynamic memory allocation on 
the heap. We may want to allow os::clone to take a pointer to the stack. In 
ns::enter, we can pre-allocate the stack on the heap.



src/linux/ns.hpp (lines 434 - 435)


I don't get this comment. f() below will eventually exec the user command, 
right? What do you mean by `exit no matter what`.



src/linux/ns.hpp (line 452)


Ditto on using `_exit`


- Jie Yu


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51277/
> ---
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an 'ns::enter' helper.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 
> 
> Diff: https://reviews.apache.org/r/51277/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 51277: Added an 'ns::enter' helper.

[Kevin Klues]

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




src/linux/ns.hpp (line 269)


s/namespaace/namespace


- Kevin Klues


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51277/
> ---
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an 'ns::enter' helper.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 
> 
> Diff: https://reviews.apache.org/r/51277/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 51277: Added an 'ns::enter' helper.

[haosdent huang]

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




src/linux/ns.hpp (lines 248 - 254)


I saw oci didn't enter `user` and `cgroup`. 
https://github.com/docker/docker/blob/master/oci/defaults_linux.go#L96-L102 

Should we follow it?


- haosdent huang


On Aug. 22, 2016, 4:54 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51277/
> ---
> 
> (Updated Aug. 22, 2016, 4:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an 'ns::enter' helper.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 2d6c359ed24d6e964882f34df60d8182491a27c9 
> 
> Diff: https://reviews.apache.org/r/51277/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>