mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 60765: Added basic `network/ports` isolator tests.
Date Wed, 23 Aug 2017 07:58:51 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message