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 2019/03/24 04:33:49 UTC
Review Request 70289: Added a TODO for additional URLs support.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70289/
-----------------------------------------------------------
Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang.
Repository: mesos
Description
-------
Added a TODO for additional URLs support.
Diffs
-----
src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc
Diff: https://reviews.apache.org/r/70289/diff/1/
Testing
-------
Thanks,
Gilbert Song
Re: Review Request 70289: Added a TODO for additional URLs support.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70289/#review214364
-----------------------------------------------------------
Ship it!
Ship It!
- Qian Zhang
On April 2, 2019, 4:11 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70289/
> -----------------------------------------------------------
>
> (Updated April 2, 2019, 4:11 p.m.)
>
>
> Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a TODO for additional URLs support.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc
>
>
> Diff: https://reviews.apache.org/r/70289/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 70289: Added a TODO for additional URLs support.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70289/
-----------------------------------------------------------
(Updated April 2, 2019, 1:11 a.m.)
Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang.
Repository: mesos
Description
-------
Added a TODO for additional URLs support.
Diffs (updated)
-----
src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc
Diff: https://reviews.apache.org/r/70289/diff/2/
Changes: https://reviews.apache.org/r/70289/diff/1-2/
Testing
-------
Thanks,
Gilbert Song
Re: Review Request 70289: Added a TODO for additional URLs support.
Posted by Gilbert Song <so...@gmail.com>.
> On March 25, 2019, 1:09 a.m., Qian Zhang wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 961-962 (patched)
> > <https://reviews.apache.org/r/70289/diff/1/?file=2133765#file2133765line961>
> >
> > I think we can support for foreign(additional) urls for all platforms now. Actually in https://docs.docker.com/registry/spec/manifest-v2-2/ , we can see there are two media types for a layer:
> > 1. `application/vnd.docker.image.rootfs.diff.tar.gzip`: layer as a gzipped tar.
> > 2. `application/vnd.docker.image.rootfs.foreign.diff.tar.gzip`: layer as a gzipped tar that may be pulled from a remote location but they should never be pushed.
> >
> > None of these two media types are Windows specific, instead they are general concepts which is allowed to be used by any registries.
> >
> > So I think we can fetch blobs based these two media types as I suggested in design doc:
> > 1. If the fetched manifest is v2 s2, when fetching a blob for a layer, we will check its `mediaType` in the manifest:
> > 1.1 If it is `application/vnd.docker.image.rootfs.diff.tar.gzip`, fetch the blob as usual (i.e., based on its digest).
> > 1.2 If it is `application/vnd.docker.image.rootfs.foreign.diff.tar.gzip`, fetch the blob based on the `urls` field (this will cover Windows case), if there is no `urls` field, fall back to fetching based on its digest.
> > 2. If the fetched manifest is v2 s1, we will keep the existing code logic: fetch blobs based on the `fsLayers` in the manifest.
> >
> > In this way, there will be no Windows specific code for fetching blob and we avoid parsing the manifest for each layer as you mentioned in this comment, and we do not need to fetch blob twice for Windows (fetch based on v2 s1 manifest for the first time and then fetch based on v2 s2 manifest for the second time).
will do as a separate patch.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70289/#review213965
-----------------------------------------------------------
On April 2, 2019, 1:11 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70289/
> -----------------------------------------------------------
>
> (Updated April 2, 2019, 1:11 a.m.)
>
>
> Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a TODO for additional URLs support.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc
>
>
> Diff: https://reviews.apache.org/r/70289/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 70289: Added a TODO for additional URLs support.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70289/#review213965
-----------------------------------------------------------
src/uri/fetchers/docker.cpp
Lines 961-962 (patched)
<https://reviews.apache.org/r/70289/#comment300090>
I think we can support for foreign(additional) urls for all platforms now. Actually in https://docs.docker.com/registry/spec/manifest-v2-2/ , we can see there are two media types for a layer:
1. `application/vnd.docker.image.rootfs.diff.tar.gzip`: layer as a gzipped tar.
2. `application/vnd.docker.image.rootfs.foreign.diff.tar.gzip`: layer as a gzipped tar that may be pulled from a remote location but they should never be pushed.
None of these two media types are Windows specific, instead they are general concepts which is allowed to be used by any registries.
So I think we can fetch blobs based these two media types as I suggested in design doc:
1. If the fetched manifest is v2 s2, when fetching a blob for a layer, we will check its `mediaType` in the manifest:
1.1 If it is `application/vnd.docker.image.rootfs.diff.tar.gzip`, fetch the blob as usual (i.e., based on its digest).
1.2 If it is `application/vnd.docker.image.rootfs.foreign.diff.tar.gzip`, fetch the blob based on the `urls` field (this will cover Windows case), if there is no `urls` field, fall back to fetching based on its digest.
2. If the fetched manifest is v2 s1, we will keep the existing code logic: fetch blobs based on the `fsLayers` in the manifest.
In this way, there will be no Windows specific code for fetching blob and we avoid parsing the manifest for each layer as you mentioned in this comment, and we do not need to fetch blob twice for Windows (fetch based on v2 s1 manifest for the first time and then fetch based on v2 s2 manifest for the second time).
- Qian Zhang
On March 24, 2019, 12:33 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70289/
> -----------------------------------------------------------
>
> (Updated March 24, 2019, 12:33 p.m.)
>
>
> Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a TODO for additional URLs support.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc
>
>
> Diff: https://reviews.apache.org/r/70289/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gilbert Song
>
>