You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2016/02/23 00:50:45 UTC
Review Request 43855: Added Appc fetcher support to store.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
This change allows store to fetch an image using Appc image fetcher when an
image is not found in the cache. It also recursively fetches the dependencies
for the image.
Diffs
-----
src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
Diff: https://reviews.apache.org/r/43855/diff/
Testing
-------
make check; local image server.
Thanks,
Jojy Varghese
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Guangya Liu <gy...@gmail.com>.
> On 二月 23, 2016, 5:21 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/store.cpp, line 288
> > <https://reviews.apache.org/r/43855/diff/1/?file=1264857#file1264857line288>
> >
> > What about using a uuid like dir here? There might be duplicate "XXXXXX" under /tmp
The docker provisioner is also using same logic, it is reasonable as this will be managed by mesos provisioner.
> On 二月 23, 2016, 5:21 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/store.cpp, line 291
> > <https://reviews.apache.org/r/43855/diff/1/?file=1264857#file1264857line291>
> >
> > If a uuid like dir, then it is better add it to the log message when error happens
Does it make sense to put the temporary fetch directory in the error message for better trouble shooting?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120260
-----------------------------------------------------------
On 二月 22, 2016, 11:50 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated 二月 22, 2016, 11:50 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Feb. 23, 2016, 5:21 a.m., Guangya Liu wrote:
> > src/slave/containerizer/mesos/provisioner/appc/store.cpp, line 334
> > <https://reviews.apache.org/r/43855/diff/1/?file=1264857#file1264857line334>
> >
> > s/Fetches/Fetch
Since i am describing what the function does, I believe the function "fetches" the image descibes what it does the best. Having said that, I am not a grammer expert and my arms could be twisted :)
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120260
-----------------------------------------------------------
On Feb. 23, 2016, 3:30 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2016, 3:30 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
> src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120260
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 214)
<https://reviews.apache.org/r/43855/#comment181686>
What about using `` to highlight move/rename?
Same comments for others highlight.
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 227)
<https://reviews.apache.org/r/43855/#comment181687>
s/We expect/Expect
Can you please add more comments for why only one directory is expected?
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 240)
<https://reviews.apache.org/r/43855/#comment181688>
What about adding the image dir the log message for better trouble shooting?
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 281)
<https://reviews.apache.org/r/43855/#comment181689>
What about using a uuid like dir here? There might be duplicate "XXXXXX" under /tmp
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 284)
<https://reviews.apache.org/r/43855/#comment181690>
If a uuid like dir, then it is better add it to the log message when error happens
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 296)
<https://reviews.apache.org/r/43855/#comment181691>
detailed failure here including both tmp and dest.
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 327)
<https://reviews.apache.org/r/43855/#comment181692>
s/Fetches/Fetch
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 346)
<https://reviews.apache.org/r/43855/#comment181684>
alignment
- Guangya Liu
On 二月 22, 2016, 11:50 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated 二月 22, 2016, 11:50 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120365
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43855]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 23, 2016, 3:30 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2016, 3:30 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
> src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/
-----------------------------------------------------------
(Updated Feb. 26, 2016, 10:42 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
review addressed.
Bugs: MESOS-3424
https://issues.apache.org/jira/browse/MESOS-3424
Repository: mesos
Description
-------
This change allows store to fetch an image using Appc image fetcher when an
image is not found in the cache. It also recursively fetches the dependencies
for the image.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
Diff: https://reviews.apache.org/r/43855/diff/
Testing
-------
make check; local image server.
Thanks,
Jojy Varghese
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/
-----------------------------------------------------------
(Updated Feb. 26, 2016, 8:36 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
review addressed.
Bugs: MESOS-3424
https://issues.apache.org/jira/browse/MESOS-3424
Repository: mesos
Description
-------
This change allows store to fetch an image using Appc image fetcher when an
image is not found in the cache. It also recursively fetches the dependencies
for the image.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
Diff: https://reviews.apache.org/r/43855/diff/
Testing
-------
make check; local image server.
Thanks,
Jojy Varghese
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120828
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 254)
<https://reviews.apache.org/r/43855/#comment182336>
Why we need this parameter? What does that mean? To me, it's clear that 'fetchImage' just take 'appc'.
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 267)
<https://reviews.apache.org/r/43855/#comment182337>
Why do you need to call validate? Any image in the store should already been validated.
You do have to check if the image id exists in the store or not (cache->get(...)?)
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 278)
<https://reviews.apache.org/r/43855/#comment182345>
I would put this to the end, so the overall logic of this method should be look like the following:
1) see if the image id in the cache or not
2) if not, fetch it using fetcher
3) once fetch is done (i.e., moved to the store), removed the staging dir
4) fetch dependencies, get a vector of image ids
5) merge the ids get from (4) and merge it with the image id get from either 1) or 3)
src/slave/containerizer/mesos/provisioner/appc/store.cpp (lines 298 - 323)
<https://reviews.apache.org/r/43855/#comment182346>
Any reason you need three lambdas? Looks like they are synchronous functions.
- Jie Yu
On Feb. 26, 2016, 4:07 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2016, 4:07 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
> src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/
-----------------------------------------------------------
(Updated Feb. 26, 2016, 4:07 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
review addressed.
Repository: mesos
Description
-------
This change allows store to fetch an image using Appc image fetcher when an
image is not found in the cache. It also recursively fetches the dependencies
for the image.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
Diff: https://reviews.apache.org/r/43855/diff/
Testing
-------
make check; local image server.
Thanks,
Jojy Varghese
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120561
-----------------------------------------------------------
First pass. Will do a second pass once the comments below are addressed. Thanks Jojy!
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 115)
<https://reviews.apache.org/r/43855/#comment182026>
What does 'load' do?
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 130)
<https://reviews.apache.org/r/43855/#comment182025>
you can do:
```
uriFetcher->share()
```
Also, this fits in one line?
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 197)
<https://reviews.apache.org/r/43855/#comment182027>
First, we rarely use 'shared_ptr' directly. Second, I think we should be able to avoid this pointer. What you can do is that:
1) `fetchImage` will return `vector<string>` which means: fetch this image and all its dependencies, return me the list of image ids.
2) I think `fetchDependencies` can return `vector<string>` as well, which basically says that fetch the dependencies of this image id, and give me the list of dependent image ids.
3) Similarly, `fetchDependency` will return `vector<string>` as well.
This recursion patter sounds more clear to me, instead of passing around the result pointer.
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 199)
<https://reviews.apache.org/r/43855/#comment182028>
This check sounds unnecessary.
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 203)
<https://reviews.apache.org/r/43855/#comment182123>
s/rootfsDirs/rootfses/
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 217)
<https://reviews.apache.org/r/43855/#comment182125>
Why return 'Path' here. Isn't returning the image id more clear?
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 284)
<https://reviews.apache.org/r/43855/#comment182124>
I would put it under a staging directory, like we did in docker.
```
<store>/staging/<tmp_XXXX>
```
Also, s/mkdir/staging/
src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 291)
<https://reviews.apache.org/r/43855/#comment182126>
s/tmpFetchDir/staging/
- Jie Yu
On Feb. 23, 2016, 3:30 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2016, 3:30 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
> src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/
-----------------------------------------------------------
(Updated Feb. 23, 2016, 3:30 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
addressed review.
Repository: mesos
Description
-------
This change allows store to fetch an image using Appc image fetcher when an
image is not found in the cache. It also recursively fetches the dependencies
for the image.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
src/tests/fetcher_cache_tests.cpp e10b3f7ebc21c8c1095564fc40f123087dcf320e
Diff: https://reviews.apache.org/r/43855/diff/
Testing
-------
make check; local image server.
Thanks,
Jojy Varghese
Re: Review Request 43855: Added Appc fetcher support to store.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43855/#review120309
-----------------------------------------------------------
Patch looks great!
Reviews applied: [43855]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Feb. 22, 2016, 11:50 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43855/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2016, 11:50 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change allows store to fetch an image using Appc image fetcher when an
> image is not found in the cache. It also recursively fetches the dependencies
> for the image.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/appc/store.cpp 4b3829175f57fb9aea2478040d96f2f127cbc551
>
> Diff: https://reviews.apache.org/r/43855/diff/
>
>
> Testing
> -------
>
> make check; local image server.
>
>
> Thanks,
>
> Jojy Varghese
>
>