You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/08/23 07:58:51 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/#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 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
>
>