You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2014/10/23 19:45:51 UTC

Review Request 27091: Move Linux namespace functions into linux/.

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

Review request for mesos, Ben Mahler and Jie Yu.


Repository: mesos-git


Description
-------

A following commit will remove this code from stout.

Added new functions (and tests for them):
- getns() to get the namespace inode.
- killall() to kill all processes in a pid namespace.


Diffs
-----

  src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
  src/linux/ns.hpp PRE-CREATION 
  src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
  src/tests/ns_tests.hpp PRE-CREATION 
  src/tests/ns_tests.cpp PRE-CREATION 
  src/tests/setns_test_helper.hpp PRE-CREATION 
  src/tests/setns_test_helper.cpp PRE-CREATION 

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


Testing
-------

make check


Thanks,

Ian Downes

Re: Review Request 27091: Move Linux namespace functions into linux/.

Posted by Ian Downes <ia...@gmail.com>.

> On Oct. 24, 2014, 4:04 p.m., Ben Mahler wrote:
> > src/tests/setns_test_helper.hpp, line 25
> > <https://reviews.apache.org/r/27091/diff/3/?file=732835#file732835line25>
> >
> >     I guess you're using subcommand because you're anticipating commands here other than "test"?
> >     
> >     Can you just use a main?

Yes, I'm anticipating using this for other namespaces tests.


- Ian


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


On Oct. 24, 2014, 2:53 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27091/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2014, 2:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> A following commit will remove this code from stout.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
>   src/linux/ns.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
>   src/tests/ns_tests.cpp PRE-CREATION 
>   src/tests/setns_test_helper.hpp PRE-CREATION 
>   src/tests/setns_test_helper.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27091/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 27091: Move Linux namespace functions into linux/.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27091/#review58419
-----------------------------------------------------------

Ship it!



src/tests/ns_tests.cpp
<https://reviews.apache.org/r/27091/#comment99423>

    Can you just expect !namespaces.empty()



src/tests/setns_test_helper.hpp
<https://reviews.apache.org/r/27091/#comment99418>

    I guess you're using subcommand because you're anticipating commands here other than "test"?
    
    Can you just use a main?



src/tests/setns_test_helper.hpp
<https://reviews.apache.org/r/27091/#comment99416>

    string include needed



src/tests/setns_test_helper.cpp
<https://reviews.apache.org/r/27091/#comment99415>

    include these please, also include try and foreach



src/tests/setns_test_helper.cpp
<https://reviews.apache.org/r/27091/#comment99421>

    Maybe a note as to why pid is special here?


- Ben Mahler


On Oct. 24, 2014, 9:53 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27091/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2014, 9:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> A following commit will remove this code from stout.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
>   src/linux/ns.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
>   src/tests/ns_tests.cpp PRE-CREATION 
>   src/tests/setns_test_helper.hpp PRE-CREATION 
>   src/tests/setns_test_helper.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27091/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 27091: Move Linux namespace functions into linux/.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27091/
-----------------------------------------------------------

(Updated Oct. 24, 2014, 2:53 p.m.)


Review request for mesos, Ben Mahler and Jie Yu.


Changes
-------

Removed erroneous includes, included missing file.

Deferred other cleanup to a subsequent review.


Repository: mesos-git


Description
-------

A following commit will remove this code from stout.


Diffs (updated)
-----

  src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
  src/linux/ns.hpp PRE-CREATION 
  src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
  src/tests/ns_tests.cpp PRE-CREATION 
  src/tests/setns_test_helper.hpp PRE-CREATION 
  src/tests/setns_test_helper.cpp PRE-CREATION 

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


Testing
-------

make check


Thanks,

Ian Downes

Re: Review Request 27091: Move Linux namespace functions into linux/.

Posted by Ian Downes <ia...@gmail.com>.

> On Oct. 23, 2014, 6:58 p.m., Ben Mahler wrote:
> > src/linux/ns.hpp, line 65
> > <https://reviews.apache.org/r/27091/diff/2/?file=731817#file731817line65>
> >
> >     s/ns/namespace/ for the argument

I'd like to avoid using the reserved word 'namespace' in code, event for argument names.


> On Oct. 23, 2014, 6:58 p.m., Ben Mahler wrote:
> > src/linux/ns.hpp, line 119
> > <https://reviews.apache.org/r/27091/diff/2/?file=731817#file731817line119>
> >
> >     s/ns/namespace/ for the argument

I'd like to avoid using the reserved word 'namespace' in code, event for argument names.


> On Oct. 23, 2014, 6:58 p.m., Ben Mahler wrote:
> > src/linux/ns.hpp, lines 63-65
> > <https://reviews.apache.org/r/27091/diff/2/?file=731817#file731817line63>
> >
> >     Looks like this can be hidden in an 'internal' namespace?

Ideally yes, but it's used in tests.


- Ian


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


On Oct. 24, 2014, 2:53 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27091/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2014, 2:53 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> A following commit will remove this code from stout.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
>   src/linux/ns.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
>   src/tests/ns_tests.cpp PRE-CREATION 
>   src/tests/setns_test_helper.hpp PRE-CREATION 
>   src/tests/setns_test_helper.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27091/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 27091: Move Linux namespace functions into linux/.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27091/#review58201
-----------------------------------------------------------


Looks like this isn't a pure code movement from a few leftover includes and the tests, can you make it just a movement?

I also left some commments for cleaning this stuff up.

Let's try to do the following:

(1) Pure code movement.
(2) Cleanups, per my comments below.

Much appreciated!!


src/Makefile.am
<https://reviews.apache.org/r/27091/#comment99150>

    How about namespaces.hpp and namespaces_tests.cpp?
    
    Then we can clean up the function naming, and avoid all the 'ns' acronyms!
    
    ```
    ns::namespaces
    ns::getns
    ns::setns
    ns::nstype
    
    ```
    
    ```
    namespaces::available
    namespaces::get
    namespaces::set
    namespaces::flag
    ```



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99155>

    Unused?



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99151>

    Looks like this can be hidden in an 'internal' namespace?



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99157>

    s/ns/namespace/ for the argument



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99152>

    Not sure why an 'if' block wasn't used here.. seems like that code would be simpler than using the map?



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99158>

    s/ns/namespace/ for the argument



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99154>

    All the O_CLOEXEC handling here is no longer necessary, thanks to os::open.hpp.



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99159>

    s/ret/status/ or s/ret/result/
    
    Who wrote this code?? :)



src/linux/ns.hpp
<https://reviews.apache.org/r/27091/#comment99160>

    s/ns/namespace/ for the argument



src/tests/ns_tests.cpp
<https://reviews.apache.org/r/27091/#comment99156>

    Does this file exist??


- Ben Mahler


On Oct. 24, 2014, 1:20 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27091/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2014, 1:20 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> A following commit will remove this code from stout.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
>   src/linux/ns.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
>   src/tests/ns_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27091/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 27091: Move Linux namespace functions into linux/.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27091/
-----------------------------------------------------------

(Updated Oct. 23, 2014, 6:20 p.m.)


Review request for mesos, Ben Mahler and Jie Yu.


Changes
-------

Made this review the move or code (and updating tests). Later review will have new code.


Repository: mesos-git


Description (updated)
-------

A following commit will remove this code from stout.


Diffs (updated)
-----

  src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09 
  src/linux/ns.hpp PRE-CREATION 
  src/slave/containerizer/isolators/network/port_mapping.cpp 62ca9bca08de55f19bfff7b8828dfa4fb6c51f30 
  src/tests/ns_tests.cpp PRE-CREATION 

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


Testing
-------

make check


Thanks,

Ian Downes