> On March 3, 2016, 12:24 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/containerizer.cpp, lines 81-84 > > <https://reviews.apache.org/r/44269/diff/1/?file=1277116#file1277116line81> > > > > Lets do this in a separate patch once the implementation for the > > isolator is complete.
Agree, the reason I did this was, I wanted to do some tests by starting agent with "network/cni" isolator created. > On March 3, 2016, 12:24 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/containerizer.cpp, line 223 > > <https://reviews.apache.org/r/44269/diff/1/?file=1277116#file1277116line223> > > > > We should introduce the `network/cni` isolator into the containerizer > > till the implementation is complete. Lets remove this and submit it in a > > separate patch once the implementation is complete. Agree, the reason I did this was, I wanted to do some tests by starting agent with "network/cni" isolator created. > On March 3, 2016, 12:24 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 49 > > <https://reviews.apache.org/r/44269/diff/1/?file=1277117#file1277117line49> > > > > Doesn't this fit in a single line? No, it will be more than 80 chars in a single line :-( > On March 3, 2016, 12:24 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 41 > > <https://reviews.apache.org/r/44269/diff/1/?file=1277118#file1277118line41> > > > > explicit checks? (==0) I think the convention is implicit check, you can check other codes in Mesos which call os::exists(), e.g., https://github.com/apache/mesos/blob/0.27.0/src/slave/slave.cpp#L418 > On March 3, 2016, 12:24 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 42 > > <https://reviews.apache.org/r/44269/diff/1/?file=1277118#file1277118line42> > > > > s/directory/location > > > > Shouldn't the indentation be: > > return Error( > > "Failed to ..." + > > "..." Agree! > On March 3, 2016, 12:24 a.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 50 > > <https://reviews.apache.org/r/44269/diff/1/?file=1277118#file1277118line50> > > > > Should we be listing all the files in the config directory and reading > > the configs over here? Yes, we should. However I see Gilbert raised a comment in https://reviews.apache.org/r/44200/ about directly passing JSON to agent, so let's wait for the decision for that comment :-) - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44269/#review121663 ----------------------------------------------------------- On March 2, 2016, 11:25 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44269/ > ----------------------------------------------------------- > > (Updated March 2, 2016, 11:25 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 > ------- > > Added the framework of 'network/cni' isolator. > > > Diffs > ----- > > src/CMakeLists.txt 9d29fad396cdac0f263c5d52460030630fc2dfaf > src/Makefile.am fe549a881e778e094d3f957e5119eeb2f55e7915 > src/slave/containerizer/mesos/containerizer.cpp > af3ff5750649497d8852b4761c78d4cae5455a02 > 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/44269/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Qian Zhang > >
