----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68366/#review207557 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp Line 152 (original), 152 (patched) <https://reviews.apache.org/r/68366/#comment290950> We can simplify this comment to something like ``` If we have a protecting a subset of ports, then only collect this listen socket if it falls within the protected range. ``` src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 298 (patched) <https://reviews.apache.org/r/68366/#comment290951> The default is to protect all ports, in which case `protectedPorts` is `None()`. I would have though that this would break tests, but maybe we don't cover that case? src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 342 (patched) <https://reviews.apache.org/r/68366/#comment290952> "protected ports range" src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 343 (patched) <https://reviews.apache.org/r/68366/#comment290946> `flags.container_ports_protected_range` is already an interval set. You don't need to convert it to resources and back again. src/slave/main.cpp Lines 329 (patched) <https://reviews.apache.org/r/68366/#comment290947> Conventionally, isolator flag checks are done in the `create` function for the isolator, so you can move this to `NetworkPortsIsolatorProcess::create`. src/tests/containerizer/ports_isolator_tests.cpp Lines 241 (patched) <https://reviews.apache.org/r/68366/#comment290959> Ports are 16 bit? If you are checking for out of range, don't you want `EXPECT_ERROR` here? src/tests/containerizer/ports_isolator_tests.cpp Lines 249 (patched) <https://reviews.apache.org/r/68366/#comment290957> This should be `EXPECT_ERROR` since you expect the flag to error out? src/tests/containerizer/ports_isolator_tests.cpp Lines 254 (patched) <https://reviews.apache.org/r/68366/#comment290956> This should be `EXPECT_ERROR` since you expect the flag to error out? src/tests/containerizer/ports_isolator_tests.cpp Lines 1017 (patched) <https://reviews.apache.org/r/68366/#comment290958> Just for robustness, ``` CHECK_NE(taskPort, usedPort); ``` - James Peach On Aug. 17, 2018, 6:27 p.m., Xudong Ni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68366/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2018, 6:27 p.m.) > > > Review request for mesos and James Peach. > > > Bugs: MESOS-9133 > https://issues.apache.org/jira/browse/MESOS-9133 > > > Repository: mesos > > > Description > ------- > > For a network isolator disabled environment, in practice, there could > be a lot of users already binding to ephemeral ports; It would take > a lot of efforts to find/notify/modify those apps; In order to take > advantage of network isolator and enable it in such system, it would > be useful to add mesos-agent configuration option to allow enforce > port isolation in only the specified certain port range > > > Diffs > ----- > > docs/configuration/agent.md 4e50b681bb956d559da6bf1d2c504099aae3cafb > docs/isolators/network-ports.md 5d14fc2985e099783b09e2a19f99641b4ddbd768 > src/slave/containerizer/mesos/isolators/network/ports.hpp > 6944d01e0f8a11eda381ef1754f19ee0cf9359c8 > src/slave/containerizer/mesos/isolators/network/ports.cpp > 2a7ff2530f898cf892739c715b07b3387b423ed9 > src/slave/flags.hpp 88c35da5fd754abbd4bd316e1fa9efa4a70a6b8c > src/slave/flags.cpp 54d9acc8693f53294bdc2a88183cac84a8dfbfd9 > src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788 > src/tests/containerizer/ports_isolator_tests.cpp > db080c4e9c8b0c036294a8f7a42617ca1231f884 > > > Diff: https://reviews.apache.org/r/68366/diff/4/ > > > Testing > ------- > > New test added to test feature: > [ OK ] NetworkPortsIsolatorTest.ROOT_NC_PortEnforcementProtectedPort > (1812 ms) > [----------] 1 test from NetworkPortsIsolatorTest (1813 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (1826 ms total) > [ PASSED ] 1 test. > > Existing test updated to test the negative cases: > > [ RUN ] NetworkPortsIsolatorTest.ROOT_IsolatorFlags > [ OK ] NetworkPortsIsolatorTest.ROOT_IsolatorFlags (69 ms) > [----------] 1 test from NetworkPortsIsolatorTest (70 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (82 ms total) > [ PASSED ] 1 test. > > Existing test for isolator feature: > > [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1821 ms) > [----------] 1 test from NetworkPortsIsolatorTest (1822 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (1836 ms total) > [ PASSED ] 1 test. > > > Thanks, > > Xudong Ni > >
