----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44706/#review124554 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 33) <https://reviews.apache.org/r/44706/#comment187166> s/like the following/as follows src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 248) <https://reviews.apache.org/r/44706/#comment187168> s/i/ifindex/ more descriptive. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 328 - 336) <https://reviews.apache.org/r/44706/#comment187170> why do we need this dispatch ? The dispatch is to itself, so seems a bit odd. Can we invoke the `subprocess` for the plugin in _connect directly ? Basically not sure why we need connect & _connect. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 339) <https://reviews.apache.org/r/44706/#comment187180> This is just a thought. Maybe its better to use `await` over here, and use a lambda or `.onAny` on the await to parse the list of futures returned on await and clean up any checkpointed data for networks that we were not able to join due to plugin errors ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 345) <https://reviews.apache.org/r/44706/#comment187171> The name of the method `connect` seems a bit odd. I think you mean "connecting container to the network". Maybe a better name would be `attach` ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 391) <https://reviews.apache.org/r/44706/#comment187178> Shouldn't we be cleaning up the check pointed data for this ifname if there is an error while launching the plugin for this ifname ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 445) <https://reviews.apache.org/r/44706/#comment187172> Why the `CHECK_READY` here ? If the future is not READY it could be in error, which is fine we should just return an ERROR ? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 471) <https://reviews.apache.org/r/44706/#comment187174> Again why the CHECK ? The result might not have an IPv4 or an IPv6 allocation due to an IPAM error in which case we should return a failure. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 481) <https://reviews.apache.org/r/44706/#comment187185> s/write/checkpoint/ src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 494) <https://reviews.apache.org/r/44706/#comment187186> s/write/checkpoint/ - Avinash sridharan On March 20, 2016, 4:27 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44706/ > ----------------------------------------------------------- > > (Updated March 20, 2016, 4:27 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 > ------- > > Implemented isolate() method of "network/cni" isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/44706/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Qian Zhang > >
