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/08/21 21:30:17 UTC
Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
Bugs: MESOS-9159
https://issues.apache.org/jira/browse/MESOS-9159
Repository: mesos
Description
-------
In addition to fetching from DockerHub, DockerFetcher now support
fetching from foreign URLs provided in V2S2 Docker image manifest.
Diffs
-----
3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/68454/diff/1/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Aug. 23, 2018, 10:45 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 99 (patched)
> > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line99>
> >
> > Can you explain a bit why you need to do this?
> >
> > this sounds to me that we need to make `URI` streaming function to be aware windows, instead of doing it adhoc here?
Yes, that might be a good future refactor, but I don't think it's worth blocking these patches. We can add a `TODO(andschwa)` here to move this logic into `URI` (we had to do something similar coming from URIs with `path::from_uri` for the fetcher, it's reasonable to do it the other way too now).
> On Aug. 23, 2018, 10:45 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 235 (patched)
> > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line235>
> >
> > Can you check with Andy. Let's be consisent on doing encoding for path on windows. If we use percentage encoding in other places, let's be consistent
I don't think this is worth blocking on right now; let's put in a `TODO(andschwa)` to reconcile with https://reviews.apache.org/r/68297/ as there is still ongoing dev-list dicussion on what and how we want to percent-encode. Nothing will take an immediate dependency on this, as this is "infrastructure" code working towards full support of the UCR. It can (and likely will) be modified in the future.
> On Aug. 23, 2018, 10:45 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 735-736 (patched)
> > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line735>
> >
> > Any reason we don't return a `Failure` here?
Yes, there was an explanation in a prior iteration. IIRC this is because if the schema 2 isn't fetched, the protocol is that we fall back to schema 1.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review207849
-----------------------------------------------------------
On Aug. 23, 2018, 3:44 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 3:44 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
> 3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Liangyu Zhao via Review Board <no...@reviews.apache.org>.
> On Aug. 24, 2018, 5:45 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 235 (patched)
> > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line235>
> >
> > Can you check with Andy. Let's be consisent on doing encoding for path on windows. If we use percentage encoding in other places, let's be consistent
>
> Andrew Schwartzmeyer wrote:
> I don't think this is worth blocking on right now; let's put in a `TODO(andschwa)` to reconcile with https://reviews.apache.org/r/68297/ as there is still ongoing dev-list dicussion on what and how we want to percent-encode. Nothing will take an immediate dependency on this, as this is "infrastructure" code working towards full support of the UCR. It can (and likely will) be modified in the future.
>
> Jie Yu wrote:
> OK, can you make sure to move the replaceColon function to this cpp file so that it's clear that it's only for this file.
But `replaceColon` is needed in later patch https://reviews.apache.org/r/67932/
- Liangyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review207849
-----------------------------------------------------------
On Aug. 23, 2018, 10:44 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 10:44 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
> 3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 24, 2018, 5:45 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 235 (patched)
> > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line235>
> >
> > Can you check with Andy. Let's be consisent on doing encoding for path on windows. If we use percentage encoding in other places, let's be consistent
>
> Andrew Schwartzmeyer wrote:
> I don't think this is worth blocking on right now; let's put in a `TODO(andschwa)` to reconcile with https://reviews.apache.org/r/68297/ as there is still ongoing dev-list dicussion on what and how we want to percent-encode. Nothing will take an immediate dependency on this, as this is "infrastructure" code working towards full support of the UCR. It can (and likely will) be modified in the future.
OK, can you make sure to move the replaceColon function to this cpp file so that it's clear that it's only for this file.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review207849
-----------------------------------------------------------
On Aug. 23, 2018, 10:44 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 10:44 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
> 3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 24, 2018, 5:45 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 235 (patched)
> > <https://reviews.apache.org/r/68454/diff/2/?file=2076952#file2076952line235>
> >
> > Can you check with Andy. Let's be consisent on doing encoding for path on windows. If we use percentage encoding in other places, let's be consistent
>
> Andrew Schwartzmeyer wrote:
> I don't think this is worth blocking on right now; let's put in a `TODO(andschwa)` to reconcile with https://reviews.apache.org/r/68297/ as there is still ongoing dev-list dicussion on what and how we want to percent-encode. Nothing will take an immediate dependency on this, as this is "infrastructure" code working towards full support of the UCR. It can (and likely will) be modified in the future.
>
> Jie Yu wrote:
> OK, can you make sure to move the replaceColon function to this cpp file so that it's clear that it's only for this file.
>
> Liangyu Zhao wrote:
> But `replaceColon` is needed in later patch https://reviews.apache.org/r/67932/
Maybe some helper defined in src/uri/fetcher/docker.hpp?
this is essentially a protocol between docker fetcher and registry puller. I'd suggest a helper like `getBlobPath(directory, blobSum)`
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review207849
-----------------------------------------------------------
On Aug. 23, 2018, 10:44 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 10:44 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
> 3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review207849
-----------------------------------------------------------
src/uri/fetchers/docker.cpp
Lines 99 (patched)
<https://reviews.apache.org/r/68454/#comment291392>
Can you explain a bit why you need to do this?
this sounds to me that we need to make `URI` streaming function to be aware windows, instead of doing it adhoc here?
src/uri/fetchers/docker.cpp
Lines 231-232 (patched)
<https://reviews.apache.org/r/68454/#comment291393>
Ditto.
src/uri/fetchers/docker.cpp
Lines 235 (patched)
<https://reviews.apache.org/r/68454/#comment291394>
Can you check with Andy. Let's be consisent on doing encoding for path on windows. If we use percentage encoding in other places, let's be consistent
src/uri/fetchers/docker.cpp
Lines 357-358 (patched)
<https://reviews.apache.org/r/68454/#comment291395>
This is super confusing to readers.
I would suggest we just leave one `download` overload, and kill the others.
Instead, introduce a helper to get blobPath:
```
static string getBlobPath(
const string& directory,
const URI& uri) {
#ifdef __WINDOWS__
...
#else
...
#endif
}
```
src/uri/fetchers/docker.cpp
Lines 712 (patched)
<https://reviews.apache.org/r/68454/#comment291385>
nits: one blank line above
src/uri/fetchers/docker.cpp
Lines 728 (patched)
<https://reviews.apache.org/r/68454/#comment291386>
since this is `.then`, the argument to the lambda should be `http::Response` instead of `Future<http::Response>`
```
.then(defer(self(), [=](const http::Response& future)
```
src/uri/fetchers/docker.cpp
Lines 735-736 (patched)
<https://reviews.apache.org/r/68454/#comment291387>
Any reason we don't return a `Failure` here?
src/uri/fetchers/docker.cpp
Lines 925-926 (patched)
<https://reviews.apache.org/r/68454/#comment291389>
I got confused. Why this is related to manifest? Please follow what I suggested in the last review.
src/uri/fetchers/docker.cpp
Lines 969 (patched)
<https://reviews.apache.org/r/68454/#comment291390>
No need for the warning. The caller should be responsible for printing
src/uri/fetchers/docker.cpp
Lines 987 (patched)
<https://reviews.apache.org/r/68454/#comment291388>
Looks like this is never used?
src/uri/fetchers/docker.cpp
Lines 1004 (patched)
<https://reviews.apache.org/r/68454/#comment291396>
we typically avoid too deep nesting by short circuit first:
```
for (int i = 0; i < manifest->layers_size(); i++) {
if (blobsum != manifest->layers(i).digest()) {
continue;
}
for (...) {
}
break;
}
```
src/uri/fetchers/docker.cpp
Lines 1005 (patched)
<https://reviews.apache.org/r/68454/#comment291397>
Please do j++ to be consistent with i++ above.
src/uri/fetchers/docker.cpp
Lines 1026 (patched)
<https://reviews.apache.org/r/68454/#comment291391>
Let's avoid nesting by dealing with failure first:
```
if (urls.empty()) {
return Failure("...");
}
...
```
- Jie Yu
On Aug. 23, 2018, 10:44 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2018, 10:44 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
> 3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review208107
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Aug. 28, 2018, 11:26 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2018, 11:26 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
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/68454/
-----------------------------------------------------------
(Updated Aug. 28, 2018, 11:26 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
Bugs: MESOS-9159
https://issues.apache.org/jira/browse/MESOS-9159
Repository: mesos
Description
-------
DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
disk when agent is running on Windows. Linux part of the code in
agent is unchanged. In addition to fetching from DockerHub,
DockerFetcher now supports fetching from foreign URLs provided in
V2S2 Docker image manifest.
Diffs (updated)
-----
src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/68454/diff/5/
Changes: https://reviews.apache.org/r/68454/diff/4-5/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
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/68454/
-----------------------------------------------------------
(Updated Aug. 28, 2018, 10:38 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
Bugs: MESOS-9159
https://issues.apache.org/jira/browse/MESOS-9159
Repository: mesos
Description
-------
DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
disk when agent is running on Windows. Linux part of the code in
agent is unchanged. In addition to fetching from DockerHub,
DockerFetcher now supports fetching from foreign URLs provided in
V2S2 Docker image manifest.
Diffs (updated)
-----
src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/68454/diff/4/
Changes: https://reviews.apache.org/r/68454/diff/3-4/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review208066
-----------------------------------------------------------
src/uri/fetchers/docker.cpp
Lines 100 (patched)
<https://reviews.apache.org/r/68454/#comment291810>
Looks like you did this because we use `path::join` to construct URL. I think this can be avoided if we just use `strings::join` when constructing url. Constructing url using `path::join` does not make sense.
src/uri/fetchers/docker.cpp
Lines 935-937 (patched)
<https://reviews.apache.org/r/68454/#comment291811>
No need for this tmp variable.
src/uri/fetchers/docker.cpp
Lines 982-986 (patched)
<https://reviews.apache.org/r/68454/#comment291812>
Ditto. No need for this tmp variable.
- Jie Yu
On Aug. 25, 2018, 9:57 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2018, 9:57 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
> disk when agent is running on Windows. Linux part of the code in
> agent is unchanged. In addition to fetching from DockerHub,
> DockerFetcher now supports fetching from foreign URLs provided in
> V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
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/68454/
-----------------------------------------------------------
(Updated Aug. 25, 2018, 9:57 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
Bugs: MESOS-9159
https://issues.apache.org/jira/browse/MESOS-9159
Repository: mesos
Description
-------
DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
disk when agent is running on Windows. Linux part of the code in
agent is unchanged. In addition to fetching from DockerHub,
DockerFetcher now supports fetching from foreign URLs provided in
V2S2 Docker image manifest.
Diffs (updated)
-----
src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/68454/diff/3/
Changes: https://reviews.apache.org/r/68454/diff/2-3/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
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/68454/
-----------------------------------------------------------
(Updated Aug. 23, 2018, 10:44 p.m.)
Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
Bugs: MESOS-9159
https://issues.apache.org/jira/browse/MESOS-9159
Repository: mesos
Description (updated)
-------
DockerFetcher now fetches both V2S1 and V2S2 manifests to save on
disk when agent is running on Windows. Linux part of the code in
agent is unchanged. In addition to fetching from DockerHub,
DockerFetcher now supports fetching from foreign URLs provided in
V2S2 Docker image manifest.
Diffs (updated)
-----
3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
Diff: https://reviews.apache.org/r/68454/diff/2/
Changes: https://reviews.apache.org/r/68454/diff/1-2/
Testing
-------
Thanks,
Liangyu Zhao
Re: Review Request 68454: Windows: Fetch blobs with V2S2 Docker image
manifest.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68454/#review207803
-----------------------------------------------------------
I am perfectly fine if you combine this patch with the previous patch if that's easier.
src/uri/fetchers/docker.cpp
Line 861 (original), 906 (patched)
<https://reviews.apache.org/r/68454/#comment291311>
this is wierd in this patch. Can you make sure it's in the previous patch? `git add -p` is your friend.
src/uri/fetchers/docker.cpp
Lines 979 (patched)
<https://reviews.apache.org/r/68454/#comment291314>
I'd pass the http code here so that we only do the rest if the code is 404.
src/uri/fetchers/docker.cpp
Lines 981 (patched)
<https://reviews.apache.org/r/68454/#comment291313>
Caller is responsible for printing errors. No need for the VLOG here, please do:
```
return Failure(
"Failed to read schema 2 manifest: " +
_manifest.error());
```
src/uri/fetchers/docker.cpp
Lines 987-988 (patched)
<https://reviews.apache.org/r/68454/#comment291318>
Ditto. No need for VLOG
src/uri/fetchers/docker.cpp
Lines 1003 (patched)
<https://reviews.apache.org/r/68454/#comment291320>
Ditto.
src/uri/fetchers/docker.cpp
Lines 1009-1016 (patched)
<https://reviews.apache.org/r/68454/#comment291327>
I wouldn't use `onAny` here. The following is probably better:
```
Future<Nothing> urlFetchBlob(...)
{
// Calculate urls
...
return _urlFetchBlob(blobUri, directory, authHeaders, urls);
}
Future<Nothing> _urlFetchBlob(
const URI& blobUri,
const string& directory,
const http::Headers& authHeaders,
vector<string> urls)
{
if (urls.empty()) {
return Failure("...");
}
string url = urls.back();
urls.pop_back();
return download(...)
.then(defer(self(), [=](int code) {
if (code == http::Status::OK) {
return Nothing();
}
return _urlFetchBlob(blobUri, directory, authHeaders, urls);
});
}
```
- Jie Yu
On Aug. 21, 2018, 9:30 p.m., Liangyu Zhao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68454/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2018, 9:30 p.m.)
>
>
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
>
>
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In addition to fetching from DockerHub, DockerFetcher now support
> fetching from foreign URLs provided in V2S2 Docker image manifest.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/path.hpp ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7
> 3rdparty/stout/tests/path_tests.cpp 452865b919c0d3644eb0ece0e17e402318aaff41
> src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7
>
>
> Diff: https://reviews.apache.org/r/68454/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Liangyu Zhao
>
>