Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-22 Thread Jie Yu 

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




src/Makefile.am (line 788)


the source is guarded by LINUX but the header is not. I removed the guard 
for LINUX for the source for now (since spec can be used on non-linux based 
systems).


- Jie Yu


On March 17, 2016, 8:56 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 17, 2016, 8:56 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-22 Thread Jie Yu 


> On March 17, 2016, 3:37 a.m., Jie Yu wrote:
> > src/CMakeLists.txt, line 279
> > 
> >
> > I don't think we put headers here.
> 
> Qian Zhang wrote:
> I had the same concern before, because it seems most source files here 
> are .cpp. But I also see the followings, so I guess it should be OK to put 
> headers here as well?
> `slave/containerizer/composing.cpp`
> `slave/containerizer/composing.hpp` 
> `slave/containerizer/docker.cpp`
> `slave/containerizer/docker.hpp`

I checked with Alex Clemmer who wrote the CMakefile at the first place. He 
suggested us to not include headers for consistency.

"The short answer is that we should technically do this so that IDEs like 
Visual Studio and Eclipse correctly report the header files in the source, but 
we don't, and it's not important for the functionality (_e.g._, we will 
correctly rebuild if you alter a header file, as long as that folder is 
included by CMake). So in other words: as implemented, including .hpp would 
require some doing, and we should sit down and do all the files if we choose 
this."


- Jie


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


On March 17, 2016, 8:56 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 17, 2016, 8:56 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-20 Thread Qian Zhang 


> On March 17, 2016, 11:37 a.m., Jie Yu wrote:
> > src/CMakeLists.txt, line 279
> > 
> >
> > I don't think we put headers here.

I had the same concern before, because it seems most source files here are 
.cpp. But I also see the followings, so I guess it should be OK to put headers 
here as well?
`slave/containerizer/composing.cpp`
`slave/containerizer/composing.hpp` 
`slave/containerizer/docker.cpp`
`slave/containerizer/docker.hpp`


- Qian


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


On March 16, 2016, 10:09 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 16, 2016, 10:09 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-19 Thread Qian Zhang 

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

(Updated March 17, 2016, 4:56 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Introduced a protobuf message "NetworkConfig".


Diffs (updated)
-

  src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-19 Thread Jie Yu 

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


Fix it, then Ship it!





src/CMakeLists.txt (line 279)


I don't think we put headers here.



src/slave/containerizer/mesos/isolators/network/cni/spec.cpp (line 37)


indentation here should be 2 as it's a statement.



src/slave/containerizer/mesos/isolators/network/cni/spec.cpp (line 38)


We typically insert a new line above if the preceding statement is a 
multiple line statement.


- Jie Yu


On March 16, 2016, 2:09 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 16, 2016, 2:09 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Qian Zhang 

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

(Updated March 16, 2016, 10:09 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Introduced a protobuf message "NetworkConfig".


Diffs (updated)
-

  src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Qian Zhang 


> On March 15, 2016, 11:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, line 40
> > 
> >
> > Why are the position identifiers not sequential ?

Sorry, my bad, will fix it soon


> On March 15, 2016, 11:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, line 42
> > 
> >
> > You will need to store the path of the network config as well, since 
> > during the isolate/prepare phase you will need to re-read the whole config 
> > (since it might have plugin specific parameters).

We are going to introduce a new struct `NetworkConfigInfo` keep path and 
protobuf for each network configuration, please see 
https://reviews.apache.org/r/44555/ for details.


- Qian


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


On March 15, 2016, 10:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 15, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Qian Zhang 


> On March 15, 2016, 11:28 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, lines 45-46
> > 
> >
> > The isolator doesn't actually need the IPAM or the DNS information. 
> > This information would be used only by the plugin and the isolator doesn't 
> > have to do anything with them. So don't think keeping these fields in the 
> > `NetworkConfig` would serve any purpose?
> > 
> > We do need to store the DNS and ROUTES returned in the `RESULT` of 
> > invoking the plugin, but thoses pieces should probably go in a separate 
> > protobuf (NetworkResult maybe) ?

As we discussed in the sync up meeting, it should be OK to keep IPAM and DNS in 
`NetworkConfig`.


- Qian


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


On March 15, 2016, 10:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 15, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Avinash sridharan 

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




src/slave/containerizer/mesos/isolators/network/cni/spec.proto (lines 45 - 46)


The isolator doesn't actually need the IPAM or the DNS information. This 
information would be used only by the plugin and the isolator doesn't have to 
do anything with them. So don't think keeping these fields in the 
`NetworkConfig` would serve any purpose?

We do need to store the DNS and ROUTES returned in the `RESULT` of invoking 
the plugin, but thoses pieces should probably go in a separate protobuf 
(NetworkResult maybe) ?


- Avinash sridharan


On March 15, 2016, 2:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 15, 2016, 2:05 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Avinash sridharan 

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




src/slave/containerizer/mesos/isolators/network/cni/spec.proto (line 40)


Why are the position identifiers not sequential ?



src/slave/containerizer/mesos/isolators/network/cni/spec.proto (line 42)


You will need to store the path of the network config as well, since during 
the isolate/prepare phase you will need to re-read the whole config (since it 
might have plugin specific parameters).



src/slave/containerizer/mesos/isolators/network/cni/spec.proto (lines 43 - 46)


Same question on positional identifiers ? Should start with 1 ?


- Avinash sridharan


On March 15, 2016, 2:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 15, 2016, 2:05 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Qian Zhang 


> On March 12, 2016, 2:19 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, lines 45-55
> > 
> >
> > What's this?

This is flannel plugin specific, let me remove it too.


- Qian


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


On March 15, 2016, 10:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 15, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-15 Thread Qian Zhang 

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

(Updated March 15, 2016, 10:05 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Introduced a protobuf message "NetworkConfig".


Diffs (updated)
-

  src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
  src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
  src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-14 Thread Qian Zhang 


> On March 11, 2016, 11:43 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 46
> > 
> >
> > Can we list the required field before the optional fields? We can keep 
> > the position identifiers as is, if required.

Actually I'd like to remove it, please take a look at 
https://github.com/appc/cni/blob/master/pkg/types/types.go#L59:L66, this field 
is even not in the struct of CNI.


> On March 11, 2016, 11:43 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 57
> > 
> >
> > Ditto on arrangement of required followed by optional fields.

Ditto.


- Qian


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


On March 10, 2016, 10:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-11 Thread Avinash sridharan 


> On March 11, 2016, 6:19 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 23
> > 
> >
> > OK, this is my bad. I didn't realize that network configuration JSON 
> > will inline some plugin specific or ipam specific fields.
> > 
> > In that case, I would suggest that we only list well-known fields and 
> > do not list plugin or ipam specific field in this protobuf.
> > 
> > You can take a look at:
> > https://github.com/appc/cni/blob/master/pkg/types/types.go#L59
> > 
> > I think json->protobuf parsing should still work if there are missing 
> > fields in the protobuf.

I agree. Have raised an issue to isolate the plugin/ipam specific fields into 
their own dictionaries:
https://github.com/appc/cni/issues/128


- Avinash


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


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-11 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/network/spec.proto (lines 25 - 28)


Let's put Route to the top level as well similar to what CNI has done.


- Jie Yu


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-11 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/network/spec.proto (line 38)


Let's put this to the top level. I think you'll need this for NetworkResult


- Jie Yu


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-11 Thread Jie Yu 

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




src/CMakeLists.txt (line 52)


+1 on moving into cni/ subdirectory.

I think we might want to introduce a few helper functions (e.g., parsing) 
in a separate hpp|cpp.



src/slave/containerizer/mesos/isolators/network/spec.proto (line 23)


OK, this is my bad. I didn't realize that network configuration JSON will 
inline some plugin specific or ipam specific fields.

In that case, I would suggest that we only list well-known fields and do 
not list plugin or ipam specific field in this protobuf.

You can take a look at:
https://github.com/appc/cni/blob/master/pkg/types/types.go#L59

I think json->protobuf parsing should still work if there are missing 
fields in the protobuf.



src/slave/containerizer/mesos/isolators/network/spec.proto (lines 31 - 34)


Those are ipam specific, let's remote those.



src/slave/containerizer/mesos/isolators/network/spec.proto (lines 45 - 55)


What's this?



src/slave/containerizer/mesos/isolators/network/spec.proto (line 60)


This is plugin specific, let's remove this from this protobuf.



src/slave/containerizer/mesos/isolators/network/spec.proto (line 61)


Ditto.



src/slave/containerizer/mesos/isolators/network/spec.proto (lines 63 - 67)


Remove these as well.


- Jie Yu


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-11 Thread Avinash sridharan 


> On March 11, 2016, 3:43 p.m., Avinash sridharan wrote:
> >

Can you remove the dependency from 44200?


- Avinash


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


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-11 Thread Avinash sridharan 

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




src/CMakeLists.txt (line 52)


Should we have a sub-directory called `cni` and add thes spec to the 
sub-directory. Just in case there are other specs that come around and we want 
to support them?



src/CMakeLists.txt (line 275)


Ditto on the `cni` path?



src/Makefile.am (lines 234 - 235)


Ditto on the `cni` path?



src/slave/containerizer/mesos/isolators/network/spec.proto (line 46)


Can we list the required field before the optional fields? We can keep the 
position identifiers as is, if required.



src/slave/containerizer/mesos/isolators/network/spec.proto (line 57)


Ditto on arrangement of required followed by optional fields.


- Avinash sridharan


On March 10, 2016, 2:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-10 Thread Qian Zhang 

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

(Updated March 10, 2016, 10:44 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Introduced a protobuf message "NetworkConfig".


Diffs (updated)
-

  src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
  src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
  src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Review Request 44549: Introduced a protobuf message "NetworkConfig".

2016-03-08 Thread Qian Zhang 

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

Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Introduced a protobuf message "NetworkConfig".


Diffs
-

  src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
  src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
  src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang