You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/07/29 00:02:16 UTC
Re: Review Request 60765: Added basic `network/ports` isolator tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60765/
-----------------------------------------------------------
(Updated July 29, 2017, 12: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 (updated)
-----
src/tests/containerizer/network_ports_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60765/diff/6/
Changes: https://reviews.apache.org/r/60765/diff/5-6/
Testing
-------
make check (Fedora 26)
Thanks,
James Peach
Re: Review Request 60765: Added basic `network/ports` isolator tests.
Posted by James Peach <jp...@apache.org>.
> On Aug. 23, 2017, 7:58 a.m., Qian Zhang wrote:
> > src/tests/containerizer/ports_isolator_tests.cpp
> > Lines 80 (patched)
> > <https://reviews.apache.org/r/60765/diff/10/?file=1801027#file1801027line80>
> >
> > Why do we need a template function?
We need this to be ablt to use the same code for the `v1` API.
> On Aug. 23, 2017, 7:58 a.m., Qian Zhang wrote:
> > src/tests/containerizer/ports_isolator_tests.cpp
> > Lines 85 (patched)
> > <https://reviews.apache.org/r/60765/diff/10/?file=1801027#file1801027line85>
> >
> > Can we just do `set_type(HealthCheck::TCP)`?
We have to deal with the old and new APIs, so this can be `mesos::HealthCheck::TCP` or `mesos::v1::HealthCheck::TCP`.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60765/#review183314
-----------------------------------------------------------
On July 29, 2017, 12: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, 12: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
>
>
Re: Review Request 60765: Added basic `network/ports` isolator tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 60765: Added basic `network/ports` isolator tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60765/#review184652
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/containerizer/ports_isolator_tests.cpp
Lines 482-483 (patched)
<https://reviews.apache.org/r/60765/#comment260842>
I do not think we need `Resources(`, just `resources = Resources::parse(` should be OK.
- 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/13/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60765: Added basic `network/ports` isolator tests.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60765/#review184165
-----------------------------------------------------------
src/tests/containerizer/ports_isolator_tests.cpp
Lines 299 (patched)
<https://reviews.apache.org/r/60765/#comment260227>
Unconventionally, the size of a range is signed (it is a protobuf field).
- James Peach
On July 29, 2017, 12: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, 12: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/13/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60765: Added basic `network/ports` isolator tests.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60765/#review188782
-----------------------------------------------------------
src/tests/containerizer/ports_isolator_tests.cpp
Lines 142 (patched)
<https://reviews.apache.org/r/60765/#comment265798>
Have you thought about setting the health checks' grace period to `1` instead of `delay_seconds`?
That'd make the checker start probing right ahead, but ignore failures for the first second.
Meaning that the tests could succeed faster if the tasks are ready before the first second passes.
- Gaston Kleiman
On Sept. 7, 2017, 2:57 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60765/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2017, 2:57 p.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/19/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60765: Added basic `network/ports` isolator tests.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60765/
-----------------------------------------------------------
(Updated Sept. 7, 2017, 9:57 p.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Changes
-------
Rebase and address review feedback.
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 (updated)
-----
src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/60765/diff/14/
Changes: https://reviews.apache.org/r/60765/diff/13-14/
Testing
-------
make check (Fedora 26)
Thanks,
James Peach