mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 70288: Supported docker manifest v2 schema2.
Date Mon, 25 Mar 2019 07:56:27 GMT

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




src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 348-349 (patched)
<https://reviews.apache.org/r/70288/#comment300081>

    We have this code in two places in this method, one for v2 s2 and another for v2 s1. Can
we just do it once in line 330? I think that would also improve the debuggability for error
case since we will see the whole manifest in the logs.



src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 500 (patched)
<https://reviews.apache.org/r/70288/#comment300083>

    There might be duplicated layer digest in the manifest, so we need to skip the duplicated
one for `layerIds`.



src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 617 (patched)
<https://reviews.apache.org/r/70288/#comment300084>

    s/blob sums/digests/ ?



src/tests/containerizer/provisioner_docker_tests.cpp
Line 1210 (original), 1208 (patched)
<https://reviews.apache.org/r/70288/#comment300085>

    Why do we want to change the digest here?



src/uri/fetchers/docker.cpp
Lines 689-694 (original), 676-682 (patched)
<https://reviews.apache.org/r/70288/#comment300086>

    Can we add some comments here? Like why do we want to specify both s1 and s2 media types
here. I think the reason that we need `v1+prettyjws` here is for some registries (like gcr)
they do not support requesting manifest with `v1+json`, and the reason that we need `v2+json`
is if registries drop the s1 support in future, UCR can still pull images with s2 manifest.



src/uri/fetchers/docker.cpp
Lines 742 (patched)
<https://reviews.apache.org/r/70288/#comment300087>

    s/support/supports/
    s/type/types/



src/uri/fetchers/docker.cpp
Line 868 (original), 775 (patched)
<https://reviews.apache.org/r/70288/#comment300088>

    Add the schema version in this log message? Like `Failed to parse the schema 1 image manifest`.
    
    Ditto for `os::write` and schema 2 code below.



src/uri/fetchers/docker.cpp
Line 875 (original), 780-781 (patched)
<https://reviews.apache.org/r/70288/#comment300096>

    These two lines can be merged into one.



src/uri/fetchers/docker.cpp
Line 1028 (original), 956 (patched)
<https://reviews.apache.org/r/70288/#comment300093>

    I think this will make `urlFetchBlob` not working as what we expect. The reason is in
your implementation we will fetch manifest only once (either v2 s1 or v2 s2, so there is only
one manifest in staging dir), but originally we will fetch manifest twice (v2 s1 for all platforms
and v2 s2 only for Windows). So now in `urlFetchBlob` we will read and parse the same manifest
again (because there is only one manifest) which I think is not what we want.


- 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/70288/
> -----------------------------------------------------------
> 
> (Updated March 24, 2019, 12:33 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Ilya Pronin, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6934
>     https://issues.apache.org/jira/browse/MESOS-6934
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Supported docker manifest v2 schema2.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp a5683e3fe15dd35596122fcc0c580ae9d3adf7f2

>   src/tests/containerizer/provisioner_docker_tests.cpp a2a7f11eeb533d7790890988061612e11a798067

>   src/uri/fetchers/docker.cpp a87d7f086378539e6c8048e935389919a1164fbc 
> 
> 
> Diff: https://reviews.apache.org/r/70288/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message