You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/04/03 04:15:23 UTC
Re: Review Request 19702: Added linux routing library for network isolation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/
-----------------------------------------------------------
(Updated April 3, 2014, 2:15 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Added impl. for IP packet filter.
Repository: mesos-git
Description (updated)
-------
UPDATE:
Added impl. (including tests) for managing IP packet filter.
------
1) adjusted a few interfaces per review comments.
2) added impl. (including tests) for managing links.
I'll be adding impl. for managing filters soon (currently, they return Error("Unimplemented").)
------
Hey guys, I send this review in order to get an idea about the interface design.
Feel free to jump in to express your thoughts, suggestions, concerns, etc.
Thanks!
Diffs (updated)
-----
configure.ac 5404dc2
src/Makefile.am 95f133d
src/linux/routing.hpp PRE-CREATION
src/linux/routing.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/19702/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39422
-----------------------------------------------------------
configure.ac
<https://reviews.apache.org/r/19702/#comment71812>
i think it's unnecessary to make this so bold. it's already an error so a simple message is better:
Network isolator is only supported on Linux.
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71813>
__LINUX_ROUTING_HPP__ ?
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71814>
consider putting each namespace in a separate header in a routing/ folder instead.
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71815>
why are these extern?
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71817>
explicit
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71816>
const std::string& return
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71818>
explicit
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment71819>
const std::set<std::string>& return
src/linux/routing.cpp
<https://reviews.apache.org/r/19702/#comment71820>
consider an internal namespace for this if it's only used in this translation unit.
maybe class ManagedNetlink ?
in c++11 this should be a std::unique_ptr with a custom deleter. you might want a TODO.
src/linux/routing.cpp
<https://reviews.apache.org/r/19702/#comment71821>
this isn't C - move these declarations down to where you use them first.
src/linux/routing.cpp
<https://reviews.apache.org/r/19702/#comment71822>
else {
return !link.isNone();
}
src/linux/routing.cpp
<https://reviews.apache.org/r/19702/#comment71824>
these magic number offsets should be constants
src/linux/routing.cpp
<https://reviews.apache.org/r/19702/#comment71826>
it seems we really need some support for this pattern as a macro somewhere.
- Dominic Hamon
On April 2, 2014, 7:15 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 7:15 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> UPDATE:
>
> Added impl. (including tests) for managing IP packet filter.
>
> ------
>
> 1) adjusted a few interfaces per review comments.
> 2) added impl. (including tests) for managing links.
>
> I'll be adding impl. for managing filters soon (currently, they return Error("Unimplemented").)
>
>
> ------
>
> Hey guys, I send this review in order to get an idea about the interface design.
>
> Feel free to jump in to express your thoughts, suggestions, concerns, etc.
>
> Thanks!
>
>
> Diffs
> -----
>
> configure.ac 5404dc2
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39739
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19981, 19982, 19702]
All tests passed.
- Mesos ReviewBot
On April 7, 2014, 6:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. It's reviewable.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39716
-----------------------------------------------------------
configure.ac
<https://reviews.apache.org/r/19702/#comment72329>
I use something like to add the include path.
PKG_CHECK_MODULES(LIBNL, libnl-3.0 >= 3.2.8 libnl-route-3.0 libnl-genl-3.0)
AC_SUBST(LIBNL_CFLAGS)
AC_SUBST(LIBNL_LIBS)
Have a look at 3.1 and 3.2 on this page:
https://www.flameeyes.eu/autotools-mythbuster/pkgconfig/pkg_check_modules.html
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72339>
reason for externs? I don't think you need to use extern unless there is a _linkage_ issue? Same for the rest of this file.
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72340>
s/links/link
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72342>
What is the inclusiveness for Value::Range? [begin, end]? Should we make this the same to avoid inconsistency? Also conversion from Value::Range could be made more straightforward?
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72343>
let's promote the comment about inclusiveness as a class level comment.
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72344>
s/ehe/the
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72346>
&& in the front or end of the line?
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72347>
I am curious about the reason for mixing two orders here?
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72348>
I am curious about the reason for mixing network and host orders here?
src/linux/routing.hpp
<https://reviews.apache.org/r/19702/#comment72350>
for create/update and maybe other functions, 'filter not found' isn't the only reason they fail.
- Chi Zhang
On April 7, 2014, 6:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. It's reviewable.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review40226
-----------------------------------------------------------
Hey guys,
I recently refactored the code in this patch. As a result, it's now much easier to add a new filter type.
I've also introduced "priority" for filters such that we can explicitly set the preference order between filters.
This patch has now been split into 4 parts:
https://reviews.apache.org/r/20292/
https://reviews.apache.org/r/20295/
https://reviews.apache.org/r/20296/
https://reviews.apache.org/r/20297/
I'll close this patch.
- Jie Yu
On April 7, 2014, 6:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. It's reviewable.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Jie Yu <yu...@gmail.com>.
> On April 8, 2014, 1:05 a.m., Chi Zhang wrote:
> > src/linux/routing.cpp, line 446
> > <https://reviews.apache.org/r/19702/diff/7/?file=551346#file551346line446>
> >
> > missing a:
> > rtnl_u32_set_cls_terminal(cls);
> >
> > see 00a3957606c4c6696dedea03c131e0af83958e90
> >
> > it stops matching following classifiers if the packet is matched at a filter without actions.
> >
See line 746 and line 1504
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39752
-----------------------------------------------------------
On April 7, 2014, 6:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. It's reviewable.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39752
-----------------------------------------------------------
src/linux/routing.cpp
<https://reviews.apache.org/r/19702/#comment72363>
missing a:
rtnl_u32_set_cls_terminal(cls);
see 00a3957606c4c6696dedea03c131e0af83958e90
it stops matching following classifiers if the packet is matched at a filter without actions.
- Chi Zhang
On April 7, 2014, 6:25 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 6:25 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. It's reviewable.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/
-----------------------------------------------------------
(Updated April 7, 2014, 6:25 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Allowed to get link statistics (e.g., tx_packets, rx_bytes, etc.).
Repository: mesos-git
Description
-------
See summary. It's reviewable.
Diffs (updated)
-----
configure.ac c1de6d7
src/Makefile.am 95f133d
src/linux/routing.hpp PRE-CREATION
src/linux/routing.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/19702/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39501
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19981, 19982, 19702]
All tests passed.
- Mesos ReviewBot
On April 4, 2014, 1:31 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 4, 2014, 1:31 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. It's reviewable.
>
>
> Diffs
> -----
>
> configure.ac 5404dc2
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/
-----------------------------------------------------------
(Updated April 4, 2014, 1:31 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Added impl. for managing ICMP packet filters.
Repository: mesos-git
Description (updated)
-------
See summary. It's reviewable.
Diffs (updated)
-----
configure.ac 5404dc2
src/Makefile.am 95f133d
src/linux/routing.hpp PRE-CREATION
src/linux/routing.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/19702/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 19702: Added linux routing library for network isolation.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19702/#review39389
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19981, 19982, 19702]
All tests passed.
- Mesos ReviewBot
On April 3, 2014, 2:15 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19702/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 2:15 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> UPDATE:
>
> Added impl. (including tests) for managing IP packet filter.
>
> ------
>
> 1) adjusted a few interfaces per review comments.
> 2) added impl. (including tests) for managing links.
>
> I'll be adding impl. for managing filters soon (currently, they return Error("Unimplemented").)
>
>
> ------
>
> Hey guys, I send this review in order to get an idea about the interface design.
>
> Feel free to jump in to express your thoughts, suggestions, concerns, etc.
>
> Thanks!
>
>
> Diffs
> -----
>
> configure.ac 5404dc2
> src/Makefile.am 95f133d
> src/linux/routing.hpp PRE-CREATION
> src/linux/routing.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/19702/diff/
>
>
> Testing
> -------
>
> make check
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>