Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56284/#review164789
---


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2017, 8:30 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 8, 2017, 8:30 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies that the layer that is missing from the store will 
> be pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Ilya Pronin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56284/
---

(Updated Feb. 8, 2017, 8:30 p.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

Fixed raised issues.


Bugs: MESOS-7045
https://issues.apache.org/jira/browse/MESOS-7045


Repository: mesos


Description (updated)
---

Added a test that verifies that the layer that is missing from the store will 
be pulled.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
99e0820fa47303896f70c81cfb91cd34f0474e55 

Diff: https://reviews.apache.org/r/56284/diff/


Testing
---

`make check`


Thanks,

Ilya Pronin

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Ilya Pronin]

> On Feb. 7, 2017, 6:57 p.m., Gilbert Song wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, lines 241-242
> > 
> >
> > Maybe you should fix the comment.

Yes, the comment should be changed.


> On Feb. 7, 2017, 6:57 p.m., Gilbert Song wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, lines 268-271
> > 
> >
> > Since you set `cached` as false and manually remove one layer from the 
> > store. This test only verifies that a missing layer will be pulled if 
> > `cached` is set as false.
> > 
> > I do not see how it verifies the local puller will skip pullin the 
> > existing layers.

I agree that this test does not explicitly verify that `LocalPuller` skips the 
layer. My first intention was to write 2 separate tests: for `LocalPuller` and 
for `Store`. But such tests assumed a lot of details about staging directory 
layout. So Jie suggested this test that is simpler and less fragile and 
exercises the skipping layers code.


- Ilya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56284/#review164539
---


On Feb. 6, 2017, 8:40 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 6, 2017, 8:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new test verifies that that local Docker puller and store will skip 
> layers that are already pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56284/#review164539
---




src/tests/containerizer/provisioner_docker_tests.cpp (lines 241 - 242)


Maybe you should fix the comment.



src/tests/containerizer/provisioner_docker_tests.cpp (lines 268 - 271)


Since you set `cached` as false and manually remove one layer from the 
store. This test only verifies that a missing layer will be pulled if `cached` 
is set as false.

I do not see how it verifies the local puller will skip pullin the existing 
layers.


- Gilbert Song


On Feb. 6, 2017, 12:40 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 6, 2017, 12:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new test verifies that that local Docker puller and store will skip 
> layers that are already pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Mesos Reviewbot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56284/#review164449
---



Patch looks great!

Reviews applied: [56174, 56284]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On Feb. 6, 2017, 8:40 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 6, 2017, 8:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new test verifies that that local Docker puller and store will skip 
> layers that are already pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Ilya Pronin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56284/
---

(Updated Feb. 6, 2017, 8:40 p.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

Used less fragile approach suggested by Jie in 
[56174](https://reviews.apache.org/r/56174/).


Summary (updated)
-

Added ProvisionerDockerLocalStoreTest.MissingLayer test.


Bugs: MESOS-7045
https://issues.apache.org/jira/browse/MESOS-7045


Repository: mesos


Description (updated)
---

The new test verifies that that local Docker puller and store will skip layers 
that are already pulled.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
99e0820fa47303896f70c81cfb91cd34f0474e55 

Diff: https://reviews.apache.org/r/56284/diff/


Testing
---

`make check`


Thanks,

Ilya Pronin