----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45383/#review125880 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 103) <https://reviews.apache.org/r/45383/#comment188776> 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? src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 139) <https://reviews.apache.org/r/45383/#comment188775> Maybe s/recoverInfo/_recover/ src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 151) <https://reviews.apache.org/r/45383/#comment188777> 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. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 136) <https://reviews.apache.org/r/45383/#comment188774> Why do we have this change as part of this patch? Can we move this out to a different patch? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 288 - 290) <https://reviews.apache.org/r/45383/#comment188778> Do we need this loop? `infos.clear` below should destroy the hashmap which in turn would destroy any of the residing objects (`networkinfos`) src/slave/containerizer/mesos/isolators/network/cni/cni.cpp <https://reviews.apache.org/r/45383/#comment188779> I think we should have this in a separate patch? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 809) <https://reviews.apache.org/r/45383/#comment188782> 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? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 824 - 827) <https://reviews.apache.org/r/45383/#comment188783> 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. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 870) <https://reviews.apache.org/r/45383/#comment188784> This error doesn't make sense. Maybe: "Found multiple interfaces for a given CNI network. This should not be allowed" ? src/slave/containerizer/mesos/isolators/network/cni/spec.cpp (line 53) <https://reviews.apache.org/r/45383/#comment188808> Why not just return `parse` ? - Avinash sridharan On March 29, 2016, 11 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45383/ > ----------------------------------------------------------- > > (Updated March 29, 2016, 11 a.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 > >
