[Kimchi-devel] [PATCH] Add OVS bridges recognition support

[Lucio Correia]

Hi Abhiram, thanks for the review. I will send a V2 because there is a 
piece missing on my patch. My replies below.

On 06-11-2015 11:56, Abhiram wrote:
> On Thu, 2015-11-05 at 17:40 -0200, Lucio Correia wrote:
>> In some distributions, i.e. Fedora, the directories "bridge"
>> and "brif" are not created for OVS bridges, and they are the
>> method Kimchi uses to detect bridges.
>>
>> This patch fixes that by recognizing OVS bridges through OVS
>> commands, allowing networks to be created using OVS bridges
>> in those distributions.
>>
>> Signed-off-by: Lucio Correia <luciojhc at linux.vnet.ibm.com>
>> ---
>>   src/wok/plugins/kimchi/model/networks.py |  2 +-
>>   src/wok/plugins/kimchi/netinfo.py        | 55 ++++++++++++++++++++++++++++----
>>   2 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/wok/plugins/kimchi/model/networks.py b/src/wok/plugins/kimchi/model/networks.py
>> index 71ea595..d78b88e 100644
>> --- a/src/wok/plugins/kimchi/model/networks.py
>> +++ b/src/wok/plugins/kimchi/model/networks.py
>> @@ -167,7 +167,7 @@ class NetworksModel(object):
>>               raise MissingParameter("KCHNET0004E", {'name': params['name']})
>>
>>           self._ensure_iface_up(iface)
>> -        if netinfo.is_bridge(iface):
>> +        if netinfo.is_bridge(iface) or netinfo.is_ovs_bridge(iface):
>>               if 'vlan_id' in params:
>>                   raise InvalidParameter('KCHNET0019E', {'name': iface})
>>               params['bridge'] = iface
>> diff --git a/src/wok/plugins/kimchi/netinfo.py b/src/wok/plugins/kimchi/netinfo.py
>> index c5746d7..f210da8 100644
>> --- a/src/wok/plugins/kimchi/netinfo.py
>> +++ b/src/wok/plugins/kimchi/netinfo.py
>> @@ -21,6 +21,8 @@ import ethtool
>>   import glob
>>   import os
>>
>> +from distutils.spawn import find_executable
>> +from wok.utils import run_command
>>
>>   NET_PATH = '/sys/class/net'
>>   NIC_PATH = '/sys/class/net/*/device'
>> @@ -79,6 +81,39 @@ def bridges():
>>   def is_bridge(iface):
>>       return iface in bridges()
>>
>> +# OVS bridges need special handling because in some distributions, like Fedora,
>> +# the files bridge and brif are not created under /sys/class/net/<ovsbridge>.
>> +def ovs_bridges():
>> +    ovs_cmd = find_executable("ovs-vsctl")
>> +
>> +    # OVS not installed: there is no OVS bridge configured
>> +    if ovs_cmd is None:
>> +        return []
>> +
>> +    out, error, rc = run_command([ovs_cmd, '--oneline', 'list-br'])
>> +    if rc != 0 or len(out) == 0:
> Do you want to log errors in case of rc!=0 ?
I think it's a good idea. I will also remove the len(out)==0 
verification since it's already done by return command below.

>> +        return []
>> +
>> +    return list(set(out.split('\n')) - set(['']))
>> +
>> +
>> +def is_ovs_bridge(iface):
>> +    return iface in ovs_bridges()
>> +
>> +
>> +def ovs_bridge_ports(ovsbr):
>> +    ovs_cmd = find_executable("ovs-vsctl")
>> +
>> +    # OVS not installed: there is no OVS bridge configured
>> +    if ovs_cmd is None:
>> +        return []
>> +
>> +    out, error, rc = run_command([ovs_cmd, '--oneline', 'list-ports', ovsbr])
>> +    if rc != 0 or len(out) == 0:
> Do you want to log errors in case of rc!=0 ?

Same as above.

>> +        return []
>> +
>> +    return list(set(out.split('\n')) - set(['']))
>
>> +
>>
>>   def all_interfaces():
>>       return [d.rsplit("/", 1)[-1] for d in glob.glob(NET_PATH + '/*')]
>> @@ -91,7 +126,12 @@ def slaves(bonding):
>>
>>
>>   def ports(bridge):
>> -    return os.listdir(BRIDGE_PORTS % bridge)
>> +    if bridge in bridges():
>> +        return os.listdir(BRIDGE_PORTS % bridge)
>> +    elif bridge in ovs_bridges():
>> +        return ovs_bridge_ports(bridge)
>> +
>> +    raise ValueError('unknown bridge %s' % bridge)
> Please add the message to i18 file.
Good catch, I will fix that. I've reused an existing message and was not 
concerning about that, but that message is not in i18n file.

>>
>>
>>   def is_brport(nic):
>> @@ -136,9 +176,11 @@ def get_vlan_device(vlan):
>>
>>   def get_bridge_port_device(bridge):
>>       """Return the nics list that belongs to bridge."""
>> -    #   br  --- v  --- bond --- nic1
>> -    if bridge not in bridges():
>> +
>> +    if bridge not in bridges() and bridge not in ovs_bridges():
>>           raise ValueError('unknown bridge %s' % bridge)
> Please add the message to i18 file.
Yes, this is the already existing message.

>> +
>> +    #   br  --- v  --- bond --- nic1
>>       nics = []
>>       for port in ports(bridge):
>>           if port in vlans():
>> @@ -155,9 +197,10 @@ def get_bridge_port_device(bridge):
>>
>>
>>   def aggregated_bridges():
>> -    return [bridge for bridge in bridges() if
>> -            (set(get_bridge_port_device(bridge)) & set(nics()))]
>> +    all_bridges = list(set(bridges() + ovs_bridges()))
>>
>> +    return [bridge for bridge in all_bridges if
>> +            (set(get_bridge_port_device(bridge)) & set(nics()))]
>>
>>   def bare_nics():
>>       "The nic is not a port of a bridge or a slave of bond."
>> @@ -184,7 +227,7 @@ def get_interface_type(iface):
>>               return "nic"
>>           if is_bonding(iface):
>>               return "bonding"
>> -        if is_bridge(iface):
>> +        if is_bridge(iface) or is_ovs_bridge(iface):
>>               return "bridge"
>>           if is_vlan(iface):
>>               return "vlan"
>
>


-- 
Lucio Correia
Software Engineer
IBM LTC Brazil