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 Thu, 31 Mar 2016 22:25:15 GMT


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


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