mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhang...@cn.ibm.com>
Subject Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.
Date Tue, 22 Mar 2016 14:48:35 GMT


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 328-336
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line328>
> >
> >     why do we need this dispatch ? The dispatch is to itself, so seems a bit odd.
Can we invoke the `subprocess` for the plugin in _connect directly ?
> >     
> >     Basically not sure why we need connect & _connect.

The idea is similar with how `MesosContainerizer` call each isolator: https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/containerizer.cpp#L1171:L1181

I'd like to handle each call to a CNI plugin in a separate libprocess dispatch event, so that's
why I call `connect` via `dispatch`.


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 339
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line339>
> >
> >     This is just a thought. Maybe its better to use `await` over here, and use a
lambda or `.onAny` on the await to parse the list of futures returned on await and clean up
any checkpointed data for networks that we were not able to join due to plugin errors ?

I think any clean up works should be handled in the `cleanup` method, that's should be the
design idea of isolator. Here we are doing all or nothing, so if there is a call to a CNI
plugin fails for any reasons, we will return a `Failure` which should be eventually handled
in `Slave::executorLaunched`, and it will call `MesosContainerizer::destroy` which will eventually
call each isolator's `cleanup` method to do the cleanup.


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 345
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line345>
> >
> >     The name of the method `connect` seems a bit odd. I think you mean "connecting
container to the network". Maybe a better name would be `attach` ?

Yes, I agree `attach` is a better name, will update the patch accordingly later.


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 391
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line391>
> >
> >     Shouldn't we be cleaning up the check pointed data for this ifname if there
is an error while launching the plugin for this ifname ?

I think we should do the cleanup in the `cleanup` method.


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 445
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line445>
> >
> >     Why the `CHECK_READY` here ? If the future is not READY it could be in error,
which is fine we should just return an ERROR ?

Because when `NetworkCniIsolatorProcess::__connect` is called, we expect `output` must be
ready otherwise `NetworkCniIsolatorProcess::__connect` should not be called. I think it is
a common way to use `then`, please take a look at: https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L3173:L3186
as a reference.


> On March 21, 2016, 11:57 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 471
> > <https://reviews.apache.org/r/44706/diff/2/?file=1307515#file1307515line471>
> >
> >     Again why the CHECK ? The result might not have an IPv4 or an IPv6 allocation
due to an IPAM error in which case we should return a failure.

I think the output of IPAM plugin has no IPv4 address and no IPv6 address is an unexpected
result, so that's why I use `CHECK` here, but I agree with you returning a `Failure` is more
appropriate, will update the patch accordingly later.


- Qian


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


On March 21, 2016, 12:27 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> -----------------------------------------------------------
> 
> (Updated March 21, 2016, 12:27 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 isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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