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 45082: Implemented cleanup() method of "network/cni" isolator.
Date Tue, 29 Mar 2016 17:47:34 GMT


> On March 22, 2016, 3: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.

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.


> On March 22, 2016, 3: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.

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.


- Avinash


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


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