> On March 29, 2016, 5:35 p.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 151 > > <https://reviews.apache.org/r/45383/diff/2/?file=1317828#file1317828line151> > > > > Should have pointed in the earlier patches, why do we need `Info` to be > > a smart pointer. We can have it as an object. We are not going to share > > this information with any other class or thread. > > Qian Zhang wrote: > I think `Owned<Info>` is the suggested way, please see: > > https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp#L110 > > https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/cgroups/mem.hpp#L129 > > https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp#L115
That's because they ended up using an `Info*` to begin with. I think there were historical reasons for using `Info*` instead of an Info object. In this case I don't see the need for storing the `Info` as a pointer since it is not being passed around. > On March 29, 2016, 5:35 p.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 136 > > <https://reviews.apache.org/r/45383/diff/2/?file=1317829#file1317829line136> > > > > Why do we have this change as part of this patch? Can we move this out > > to a different patch? > > Qian Zhang wrote: > Because when I wrote this patch, I found I need a new method to parse > `NetworkInfo`, but I can not name it as `parse()` since there is already a > `parse()` method for parsing `NetworkConfig`, so I have to rename that one as > `parseNetworkConfig()`, and introduce the new one as `parseNetworkInfo()`. Sure, but can we make it a separate patch ? > On March 29, 2016, 5:35 p.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 817 > > <https://reviews.apache.org/r/45383/diff/2/?file=1317829#file1317829line817> > > > > Isn't this a problem? We are returning `Nothing` here, which implies > > that in the `recover` method the container information is assumed to have > > been stored correctly. However, there is NO container informationt that has > > been stored!! I thought we should never run into this issue, since we > > should be creating the directories before calling the CNI plugin? > > Qian Zhang wrote: > > which implies that in the recover method the container information is > assumed to have been stored correctly. > > It does not imply the container info has been stored correctly, actually > it implies that for this kind of container (which has no networkInfoDir > created yet), we do not need to store anything for it (actually we have > nothing to restore from since there is no networkInfoDir for the container). That doesn't sound right. This effectively means that we have a container without a IP address or even a veth associated with the network namespace? This seems like an error. > On March 29, 2016, 5:35 p.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 832-835 > > <https://reviews.apache.org/r/45383/diff/2/?file=1317829#file1317829line832> > > > > What if the isolator dies right after creating the directories, but > > after invoking the plugin? If we just return `Nothing` we might end up > > leaking state information from the CNI plugin (IPAM). I think we should > > return `Error` here and ask the isolator to cleanup the containers and > > start over. > > Qian Zhang wrote: > If the plugin has been invoked, then the `networkDirs` will not be empty. Well, the ip address and DNS information returned by the plugin would have been lost, because the plugin would be a zombie process (child died). In this case not sure if it makes sense to recover the containers. > On March 29, 2016, 5:35 p.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 878 > > <https://reviews.apache.org/r/45383/diff/2/?file=1317829#file1317829line878> > > > > This error doesn't make sense. Maybe: > > "Found multiple interfaces for a given CNI network. This should not be > > allowed" ? > > Qian Zhang wrote: > But it will miss the case that interfaces.get().size() == 0. Do we throw an error in `isolate` if a container tries to join the same network with multiple `NetworkInfo` messages ? I agree with your comment though. If we are handling the case that a container can never have more than one interface attached to the network, we can replace the existing string by "Unable to find any interface attached to " + <network name> +" for this container" .... > On March 29, 2016, 5:35 p.m., Avinash sridharan wrote: > > src/slave/containerizer/mesos/isolators/network/cni/spec.cpp, line 53 > > <https://reviews.apache.org/r/45383/diff/2/?file=1317833#file1317833line53> > > > > Why not just return `parse` ? > > Qian Zhang wrote: > Either way should be OK? I see > https://github.com/apache/mesos/blob/0.28.0/src/docker/spec.cpp#L110:L124 > does the similar. Would prefer having less code if possible. - Avinash ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45383/#review125880 ----------------------------------------------------------- On March 31, 2016, 11:33 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45383/ > ----------------------------------------------------------- > > (Updated March 31, 2016, 11:33 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 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 > >
