----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45383/#review126143 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 322) <https://reviews.apache.org/r/45383/#comment189040> `s/recoverInfo/_recover/` We usually move `_xxx` right below `xxx` for readability. So please move the function to the correct place. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 324) <https://reviews.apache.org/r/45383/#comment189041> I think this is not needed because if recover fails, slave will restart. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 333) <https://reviews.apache.org/r/45383/#comment189044> Ditto. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 350) <https://reviews.apache.org/r/45383/#comment189045> Ditto. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 840) <https://reviews.apache.org/r/45383/#comment189046> OK, now I realized that `getNetworkInfoDir` is a little confusing with `getNetworkDir`. Can we `s/getNetworkInfoDir/getContainerDir/` in a subsequent patch? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 858 - 859) <https://reviews.apache.org/r/45383/#comment189047> Please align the parameter properly. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 862) <https://reviews.apache.org/r/45383/#comment189048> `networkDirs->empty()` src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 863 - 866) <https://reviews.apache.org/r/45383/#comment189051> Or agent crashes right before removing the containerDir? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 867 - 884) <https://reviews.apache.org/r/45383/#comment189052> This basically duplicates what's in the cleanup. I am wondering if we should make cleanup more robust to tolerate a container dir without network name dirs. In other words, remove the logic here, change cleanup so that it can handle empty network dirs case. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 889) <https://reviews.apache.org/r/45383/#comment189055> instead of that, can you instead ``` s/getNetworkDirs/getNetworkNames/ ``` In other words, return a list of names, rather than dirs. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 894) <https://reviews.apache.org/r/45383/#comment189058> Ditto. s/getInterfaceDirs/getInterfaces/ - Jie Yu On March 30, 2016, 1:39 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45383/ > ----------------------------------------------------------- > > (Updated March 30, 2016, 1:39 p.m.) > > > Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Implemented recover() method of "network/cni" isolator. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp > 873e0c52475f4868e611bd24a6782ad5eb261a99 > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 1c8e231813c0579b79681c5d18b1f799a727ead7 > src/slave/containerizer/mesos/isolators/network/cni/paths.hpp > f627ec9499a34ca104d2c1a4d28e1d2f4b849f64 > src/slave/containerizer/mesos/isolators/network/cni/paths.cpp > 611f3869402b9033081b7f9ecc1bdf006f61918b > src/slave/containerizer/mesos/isolators/network/cni/spec.hpp > 6a3c33645bab73edaf5af4d298a671852ea59c46 > src/slave/containerizer/mesos/isolators/network/cni/spec.cpp > 5b5f904def9ef6dcc4462a03a2d024ad4eb3d946 > > Diff: https://reviews.apache.org/r/45383/diff/ > > > Testing > ------- > > > Thanks, > > Qian Zhang > >
