mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 49598: Added ability to parse docker v1 'ImageManifest' from 'docker inspect'.
Date Thu, 07 Jul 2016 01:05:41 GMT


> On July 6, 2016, 11:50 p.m., Jie Yu wrote:
> > Can you verify with Docker code that `docker inspect` for image actually return
the v1 image manifest? Better to add a link to the comments.
> 
> Kevin Klues wrote:
>     Docker inspect seems to return its own type which is a superset of the fields listed
in the v1 image manifest spec:
>     
>     Function to do the inspect:
>     https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/daemon/image_inspect.go
>     
>     ImageInspect Type:
>     https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go
>     
>     There doesn't seem to be any sort of "inheritance" going on where the results of
a `docker store` and the results of a `docker inspect` return types that are based on on another.
 Probably part of the reason the two outputs differ in terms of how they case the labels.
> 
> Jie Yu wrote:
>     Looks like some fields use the same type, but yeah, there is no inheritence.
>     https://github.com/docker/docker/blob/b316bc42fe1ad8edc709bf28975fb3a52e766344/image/image.go#L21
>     https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/types.go#L116
>     
>     Maybe we should model container.Config in our protobuf since this is shared:
>     https://github.com/docker/docker/blob/972c6a7113bffc91c6206f13758951acafa2e461/vendor/src/github.com/docker/engine-api/types/container/config.go#L36
>     
>     and then, module ImageInspect result as well in src/docker/spec.proto?
> 
> Kevin Klues wrote:
>     That seems reasonable. The only field we specifically care about is `Config.Labels`,
so we could even think about passing just the `config` into the `shouldInject()` function
instead of a Manifest.

Yeah, that makes sense! there are also some tech debts i realized in `include/mesos/docker/*.proto`.
We probably should split the proto message into namespaces, following docker:
```
docker.spec.container (docker.spec.container.Config)
docker.spec.engine (docker.spec.engine.ImageInspect)
docker.spec.image.v1
docker.spec.image.v2
```

Let's read the daemon code more to have a proper modeling.


- Jie


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


On July 6, 2016, 11:15 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49598/
> -----------------------------------------------------------
> 
> (Updated July 6, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Yubo Li.
> 
> 
> Bugs: MESOS-5779
>     https://issues.apache.org/jira/browse/MESOS-5779
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `docker::spec::v1::ImageManifest` protobuf implements the
> official v1 image manifest specification found at:
> 
> https://github.com/docker/docker/blob/master/image/spec/v1.md
> 
> The field names in this spec are all written in snake_case as are the
> field names of the JSON representing the image manifest when reading
> it from disk (for example after performing a `docker save`). As such,
> the protobuf for ImageManifest also provides these fields in
> snake_case. Unfortunately, the `docker inspect` command also provides
> a method of retrieving the JSON for an image manifest, with one major
> caveat -- it represents all of its top level keys in CamelCase.
> 
> To allow both representations to be parsed in the same way, we
> intercept the incoming JSON from either source (disk or `docker
> inspect`) and convert it to a canonical snake_case representation.
> 
> 
> Diffs
> -----
> 
>   src/docker/spec.cpp 2711578626dd1847f73cbf7bd3771f36e6755a99 
>   src/tests/containerizer/docker_tests.cpp 00ccc2fc54a984f3a67573ce6fe7aee3e60ce3a2 
> 
> Diff: https://reviews.apache.org/r/49598/diff/
> 
> 
> Testing
> -------
> 
> `sudo GTEST_FILTER="*ROOT_DOCKER_CompareImageManifests" bin/mesos-tests.sh`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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