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