You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2014/06/11 20:54:15 UTC
Review Request 22471: Added unit tests for Port Mapping Network Isolator
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs
-----
src/Makefile.am 4a3f2e1
src/tests/port_mapping_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review45560
-----------------------------------------------------------
Bad patch!
Reviews applied: [21594, 22471]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
../../src/tests/port_mapping_tests.cpp: In member function 'virtual void PortMappingTest_ROOT_TooManyContainersTest_Test<gtest_TypeParam_>::TestBody()':
../../src/tests/port_mapping_tests.cpp:1015:9: error: 'class mesos::internal::slave::Flags' has no member named 'max_containers'
make[4]: *** [tests/mesos_tests-port_mapping_tests.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [check-am] Error 2
make[2]: *** [check] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On June 12, 2014, 6:45 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 12, 2014, 6:45 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/tests/port_mapping_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
> On June 24, 2014, 9:43 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, line 780
> > <https://reviews.apache.org/r/22471/diff/4/?file=616170#file616170line780>
> >
> > Should we parse them into net::IP?
I thought about it, but we only just need the string representation.
> On June 24, 2014, 9:43 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, line 810
> > <https://reviews.apache.org/r/22471/diff/4/?file=616170#file616170line810>
> >
> > Why option here?
net::IP doesn't have a default constructor.
> On June 24, 2014, 9:43 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, line 910
> > <https://reviews.apache.org/r/22471/diff/4/?file=616170#file616170line910>
> >
> > Why this comment?
not really a reason, other than it is used in all the test cases in this file.
> On June 24, 2014, 9:43 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, lines 903-904
> > <https://reviews.apache.org/r/22471/diff/4/?file=616170#file616170line903>
> >
> > You don't need these two 'None()' here.
won't compile.
> On June 24, 2014, 9:43 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, line 922
> > <https://reviews.apache.org/r/22471/diff/4/?file=616170#file616170line922>
> >
> > Probably add a sleep here (instead of busy waiting)? like os::sleep(Milliseconds(10));
I think it would help, but i don't think performance is a big deal here. Also for being consistent with other tests.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46562
-----------------------------------------------------------
On June 25, 2014, 2:10 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 2:10 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46562
-----------------------------------------------------------
My comments are only for the first test. Apply the same comments to all other tests.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment81998>
Please move this up (below 'using namespace process;').
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82003>
Result<string> _eth0 = link::eth0();
...
eth0 = _eth0.get();
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82004>
This fits in one line?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment81999>
LOG(INFO) << "Using " << eth0 << " ...";
Here and everywhere else.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82005>
Result<net::IP> _hostIP = net::ip(eth0);
...
hostIP = _hostIP.get();
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82001>
CHECK_SOME should be sufficient.
(CHECK_SOME checks isSome())
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82002>
Do not use '+' when streaming.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82006>
Fit in one line?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82033>
Should we parse them into net::IP?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82007>
ASSERT_SOME(os::user());
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82008>
Why option here?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82035>
can you rename 'file1' to 'container1OK'. It's more easy to read in that way.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82037>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82039>
Rename 'fileLoopback' to 'contentViaLoopback'
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82040>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82041>
It's a little hard to follow here. Would you please add some comments to explain the flow here?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82043>
Create a helper function to create the launchFlags?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82044>
You don't need these two 'None()' here.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82046>
Why this comment?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82047>
Probably add a sleep here (instead of busy waiting)? like os::sleep(Milliseconds(10));
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82048>
I would prefer using 'command1' and 'command2'. That's more clear and align with container1 and container2.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82049>
Ditto. A helper function can remove all these duplicated logics.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82051>
Ditto.
- Jie Yu
On June 24, 2014, 5:02 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 5:02 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46614
-----------------------------------------------------------
Bad patch!
Reviews applied: [22831, 22851, 22764, 22852, 21594]
Failed command: git apply --index 21594.patch
Error:
error: patch failed: src/Makefile.am:566
error: src/Makefile.am: patch does not apply
- Mesos ReviewBot
On June 25, 2014, 2:10 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 2:10 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
> On June 25, 2014, 4:32 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, line 743
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line743>
> >
> > Can you remind me why this needs to be MesosTest?
> On June 25, 2014, 4:32 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, lines 859-861
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line859>
> >
> > I would suggest not making 'pipes' and 'command1' 'command2' a class field. Especially for pipes, make sure all pipes you created get closed.
kept pipes as a class member so that it doesn't have to passed to launchHelper every time, but I moved the ::pipe call down to individual tests.
command1 and command2 are local var now.
> On June 25, 2014, 4:32 p.m., Jie Yu wrote:
> > src/tests/isolator_tests.cpp, line 841
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line841>
> >
> > We've changed to MesosContainerizerLaunch::NAME
will wait until I merge with master.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46636
-----------------------------------------------------------
On June 26, 2014, 6:03 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 6:03 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46636
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82141>
Please separate those includes. It's ok that you include json.hpp, net.hpp, tests/flags.hpp and mesos_containerizer.hpp, without the ifdef guard.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82145>
Can you remind me why this needs to be MesosTest?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82144>
Please stick to style guide. We put { in the next line for functions. Here and everywhere else.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82146>
Could you please explain why we need to get external name servers? For example, to do DNS testing? to get an valid external host?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82147>
You should be able to combine these two lines:
foreach (const string& line, strings::split(read.get(), "\n")) {
...
}
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82143>
Please stick to style guide. The indent for those parameters should be 4 spaces.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82149>
Would you please swap these two arguments to be consistent with the launcher code?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82154>
Would you please move this down below ommandInfo.set_value(command);
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82150>
New lines above and below.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82153>
We've changed to MesosContainerizerLaunch::NAME
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82157>
I would suggest not making 'pipes' and 'command1' 'command2' a class field. Especially for pipes, make sure all pipes you created get closed.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82158>
Style. 4 spaces please.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82159>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82162>
You can combine these two:
EXPECT_EQ(0, os::system(
...
...));
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82160>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82161>
Not checking the error code?
EXPECT_EQ(0, os::system(...));
Here and every where else.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82163>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82164>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82165>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82166>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82167>
Ditto.
- Jie Yu
On June 25, 2014, 2:10 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 2:10 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46764
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82332>
Move this above #include "linux/routing/link/link.hpp"
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82338>
You should call "cleanup" here?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82333>
Use tests::flags.launcher_dir to avoid confusion
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82336>
It should be:
char dummy;
ASSERT_LT(0, ::write(pipes[1], &dummy, sizeof(dummy)));
Here and everywhere else
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82337>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82339>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82340>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82341>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82342>
Ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82343>
Ditto.
- Jie Yu
On June 26, 2014, 6:03 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 6:03 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
> On June 26, 2014, 6:29 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, lines 895-906
> > <https://reviews.apache.org/r/22471/diff/5-6/?file=616523#file616523line895>
> >
> > Could these be setup by the operators on the host irrespective of the test? If yes, removing them seems dangerous?
In theory we probably shouldn't do this, as eth0 and lo are indeed shared resources of the host, but we've in effect taken full control of the network resources anyway, such as host ephemeral range, etc.
The isolator is fine even if we don't remove the qdiscs here and I am fine with that. The only concern I can think of right now is these qidcs could have fiters with port ranges that overlap with the ones we are going to add. Things might fail later in a more obscure way.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46771
-----------------------------------------------------------
On June 26, 2014, 7:05 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 7:05 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46771
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82346>
Only some tests depend on nameservers. Do the ASSERT in those tests.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82347>
Could these be setup by the operators on the host irrespective of the test? If yes, removing them seems dangerous?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82348>
s/remove/removing/
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82349>
Instead of making this a member variable, just pass them to the launchHelper.
- Vinod Kone
On June 26, 2014, 6:03 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 6:03 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46793
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On June 26, 2014, 7:05 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 7:05 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46790
-----------------------------------------------------------
Patch looks great!
Reviews applied: [22471]
All tests passed.
- Mesos ReviewBot
On June 26, 2014, 7:05 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 7:05 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 26, 2014, 7:05 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description (updated)
-------
These tests test different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs (updated)
-----
src/tests/isolator_tests.cpp 6fef4e0
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 26, 2014, 6:03 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs (updated)
-----
src/tests/isolator_tests.cpp 6fef4e0
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
> On June 25, 2014, 7:15 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, lines 55-66
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line55>
> >
> > @Jie: Why? If those includes are only used by network isolator code I prefer them to be guarded by #ifdef.
properly #ifdef'ed and in alphabetical order, both for includes and 'using' statements. #ifdef's are split up when necessary.
> On June 25, 2014, 7:15 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, line 769
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line769>
> >
> > s/external//
trying to make a distinction to the name server on localhost?
> On June 25, 2014, 7:15 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, lines 786-800
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line786>
> >
> > Initialization of these is very straightforward. I wonder if it's better to just define them in each test for better readability. I'll leave it up to you.
Tt was like that originally. I felt this could be better because it's a lot of duplicate code and different cases actually use different subsets of them.
> On June 25, 2014, 7:15 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, line 927
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line927>
> >
> > Is 'nc' guaranteed to be present on every linux box by default? If not maybe the tests should be disabled? Same with arping?
checked in SetupTestCase.
> On June 25, 2014, 7:15 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, line 780
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line780>
> >
> > Is resolv.conf guaranteed to have atleast one external name server? If not, looks like the tests that use this will fail.
will abort for now. added a TODO.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46659
-----------------------------------------------------------
On June 26, 2014, 6:03 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 6:03 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Vinod Kone <vi...@gmail.com>.
> On June 25, 2014, 7:15 p.m., Vinod Kone wrote:
> > src/tests/isolator_tests.cpp, line 769
> > <https://reviews.apache.org/r/22471/diff/5/?file=616523#file616523line769>
> >
> > s/external//
>
> Chi Zhang wrote:
> trying to make a distinction to the name server on localhost?
yup. you can drop this issue.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46659
-----------------------------------------------------------
On June 26, 2014, 6:03 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 26, 2014, 6:03 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46659
-----------------------------------------------------------
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82189>
@Jie: Why? If those includes are only used by network isolator code I prefer them to be guarded by #ifdef.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82187>
should this be ifdefed too?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82190>
reorder alphabetically.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82191>
s/external//
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82208>
Is resolv.conf guaranteed to have atleast one external name server? If not, looks like the tests that use this will fail.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82210>
Initialization of these is very straightforward. I wonder if it's better to just define them in each test for better readability. I'll leave it up to you.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82192>
s/assit/assist/
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82194>
s/Port/port/
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82195>
s/partialPorts1/container1Ports/ ?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82196>
ditto.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82207>
These need not be member variables. Just define them in the tests.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82197>
s/connects to/attempts to connect to/
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82211>
Is 'nc' guaranteed to be present on every linux box by default? If not maybe the tests should be disabled? Same with arping?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82200>
what about public port like you did in the previous test?
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82202>
s/erroPort/errorPort/
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22471/#comment82203>
s/UDP/TCP/
- Vinod Kone
On June 25, 2014, 2:10 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 2:10 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 25, 2014, 2:10 a.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs (updated)
-----
src/tests/isolator_tests.cpp 0bbec09
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 24, 2014, 5:02 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
fixed diff.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs (updated)
-----
src/tests/isolator_tests.cpp 0bbec09
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review46541
-----------------------------------------------------------
Bad diff?
- Jie Yu
On June 24, 2014, 7:33 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 7:33 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 24, 2014, 7:33 a.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
depends-on: 21594
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs
-----
src/tests/isolator_tests.cpp 6fef4e0
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 24, 2014, 7:32 a.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
converted to isolator_tests.
rebased to use the new subprocess API.
addressed comments.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs (updated)
-----
src/tests/isolator_tests.cpp 6fef4e0
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
> On June 17, 2014, 12:26 a.m., Ian Downes wrote:
> >
changed to isolator tests. i've dropped issues that no longer apply.
> On June 17, 2014, 12:26 a.m., Ian Downes wrote:
> > src/tests/port_mapping_tests.cpp, line 111
> > <https://reviews.apache.org/r/22471/diff/2/?file=608378#file608378line111>
> >
> > Also seen this more than once - put an implementation into stout?
This is only used here once. Mainly I am not sure how good this idea is to rely on /etc/resolv.conf to get some external hosts.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review45851
-----------------------------------------------------------
On June 24, 2014, 7:33 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 24, 2014, 7:33 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/tests/isolator_tests.cpp 6fef4e0
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review45851
-----------------------------------------------------------
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80909>
Why is this a ContainerizerTest rather than an IsolatorTest?
Directly testing the isolator itself should simplify the tests! Don't need to worry about offers, can set the ContainerID, ....
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80908>
Could implement CreateSlaveFlags here to selectively enable the network isolator.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80862>
I've seen this used at least twice to get the main interface - what about implementing it in stout?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80859>
Correct indentation.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80865>
Also seen this more than once - put an implementation into stout?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80878>
Why is this randomized? If there's a particular reason could you please comment on it, else just pick a port.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80866>
It won't ever make sense to run these tests again any other containerizer would it? Could just use ContainerizerTest<MesosContainerizer> and avoid having typed tests.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80914>
What does it mean if it already exists...?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80883>
can you get each "nc" command on a single line so it's easier to read?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80884>
Why is this-> needed?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80888>
Can you comment at the start of the test what it's doing and which connections should work?
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80891>
ditto to have each command chain on a separate line
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80885>
Other tests clear and re-use the tasks vector.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80890>
s/nee/need/
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80905>
use EXPECT_ unless you absolutely require something for the test to be able to continue.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80906>
ditto.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80900>
ditto here and elsewhere
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80901>
style, move first "echo..." to next line
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80910>
This only needs to be done explicitly when there's no slave + containerizer in the cluster that knows about the container(s), otherwise it gets done in Shutdown().
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80902>
ditto.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80903>
ditto
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80904>
ditto
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80915>
better to pluralize this and use containers2 later when you call again
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80922>
ditto
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80916>
style
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80921>
ditto
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80924>
ditto.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80926>
style for the command string
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80925>
ditto.
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80927>
ditto
src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/22471/#comment80929>
ditto, not necessary.
- Ian Downes
On June 13, 2014, 11:02 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 13, 2014, 11:02 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/tests/port_mapping_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review45659
-----------------------------------------------------------
Bad patch!
Reviews applied: [21594, 22471]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
make[1]: *** No rule to make target `mesos_tests_SOURCES', needed by `distdir'. Stop.
make: *** [distdir] Error 1
- Mesos ReviewBot
On June 13, 2014, 6:02 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 13, 2014, 6:02 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/tests/port_mapping_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 13, 2014, 6:02 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
depends on got dropped again during update.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs
-----
src/Makefile.am c91b438
src/tests/port_mapping_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review45594
-----------------------------------------------------------
Bad patch!
Reviews applied: [22471]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
make[1]: *** No rule to make target `mesos_tests_SOURCES', needed by `distdir'. Stop.
make: *** [distdir] Error 1
- Mesos ReviewBot
On June 13, 2014, 12:38 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 13, 2014, 12:38 a.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/Makefile.am c91b438
> src/tests/port_mapping_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 13, 2014, 12:38 a.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
guard port_mapping_test.cpp by WITH_NETWORK_ISOLATOR in src/Makefile.am
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs (updated)
-----
src/Makefile.am c91b438
src/tests/port_mapping_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 12, 2014, 6:45 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Changes
-------
added 'depends on'
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs
-----
src/Makefile.am 4a3f2e1
src/tests/port_mapping_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/#review45457
-----------------------------------------------------------
Bad patch!
Reviews applied: [22471]
Failed command: make -j3 distcheck GTEST_FILTER='' >/dev/null
Error:
configure: WARNING: can not find python-boto
-------------------------------------------------------------------
mesos-ec2 services will not function.
-------------------------------------------------------------------
ev.c:1531:31: warning: 'ev_default_loop_ptr' initialized and declared 'extern' [enabled by default]
ev.c: In function 'evpipe_write':
ev.c:2160:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2172:17: warning: ignoring return value of 'write', declared with attribute warn_unused_result [-Wunused-result]
ev.c: In function 'pipecb':
ev.c:2193:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
ev.c:2207:16: warning: ignoring return value of 'read', declared with attribute warn_unused_result [-Wunused-result]
In file included from /usr/include/c++/4.6/ext/hash_set:61:0,
from src/glog/stl_logging.h:54,
from src/stl_logging_unittest.cc:34:
/usr/include/c++/4.6/backward/backward_warning.h:33:2: warning: #warning This file includes at least one deprecated or antiquated header which may be removed without further notice at a future date. Please use a non-deprecated interface with equivalent functionality instead. For a listing of replacement headers and interfaces, consult the file backward_warning.h. To disable this warning use -Wno-deprecated. [-Wcpp]
In file included from src/utilities.h:73:0,
from src/googletest.h:38,
from src/stl_logging_unittest.cc:48:
src/base/mutex.h:137:0: warning: "_XOPEN_SOURCE" redefined [enabled by default]
/usr/include/features.h:166:0: note: this is the location of the previous definition
warning: no files found matching 'Makefile' under directory 'docs'
warning: no files found matching 'indexsidebar.html' under directory 'docs'
ar: creating libleveldb.a
zip_safe flag not set; analyzing archive contents...
WARNING: '.' not a valid package name; please use only.-separated package names in setup.py
package init file 'src/__init__.py' not found (or not a regular file)
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
cc1plus: warning: command line option '-Wstrict-prototypes' is valid for Ada/C/ObjC but not for C++ [enabled by default]
zip_safe flag not set; analyzing archive contents...
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
../../src/tests/port_mapping_tests.cpp: In member function 'virtual void PortMappingTest_ROOT_TooManyContainersTest_Test<gtest_TypeParam_>::TestBody()':
../../src/tests/port_mapping_tests.cpp:1015:9: error: 'class mesos::internal::slave::Flags' has no member named 'max_containers'
make[4]: *** [tests/mesos_tests-port_mapping_tests.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [check-am] Error 2
make[2]: *** [check] Error 2
make[1]: *** [check-recursive] Error 1
make: *** [distcheck] Error 1
- Mesos ReviewBot
On June 11, 2014, 6:54 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22471/
> -----------------------------------------------------------
>
> (Updated June 11, 2014, 6:54 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
>
>
> Diffs
> -----
>
> src/Makefile.am 4a3f2e1
> src/tests/port_mapping_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/22471/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22471: Added unit tests for Port Mapping Network Isolator
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22471/
-----------------------------------------------------------
(Updated June 11, 2014, 6:54 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Repository: mesos-git
Description
-------
These tests test different different protocols (TCP, UDP, ICMP, ARP, DNS) in different connection scenarios (C2C, H2C).
Diffs
-----
src/Makefile.am 4a3f2e1
src/tests/port_mapping_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/22471/diff/
Testing
-------
make check.
Thanks,
Chi Zhang