You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2016/12/14 19:45:59 UTC

Re: Review Request 53264: Added test for CNI port-mapper plugin.

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

(Updated Dec. 14, 2016, 7:45 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 (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp 0380f2ca122c7b8ab1dac351b14f546526580e72 

Diff: https://reviews.apache.org/r/53264/diff/


Testing
-------

sudo make check


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/
-----------------------------------------------------------

(Updated March 17, 2017, 5:39 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
-------

Updated the dependency of this patch.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp 393c3e5a0adeefa02429276fc7271340fa2edb5b 


Diff: https://reviews.apache.org/r/53264/diff/9/

Changes: https://reviews.apache.org/r/53264/diff/8-9/


Testing
-------

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/
-----------------------------------------------------------

(Updated March 17, 2017, 5:33 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
-------

Addressed Jie's comemnts and rebased.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp 393c3e5a0adeefa02429276fc7271340fa2edb5b 


Diff: https://reviews.apache.org/r/53264/diff/8/

Changes: https://reviews.apache.org/r/53264/diff/7-8/


Testing
-------

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On March 16, 2017, 10:42 p.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 54 (patched)
> > <https://reviews.apache.org/r/53264/diff/7/?file=1666044#file1666044line56>
> >
> >     `__MESOS_TEST_PORT_MAPPER__`

The `network/cni` isolator looks for `__MESOS_TEST__` to mark it as a test network. So `__MESOS_TEST_PORT_MAPPER__` won't work. We could do `__MESOS_TEST__PORT_MAPPER__` but that looked weird?


- Avinash


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


On March 16, 2017, 5:24 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53264/
> -----------------------------------------------------------
> 
> (Updated March 16, 2017, 5:24 a.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/7/
> 
> 
> Testing
> -------
> 
> sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/#review169217
-----------------------------------------------------------


Fix it, then Ship it!





src/tests/containerizer/cni_isolator_tests.cpp
Lines 54 (patched)
<https://reviews.apache.org/r/53264/#comment241550>

    `__MESOS_TEST_PORT_MAPPER__`



src/tests/containerizer/cni_isolator_tests.cpp
Lines 133-136 (patched)
<https://reviews.apache.org/r/53264/#comment241551>

    No need for this?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1016 (patched)
<https://reviews.apache.org/r/53264/#comment241553>

    I think getLauncherDir should also work. No need the `.libs` suffix?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1053-1063 (patched)
<https://reviews.apache.org/r/53264/#comment241558>

    I think you can leverage `std::random_shuffle` here. I think it supports any iterator.


- Jie Yu


On March 16, 2017, 5:24 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53264/
> -----------------------------------------------------------
> 
> (Updated March 16, 2017, 5:24 a.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/7/
> 
> 
> Testing
> -------
> 
> sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/
-----------------------------------------------------------

(Updated March 16, 2017, 5:24 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
-------

Removed an unwanted blank line.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp cb893d3ef005a9cc60c40768fa669b27c4863020 


Diff: https://reviews.apache.org/r/53264/diff/7/

Changes: https://reviews.apache.org/r/53264/diff/6-7/


Testing
-------

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/
-----------------------------------------------------------

(Updated March 14, 2017, 11:13 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
-------

Refactored the code to use separate test network for port-mapper tests.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp cb893d3ef005a9cc60c40768fa669b27c4863020 


Diff: https://reviews.apache.org/r/53264/diff/6/

Changes: https://reviews.apache.org/r/53264/diff/5-6/


Testing (updated)
-------

sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/
-----------------------------------------------------------

(Updated March 13, 2017, 4:03 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
-------

Removed some unecessary headers.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp cb893d3ef005a9cc60c40768fa669b27c4863020 


Diff: https://reviews.apache.org/r/53264/diff/5/

Changes: https://reviews.apache.org/r/53264/diff/4-5/


Testing
-------

sudo make check


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53264/
-----------------------------------------------------------

(Updated March 12, 2017, 7:11 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
-------

Addressed Jie's comments.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp cb893d3ef005a9cc60c40768fa669b27c4863020 


Diff: https://reviews.apache.org/r/53264/diff/4/

Changes: https://reviews.apache.org/r/53264/diff/3-4/


Testing
-------

sudo make check


Thanks,

Avinash sridharan

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1093>
> >
> >     Instead of that, can we use `__MESOS_TEST__2`?
> 
> Avinash sridharan wrote:
>     Hi Jie, 
>     Sorry for the delay here. We will need to change the `network/cni` isolator for this right? Didn't really see a reason for this since replacing the existing CNI config would just work the same. Will keep the logic in the `network/cni` isolator simpler?
> 
> Jie Yu wrote:
>     I thought we already do 'contains' check in CNI isolator?
> 
> Avinash sridharan wrote:
>     https://github.com/apache/mesos/blob/7c3c0b03a8371922f6cc811ad74d85c97eefb494/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L693
>     
>     I think you are referring to using `strings::contains`. No unfortunately haven't added that to the `network/cni` isolator.
> 
> Jie Yu wrote:
>     Can we do that. I remember you already have the patch?

We were planning to do that for the tests for dynamically `add/delete/modify` CNI configuration, but as in this case it turned out we didn't need to so ended up not implementing it. Let me see if I can do it as part of this ticket.


- Avinash


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


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/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Jie Yu <yu...@gmail.com>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1093>
> >
> >     Instead of that, can we use `__MESOS_TEST__2`?
> 
> Avinash sridharan wrote:
>     Hi Jie, 
>     Sorry for the delay here. We will need to change the `network/cni` isolator for this right? Didn't really see a reason for this since replacing the existing CNI config would just work the same. Will keep the logic in the `network/cni` isolator simpler?
> 
> Jie Yu wrote:
>     I thought we already do 'contains' check in CNI isolator?
> 
> Avinash sridharan wrote:
>     https://github.com/apache/mesos/blob/7c3c0b03a8371922f6cc811ad74d85c97eefb494/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L693
>     
>     I think you are referring to using `strings::contains`. No unfortunately haven't added that to the `network/cni` isolator.

Can we do that. I remember you already have the patch?


- Jie


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


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/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1093>
> >
> >     Instead of that, can we use `__MESOS_TEST__2`?

Hi Jie, 
Sorry for the delay here. We will need to change the `network/cni` isolator for this right? Didn't really see a reason for this since replacing the existing CNI config would just work the same. Will keep the logic in the `network/cni` isolator simpler?


- Avinash


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


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/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1093>
> >
> >     Instead of that, can we use `__MESOS_TEST__2`?
> 
> Avinash sridharan wrote:
>     Hi Jie, 
>     Sorry for the delay here. We will need to change the `network/cni` isolator for this right? Didn't really see a reason for this since replacing the existing CNI config would just work the same. Will keep the logic in the `network/cni` isolator simpler?
> 
> Jie Yu wrote:
>     I thought we already do 'contains' check in CNI isolator?
> 
> Avinash sridharan wrote:
>     https://github.com/apache/mesos/blob/7c3c0b03a8371922f6cc811ad74d85c97eefb494/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L693
>     
>     I think you are referring to using `strings::contains`. No unfortunately haven't added that to the `network/cni` isolator.
> 
> Jie Yu wrote:
>     Can we do that. I remember you already have the patch?
> 
> Avinash sridharan wrote:
>     We were planning to do that for the tests for dynamically `add/delete/modify` CNI configuration, but as in this case it turned out we didn't need to so ended up not implementing it. Let me see if I can do it as part of this ticket.

Created a separate patch for the changes to the `network/cni` isolator.


- Avinash


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


On March 14, 2017, 11:13 p.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53264/
> -----------------------------------------------------------
> 
> (Updated March 14, 2017, 11: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/6/
> 
> 
> Testing
> -------
> 
> sudo ./bin/mesos-tests.sh --gtest_filter=CniIsolator*.*
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1212-1227 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1212>
> >
> >     Can we use `__address__.ip` here? We should skip the test if `__address__.ip` is local address.

Sorry, didn't follow your comment? You mean skip the test if __address__.ip is loopback address right?


- Avinash


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


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/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1093>
> >
> >     Instead of that, can we use `__MESOS_TEST__2`?
> 
> Avinash sridharan wrote:
>     Hi Jie, 
>     Sorry for the delay here. We will need to change the `network/cni` isolator for this right? Didn't really see a reason for this since replacing the existing CNI config would just work the same. Will keep the logic in the `network/cni` isolator simpler?
> 
> Jie Yu wrote:
>     I thought we already do 'contains' check in CNI isolator?

https://github.com/apache/mesos/blob/7c3c0b03a8371922f6cc811ad74d85c97eefb494/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L693

I think you are referring to using `strings::contains`. No unfortunately haven't added that to the `network/cni` isolator.


- Avinash


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


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/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Jie Yu <yu...@gmail.com>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1093-1097 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1093>
> >
> >     Instead of that, can we use `__MESOS_TEST__2`?
> 
> Avinash sridharan wrote:
>     Hi Jie, 
>     Sorry for the delay here. We will need to change the `network/cni` isolator for this right? Didn't really see a reason for this since replacing the existing CNI config would just work the same. Will keep the logic in the `network/cni` isolator simpler?

I thought we already do 'contains' check in CNI isolator?


- Jie


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


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/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.

> On Feb. 28, 2017, 4:42 a.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1207-1209 (patched)
> > <https://reviews.apache.org/r/53264/diff/3/?file=1650779#file1650779line1207>
> >
> >     I'd suggest we don't directly check ip table rules. I think using curl below is sufficient.

Any specific reason for not checking the iptable rules? I wanted to strengthen the test by making sure the iptable rules installed are what is expected, and the installed iptable rules actually result in the correct port mapping (tested by curl). Hence, would like to keep the iptables check unless you feel it might make the test flaky.


- Avinash


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


On March 12, 2017, 7:11 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53264/
> -----------------------------------------------------------
> 
> (Updated March 12, 2017, 7:11 a.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/4/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 53264: Added test for CNI port-mapper plugin.

Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
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.


Changes
-------

rebased.


Bugs: MESOS-6022
    https://issues.apache.org/jira/browse/MESOS-6022


Repository: mesos


Description
-------

Added test for CNI port-mapper plugin.


Diffs (updated)
-----

  src/tests/containerizer/cni_isolator_tests.cpp cb893d3ef005a9cc60c40768fa669b27c4863020 

Diff: https://reviews.apache.org/r/53264/diff/


Testing
-------

sudo make check


Thanks,

Avinash sridharan