----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44555/#review123192 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni.hpp (lines 81 - 82) <https://reviews.apache.org/r/44555/#comment185382> I guess storing the protobuf is not sufficient because the JSON might contain plugin specific information. I would suggest we define a `NetworkConfigInfo` nested type here: ``` struct NetworkConfigInfo { std::string path; cni::NetworkConfig config; }; hashmap<std::string, NetworkConfigInfo> networkConfigInfos; ``` src/slave/containerizer/mesos/isolators/network/cni.cpp (line 48) <https://reviews.apache.org/r/44555/#comment185386> Let's remove this 'empty' check here since you hae the os::exists check below. src/slave/containerizer/mesos/isolators/network/cni.cpp (line 53) <https://reviews.apache.org/r/44555/#comment185387> Ditto on removing this check. src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 73 - 77) <https://reviews.apache.org/r/44555/#comment185391> I think the CNI plugin should also support the case where the user does not specify a name for the network. In other words, the CNI network isolator should support host network as well. So it's OK to have no plugins. src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 100 - 102) <https://reviews.apache.org/r/44555/#comment185392> No need for this check. We can always rely on the parse function below. src/slave/containerizer/mesos/isolators/network/cni.cpp (lines 177 - 181) <https://reviews.apache.org/r/44555/#comment185396> Again, this check should be removed to support containers that do not specify NetworkInfo. src/slave/containerizer/mesos/isolators/network/cni.cpp (line 186) <https://reviews.apache.org/r/44555/#comment185397> 2 lines apart. Please fix all places. src/slave/containerizer/mesos/isolators/network/cni.cpp (line 189) <https://reviews.apache.org/r/44555/#comment185398> '{' should be in the next line. Please fix it in all places. - Jie Yu On March 9, 2016, 6:01 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44555/ > ----------------------------------------------------------- > > (Updated March 9, 2016, 6:01 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 > ------- > > Implemented the framework and create() method of "network/cni" isolator. > > > Diffs > ----- > > src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 > src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 > src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/44555/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Qian Zhang > >
