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/10/17 19:15:26 UTC
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/
-----------------------------------------------------------
(Updated Oct. 17, 2017, 7:15 p.m.)
Review request for mesos, Qian Zhang and Jiang Yan Xu.
Changes
-------
Updated status update checks.
Bugs: MESOS-7675
https://issues.apache.org/jira/browse/MESOS-7675
Repository: mesos
Description
-------
Added `network/ports` isolator nested container tests using the v1
TaskGroups API. This tests that rogue port usage by a nested task is
detected both with and without agent recovery, and that a well-behaved
task is preserved across agent recovery.
Diffs (updated)
-----
src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
Diff: https://reviews.apache.org/r/62003/diff/6/
Changes: https://reviews.apache.org/r/62003/diff/5-6/
Testing
-------
make check (Fedora 26)
Thanks,
James Peach
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188458
-----------------------------------------------------------
Patch looks great!
Reviews applied: [60491, 60493, 60494, 60764, 60901, 60836, 61538, 60902, 60492, 60495, 61536, 60496, 60592, 60591, 60766, 60903, 60765, 60593, 62003]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Oct. 17, 2017, 7:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 7:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/6/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188814
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 60491.
Failed command: `python.exe .\support\apply-reviews.py -n -r 60491`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003
Relevant logs:
- [apply-review-60491-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003/logs/apply-review-60491-stdout.log):
```
error: patch failed: src/linux/routing/diagnosis/diagnosis.hpp:60
error: src/linux/routing/diagnosis/diagnosis.hpp: patch does not apply
error: patch failed: src/linux/routing/diagnosis/diagnosis.cpp:67
error: src/linux/routing/diagnosis/diagnosis.cpp: patch does not apply
```
- Mesos Reviewbot Windows
On Oct. 17, 2017, 7:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 7:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/8/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188751
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 60491.
Failed command: `python.exe .\support\apply-reviews.py -n -r 60491`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003
Relevant logs:
- [apply-review-60491-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003/logs/apply-review-60491-stdout.log):
```
error: patch failed: src/linux/routing/diagnosis/diagnosis.hpp:60
error: src/linux/routing/diagnosis/diagnosis.hpp: patch does not apply
error: patch failed: src/linux/routing/diagnosis/diagnosis.cpp:67
error: src/linux/routing/diagnosis/diagnosis.cpp: patch does not apply
```
- Mesos Reviewbot Windows
On Oct. 17, 2017, 7:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 7:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/7/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by James Peach <jp...@apache.org>.
> On Oct. 19, 2017, 10:44 p.m., Gaston Kleiman wrote:
> > src/tests/containerizer/ports_isolator_tests.cpp
> > Lines 1133 (patched)
> > <https://reviews.apache.org/r/62003/diff/7/?file=1864211#file1864211line1133>
> >
> > Nit: I prefer `ASSERT_FALSE(offers->offers().empty());`
Yes `ASSERT_FALSE` is a bit cleaner, but doing it this way will show the numeric actual size, which is arguably more helpful.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188777
-----------------------------------------------------------
On Oct. 17, 2017, 7:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 7:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/8/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by Gaston Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188777
-----------------------------------------------------------
I love the new helper ;-).
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1097 (patched)
<https://reviews.apache.org/r/62003/#comment265791>
do we really need this `Future`?
I think that we could get rid of it if we move `subscribed` and `offers` before `TestMesos` is instantiated.
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1133 (patched)
<https://reviews.apache.org/r/62003/#comment265792>
Nit: I prefer `ASSERT_FALSE(offers->offers().empty());`
- Gaston Kleiman
On Oct. 17, 2017, 12:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 12:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/8/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188422
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 60491.
Failed command: `python.exe .\support\apply-reviews.py -n -r 60491`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003
Relevant logs:
- [apply-review-60491-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62003/logs/apply-review-60491-stdout.log):
```
error: patch failed: src/linux/routing/diagnosis/diagnosis.hpp:60
error: src/linux/routing/diagnosis/diagnosis.hpp: patch does not apply
error: patch failed: src/linux/routing/diagnosis/diagnosis.cpp:67
error: src/linux/routing/diagnosis/diagnosis.cpp: patch does not apply
```
- Mesos Reviewbot Windows
On Oct. 17, 2017, 7:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 7:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/6/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by James Peach <jp...@apache.org>.
> On Oct. 18, 2017, 8:38 a.m., Qian Zhang wrote:
> > src/tests/containerizer/ports_isolator_tests.cpp
> > Lines 1225 (patched)
> > <https://reviews.apache.org/r/62003/diff/6/?file=1860962#file1860962line1225>
> >
> > Why do we need the `slaveId`? Can we just call `StartSlave()` like what you does in the test `ROOT_NC_TaskGroup`.
I need to keep the same slave ID across the restart so that the tasks will (or won't reattach).
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188461
-----------------------------------------------------------
On Oct. 17, 2017, 7:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 7:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/6/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 62003: Added `network/ports` isolator nested
container tests.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62003/#review188461
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1083-1090 (patched)
<https://reviews.apache.org/r/62003/#comment265468>
I think these code can be changed to:
```
v1::ExecutorInfo executorInfo = v1::createExecutorInfo(
v1::DEFAULT_EXECUTOR_ID,
None(),
"cpus:0.1;mem:32;disk:32",
v1::ExecutorInfo::DEFAULT,
frameworkId);
```
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1148 (patched)
<https://reviews.apache.org/r/62003/#comment265470>
Kill this blank line.
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1190-1199 (patched)
<https://reviews.apache.org/r/62003/#comment265476>
It seems we only do these checks for nested container tests, we should do it for the normal container tests in your previous patches as well?
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1225 (patched)
<https://reviews.apache.org/r/62003/#comment265471>
Why do we need the `slaveId`? Can we just call `StartSlave()` like what you does in the test `ROOT_NC_TaskGroup`.
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1262-1269 (patched)
<https://reviews.apache.org/r/62003/#comment265472>
Ditto.
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1400 (patched)
<https://reviews.apache.org/r/62003/#comment265473>
s/after and agent restart/after an agent restart/
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1421 (patched)
<https://reviews.apache.org/r/62003/#comment265474>
Ditto.
src/tests/containerizer/ports_isolator_tests.cpp
Lines 1458-1465 (patched)
<https://reviews.apache.org/r/62003/#comment265475>
Ditto.
- Qian Zhang
On Oct. 18, 2017, 3:15 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62003/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2017, 3:15 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 `network/ports` isolator nested container tests using the v1
> TaskGroups API. This tests that rogue port usage by a nested task is
> detected both with and without agent recovery, and that a well-behaved
> task is preserved across agent recovery.
>
>
> Diffs
> -----
>
> src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION
> src/tests/mesos.hpp 24d220e292bc1e137992e8f81484477b62bd0896
>
>
> Diff: https://reviews.apache.org/r/62003/diff/6/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>