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 45082: Implemented cleanup() method of "network/cni" isolator.
Date Wed, 30 Mar 2016 02:46:40 GMT


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 644-645
> > <https://reviews.apache.org/r/45082/diff/1/?file=1307517#file1307517line644>
> >
> >     These two CHECKS don't make sense. What if the plugin got deleted or there was
a bug in the plugin because of which it wasn't able to delete the interfaces or release the
IP addresses. The Agent should not die because of an error in a 3rd party plugin.
> 
> Qian Zhang wrote:
>     If the plugin got deleted, then `Subprocess.isError()` will be true and we will return
a `Failure`, if there was a bug in the plugin, then `Subprocess.status()` will be non-zero
and we will read its output for the detailed error message. For either of these cases, agent
will not die.
> 
> Avinash sridharan wrote:
>     The fact that we are failing on the plugin returning an error seems very dangerous.
A malicious plugin can start crashing the Agent which is just bad behavior. We should avoid
this at all costs. CHECKS are more defensive to test an internal logic, they should not be
used for any external inputs. I would rather throw an error and get out.

We do not have these two CHECKs in the latest patch anymore :-)


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 647-649
> > <https://reviews.apache.org/r/45082/diff/1/?file=1307517#file1307517line647>
> >
> >     This is a bit odd, __disconnect always returns an error ? The plugin can return
error codes and error logs which we should be propagating upstream through failure semantics.
> 
> Qian Zhang wrote:
>     The reason that `__disconnect()` always returns a `Failure` is, it will be only called
when the exit code of plugin is not 0 (i.e., plugin fails for a reason), in case of plugin
success, we will return `Nothing()` in `_disconnect()`. So we only need `__disconnect()` in
case of plugin failure to get the output (i.e., detailed failure reason) of plugin and propagate
it upstream.
> 
> Avinash sridharan wrote:
>     I would suggest s/Failed to execute the CNI plugin/CNI plugin failed to detach network/
. "Failed to execute ..." implies that we not able to launch the plugin, which might not be
the case here.

Agree!


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 653
> > <https://reviews.apache.org/r/45082/diff/1/?file=1307517#file1307517line653>
> >
> >     Why are we returning a Future<list<Nothing>> over here? We should
be returning a Future<Nothing> there is no list since you are deletnig the entire container
directory in one shot. This seems a bit odd.
> 
> Qian Zhang wrote:
>     I tend to agree with you, however, I see CPU share isolator does the similar thing:
https://github.com/apache/mesos/blob/0.28.0/src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp#L536:L547,
maybe we should change it as well?

I have made `_cleanup()` as a void method so we do not need to return `Future<list<Nothing>>`
anymore.


> On March 22, 2016, 11:37 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 686
> > <https://reviews.apache.org/r/45082/diff/1/?file=1307517#file1307517line686>
> >
> >     This is dangerous we should be returning a const reference passed down in the
call as a return argument. Just do a return list<Nothing>() over here. 
> >     
> >     PS: Please see my comment on returning Future<list<Nothing>> above.

Ditto.


- Qian


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


On March 29, 2016, 6:59 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> -----------------------------------------------------------
> 
> (Updated March 29, 2016, 6:59 p.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 cleanup() 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

> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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