You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@cn.ibm.com> on 2016/03/09 03:56:15 UTC
Review Request 44549: Introduced a protobuf message "NetworkConfig".
-----------------------------------------------------------
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
Re: Review Request 44549: Introduced a protobuf message
"NetworkConfig".
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/44549/#comment185378>
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".
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/44549/#comment185379>
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On March 15, 2016, 11:28 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, lines 45-46
> > <https://reviews.apache.org/r/44549/diff/3/?file=1299735#file1299735line45>
> >
> > 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".
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/44549/#comment185896>
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".
Posted by Jie Yu <yu...@gmail.com>.
> On March 17, 2016, 3:37 a.m., Jie Yu wrote:
> > src/CMakeLists.txt, line 279
> > <https://reviews.apache.org/r/44549/diff/4/?file=1300631#file1300631line279>
> >
> > 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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On March 17, 2016, 11:37 a.m., Jie Yu wrote:
> > src/CMakeLists.txt, line 279
> > <https://reviews.apache.org/r/44549/diff/4/?file=1300631#file1300631line279>
> >
> > 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".
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/44549/#comment186315>
I don't think we put headers here.
src/slave/containerizer/mesos/isolators/network/cni/spec.cpp (line 37)
<https://reviews.apache.org/r/44549/#comment186316>
indentation here should be 2 as it's a statement.
src/slave/containerizer/mesos/isolators/network/cni/spec.cpp (line 38)
<https://reviews.apache.org/r/44549/#comment186317>
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".
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44549/#review124849
-----------------------------------------------------------
src/Makefile.am (line 788)
<https://reviews.apache.org/r/44549/#comment187535>
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On March 15, 2016, 11:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/spec.proto, line 40
> > <https://reviews.apache.org/r/44549/diff/3/?file=1299735#file1299735line40>
> >
> > 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
> > <https://reviews.apache.org/r/44549/diff/3/?file=1299735#file1299735line42>
> >
> > 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".
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/44549/#comment185891>
Why are the position identifiers not sequential ?
src/slave/containerizer/mesos/isolators/network/cni/spec.proto (line 42)
<https://reviews.apache.org/r/44549/#comment185893>
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)
<https://reviews.apache.org/r/44549/#comment185892>
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
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".
Posted by Avinash sridharan <av...@mesosphere.io>.
> 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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On March 11, 2016, 11:43 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 46
> > <https://reviews.apache.org/r/44549/diff/2/?file=1294410#file1294410line46>
> >
> > 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
> > <https://reviews.apache.org/r/44549/diff/2/?file=1294410#file1294410line57>
> >
> > 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".
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44549/#review123148
-----------------------------------------------------------
src/CMakeLists.txt (line 52)
<https://reviews.apache.org/r/44549/#comment185323>
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)
<https://reviews.apache.org/r/44549/#comment185324>
Ditto on the `cni` path?
src/Makefile.am (lines 234 - 235)
<https://reviews.apache.org/r/44549/#comment185325>
Ditto on the `cni` path?
src/slave/containerizer/mesos/isolators/network/spec.proto (line 46)
<https://reviews.apache.org/r/44549/#comment185327>
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)
<https://reviews.apache.org/r/44549/#comment185326>
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On March 12, 2016, 2:19 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, lines 45-55
> > <https://reviews.apache.org/r/44549/diff/2/?file=1294410#file1294410line45>
> >
> > 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".
Posted by Avinash sridharan <av...@mesosphere.io>.
> On March 11, 2016, 6:19 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 23
> > <https://reviews.apache.org/r/44549/diff/2/?file=1294410#file1294410line23>
> >
> > 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".
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44549/#review123170
-----------------------------------------------------------
src/CMakeLists.txt (line 52)
<https://reviews.apache.org/r/44549/#comment185342>
+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)
<https://reviews.apache.org/r/44549/#comment185348>
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)
<https://reviews.apache.org/r/44549/#comment185351>
Those are ipam specific, let's remote those.
src/slave/containerizer/mesos/isolators/network/spec.proto (lines 45 - 55)
<https://reviews.apache.org/r/44549/#comment185343>
What's this?
src/slave/containerizer/mesos/isolators/network/spec.proto (line 60)
<https://reviews.apache.org/r/44549/#comment185352>
This is plugin specific, let's remove this from this protobuf.
src/slave/containerizer/mesos/isolators/network/spec.proto (line 61)
<https://reviews.apache.org/r/44549/#comment185353>
Ditto.
src/slave/containerizer/mesos/isolators/network/spec.proto (lines 63 - 67)
<https://reviews.apache.org/r/44549/#comment185354>
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".
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
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