----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60765/#review183314 -----------------------------------------------------------
src/tests/containerizer/ports_isolator_tests.cpp Lines 39-40 (patched) <https://reviews.apache.org/r/60765/#comment259611> These two lines should be put under the two lines below. src/tests/containerizer/ports_isolator_tests.cpp Lines 58-59 (patched) <https://reviews.apache.org/r/60765/#comment259623> Mind to explain why advancing the clock will cause updates to be resent? src/tests/containerizer/ports_isolator_tests.cpp Lines 60 (patched) <https://reviews.apache.org/r/60765/#comment259612> Suggest to rename to `awaitStatusUpdateAcked`. src/tests/containerizer/ports_isolator_tests.cpp Lines 80 (patched) <https://reviews.apache.org/r/60765/#comment259625> Why do we need a template function? src/tests/containerizer/ports_isolator_tests.cpp Lines 85 (patched) <https://reviews.apache.org/r/60765/#comment259626> Can we just do `set_type(HealthCheck::TCP)`? src/tests/containerizer/ports_isolator_tests.cpp Lines 145 (patched) <https://reviews.apache.org/r/60765/#comment259622> s/Verify/This test verifies/ Ditto for all other places. src/tests/containerizer/ports_isolator_tests.cpp Lines 260 (patched) <https://reviews.apache.org/r/60765/#comment259629> In `CreateSlaveFlags()`, I see `flags.resources` will be explicitly set to `defaultAgentResourcesString` which is `cpus:2;gpus:0;mem:1024;disk:1024;ports:[31000-32000]`, so this test actually covers the case that operator explicitly sets ports resources in agent's `--resources` flag. I think we may need another test that we set `flags.resources` to `None()` after `CreateSlaveFlags()` is called so that the code in `NetworkPortsIsolatorProcess::create()` to default the ports if it is missing can be covered. src/tests/containerizer/ports_isolator_tests.cpp Lines 299 (patched) <https://reviews.apache.org/r/60765/#comment259624> s/1/1u/ Ditto for the other place. src/tests/containerizer/ports_isolator_tests.cpp Lines 328-329 (patched) <https://reviews.apache.org/r/60765/#comment259627> Since this is a health check status update, I think we also need: ``` EXPECT_EQ( TaskStatus::REASON_TASK_HEALTH_CHECK_STATUS_UPDATED, statusHealthy->reason()); EXPECT_TRUE(statusHealthy->has_healthy()); EXPECT_TRUE(statusHealthy->healthy()); ``` Ditto for all other places. src/tests/containerizer/ports_isolator_tests.cpp Lines 567 (patched) <https://reviews.apache.org/r/60765/#comment259634> What if `taskPort + 1` making it out of agent port range? - Qian Zhang On July 29, 2017, 8:02 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60765/ > ----------------------------------------------------------- > > (Updated July 29, 2017, 8:02 a.m.) > > > Review request for mesos, Qian Zhang and Jiang Yan Xu. > > > Bugs: MESOS-7675 > https://issues.apache.org/jira/browse/MESOS-7675 > > > Repository: mesos > > > Description > ------- > > Added tests to verify that the `network/ports` is able to correctly > terminate only tasks that use rogue TCP ports. > > > Diffs > ----- > > src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/60765/diff/10/ > > > Testing > ------- > > make check (Fedora 26) > > > Thanks, > > James Peach > >
