Re: Review Request 64271: Added an unit test for docker image auto gc.

[Gilbert Song]

> On Dec. 3, 2017, 2:53 p.m., Zhitao Li wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 463 (patched)
> > 
> >
> > 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.

[Gilbert Song]

> On Dec. 2, 2017, 1:27 a.m., Qian Zhang wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 561 (patched)
> > 
> >
> > 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.

[Gilbert Song]

---
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.

[Mesos Reviewbot Windows]

---
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.

[Zhitao Li]

> On Dec. 3, 2017, 10:53 p.m., Zhitao Li wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp
> > Lines 465 (patched)
> > 
> >
> > 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.

[Zhitao Li]

---
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)


Indicate why this needs `ROOT_` permission?



src/tests/containerizer/provisioner_docker_tests.cpp
Lines 465 (patched)


Why do we need a master?



src/tests/containerizer/provisioner_docker_tests.cpp
Lines 548-564 (patched)


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
> 
>

Re: Review Request 64271: Added an unit test for docker image auto gc.

[Qian Zhang]

---
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)


Please add a comment to describe the purpose of this test.



src/tests/containerizer/provisioner_docker_tests.cpp
Lines 561 (patched)


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
> 
>

Review Request 64271: Added an unit test for docker image auto gc.

[Gilbert Song]

---
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