Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

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

(Updated May 24, 2016, 10:18 a.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Implemented parsing docker labels in v1 spec.


Diffs (updated)
-

  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

Diff: https://reviews.apache.org/r/47199/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Jojy Varghese]

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




src/docker/spec.cpp (lines 207 - 230)


Looks like there is scope for de-duplicating code between this block and 
the block above(`config` and `container_config`)?


- Jojy Varghese


On May 13, 2016, 8:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 13, 2016, 8:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 13, 2016, 8:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 13, 2016, 8:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Guangya Liu]

> On 五月 12, 2016, 12:44 p.m., Guangya Liu wrote:
> > src/docker/spec.cpp, lines 226-230
> > 
> >
> > Does there are any possiblity that there are duplicat labels in 
> > `config` and `container_config`? If so, how to handle the case if there are 
> > duplicate labels with `config`?
> 
> Gilbert Song wrote:
> From the docker spec, we are assuming the key should be unique and for 
> each Dockerfile that contains duplicate label for the same key, they would 
> overwrite the privious key. So we dont need to handle duplicates here. Good 
> thoughts!

Good to know, can you please add some comments here to clarify? Another point 
is that as we are `assuming the key should be unique` for each Dockerfile, but 
the Dockerfile is written by 3rd party, does it make sense to add some error 
handling here?


- Guangya


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


On 五月 13, 2016, 8:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated 五月 13, 2016, 8:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

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

(Updated May 13, 2016, 1:49 p.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Implemented parsing docker labels in v1 spec.


Diffs (updated)
-

  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

Diff: https://reviews.apache.org/r/47199/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

> On May 12, 2016, 5:44 a.m., Guangya Liu wrote:
> > src/docker/spec.cpp, lines 226-230
> > 
> >
> > Does there are any possiblity that there are duplicat labels in 
> > `config` and `container_config`? If so, how to handle the case if there are 
> > duplicate labels with `config`?

>From the docker spec, we are assuming the key should be unique and for each 
>Dockerfile that contains duplicate label for the same key, they would 
>overwrite the privious key. So we dont need to handle duplicates here. Good 
>thoughts!


- Gilbert


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


On May 11, 2016, 8:56 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 11, 2016, 8:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Guangya Liu]

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




src/docker/spec.cpp (line 171)


s/'Labels'/`Labels`

Ditto as following where you are using '' but not ``



src/docker/spec.cpp (lines 226 - 230)


Does there are any possiblity that there are duplicat labels in `config` 
and `container_config`? If so, how to handle the case if there are duplicate 
labels with `config`?


- Guangya Liu


On 五月 12, 2016, 3:56 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated 五月 12, 2016, 3:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

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

(Updated May 11, 2016, 8:56 p.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Implemented parsing docker labels in v1 spec.


Diffs (updated)
-

  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

Diff: https://reviews.apache.org/r/47199/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

> On May 10, 2016, 5:50 p.m., Kevin Klues wrote:
> > src/docker/spec.cpp, line 164
> > 
> >
> > Is there always a "config" and a "container_config" (below) in every 
> > image? If not, won't we error out unnecessarily here?
> 
> Gilbert Song wrote:
> I would say yes here, depending on docker image v1 spec.
> 
> And actually in our code base, this assumption has been there for a while:
> 
> https://github.com/apache/mesos/blob/c5bbd5af061a663ba4488a180fede3549e8082fa/src/docker/docker.cpp#L359~#L401
> 
> Please note that inside config or container_config, some fields should 
> always have their sections, they can be `JSON::Null` or just `{}` or `[]`, 
> but some fields are added after v1.0 spec so they may not be included in some 
> old images.

I step back to think about it. Even though I am sure they should exist, it 
might be safe to do a check instead of returning an error. Thanks!


- Gilbert


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


On May 10, 2016, 4:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 10, 2016, 4:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

> On May 10, 2016, 5:50 p.m., Kevin Klues wrote:
> > src/docker/spec.cpp, line 164
> > 
> >
> > Is there always a "config" and a "container_config" (below) in every 
> > image? If not, won't we error out unnecessarily here?

I would say yes here, depending on docker image v1 spec.

And actually in our code base, this assumption has been there for a while:
https://github.com/apache/mesos/blob/c5bbd5af061a663ba4488a180fede3549e8082fa/src/docker/docker.cpp#L359~#L401

Please note that inside config or container_config, some fields should always 
have their sections, they can be `JSON::Null` or just `{}` or `[]`, but some 
fields are added after v1.0 spec so they may not be included in some old images.


> On May 10, 2016, 5:50 p.m., Kevin Klues wrote:
> > src/docker/spec.cpp, lines 171-174
> > 
> >
> > There are certainly not labels in every image. Is there always a 
> > section for them though, even if they are not present?  Maybe not, because 
> > you call `isSome()` later on, but then the error message here is confusing.

The error msg is a little confusing. Good catch, thanks!

How about `Failed to parse 'Labels' as a JSON object: ...`? 

For most images I inspected they all have the labels section, if no labels 
presented they will be `"Labels": {}` , and in docker they are defined as 
string-string map. Since `Labels` is not introduced in docker image v1.0 spec, 
so for some old images they may not show up.


- Gilbert


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


On May 10, 2016, 4:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 10, 2016, 4:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Kevin Klues]

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




src/docker/spec.cpp (line 164)


Is there always a "config" and a "container_config" (below) in every image? 
If not, won't we error out unnecessarily here?



src/docker/spec.cpp (line 171)


There are certainly not labels in every image. Is there always a section 
for them though, even if they are not present?  Maybe not, because you call 
`isSome()` later on, but then the error message here is confusing.



src/docker/spec.cpp (lines 171 - 174)


There are certainly not labels in every image. Is there always a section 
for them though, even if they are not present?  Maybe not, because you call 
`isSome()` later on, but then the error message here is confusing.


- Kevin Klues


On May 10, 2016, 11:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 10, 2016, 11:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

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

Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Implemented parsing docker labels in v1 spec.


Diffs
-

  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

Diff: https://reviews.apache.org/r/47199/diff/


Testing
---

make check


Thanks,

Gilbert Song