8:33 a.m.
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(a)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