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 2017/01/03 22:47:13 UTC
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/
-----------------------------------------------------------
(Updated Jan. 3, 2017, 2:47 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
Changes
-------
Rebased.
Bugs: MESOS-6654
https://issues.apache.org/jira/browse/MESOS-6654
Repository: mesos
Description
-------
This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
Diff: https://reviews.apache.org/r/54215/diff/
Testing
-------
make check
Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
Thanks,
Gilbert Song
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
> On Jan. 22, 2017, 11:19 a.m., Jie Yu wrote:
> > Let's take a look how docker handles this case.
> >
> > My feeling is that it's ok to have duplicated layers because what if I want to apply the same content more than once?
If there exists duplicate layers in an image. Those layers will still be pulled and untarred to the store. Since the layer id is unique depending on the content, so there is only single layer in the store. For the same content, it does not change any if we mount/copy them twice.
In docker the layer ids are imported from the store by reading the disk:
https://github.com/docker/docker/blob/master/layer/filestore.go#L310
so the layer ids should be unique when the layer store loads the layers:
https://github.com/docker/docker/blob/master/layer/layer_store.go#L90
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162580
-----------------------------------------------------------
On Jan. 19, 2017, 4:08 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 4:08 a.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 22, 2017, 7:19 p.m., Jie Yu wrote:
> > Let's take a look how docker handles this case.
> >
> > My feeling is that it's ok to have duplicated layers because what if I want to apply the same content more than once?
>
> Gilbert Song wrote:
> If there exists duplicate layers in an image. Those layers will still be pulled and untarred to the store. Since the layer id is unique depending on the content, so there is only single layer in the store. For the same content, it does not change any if we mount/copy them twice.
>
> In docker the layer ids are imported from the store by reading the disk:
> https://github.com/docker/docker/blob/master/layer/filestore.go#L310
>
> so the layer ids should be unique when the layer store loads the layers:
> https://github.com/docker/docker/blob/master/layer/layer_store.go#L90
>
> Jie Yu wrote:
> Can you paste the link to the comment?
Please use a fixed version so that the line number does not change.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162580
-----------------------------------------------------------
On Jan. 24, 2017, 8:35 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 8:35 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 22, 2017, 7:19 p.m., Jie Yu wrote:
> > Let's take a look how docker handles this case.
> >
> > My feeling is that it's ok to have duplicated layers because what if I want to apply the same content more than once?
>
> Gilbert Song wrote:
> If there exists duplicate layers in an image. Those layers will still be pulled and untarred to the store. Since the layer id is unique depending on the content, so there is only single layer in the store. For the same content, it does not change any if we mount/copy them twice.
>
> In docker the layer ids are imported from the store by reading the disk:
> https://github.com/docker/docker/blob/master/layer/filestore.go#L310
>
> so the layer ids should be unique when the layer store loads the layers:
> https://github.com/docker/docker/blob/master/layer/layer_store.go#L90
Can you paste the link to the comment?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162580
-----------------------------------------------------------
On Jan. 24, 2017, 8:35 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 8:35 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162580
-----------------------------------------------------------
Let's take a look how docker handles this case.
My feeling is that it's ok to have duplicated layers because what if I want to apply the same content more than once?
- Jie Yu
On Jan. 19, 2017, 12:08 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2017, 12:08 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
> On Jan. 27, 2017, 8:43 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp, line 301
> > <https://reviews.apache.org/r/54215/diff/7/?file=1616069#file1616069line301>
> >
> > I'd add some comment about the order of `fslayers` here. whether it is:
> > ```
> > [parent, child]
> > ```
> > or
> > ```
> > [child, parent]
> > ```
> >
> > Since child layer can overwrite contents in the parent layer, I think what backend expects is `[parent, child]`.
> >
> > That means fslayers here is in `[child, parent]` order. Now, can you verify that the overwriting is ok. I'd improve the comment here.
You are correct. It can be proved in any docker image manifest.
```
"history": [
{
"v1Compatibility": "{\"architecture\":\"amd64\",\"config\":{\"Hostname\":\"26ba10d264c2\",\"Domainname\":\"\",\"User\":\"\",\"AttachStdin\":false,\"AttachStdout\":false,\"AttachStderr\":false,\"ExposedPorts\":{\"5000/tcp\":{}},\"Tty\":false,\"OpenStdin\":false,\"StdinOnce\":false,\"Env\":[\"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\"],\"Cmd\":[\"/etc/docker/registry/config.yml\"],\"Image\":\"sha256:ab9c83929fc3997f57912fb54a5dbaeff61a506b23d56b91591e665082f2ec9e\",\"Volumes\":{\"/var/lib/registry\":{}},\"WorkingDir\":\"\",\"Entrypoint\":[\"/entrypoint.sh\"],\"OnBuild\":[],\"Labels\":{}},\"container\":\"876f643aaa16e30f99d14d6fd707432c8c8606696a18c53303d382761fb6573f\",\"container_config\":{\"Hostname\":\"26ba10d264c2\",\"Domainname\":\"\",\"User\":\"\",\"AttachStdin\":false,\"AttachStdout\":false,\"AttachStderr\":false,\"ExposedPorts\":{\"5000/tcp\":{}},\"Tty\":false,\"OpenStdin\":false,\"StdinOnce\":false,\"Env\":[\"PATH=/usr/local/sbin:/usr/local/bi
n:/usr/sbin:/usr/bin:/sbin:/bin\"],\"Cmd\":[\"/bin/sh\",\"-c\",\"#(nop) \",\"CMD [\\"/etc/docker/registry/config.yml\\"]\"],\"Image\":\"sha256:ab9c83929fc3997f57912fb54a5dbaeff61a506b23d56b91591e665082f2ec9e\",\"Volumes\":{\"/var/lib/registry\":{}},\"WorkingDir\":\"\",\"Entrypoint\":[\"/entrypoint.sh\"],\"OnBuild\":[],\"Labels\":{}},\"created\":\"2017-01-18T05:14:00.691185561Z\",\"docker_version\":\"1.12.6\",\"id\":\"46afc3aaeab95b4c8226da6a3a65a7c24dd012a9c756e8134e136c47333f8271\",\"os\":\"linux\",\"parent\":\"a375b425d3042b81f88b2cea92aecc1ce4e621bcdbfcd96807db2f9878687e76\",\"throwaway\":true}"
},
{
"v1Compatibility": "{\"id\":\"a375b425d3042b81f88b2cea92aecc1ce4e621bcdbfcd96807db2f9878687e76\",\"parent\":\"ca1c9bbf547c023906271d9a4cc8b5a2c006dada5aecf221768c08ed052f2b28\",\"created\":\"2017-01-18T05:14:00.170761825Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) ENTRYPOINT [\\"/entrypoint.sh\\"]\"]},\"throwaway\":true}"
},
{
"v1Compatibility": "{\"id\":\"ca1c9bbf547c023906271d9a4cc8b5a2c006dada5aecf221768c08ed052f2b28\",\"parent\":\"262f434a6db5dfc7e5088923b4eed39b6e837f160a403de168d255782896ca8c\",\"created\":\"2017-01-18T05:13:59.523763783Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) COPY file:7b57f7ab1a8cf85c00768560fffc926543a60c9c9f7a2b172767dcc9a3203394 in /entrypoint.sh \"]}}"
},
{
"v1Compatibility": "{\"id\":\"262f434a6db5dfc7e5088923b4eed39b6e837f160a403de168d255782896ca8c\",\"parent\":\"8da0a1a131a17ca30fe02d5a2c5c64ddd7ca7401efc4c81129b1740ba49a216b\",\"created\":\"2017-01-18T05:13:58.66681134Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) EXPOSE 5000/tcp\"]},\"throwaway\":true}"
},
{
"v1Compatibility": "{\"id\":\"8da0a1a131a17ca30fe02d5a2c5c64ddd7ca7401efc4c81129b1740ba49a216b\",\"parent\":\"343c0e0e9a064d07213c8a9026626f4ca3b3f12ce9b955b92d5114862675ce3c\",\"created\":\"2017-01-18T05:13:57.905190944Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) VOLUME [/var/lib/registry]\"]},\"throwaway\":true}"
},
{
"v1Compatibility": "{\"id\":\"343c0e0e9a064d07213c8a9026626f4ca3b3f12ce9b955b92d5114862675ce3c\",\"parent\":\"f65507bfd5af91b293780b3a955f3b5a1e4f3fa58bcfb6fbfd56814805db1f9c\",\"created\":\"2017-01-18T05:13:53.583988605Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) COPY file:6c4758d509045dc45381fa2df2e7ffcc661afcaa29805c75f8f1976f2b016db8 in /etc/docker/registry/config.yml \"]}}"
},
{
"v1Compatibility": "{\"id\":\"f65507bfd5af91b293780b3a955f3b5a1e4f3fa58bcfb6fbfd56814805db1f9c\",\"parent\":\"38535c2527ab54323dd4d430bb9e5e0f95b66faf27f0fd70c794efbf6979fbdc\",\"created\":\"2017-01-18T05:13:52.694455556Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) COPY file:6ccb0558ad0a49a3f19a99428209cf72cb67a21381c8e4286365d5e0aebebd50 in /bin/registry \"]}}"
},
{
"v1Compatibility": "{\"id\":\"38535c2527ab54323dd4d430bb9e5e0f95b66faf27f0fd70c794efbf6979fbdc\",\"parent\":\"6f8afccfec171d02b03a207536fcc5585825c5161a2923f2e9043e61cab97343\",\"created\":\"2016-12-27T21:38:19.807455414Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c set -ex \u0026\u0026 apk add --no-cache ca-certificates apache2-utils\"]}}"
},
{
"v1Compatibility": "{\"id\":\"6f8afccfec171d02b03a207536fcc5585825c5161a2923f2e9043e61cab97343\",\"created\":\"2016-12-27T18:17:13.762716133Z\",\"container_config\":{\"Cmd\":[\"/bin/sh -c #(nop) ADD file:eeed5f514a35d18fcd9cbfe6c40c582211020bffdd53e4799018d33826fe5067 in / \"]}}"
}
],
```
The comment was updated.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review163283
-----------------------------------------------------------
On Jan. 25, 2017, 5:37 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 5:37 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp cecf34a23329a64fdbce7de4b83827a30975e9a4
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review163283
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp (lines 293 - 296)
<https://reviews.apache.org/r/54215/#comment234717>
This comment here is not quite descripative. I'd suggest use the reply you wrote to me in the review board.
ALso, make sure to paste the link to docker code.
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp (line 301)
<https://reviews.apache.org/r/54215/#comment234718>
I'd add some comment about the order of `fslayers` here. whether it is:
```
[parent, child]
```
or
```
[child, parent]
```
Since child layer can overwrite contents in the parent layer, I think what backend expects is `[parent, child]`.
That means fslayers here is in `[child, parent]` order. Now, can you verify that the overwriting is ok. I'd improve the comment here.
- Jie Yu
On Jan. 26, 2017, 1:37 a.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 1:37 a.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp cecf34a23329a64fdbce7de4b83827a30975e9a4
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/
-----------------------------------------------------------
(Updated Jan. 27, 2017, 11:46 a.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
Bugs: MESOS-6654
https://issues.apache.org/jira/browse/MESOS-6654
Repository: mesos
Description
-------
This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp cecf34a23329a64fdbce7de4b83827a30975e9a4
Diff: https://reviews.apache.org/r/54215/diff/
Testing
-------
make check
Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
Thanks,
Gilbert Song
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/
-----------------------------------------------------------
(Updated Jan. 25, 2017, 5:37 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
Changes
-------
Implementation using my proposal (2).
Bugs: MESOS-6654
https://issues.apache.org/jira/browse/MESOS-6654
Repository: mesos
Description
-------
This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp cecf34a23329a64fdbce7de4b83827a30975e9a4
Diff: https://reviews.apache.org/r/54215/diff/
Testing
-------
make check
Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
Thanks,
Gilbert Song
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
> On Jan. 25, 2017, 7:45 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp, lines 276-279
> > <https://reviews.apache.org/r/54215/diff/6/?file=1613732#file1613732line276>
> >
> > Can you also validate the ordering with docker code? For instance, say the layer paths are:
> >
> > [a, b, c, d, a, e, f]
> >
> > will the end result be:
> >
> > [b, c, d, a, e, f]
> >
> > or
> >
> > [a, b, c, d, e, f]
> >
> > Please note that the ordering we have here in our code might not be the same as that in docker (i.e., whether base layer is to the left most or to the right most?)
>
> Gilbert Song wrote:
> Did some research on docker. Basically, as I mentioned above, the layer ids are read from the disk, so they are `unique` and should be ordered `alphabetically`. Then, docker loads each layer depending on the layer `child-parent` relationship. It is similar to what we currently do in local puller and it would not have the case of duplicate layer paths since it is like a linked list from bottom to the top. The order should be:
>
> [Grandparent, parent, child]
>
> In our registry puller, the vector of layer ids are collected differently (not relying on finding next parent from the base layer). We just queue all the layer ids using the returned manifest, and fortunately the `v1Compatibility::id` from the manifest are sorted before hand depending on the `parent-child` relationship.
>
> Only some self-built images may contain duplicate layers (e.g., [a, a, b, b, b, c] from the manifest). We should still fix it for them though. It seems to me that our current fix in docker store is just a workaround. I would propose to fix it in registry puller `pull()`:
>
> 1. Depending on the leaf layer id, collect all layer ids using `parent-child` relationship.
> 2. Filter the vector of layer ids to make sure ids are unique in registry puller.
>
> I prefer (2). Thoughts?
Tested the same image `mesosphere/inky` using both pullers on master branch. As expected, `local puller` passed and `registry puller` failed.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162963
-----------------------------------------------------------
On Jan. 24, 2017, 12:35 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 12:35 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
> On Jan. 25, 2017, 7:45 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp, lines 276-279
> > <https://reviews.apache.org/r/54215/diff/6/?file=1613732#file1613732line276>
> >
> > Can you also validate the ordering with docker code? For instance, say the layer paths are:
> >
> > [a, b, c, d, a, e, f]
> >
> > will the end result be:
> >
> > [b, c, d, a, e, f]
> >
> > or
> >
> > [a, b, c, d, e, f]
> >
> > Please note that the ordering we have here in our code might not be the same as that in docker (i.e., whether base layer is to the left most or to the right most?)
Did some research on docker. Basically, as I mentioned above, the layer ids are read from the disk, so they are `unique` and should be ordered `alphabetically`. Then, docker loads each layer depending on the layer `child-parent` relationship. It is similar to what we currently do in local puller and it would not have the case of duplicate layer paths since it is like a linked list from bottom to the top. The order should be:
[Grandparent, parent, child]
In our registry puller, the vector of layer ids are collected differently (not relying on finding next parent from the base layer). We just queue all the layer ids using the returned manifest, and fortunately the `v1Compatibility::id` from the manifest are sorted before hand depending on the `parent-child` relationship.
Only some self-built images may contain duplicate layers (e.g., [a, a, b, b, b, c] from the manifest). We should still fix it for them though. It seems to me that our current fix in docker store is just a workaround. I would propose to fix it in registry puller `pull()`:
1. Depending on the leaf layer id, collect all layer ids using `parent-child` relationship.
2. Filter the vector of layer ids to make sure ids are unique in registry puller.
I prefer (2). Thoughts?
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162963
-----------------------------------------------------------
On Jan. 24, 2017, 12:35 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 12:35 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review162963
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/store.cpp (lines 276 - 279)
<https://reviews.apache.org/r/54215/#comment234339>
Can you also validate the ordering with docker code? For instance, say the layer paths are:
[a, b, c, d, a, e, f]
will the end result be:
[b, c, d, a, e, f]
or
[a, b, c, d, e, f]
Please note that the ordering we have here in our code might not be the same as that in docker (i.e., whether base layer is to the left most or to the right most?)
- Jie Yu
On Jan. 24, 2017, 8:35 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 8:35 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/
-----------------------------------------------------------
(Updated Jan. 24, 2017, 12:35 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
Changes
-------
Rebased.
Bugs: MESOS-6654
https://issues.apache.org/jira/browse/MESOS-6654
Repository: mesos
Description
-------
This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
Diff: https://reviews.apache.org/r/54215/diff/
Testing
-------
make check
Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
Thanks,
Gilbert Song
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/
-----------------------------------------------------------
(Updated Jan. 19, 2017, 4:08 a.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
Changes
-------
Rebased.
Bugs: MESOS-6654
https://issues.apache.org/jira/browse/MESOS-6654
Repository: mesos
Description
-------
This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
Diff: https://reviews.apache.org/r/54215/diff/
Testing
-------
make check
Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
Thanks,
Gilbert Song
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Gilbert Song <so...@gmail.com>.
> On Jan. 6, 2017, 11:18 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/provisioner/docker/store.cpp, line 268
> > <https://reviews.apache.org/r/54215/diff/4/?file=1596399#file1596399line268>
> >
> > Given that the `layerPaths` are meant to be unique why not make the `layerPaths` itself a `hashset`. Will be more explicit in terms of its usage.
We need to keep the layer ids in order. There is dependency on that.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review160808
-----------------------------------------------------------
On Jan. 3, 2017, 2:47 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 2:47 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 54215: Fixed duplicate image layer ids returned by
docker store.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54215/#review160808
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/store.cpp (line 268)
<https://reviews.apache.org/r/54215/#comment232035>
Given that the `layerPaths` are meant to be unique why not make the `layerPaths` itself a `hashset`. Will be more explicit in terms of its usage.
- Avinash sridharan
On Jan. 3, 2017, 10:47 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54215/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 10:47 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian Zhang, and Zhitao Li.
>
>
> Bugs: MESOS-6654
> https://issues.apache.org/jira/browse/MESOS-6654
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This issue is exposed by pulling the 'mesosphere/inky' docker
> image using registry puller. Due to the duplicate layer id
> from the manifest, there are duplicate layer pathes passed
> to the backend. The aufs backend cannot handle this case and
> returns 'invalid arguments' error. Ideally, we should make
> sure that layer paths that are passed to the backend are
> unique.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/store.cpp 9dccd0673dbc0c61abfd4b88097f86e7d7131c46
>
> Diff: https://reviews.apache.org/r/54215/diff/
>
>
> Testing
> -------
>
> make check
>
> Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.
>
> Manually tested using the `mesosphere/inky` image, which contains duplicate layer ids.
>
>
> Thanks,
>
> Gilbert Song
>
>