mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avinash sridharan <avin...@mesosphere.io>
Subject Re: Review Request 45383: Implemented recover() method of "network/cni" isolator.
Date Tue, 29 Mar 2016 17:35:46 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message