[Kimchi-devel] [PATCH] Fix issue 766 - Define network interface in libvirt
Lucio Correia
luciojhc at linux.vnet.ibm.com
Thu Nov 26 18:28:19 UTC 2015
Thanks for the review, see my inline answers.
On 26-11-2015 14:31, Daniel Henrique Barboza wrote:
> Review: +1, just a few inline comments
I've sent a V2.
>
> On 11/26/2015 01:48 PM, Lucio Correia wrote:
>> When a network interface was only visible to the system and
>> not to libvirt, libvirt throws an error when user asks a
>> bridged network to be created using that interface, since
>> it is not visible to libvirt.
>>
>> This patch adds network interface redefinition code to make
>> it visible to libvirt, allowing the bridged network to be
>> created.
>>
>> Signed-off-by: Lucio Correia <luciojhc at linux.vnet.ibm.com>
>> ---
>> src/wok/plugins/kimchi/model/networks.py | 40
>> +++++++++++++++-------------
>> src/wok/plugins/kimchi/network.py | 5 ++++
>> src/wok/plugins/kimchi/xmlutils/interface.py | 20 +++++++++++---
>> 3 files changed, 42 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/wok/plugins/kimchi/model/networks.py
>> b/src/wok/plugins/kimchi/model/networks.py
>> index 7f50003..25640a4 100644
>> --- a/src/wok/plugins/kimchi/model/networks.py
>> +++ b/src/wok/plugins/kimchi/model/networks.py
>> @@ -33,6 +33,7 @@ from wok.xmlutils.utils import xpath_get_text
>> from wok.plugins.kimchi import netinfo
>> from wok.plugins.kimchi import network as knetwork
>> from wok.plugins.kimchi.osinfo import defaults as tmpl_defaults
>> +from wok.plugins.kimchi.xmlutils.interface import get_iface_xml
>> from wok.plugins.kimchi.xmlutils.network import create_linux_bridge_xml
>> from wok.plugins.kimchi.xmlutils.network import
>> create_vlan_tagged_bridge_xml
>> from wok.plugins.kimchi.xmlutils.network import
>> get_no_network_config_xml
>> @@ -237,8 +238,23 @@ class NetworksModel(object):
>> 'err': e.get_error_message()})
>> def _create_linux_bridge(self, interface):
>> - # get xml definition of interface
>> - iface_xml = self._get_interface_desc_xml(interface)
>> + iface_defined = False
>> + try:
>> + # get xml definition of configured interface
>> + conn = self.conn.get()
>> + iface = conn.interfaceLookupByName(interface)
>> + iface_xml = iface.XMLDesc(flags=VIR_INTERFACE_XML_INACTIVE)
>> + except libvirt.libvirtError, e:
>> + try:
>> + # try to define that interface in libvirt
>> + mac = knetwork.get_dev_macaddr(str(interface))
>> + iface_xml = get_iface_xml({'name': interface, 'mac':
>> mac,
>> + 'startmode': "onboot"})
>> + conn.interfaceDefineXML(iface_xml.encode("utf-8"))
>> + iface_defined = True
>> + except libvirt.libvirtError, e:
>> + raise OperationFailed("KCHNET0023E", {'name': interface,
>> + 'err': e.get_error_message()})
>
> You could have avoided this nested exception by creating nested methods,
> like:
>
> def _create_linux_bridge(...)
> def get_configured_interface_xml_def(...)
> (return iface_xml or None if exception thrown)
>
> def define_libvirt_interface()
> (do stuff)
>
> iface_xml = get_configured_interface_xml_def()
>
> if iface_xml is None:
> iface_xml = define_libvirt_interface()
>
>
Agreed, included in V2.
>
>> # Truncate the interface name if it exceeds 13 characters to
>> make sure
>> # the length of bridge name is less than 15 (its maximum
>> value).
>> @@ -246,7 +262,7 @@ class NetworksModel(object):
>> br_xml = create_linux_bridge_xml(br_name, interface, iface_xml)
>> # drop network config from interface
>> - self._redefine_iface_no_network(interface)
>> + iface_defined or self._redefine_iface_no_network(interface,
>> iface_xml)
>> # create and start bridge
>> self._create_bridge(br_name, br_xml)
>> @@ -263,27 +279,13 @@ class NetworksModel(object):
>> return br_name
>> - def _get_interface_desc_xml(self, name):
>> - conn = self.conn.get()
>> -
>> - try:
>> - iface = conn.interfaceLookupByName(name)
>> - xml = iface.XMLDesc(flags=VIR_INTERFACE_XML_INACTIVE)
>> - except libvirt.libvirtError, e:
>> - raise OperationFailed("KCHNET0023E",
>> - {'name': name, 'err':
>> e.get_error_message()})
>> -
>> - return xml
>> -
>> - def _redefine_iface_no_network(self, name):
>> - conn = self.conn.get()
>> -
>> + def _redefine_iface_no_network(self, name, iface_xml):
>> # drop network config from definition of interface
>> - iface_xml = self._get_interface_desc_xml(name)
>> xml = get_no_network_config_xml(iface_xml.encode("utf-8"))
>> try:
>> # redefine interface
>> + conn = self.conn.get()
>> conn.interfaceDefineXML(xml.encode("utf-8"))
>> except libvirt.libvirtError as e:
>> raise OperationFailed("KCHNET0024E", {'name': name,
>> diff --git a/src/wok/plugins/kimchi/network.py
>> b/src/wok/plugins/kimchi/network.py
>> index 1433b8a..eee0e8c 100644
>> --- a/src/wok/plugins/kimchi/network.py
>> +++ b/src/wok/plugins/kimchi/network.py
>> @@ -31,6 +31,11 @@ DefaultNetsPool =
>> [ipaddr.IPNetwork('192.168.122.0/23'),
>> ipaddr.IPNetwork('192.168.128.0/17')]
>> +def get_dev_macaddr(dev):
>> + info = ethtool.get_interfaces_info(dev)[0]
>> + return info.mac_address
>> +
>> +
>> def get_dev_netaddr(dev):
>> info = ethtool.get_interfaces_info(dev)[0]
>> return (info.ipv4_address and
>> diff --git a/src/wok/plugins/kimchi/xmlutils/interface.py
>> b/src/wok/plugins/kimchi/xmlutils/interface.py
>> index 64df8cd..7730407 100644
>> --- a/src/wok/plugins/kimchi/xmlutils/interface.py
>> +++ b/src/wok/plugins/kimchi/xmlutils/interface.py
>> @@ -26,15 +26,27 @@ from wok.plugins.kimchi import osinfo
>> def get_iface_xml(params, arch=None, os_distro=None, os_version=None):
>> """
>> - <interface type='network'>
>> + <interface type='network' name='ethX'>
>> + <start mode='onboot'/>
>> <source network='default'/>
>> <model type='virtio'/>
>> </interface>
>> """
>> - interface = E.interface(type=params['type'])
>> - interface.append(E.source(network=params['network']))
>> + name = params.get('name', None)
> You don't need to specify 'None' as default return value of dict.get().
> 'None'
> is the default return value when you don't define it. On the other hand,
> we can appreciate the clearness of showing that the default value is
> 'None'.
Yes, went for clearness here, and also to keep same standard of code I
added.
>
> Just a general comment.
>> + if name:
>> + interface = E.interface(type=params.get('type', 'ethernet'),
>> name=name)
>> + else:
>> + interface = E.interface(type=params.get('type', 'ethernet'))
>> - model = params.get('model')
>> + stmode = params.get('startmode', None)
>> + if stmode:
>> + interface.append(E.start(mode=stmode))
>> +
>> + nw = params.get('network', None)
>> + if nw:
>> + interface.append(E.source(network=nw))
>> +
>> + model = params.get('model', None)
>> # no model specified; let's try querying osinfo
>> if model is None:
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>
--
Lucio Correia
Software Engineer
IBM LTC Brazil
More information about the Kimchi-devel
mailing list