mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 53264: Added test for CNI port-mapper plugin.
Date Tue, 28 Feb 2017 04:42:21 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/#review166956
-----------------------------------------------------------




src/tests/containerizer/cni_isolator_tests.cpp (line 146)
<https://reviews.apache.org/r/53264/#comment239077>

    In fact, let's create a CniIsolatorPortMapperTest that inherits form CniIsolatorTest,
and move all the port mapper related logic to that fixture.



src/tests/containerizer/cni_isolator_tests.cpp (line 164)
<https://reviews.apache.org/r/53264/#comment239049>

    Align 'fi'?



src/tests/containerizer/cni_isolator_tests.cpp (lines 1093 - 1097)
<https://reviews.apache.org/r/53264/#comment239076>

    Instead of that, can we use `__MESOS_TEST__2`?



src/tests/containerizer/cni_isolator_tests.cpp (line 1121)
<https://reviews.apache.org/r/53264/#comment239103>

    you can use `getLauncherDir` here (in src/tests/utils.cpp)?



src/tests/containerizer/cni_isolator_tests.cpp (lines 1131 - 1132)
<https://reviews.apache.org/r/53264/#comment239079>

    Any reason you need to create the containerizer yourself? I think container status is
part of the task status update as well. Can you get that from there instead?



src/tests/containerizer/cni_isolator_tests.cpp (lines 1172 - 1178)
<https://reviews.apache.org/r/53264/#comment239104>

    You can combine these into `createContainerInfo`



src/tests/containerizer/cni_isolator_tests.cpp (line 1186)
<https://reviews.apache.org/r/53264/#comment239105>

    Instead of that, can you get a random port from the offer?



src/tests/containerizer/cni_isolator_tests.cpp (lines 1198 - 1205)
<https://reviews.apache.org/r/53264/#comment239106>

    I think you can get container status from `TaskStatus`, right?



src/tests/containerizer/cni_isolator_tests.cpp (lines 1207 - 1209)
<https://reviews.apache.org/r/53264/#comment239107>

    I'd suggest we don't directly check ip table rules. I think using curl below is sufficient.



src/tests/containerizer/cni_isolator_tests.cpp (lines 1212 - 1227)
<https://reviews.apache.org/r/53264/#comment239108>

    Can we use `__address__.ip` here? We should skip the test if `__address__.ip` is local
address.



src/tests/containerizer/cni_isolator_tests.cpp (lines 1231 - 1242)
<https://reviews.apache.org/r/53264/#comment239109>

    I'll probably set a time limit, rather than using number of retries:
    ```
    Duration waited = Duration::zero();
    do {
      ...
      
      os::sleep(Milliseconds(100));
      waited += Milliseconds(100);
    } while (waited < Seconds(10));
    
    EXPECT_LE(waited, Seconds(5));
    ```


- Jie Yu


On Feb. 27, 2017, 10:13 p.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53264/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2017, 10:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6022
>     https://issues.apache.org/jira/browse/MESOS-6022
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test for CNI port-mapper plugin.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/cni_isolator_tests.cpp cb893d3ef005a9cc60c40768fa669b27c4863020

> 
> Diff: https://reviews.apache.org/r/53264/diff/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>


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