You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2018/08/06 16:53:21 UTC
Review Request 68239: Updated port mapper CNI test.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Repository: mesos
Description
-------
This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.
This test fails if the fix for MESOS-9125 is not included.
Diffs
-----
src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
Diff: https://reviews.apache.org/r/68239/diff/1/
Testing
-------
sudo make check
Thanks,
Jie Yu
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/#review206895
-----------------------------------------------------------
src/tests/containerizer/cni_isolator_tests.cpp
Lines 2002 (patched)
<https://reviews.apache.org/r/68239/#comment290017>
indent
- Jie Yu
On Aug. 6, 2018, 4:53 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 4:53 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
>
> This test fails if the fix for MESOS-9125 is not included.
>
>
> Diffs
> -----
>
> src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
> src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
>
>
> Diff: https://reviews.apache.org/r/68239/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/#review206920
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/containerizer/cni_isolator_tests.cpp
Line 1946 (original), 1974 (patched)
<https://reviews.apache.org/r/68239/#comment290068>
Nit: Move the declaration close to the use.
- Chun-Hung Hsiao
On Aug. 6, 2018, 11:42 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 11:42 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
>
> This test fails if the fix for MESOS-9125 is not included.
>
>
> Diffs
> -----
>
> src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
> src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
>
>
> Diff: https://reviews.apache.org/r/68239/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/#review206928
-----------------------------------------------------------
PASS: Mesos patch 68239 was successfully built and tested.
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2104/mesos-review-68239
- Mesos Reviewbot Windows
On Aug. 6, 2018, 11:42 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 11:42 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
>
> This test fails if the fix for MESOS-9125 is not included.
>
>
> Diffs
> -----
>
> src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
> src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
>
>
> Diff: https://reviews.apache.org/r/68239/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/
-----------------------------------------------------------
(Updated Aug. 6, 2018, 11:42 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.
This test fails if the fix for MESOS-9125 is not included.
Diffs (updated)
-----
src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
Diff: https://reviews.apache.org/r/68239/diff/3/
Changes: https://reviews.apache.org/r/68239/diff/2-3/
Testing
-------
sudo make check
Thanks,
Jie Yu
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1957-1958 (original), 1990-1996 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1997>
> >
> > This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What if the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?
I missed the fact that the tasks won't finish until the `echo foo | nc` commands happen. Dropping this.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/#review206907
-----------------------------------------------------------
On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 8:31 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
>
> This test fails if the fix for MESOS-9125 is not included.
>
>
> Diffs
> -----
>
> src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
> src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
>
>
> Diff: https://reviews.apache.org/r/68239/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1799 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1799>
> >
> > What's the reason of doing a pre-test cleanup first? To prevent residual rules from the previous run?
Yeah, that's the typical pattern we used in the test.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1929-1932 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1932>
> >
> > Why do we need to consume a new port ID for the container port?
Because container is running on the host network `__MESOS_TEST__` network does not fork a new network namespace.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1988 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1995>
> >
> > Nit: s/`containerId`/`containerIds`/
> >
> > Also, it seems we don't use `containerId` in this test once we get them at all?
Removed.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1957-1958 (original), 1990-1996 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1997>
> >
> > This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What if the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?
>
> Chun-Hung Hsiao wrote:
> I missed the fact that the tasks won't finish until the `echo foo | nc` commands happen. Dropping this.
I added some comment about the nc server behavior.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 2000-2004 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2007>
> >
> > Is it possible to just use this loop to get the container IDs and network infos?
It's logically a bit weird to combine the loop. I'll stick to the current way.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 2006-2012 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2013>
> >
> > How about setting up expectations for `lostExecutor` instead?
I'll use this for now since this is how it was the case before.
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1984-1987 (original), 2040-2043 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2047>
> >
> > If we use a matcher above, and use a`vector` instead of an array we can simply do `AWAIT_READY(collect(statusesFinished))` here. But I'm fine using loop here so not opening an issue.
Good idea!
> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Line 2000 (original), 2045-2047 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2063>
> >
> > We could do `AWAIT_READY(collect(gcSchedule))` if `gcSchedule` is a vector.
Good idea!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/#review206907
-----------------------------------------------------------
On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 8:31 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
>
> This test fails if the fix for MESOS-9125 is not included.
>
>
> Diffs
> -----
>
> src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
> src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
>
>
> Diff: https://reviews.apache.org/r/68239/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/#review206907
-----------------------------------------------------------
src/tests/containerizer/cni_isolator_tests.cpp
Lines 1799 (patched)
<https://reviews.apache.org/r/68239/#comment290039>
What's the reason of doing a pre-test cleanup first? To prevent residual rules from the previous run?
src/tests/containerizer/cni_isolator_tests.cpp
Lines 1929-1932 (patched)
<https://reviews.apache.org/r/68239/#comment290046>
Why do we need to consume a new port ID for the container port?
src/tests/containerizer/cni_isolator_tests.cpp
Lines 1940-1941 (patched)
<https://reviews.apache.org/r/68239/#comment290042>
s/`hostPort`/`hostPorts`/
s/`containerPort`/`containerPorts`/
src/tests/containerizer/cni_isolator_tests.cpp
Line 1946 (original), 1975 (patched)
<https://reviews.apache.org/r/68239/#comment290048>
Nits:
s/`statusRunning`/`statusesRunning`/
s/`statusFinished`/`statusesFinished`/
src/tests/containerizer/cni_isolator_tests.cpp
Lines 1988 (patched)
<https://reviews.apache.org/r/68239/#comment290050>
Nit: s/`containerId`/`containerIds`/
Also, it seems we don't use `containerId` in this test once we get them at all?
src/tests/containerizer/cni_isolator_tests.cpp
Lines 1957-1958 (original), 1990-1996 (patched)
<https://reviews.apache.org/r/68239/#comment290047>
This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What if the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?
src/tests/containerizer/cni_isolator_tests.cpp
Lines 2000-2004 (patched)
<https://reviews.apache.org/r/68239/#comment290049>
Is it possible to just use this loop to get the container IDs and network infos?
src/tests/containerizer/cni_isolator_tests.cpp
Lines 2001 (patched)
<https://reviews.apache.org/r/68239/#comment290054>
We could use the matcher `TaskStatusStateEq(TASK_FINISHED)` to avoid the `EXPECT_EQ` below.
src/tests/containerizer/cni_isolator_tests.cpp
Lines 2006-2012 (patched)
<https://reviews.apache.org/r/68239/#comment290052>
How about setting up expectations for `lostExecutor` instead?
src/tests/containerizer/cni_isolator_tests.cpp
Lines 2008 (patched)
<https://reviews.apache.org/r/68239/#comment290053>
Nit: s/`gcSchedule`/`gcSchedules`/
src/tests/containerizer/cni_isolator_tests.cpp
Lines 1984-1987 (original), 2040-2043 (patched)
<https://reviews.apache.org/r/68239/#comment290055>
If we use a matcher above, and use a`vector` instead of an array we can simply do `AWAIT_READY(collect(statusesFinished))` here. But I'm fine using loop here so not opening an issue.
src/tests/containerizer/cni_isolator_tests.cpp
Line 2000 (original), 2045-2047 (patched)
<https://reviews.apache.org/r/68239/#comment290056>
We could do `AWAIT_READY(collect(gcSchedule))` if `gcSchedule` is a vector.
- Chun-Hung Hsiao
On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 8:31 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
>
> This test fails if the fix for MESOS-9125 is not included.
>
>
> Diffs
> -----
>
> src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
> src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
>
>
> Diff: https://reviews.apache.org/r/68239/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 68239: Updated port mapper CNI test.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68239/
-----------------------------------------------------------
(Updated Aug. 6, 2018, 8:31 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Repository: mesos
Description
-------
This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.
This test fails if the fix for MESOS-9125 is not included.
Diffs (updated)
-----
src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e
src/tests/containerizer/cni_isolator_tests.cpp 90d2d4103c8136d2dd883318acc135f7efca80d8
Diff: https://reviews.apache.org/r/68239/diff/2/
Changes: https://reviews.apache.org/r/68239/diff/1-2/
Testing
-------
sudo make check
Thanks,
Jie Yu