----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45956/#review128609 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 108 - 109) <https://reviews.apache.org/r/45956/#comment192060> can you follow style guide here? src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 123) <https://reviews.apache.org/r/45956/#comment192061> `const Flags& _flags` src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 128) <https://reviews.apache.org/r/45956/#comment192062> move up. Please following the original style. src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 170) <https://reviews.apache.org/r/45956/#comment192063> const Flags flags; src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 80 - 81) <https://reviews.apache.org/r/45956/#comment192064> ``` new NetworkCniIsolatorProcess( flags, hashmap<string, NetworkConfigInfo>()))); ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 568 - 571) <https://reviews.apache.org/r/45956/#comment192066> ``` Owned<info> info(new Info( containerNetworks, containerConfig.rootfs())); infos.put(containerId, info); ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 661 - 673) <https://reviews.apache.org/r/45956/#comment192067> Can you move that to `_isolate` as well? ``` return await(futures) .then(defer( self(), &Self::_isolate, containerId, pid, const list<Future<Nothing>>& attaches)); ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 675) <https://reviews.apache.org/r/45956/#comment192068> See my above comments. Also, capture `this` pointer is in general bad. If this isolator terminates (e.g., during teardown in tests) while the above future is still pending, when the future becomes ready, it'll cause a segfault. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 689 - 693) <https://reviews.apache.org/r/45956/#comment192070> This should be a CHECK instead? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 702) <https://reviews.apache.org/r/45956/#comment192071> Kill this line. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 706) <https://reviews.apache.org/r/45956/#comment192072> No need to print containerId in the error message here since the caller will print it anyway. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 709) <https://reviews.apache.org/r/45956/#comment192073> Add a TODO saying that we only support ipv4 for now src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 714) <https://reviews.apache.org/r/45956/#comment192074> Please align the parameters. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 716 - 717) <https://reviews.apache.org/r/45956/#comment192077> What if IP is not found? Should we use 127.0.0.1? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 722 - 725) <https://reviews.apache.org/r/45956/#comment192075> ``` Try<net::IPNetwork> ipNetwork = net::IPNetwork::parse( network.cniNetworkInfo->ip4().ip(), AF_INET); if (ipNetwork.isError()) { return Failure(""); } hosts << ipNetwork->address() << " " << containerId << endl; ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 732 - 733) <https://reviews.apache.org/r/45956/#comment192076> This fits in one line? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 750 - 751) <https://reviews.apache.org/r/45956/#comment192078> DItto on alignment. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 752 - 761) <https://reviews.apache.org/r/45956/#comment192079> I probably do ``` if (network.cniNetworkInfo.isNone() || !network.cniNetworkInfo->has_dns()) { continue; } foreach (const string& nameserver, network.cniNetwork->dns().nameservers()) { resolv << "nameserver " << nameserver << endl; } ``` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 754) <https://reviews.apache.org/r/45956/#comment192080> THis is not needed. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 771 - 773) <https://reviews.apache.org/r/45956/#comment192081> Please align `<<`. See other files in the code base. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 776) <https://reviews.apache.org/r/45956/#comment192082> Ditto. Fix all such cases. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 788 - 797) <https://reviews.apache.org/r/45956/#comment192083> I think subprocess supports 'Flags' directly. Please see how to use that in port mapping isolator. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 807 - 808) <https://reviews.apache.org/r/45956/#comment192084> Fix the style here. - Jie Yu On April 13, 2016, 4:40 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45956/ > ----------------------------------------------------------- > > (Updated April 13, 2016, 4:40 a.m.) > > > Review request for mesos, Jie Yu and Qian Zhang. > > > Bugs: MESOS-4922 > https://issues.apache.org/jira/browse/MESOS-4922 > > > Repository: mesos > > > Description > ------- > > Once the `isolate` is successful, the `_isolate` method calls out the > `mesos-cni-helper` to setup the /etc/hosts, /etc/hostname and > /etc/resolv.conf for the container. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp > 654137c552a7c416f394365e43ea80770fe1ef8d > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 159152a01b68a667dbd57fa6452c6c2a3422787c > > Diff: https://reviews.apache.org/r/45956/diff/ > > > Testing > ------- > > make > > *Ran mesos_execute with single master/slave setup to verify that containers > get the right hostname and network files when attached to a CNI network. > > > Thanks, > > Avinash sridharan > >
