You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Liangyu Zhao via Review Board <no...@reviews.apache.org> on 2018/07/16 18:35:50 UTC
Review Request 67931: Support Image Manifest Version 2 Schema 2.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
Repository: mesos
Description
-------
Support parsing schema 2 and fetching blob with multiple URLs as
specified in schema 2. Update `curl` version to 7.60.0 because of bug
encountered in version 7.57.0.
Diffs
-----
3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
include/mesos/docker/v2_2.hpp PRE-CREATION
include/mesos/docker/v2_2.proto PRE-CREATION
include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/67931/diff/1/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
> On July 17, 2018, 6:16 p.m., Andrew Schwartzmeyer wrote:
> > include/mesos/docker/spec.hpp
> > Lines 141 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line141>
> >
> > Dumb question, but is there an S1/S2 for V1 as well?
I don't think so.
> On July 17, 2018, 6:16 p.m., Andrew Schwartzmeyer wrote:
> > include/mesos/docker/spec.hpp
> > Lines 141-164 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line141>
> >
> > I'm not sure, but would this be better as a variant?
> >
> > ```
> > std::unique_ptr<Variant<v1::ImageManifest, v2::ImageManifest, v2_2::ImageManifest>> manifest;
> > ```
> >
> > (Declared at point of use.)
> >
> > P.S. Why do we need pointers?
I haven't used the `std::Variant` before. I think it is a better choice.
The reason why I used pointers is trying to avoid copying the object. This is just a minor optimization of efficiency.
> On July 17, 2018, 6:16 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
> > Lines 467 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059710#file2059710line467>
> >
> > Why do we need a `shared_ptr` here?
Avoiding copying too. Just a minor optimization.
> On July 17, 2018, 6:16 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
> > Lines 473 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059710#file2059710line473>
> >
> > Heyyy what is this...
I think using labels for flow control makes this part of the code more clean and readable.
> On July 17, 2018, 6:16 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetcher.cpp
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059712#file2059712line90>
> >
> > Why does this function throw away `urls`?
This is just a default parent class function. Currently, only `DockerFetcherPulgin` supports multiple URLs fetch, so it overrides this function. As for other plugins, this is the default way of handling extra URLs. Commented at `include\mesos\uri\fetcher.hpp` line 89.
- Liangyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206157
-----------------------------------------------------------
On July 16, 2018, 6:35 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 16, 2018, 6:35 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.60.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote:
> > include/mesos/docker/spec.hpp
> > Lines 124-126 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line124>
> >
> > I think this and the commit before need to be re-ordered, as this commit is introducing logic required and used by the previous commit.
I think you fixed this?
> On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote:
> > include/mesos/docker/spec.hpp
> > Lines 141 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line141>
> >
> > Dumb question, but is there an S1/S2 for V1 as well?
>
> Liangyu Zhao wrote:
> I don't think so.
There is, but it's not in use, so I'll drop this.
> On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote:
> > include/mesos/docker/spec.hpp
> > Lines 141-164 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059702#file2059702line141>
> >
> > I'm not sure, but would this be better as a variant?
> >
> > ```
> > std::unique_ptr<Variant<v1::ImageManifest, v2::ImageManifest, v2_2::ImageManifest>> manifest;
> > ```
> >
> > (Declared at point of use.)
> >
> > P.S. Why do we need pointers?
>
> Liangyu Zhao wrote:
> I haven't used the `std::Variant` before. I think it is a better choice.
>
> The reason why I used pointers is trying to avoid copying the object. This is just a minor optimization of efficiency.
Avoid pointers whenever possible; the minor optimization isn't worth the potential bug cost. Plus with move semantics the optimization probably won't even matter.
> On July 17, 2018, 11:16 a.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetcher.cpp
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67931/diff/1/?file=2059712#file2059712line90>
> >
> > Why does this function throw away `urls`?
>
> Liangyu Zhao wrote:
> This is just a default parent class function. Currently, only `DockerFetcherPulgin` supports multiple URLs fetch, so it overrides this function. As for other plugins, this is the default way of handling extra URLs. Commented at `include\mesos\uri\fetcher.hpp` line 89.
Gotcha.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206157
-----------------------------------------------------------
On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 18, 2018, 2:27 a.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206157
-----------------------------------------------------------
There is still more to review... I'll get to it.
include/mesos/docker/spec.hpp
Lines 124-126 (patched)
<https://reviews.apache.org/r/67931/#comment289057>
I think this and the commit before need to be re-ordered, as this commit is introducing logic required and used by the previous commit.
include/mesos/docker/spec.hpp
Lines 141 (patched)
<https://reviews.apache.org/r/67931/#comment289058>
Dumb question, but is there an S1/S2 for V1 as well?
include/mesos/docker/spec.hpp
Lines 141-164 (patched)
<https://reviews.apache.org/r/67931/#comment289060>
I'm not sure, but would this be better as a variant?
```
std::unique_ptr<Variant<v1::ImageManifest, v2::ImageManifest, v2_2::ImageManifest>> manifest;
```
(Declared at point of use.)
P.S. Why do we need pointers?
include/mesos/uri/fetcher.hpp
Lines 84-96 (patched)
<https://reviews.apache.org/r/67931/#comment289059>
Maybe add to this comment (and the one below) that this is used by the V2_S2 schema.
src/docker/spec.cpp
Lines 390-426 (patched)
<https://reviews.apache.org/r/67931/#comment289061>
I think you'd get all of this for "free" out of a variant.
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 467 (patched)
<https://reviews.apache.org/r/67931/#comment289062>
Why do we need a `shared_ptr` here?
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 473 (patched)
<https://reviews.apache.org/r/67931/#comment289064>
Heyyy what is this...
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 490 (patched)
<https://reviews.apache.org/r/67931/#comment289063>
Why are we making this on the heap and using a pointer?
src/uri/fetcher.cpp
Line 56 (original), 56 (patched)
<https://reviews.apache.org/r/67931/#comment289065>
We can delete this now! Also, add `MESOS-8570` to the "bug" field of this review, and let's get you a JIRA account so I can assign it to you.
src/uri/fetcher.cpp
Lines 88 (patched)
<https://reviews.apache.org/r/67931/#comment289066>
Why does this function throw away `urls`?
- Andrew Schwartzmeyer
On July 16, 2018, 11:35 a.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 16, 2018, 11:35 a.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.60.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206134
-----------------------------------------------------------
Ah, sorry, I will get the rest of this reviewed tomorrow.
3rdparty/cmake/Versions.cmake
Lines 7-8 (original), 7-8 (patched)
<https://reviews.apache.org/r/67931/#comment289034>
It looks like cURL has since posted 7.61.0, so as long as we're updating this, let's update to 7.61.0, and get it added to https://github.com/mesos/3rdparty/ so we can get the ReviewBot to pass. (We spoke offine so I know you're already doing this, just posting it for posterity.)
src/CMakeLists.txt
Lines 170 (patched)
<https://reviews.apache.org/r/67931/#comment289038>
I'm not certain we can just add it to `AGENT_SRC` as that'll also add it for POSIX systems. Instead, like how it's added to `LINUX_SRC`, let's also add it to `WIN32_SRC`.
I would suggest, take this existing snippet in this file (which shouldn't exist given the existence of `WIN32_SRC`:
```
if (WIN32)
list(APPEND AGENT_SRC
slave/containerizer/mesos/isolators/windows/cpu.cpp
slave/containerizer/mesos/isolators/windows/mem.cpp)
else ()
```
and instead make:
```
set(WIN32)SRC
slave/containerizer/mesos/isolators/docker/runtime.cpp
slave/containerizer/mesos/isolators/windows/cpu.cpp
slave/containerizer/mesos/isolators/windows/mem.cpp
... <the other three files in this variable already>)
```
src/CMakeLists.txt
Lines 424-427 (original), 426-427 (patched)
<https://reviews.apache.org/r/67931/#comment289039>
With this being true, we can just move `uri/fetchers/docker.cpp` to the point where we're setting `URI_SRC` to begin with.
src/uri/fetchers/docker.cpp
Lines 102-105 (patched)
<https://reviews.apache.org/r/67931/#comment289041>
I think there's a `strings::replace` to do this.
- Andrew Schwartzmeyer
On July 16, 2018, 11:35 a.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 16, 2018, 11:35 a.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.60.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
> On July 30, 2018, 11:39 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 1016-1017 (patched)
> > <https://reviews.apache.org/r/67931/diff/3/?file=2061476#file2061476line1020>
> >
> > Are we just skipping a failed blob here and trying to process the rest? Does that make sense to do?
Yes, the code will try each url until one succeeds. If all fail, the fetch will be considered failed.
- Liangyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206622
-----------------------------------------------------------
On July 30, 2018, 10:40 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 10:40 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 30, 2018, 4:39 p.m., Andrew Schwartzmeyer wrote:
> > include/mesos/uri/fetcher.hpp
> > Lines 101 (patched)
> > <https://reviews.apache.org/r/67931/diff/3/?file=2061467#file2061467line101>
> >
> > I am not convinced we need to be passing this as a shared_ptr, wouldn't const-ref be just fine?
Spoke offline; the reason this is the right choice is that this vector of strings is later used in several async lambdas. So by sharing a pointer to the vector instead, we utilize the ref counting of `shared_ptr` to avoid having to copy the vector for each lambda, and the size of the vector is ~10KB, so it's reasonable to want to avoid this.
> On July 30, 2018, 4:39 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 1016-1017 (patched)
> > <https://reviews.apache.org/r/67931/diff/3/?file=2061476#file2061476line1020>
> >
> > Are we just skipping a failed blob here and trying to process the rest? Does that make sense to do?
>
> Liangyu Zhao wrote:
> Yes, the code will try each url until one succeeds. If all fail, the fetch will be considered failed.
I'm not sure I understand why that works. Are these multiple URLs for the same blob? I would have that any single failed fetch would result in a total failure.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206622
-----------------------------------------------------------
On July 30, 2018, 3:40 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 3:40 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206622
-----------------------------------------------------------
3rdparty/CMakeLists.txt
Lines 721-736 (original), 721-736 (patched)
<https://reviews.apache.org/r/67931/#comment289655>
Note for others: this was forced by an upstream build change in curl.
include/mesos/docker/spec.hpp
Lines 40-42 (patched)
<https://reviews.apache.org/r/67931/#comment289656>
+1
include/mesos/uri/fetcher.hpp
Lines 101 (patched)
<https://reviews.apache.org/r/67931/#comment289657>
I am not convinced we need to be passing this as a shared_ptr, wouldn't const-ref be just fine?
src/Makefile.am
Lines 692-697 (original), 695-703 (patched)
<https://reviews.apache.org/r/67931/#comment289658>
Whitespace I'll fix up when committing...
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 467-505 (patched)
<https://reviews.apache.org/r/67931/#comment289661>
I believe you could accomplish what you're doing here with `goto fail/succeed` by refactoring this into a function (or even an in-place lambda) where the `goto`s would turn into `return`s. But I'm not sure if it'd be any cleaner. I have nothing against `goto` when used right, and this seems reasonable.
src/uri/fetcher.hpp
Line 46 (original), 43 (patched)
<https://reviews.apache.org/r/67931/#comment289654>
This can go.
src/uri/fetchers/docker.cpp
Lines 791-818 (patched)
<https://reviews.apache.org/r/67931/#comment289665>
Hey that worked out nicely.
src/uri/fetchers/docker.cpp
Lines 1016-1017 (patched)
<https://reviews.apache.org/r/67931/#comment289666>
Are we just skipping a failed blob here and trying to process the rest? Does that make sense to do?
- Andrew Schwartzmeyer
On July 30, 2018, 3:40 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 30, 2018, 3:40 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206696
-----------------------------------------------------------
Ship it!
I'll clean up for Mesos whitespace/formatting/commenting etc., and double-check the schema, but otherwise LGTM
- Andrew Schwartzmeyer
On July 31, 2018, 2:41 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 31, 2018, 2:41 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/
-----------------------------------------------------------
(Updated July 31, 2018, 9:41 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
Repository: mesos
Description
-------
Support parsing schema 2 and fetching blob with multiple URLs as
specified in schema 2. Update `curl` version to 7.61.0 because of bug
encountered in version 7.57.0.
Diffs (updated)
-----
3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
include/mesos/docker/v2_2.hpp PRE-CREATION
include/mesos/docker/v2_2.proto PRE-CREATION
include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/67931/diff/4/
Changes: https://reviews.apache.org/r/67931/diff/3-4/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/
-----------------------------------------------------------
(Updated July 30, 2018, 3:40 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
Changes
-------
Remove dependency so 30 -> 31.
Repository: mesos
Description
-------
Support parsing schema 2 and fetching blob with multiple URLs as
specified in schema 2. Update `curl` version to 7.61.0 because of bug
encountered in version 7.57.0.
Diffs
-----
3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
include/mesos/docker/v2_2.hpp PRE-CREATION
include/mesos/docker/v2_2.proto PRE-CREATION
include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/67931/diff/3/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/#review206201
-----------------------------------------------------------
Didn't look at the patch, but some time ago I posted a patchset that added V2 Schema 2 manifest support here: https://issues.apache.org/jira/browse/MESOS-6934. Just in case it may be helpful in any way :)
- Ilya Pronin
On July 18, 2018, 2:27 a.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------------------------------------------------------
>
> (Updated July 18, 2018, 2:27 a.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Support parsing schema 2 and fetching blob with multiple URLs as
> specified in schema 2. Update `curl` version to 7.61.0 because of bug
> encountered in version 7.57.0.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
> 3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
> include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
> include/mesos/docker/v2_2.hpp PRE-CREATION
> include/mesos/docker/v2_2.proto PRE-CREATION
> include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
> src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
> src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
> src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
> src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
> src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
> src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
> src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/67931/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/
-----------------------------------------------------------
(Updated July 18, 2018, 9:27 a.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
Repository: mesos
Description
-------
Support parsing schema 2 and fetching blob with multiple URLs as
specified in schema 2. Update `curl` version to 7.61.0 because of bug
encountered in version 7.57.0.
Diffs (updated)
-----
3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
include/mesos/docker/v2_2.hpp PRE-CREATION
include/mesos/docker/v2_2.proto PRE-CREATION
include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/67931/diff/3/
Changes: https://reviews.apache.org/r/67931/diff/2-3/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 67931: Support Image Manifest Version 2 Schema 2.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67931/
-----------------------------------------------------------
(Updated July 17, 2018, 11:14 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
Repository: mesos
Description (updated)
-------
Support parsing schema 2 and fetching blob with multiple URLs as
specified in schema 2. Update `curl` version to 7.61.0 because of bug
encountered in version 7.57.0.
Diffs (updated)
-----
3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f
3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a
include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162
include/mesos/docker/v2_2.hpp PRE-CREATION
include/mesos/docker/v2_2.proto PRE-CREATION
include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b
src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5
src/Makefile.am 228e168c22f3fd0367f029c506171c6979b31c07
src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9
src/slave/containerizer/mesos/containerizer.cpp 98129d006cda9b65804b518619b6addc8990410a
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2
src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36
src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8
src/uri/fetchers/docker.hpp cdbab9a5684a68a729be12bc06d331acca137da5
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/67931/diff/2/
Changes: https://reviews.apache.org/r/67931/diff/1-2/
Testing
-------
Thanks,
Liangyu Zhao