> On March 30, 2016, 1:35 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 103
> > <https://reviews.apache.org/r/45383/diff/2/?file=1317828#file1317828line103>
> >
> >     Why do we need to use a hashmap over here? Why not a vector or a list? 
> > Hashmap's are expensive in terms of memory. Usually a container won't be 
> > associated with more than one network, so seems pretty inefficient?

This was suggested by Jie when I wrote the `prepare()` patch, you can check 
https://reviews.apache.org/r/44514/diff/5?file=1304896#file1304896line82 for 
details.


> On March 30, 2016, 1:35 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 139
> > <https://reviews.apache.org/r/45383/diff/2/?file=1317828#file1317828line139>
> >
> >     Maybe s/recoverInfo/_recover/

I'd like to name this method more meaningful, `recoverInfo` implies that this 
method will recover/rebuild the `Info` struct for the container.


> On March 30, 2016, 1:35 a.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.

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


> On March 30, 2016, 1:35 a.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?

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()`.


> On March 30, 2016, 1:35 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 288-290
> > <https://reviews.apache.org/r/45383/diff/2/?file=1317829#file1317829line288>
> >
> >     Do we need this loop? `infos.clear` below should destroy the hashmap 
> > which in turn would destroy any of the residing objects (`networkinfos`)

Yes, I agree, and I will fix this in `_cleanup()` as well.


> On March 30, 2016, 1:35 a.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?

> 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).


> On March 30, 2016, 1:35 a.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" ?

But it will miss the case that interfaces.get().size() == 0.


> On March 30, 2016, 1:35 a.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` ?

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.


> On March 30, 2016, 1:35 a.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.

If the plugin has been invoked, then the `networkDirs` will not be empty.


- Qian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45383/#review125880
-----------------------------------------------------------


On March 29, 2016, 7 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45383/
> -----------------------------------------------------------
> 
> (Updated March 29, 2016, 7 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 
> b1b7205f4f10b6dc256fcc4ecb3210105c1240b4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 7cda5715814a0cfc4b394eb04437831e6dc44e3f 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp PRE-CREATION 
>   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
> 
>

Reply via email to