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

Reply via email to