You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2017/12/02 01:10:21 UTC
Review Request 64271: Added an unit test for docker image auto gc.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/
-----------------------------------------------------------
Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
Bugs: MESOS-8294
https://issues.apache.org/jira/browse/MESOS-8294
Repository: mesos
Description
-------
Added an unit test for docker image auto gc.
Diffs
-----
src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
Diff: https://reviews.apache.org/r/64271/diff/1/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/#review192667
-----------------------------------------------------------
FAIL: Some Mesos libprocess-tests failed.
Reviews applied: `['64264', '64265', '64266', '64267', '64268', '64269', '64270', '64271']`
Failed command: `C:\DCOS\mesos\3rdparty\libprocess\src\tests\Debug\libprocess-tests.exe`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64271
Relevant logs:
- [libprocess-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64271/logs/libprocess-tests-stdout.log):
```
[ OK ] LimiterTest.THREADSAFE_Acquire (2 ms)
[ RUN ] LimiterTest.THREADSAFE_DiscardMiddle
[ OK ] LimiterTest.THREADSAFE_DiscardMiddle (2 ms)
[ RUN ] LimiterTest.THREADSAFE_DiscardLast
[ OK ] LimiterTest.THREADSAFE_DiscardLast (2 ms)
[----------] 3 tests from LimiterTest (67 ms total)
[----------] 4 tests from LoopTest
[ RUN ] LoopTest.Sync
[ OK ] LoopTest.Sync (0 ms)
[ RUN ] LoopTest.Async
[ OK ] LoopTest.Async (1 ms)
[ RUN ] LoopTest.DiscardIterate
[ OK ] LoopTest.DiscardIterate (1 ms)
[ RUN ] LoopTest.DiscardBody
[ OK ] LoopTest.DiscardBody (2 ms)
[----------] 4 tests from LoopTest (89 ms total)
[----------] 9 tests from MetricsTest
[ RUN ] MetricsTest.Counter
[ OK ] MetricsTest.Counter (3 ms)
[ RUN ] MetricsTest.THREADSAFE_Gauge
[ OK ] MetricsTest.THREADSAFE_Gauge (3 ms)
[ RUN ] MetricsTest.Statistics
[ OK ] MetricsTest.Statistics (3 ms)
[ RUN ] MetricsTest.THREADSAFE_Snapshot
[ OK ] MetricsTest.THREADSAFE_Snapshot (63 ms)
[ RUN ] MetricsTest.THREADSAFE_SnapshotTimeout
C:\DCOS\mesos\mesos\3rdparty\libprocess\src\tests\metrics_tests.cpp(335): error: Failed to wait 15secs for response
```
- Mesos Reviewbot Windows
On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64271/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 1:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an unit test for docker image auto gc.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
>
>
> Diff: https://reviews.apache.org/r/64271/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 2, 2017, 1:27 a.m., Qian Zhang wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 561 (patched)
> > <https://reviews.apache.org/r/64271/diff/1/?file=1906562#file1906562line561>
> >
> > Why calling `paths::listLayer()` again here? Can we directly check `layers` returned from the above `while` loop?
No, cannot define a `Try` global variable outwide of the `while` loop.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/#review192625
-----------------------------------------------------------
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64271/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an unit test for docker image auto gc.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
>
>
> Diff: https://reviews.apache.org/r/64271/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/#review192625
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/containerizer/provisioner_docker_tests.cpp
Lines 463 (patched)
<https://reviews.apache.org/r/64271/#comment270888>
Please add a comment to describe the purpose of this test.
src/tests/containerizer/provisioner_docker_tests.cpp
Lines 561 (patched)
<https://reviews.apache.org/r/64271/#comment270889>
Why calling `paths::listLayer()` again here? Can we directly check `layers` returned from the above `while` loop?
- Qian Zhang
On Dec. 2, 2017, 9:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64271/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 9:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an unit test for docker image auto gc.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
>
>
> Diff: https://reviews.apache.org/r/64271/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/
-----------------------------------------------------------
(Updated Dec. 6, 2017, 11:15 a.m.)
Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
Bugs: MESOS-8294
https://issues.apache.org/jira/browse/MESOS-8294
Repository: mesos
Description
-------
Added an unit test for docker image auto gc.
Diffs (updated)
-----
src/tests/containerizer/provisioner_docker_tests.cpp 13f535077003e2032824b90f3898538abc16fa9b
Diff: https://reviews.apache.org/r/64271/diff/2/
Changes: https://reviews.apache.org/r/64271/diff/1-2/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 3, 2017, 2:53 p.m., Zhitao Li wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 463 (patched)
> > <https://reviews.apache.org/r/64271/diff/1/?file=1906562#file1906562line463>
> >
> > Indicate why this needs `ROOT_` permission?
due to the chroot. all other unit tests in this file does not mention that yet.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/#review192657
-----------------------------------------------------------
On Dec. 1, 2017, 5:10 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64271/
> -----------------------------------------------------------
>
> (Updated Dec. 1, 2017, 5:10 p.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an unit test for docker image auto gc.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
>
>
> Diff: https://reviews.apache.org/r/64271/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Zhitao Li <zh...@gmail.com>.
> On Dec. 3, 2017, 10:53 p.m., Zhitao Li wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 465 (patched)
> > <https://reviews.apache.org/r/64271/diff/1/?file=1906562#file1906562line465>
> >
> > Why do we need a master?
Never mind, I see we need to launch a task below.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/#review192657
-----------------------------------------------------------
On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64271/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 1:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an unit test for docker image auto gc.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
>
>
> Diff: https://reviews.apache.org/r/64271/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 64271: Added an unit test for docker image auto gc.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64271/#review192657
-----------------------------------------------------------
src/tests/containerizer/provisioner_docker_tests.cpp
Lines 463 (patched)
<https://reviews.apache.org/r/64271/#comment270915>
Indicate why this needs `ROOT_` permission?
src/tests/containerizer/provisioner_docker_tests.cpp
Lines 465 (patched)
<https://reviews.apache.org/r/64271/#comment270916>
Why do we need a master?
src/tests/containerizer/provisioner_docker_tests.cpp
Lines 548-564 (patched)
<https://reviews.apache.org/r/64271/#comment270919>
I guess the goal of this sleep based code block is to verify an image pruning really happened. I don't like `sleep` in test code in general, as it slows ourselves down.
A couple of alternatives:
a) use a mocked containerizer so we can capture and verify that `pruneImages` are called properly;
b) after first container completed, and image pruning finished, empty out the `directory` (which is used for docker registry). If we try to run the same task again, we should get an error because the image is pruned in store cache nor available in "registrY" anymore.
- Zhitao Li
On Dec. 2, 2017, 1:10 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64271/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2017, 1:10 a.m.)
>
>
> Review request for mesos, Jie Yu, Qian Zhang, Vinod Kone, and Zhitao Li.
>
>
> Bugs: MESOS-8294
> https://issues.apache.org/jira/browse/MESOS-8294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added an unit test for docker image auto gc.
>
>
> Diffs
> -----
>
> src/tests/containerizer/provisioner_docker_tests.cpp 832c81fe88d753b0f00dfab870d7725cf556fcef
>
>
> Diff: https://reviews.apache.org/r/64271/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>