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 44269: Added the framework of 'network/cni' isolator.
Date Sun, 06 Mar 2016 15:57:25 GMT


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 134
> > <https://reviews.apache.org/r/44269/diff/4/?file=1280944#file1280944line134>
> >
> >     I don't understand this comment. We just made sure the plugin does not exist?
So what does the comment imply "it can
> >             // still be valid as long as operator puts the CNI plugin binary
> >             // that it uses under '--network_cni_plugins_dir'." ?
> >             
> >     I think at this point we should return an error. If can't find an executable
for a named network, the behavior will become undefined. We should bail at this point.
> 
> Qian Zhang wrote:
>     My point is, if we can not find a plugin for a named network during initilization,
log a warning message to let operator know this issue, and afterward operator can put the
plugin in the plugin directory without restarting agent, then the named network can still
work.
> 
> Avinash sridharan wrote:
>     Lets not rely on the operator heeding WARNING messages and fixing the problem. My
concern is that this is a `FATAL` error since before the operator can rectify the error if
containers are launched the behavior becomes undefined.
> 
> Qian Zhang wrote:
>     Agree, let's return an error :-)

After more thinking, I think in this case, it makes more sense to log a warning message and
ignore the network config file rather than bail at this point, because there can be other
valid network config files. If in the end there is no any valid network config files, we should
definitely bail.


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 108
> > <https://reviews.apache.org/r/44269/diff/4/?file=1280944#file1280944line108>
> >
> >     Conver this to LOG(ERROR)

I think it should be LOG(WARNING) if we are going to ignore something in the `create()` method
of an isolator, please see `PortMappingIsolatorProcess::create()`:
https://github.com/apache/mesos/blob/0.27.1/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L1321


> On March 4, 2016, 11:30 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 129
> > <https://reviews.apache.org/r/44269/diff/4/?file=1280944#file1280944line129>
> >
> >     Can we also check if the plugin is executable? Since later we are going to try
and execute the plugin.
> 
> Avinash sridharan wrote:
>     This might be slightly more involved then I thought. You will have to figure out
if the owner or group of the binary is root (since the assumption is agent is running with
sudo). If yes then check the permission of the user and the group. If no then check the permission
of others. os::permissions should allow you to check all the permissions.

Yes, we should check it. And since the assumption is agent is running with root permission,
we only need to check if any of user/group/other has "x" permission, if yes, root will be
able to execute the plugin.


- Qian


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


On March 5, 2016, 1:07 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44269/
> -----------------------------------------------------------
> 
> (Updated March 5, 2016, 1:07 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
> -------
> 
> Added the framework of 'network/cni' isolator.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 0eabfad66f43adff12d08dbf34ca3db1b801c49e 
>   src/Makefile.am 5b54fe04734afc5faa1bba3b84f43a1b8efe8753 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44269/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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