Change in vdsm[master]: net libvirt: make network/libvirt.py suitable for OVS

Petr Horáček has uploaded a new change for review.

Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................

net libvirt: make network/libvirt.py suitable for OVS

Make network/libvirt.py suitable for OVS (add support for vlan and
virtualport).

Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Signed-off-by: Petr Horáček <phoracek(a)redhat.com&gt;
---
M lib/vdsm/network/configurators/ifcfg.py
M lib/vdsm/network/configurators/iproute2.py
M lib/vdsm/network/configurators/pyroute_two.py
M lib/vdsm/network/legacy_switch.py
M lib/vdsm/network/libvirt.py
M lib/vdsm/network/netinfo/cache.py
M lib/vdsm/network/netswitch.py
M tests/network/libvirt_test.py
M vdsm_hooks/ovs/ovs_before_network_setup_libvirt.py
9 files changed, 141 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/65/65065/4

diff --git a/lib/vdsm/network/configurators/ifcfg.py
b/lib/vdsm/network/configurators/ifcfg.py
index aac39d1..b9d093f 100644
--- a/lib/vdsm/network/configurators/ifcfg.py
+++ b/lib/vdsm/network/configurators/ifcfg.py
@@ -370,7 +370,9 @@
 
     def createLibvirtNetwork(self, network, bridged=True, iface=None,
                              skipBackup=False):
-        netXml = libvirt.createNetworkDef(network, bridged, iface)
+        netXml = libvirt.createNetworkDef(network,
+                                          bridge=network if bridged else None,
+                                          iface=iface if not bridged else None)
         if not skipBackup:
             self._networkBackup(network)
         libvirt.createNetwork(netXml)
diff --git a/lib/vdsm/network/configurators/iproute2.py
b/lib/vdsm/network/configurators/iproute2.py
index 4a96966..317dc5a 100644
--- a/lib/vdsm/network/configurators/iproute2.py
+++ b/lib/vdsm/network/configurators/iproute2.py
@@ -333,7 +333,9 @@
                 f.write(value)
 
     def createLibvirtNetwork(self, network, bridged=True, iface=None):
-        netXml = libvirt.createNetworkDef(network, bridged, iface)
+        netXml = libvirt.createNetworkDef(network,
+                                          bridge=network if bridged else None,
+                                          iface=iface if not bridged else None)
         libvirt.createNetwork(netXml)
 
     def removeLibvirtNetwork(self, network):
diff --git a/lib/vdsm/network/configurators/pyroute_two.py
b/lib/vdsm/network/configurators/pyroute_two.py
index 8aece53..ff0b95f 100644
--- a/lib/vdsm/network/configurators/pyroute_two.py
+++ b/lib/vdsm/network/configurators/pyroute_two.py
@@ -164,7 +164,9 @@
                 f.write(value)
 
     def createLibvirtNetwork(self, network, bridged=True, iface=None):
-        netXml = libvirt.createNetworkDef(network, bridged, iface)
+        netXml = libvirt.createNetworkDef(network,
+                                          bridge=network if bridged else None,
+                                          iface=iface if not bridged else None)
         libvirt.createNetwork(netXml)
 
     def removeLibvirtNetwork(self, network):
diff --git a/lib/vdsm/network/legacy_switch.py b/lib/vdsm/network/legacy_switch.py
index 139ddfc..24d13cb 100644
--- a/lib/vdsm/network/legacy_switch.py
+++ b/lib/vdsm/network/legacy_switch.py
@@ -398,6 +398,7 @@
     """
     _netinfo = CachingNetInfo()
     _netinfo.networks[network] = netAttr
+    _netinfo.networks[network]['bridged'] = netAttr['bridge'] is not
None
     _netinfo.networks[network]['dhcpv4'] = False
 
     if _netinfo.networks[network]['bridged']:
diff --git a/lib/vdsm/network/libvirt.py b/lib/vdsm/network/libvirt.py
index 12271d7..e80ba54 100644
--- a/lib/vdsm/network/libvirt.py
+++ b/lib/vdsm/network/libvirt.py
@@ -42,7 +42,8 @@
         raise
 
 
-def createNetworkDef(network, bridged=True, iface=None):
+def createNetworkDef(network, bridge=None, vlan=None, iface=None,
+                     virtualport=None):
     """
     Creates Network Xml e.g.:
     <network>
@@ -58,6 +59,9 @@
     or interface subelement.
     """
 
+    if bridge and iface:
+        raise AttributeError('Cannot set both bridge and iface at once')
+
     netName = LIBVIRT_NET_PREFIX + network
 
     def EtreeElement(tagName, text=None, **attrs):
@@ -70,16 +74,25 @@
         return elem
 
     root = etree.Element('network')
-    nameElem = EtreeElement('name', netName)
+
+    root.append(EtreeElement('name', netName))
+
     forwardElem = EtreeElement(
-        'forward', mode='bridge' if bridged else 'passthrough'
-    )
-    root.append(nameElem)
+        'forward', mode='bridge' if bridge else 'passthrough')
     root.append(forwardElem)
-    if bridged:
-        root.append(EtreeElement('bridge', name=network))
-    else:
+    if iface:
         forwardElem.append(EtreeElement('interface', dev=iface))
+    elif bridge:
+        root.append(EtreeElement('bridge', name=bridge))
+
+    if vlan is not None:
+        vlanElem = EtreeElement('vlan')
+        root.append(vlanElem)
+        vlanElem.append(EtreeElement('tag', id=vlan))
+
+    if virtualport:
+        root.append(EtreeElement('virtualport', type=virtualport))
+
     return etree.tostring(root)
 
 
@@ -107,8 +120,17 @@
 
     :returns: dict of networkname={properties}
     :rtype: dict of dict
-            { 'ovirtmgmt': { 'bridge': 'ovirtmgmt',
'bridged': True}
-              'red': { 'iface': 'red', 'bridged':
False}}
+            for legacy:
+            {'ovirtmgmt': {
+                'bridge': 'ovirtmgmt', 'vlan': None,
'iface': None,
+                'virtualport': None}
+             'red': {
+                'bridge': None, 'vlan': None, 'iface':
'red',
+                'virtualport': None}}
+            for ovs:
+            {'ovirtmgmt': {
+                'bridge': 'vdsmbr_xxx', 'vlan': 10,
'iface': None,
+                'virtualport': 'openvswitch'}}
     """
     nets = {}
     conn = libvirtconnection.get()
@@ -116,13 +138,27 @@
     for net, netname in allNets:
         if netname.startswith(LIBVIRT_NET_PREFIX):
             netname = netname[len(LIBVIRT_NET_PREFIX):]
-            nets[netname] = {}
             xml = etree.fromstring(net.XMLDesc(0))
-            interface = xml.find('.//interface')
-            if interface is not None:
-                nets[netname]['iface'] = interface.get('dev')
-                nets[netname]['bridged'] = False
-            else:
-                nets[netname]['bridge'] =
xml.find('.//bridge').get('name')
-                nets[netname]['bridged'] = True
+
+            _bridge = xml.find('.//bridge')
+            bridge = _bridge.get('name') if _bridge is not None else None
+
+            _vlan = xml.find('.//vlan')
+            _tag = _vlan.find('.//tag') if _vlan is not None else None
+            vlan = _tag.get('id') if _tag is not None else None
+            canonicalized_vlan = int(vlan) if vlan is not None else vlan
+
+            _iface = xml.find('.//interface')
+            iface = _iface.get('dev') if _iface is not None else None
+
+            _virtualport = xml.find('.//virtualport')
+            virtualport = (_virtualport.get('type') if _virtualport is not None
+                           else None)
+
+            nets[netname] = {
+                'bridge': bridge,
+                'vlan': canonicalized_vlan,
+                'iface': iface,
+                'virtualport': virtualport
+            }
     return nets
diff --git a/lib/vdsm/network/netinfo/cache.py b/lib/vdsm/network/netinfo/cache.py
index 73c87c6..d01d2a8 100644
--- a/lib/vdsm/network/netinfo/cache.py
+++ b/lib/vdsm/network/netinfo/cache.py
@@ -126,7 +126,8 @@
     for net, netAttr in six.iteritems(nets):
         try:
             # Pass the iface if the net is _not_ bridged, the bridge otherwise
-            d[net] = _getNetInfo(netAttr.get('iface', net),
netAttr['bridged'],
+            d[net] = _getNetInfo(netAttr['iface'] or net,
+                                 netAttr['bridge'] is not None,
                                  routes, ipAddrs,
                                  running_config.networks.get(net, None))
         except KeyError:
diff --git a/lib/vdsm/network/netswitch.py b/lib/vdsm/network/netswitch.py
index a5d393c..357d62c 100644
--- a/lib/vdsm/network/netswitch.py
+++ b/lib/vdsm/network/netswitch.py
@@ -22,9 +22,9 @@
 
 import six
 
+from vdsm.network import libvirt
 from vdsm.network.ip import address
 from vdsm.network.ip import dhclient
-from vdsm.network.libvirt import networks as libvirt_nets
 from vdsm.network.link import iface
 from vdsm.network.link.bond import Bond
 from vdsm.network.link.setup import SetupBonds
@@ -136,7 +136,7 @@
 
 def _setup_legacy(networks, bondings, options, in_rollback):
 
-    _libvirt_nets = libvirt_nets()
+    _libvirt_nets = libvirt.networks()
     _netinfo = CachingNetInfo(netinfo_get(libvirtNets2vdsm(_libvirt_nets)))
 
     with legacy_switch.ConfiguratorClass(in_rollback) as configurator:
diff --git a/tests/network/libvirt_test.py b/tests/network/libvirt_test.py
index b62351a..a182024 100644
--- a/tests/network/libvirt_test.py
+++ b/tests/network/libvirt_test.py
@@ -19,6 +19,7 @@
 #
 from __future__ import absolute_import
 
+from contextlib import contextmanager
 import re
 import xml.etree.ElementTree as ET
 
@@ -30,11 +31,13 @@
 
 NETWORK = 'awesome_net'
 LIBVIRT_NETWORK = 'vdsm-' + NETWORK
+OVS_BRIDGE = 'vdsmbr_123'
+VLAN = 10
 IFACE = 'dummy'
 
 
 @attr(type='unit')
-class LibvirtTests(TestCaseBase):
+class LibvirtUnitTests(TestCaseBase):
 
     def assertEqualXml(self, a, b):
         """Compare two xml strings for equality"""
@@ -53,7 +56,7 @@
                             <forward mode='bridge'/>
                             <bridge name='{}'/>
                          </network>""".format(LIBVIRT_NETWORK,
NETWORK)
-        actual_doc = libvirt.createNetworkDef(NETWORK, bridged=True)
+        actual_doc = libvirt.createNetworkDef(NETWORK, bridge=NETWORK)
         self.assertEqualXml(expected_doc, actual_doc)
 
     def test_create_net_xml_with_iface(self):
@@ -63,6 +66,71 @@
                               <interface dev='{}'/>
                             </forward>
                           </network>""".format(LIBVIRT_NETWORK,
IFACE)
-        actual_doc = libvirt.createNetworkDef(
-            NETWORK, bridged=False, iface=IFACE)
+        actual_doc = libvirt.createNetworkDef(NETWORK, iface=IFACE)
         self.assertEqualXml(expected_doc, actual_doc)
+
+    def test_create_net_xml_with_ovs_bridge(self):
+        expected_doc = """<network>
+                           <name>{}</name>
+                           <forward mode='bridge'/>
+                           <bridge name='{}'/>
+                           <virtualport type='openvswitch'/>
+                         </network>""".format(LIBVIRT_NETWORK,
OVS_BRIDGE)
+        actual_doc = libvirt.createNetworkDef(
+            NETWORK, bridge=OVS_BRIDGE, virtualport='openvswitch')
+        self.assertEqualXml(expected_doc, actual_doc)
+
+    def test_create_net_xml_with_ovs_bridge_and_vlan(self):
+        expected_doc = """<network>
+                           <name>{}</name>
+                           <forward mode='bridge'/>
+                           <bridge name='{}'/>
+                           <vlan>
+                             <tag id='{}'/>
+                           </vlan>
+                           <virtualport type='openvswitch'/>
+                         </network>""".format(LIBVIRT_NETWORK,
OVS_BRIDGE,
+                                              VLAN)
+        actual_doc = libvirt.createNetworkDef(
+            NETWORK, bridge=OVS_BRIDGE, vlan=VLAN, virtualport='openvswitch')
+        self.assertEqualXml(expected_doc, actual_doc)
+
+
+@attr(type='integration')
+class LibvirtIntegrationTests(TestCaseBase):
+
+    def _test_create_net(self, **net_attrs):
+        net_def = libvirt.createNetworkDef(NETWORK, **net_attrs)
+        with libvirt_net(NETWORK, net_def):
+            nets = libvirt.networks()
+            self.assertIn(NETWORK, nets)
+            self.assertEqual(nets[NETWORK], net_attrs)
+        nets = libvirt.networks()
+        self.assertNotIn(NETWORK, nets)
+
+    def test_create_net_with_bridge(self):
+        self._test_create_net(
+            bridge=NETWORK, iface=None, vlan=None, virtualport=None)
+
+    def test_create_net_with_iface(self):
+        self._test_create_net(
+            bridge=None, iface=IFACE, vlan=None, virtualport=None)
+
+    def test_create_net_with_ovs_bridge(self):
+        self._test_create_net(
+            bridge=OVS_BRIDGE, iface=None, vlan=None,
+            virtualport='openvswitch')
+
+    def test_create_net_with_ovs_bridge_and_vlan(self):
+        self._test_create_net(
+            bridge=OVS_BRIDGE, iface=None, vlan=VLAN,
+            virtualport='openvswitch')
+
+
+@contextmanager
+def libvirt_net(net, net_def):
+    try:
+        libvirt.createNetwork(net_def)
+        yield
+    finally:
+        libvirt.removeNetwork(net)
diff --git a/vdsm_hooks/ovs/ovs_before_network_setup_libvirt.py
b/vdsm_hooks/ovs/ovs_before_network_setup_libvirt.py
index 9eb7495..55d8e75 100644
--- a/vdsm_hooks/ovs/ovs_before_network_setup_libvirt.py
+++ b/vdsm_hooks/ovs/ovs_before_network_setup_libvirt.py
@@ -34,9 +34,7 @@
         else:
             if net in running_config.networks:
                 libvirt_remove.add(net)
-            libvirt_network_xml = libvirt.createNetworkDef(
-                net, bridged=True, iface=(attrs.get('nic') or
-                                          attrs.get('bonding')))
+            libvirt_network_xml = libvirt.createNetworkDef(net, bridge=net)
             libvirt_create[net] = libvirt_network_xml
 
     return libvirt_create, libvirt_remove


-- 
To view, visit https://gerrit.ovirt.org/65065
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;

gerrit-hooks has posted comments on this change.

Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................


Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/65065
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/65065
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................


Patch Set 7:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/65065
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

gerrit-hooks has posted comments on this change.

Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................


Patch Set 8:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE,         Not in stable branch (['ovirt-3.6',
'ovirt-4.0'])

-- 
To view, visit https://gerrit.ovirt.org/65065
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: No

Edward Haas has posted comments on this change.

Change subject: net libvirt: make network/libvirt.py suitable for OVS
......................................................................


Patch Set 8: Code-Review-1

(8 comments)

https://gerrit.ovirt.org/#/c/65065/8/lib/vdsm/network/libvirt.py
File lib/vdsm/network/libvirt.py:

PS8, Line 45: bridge
There is a hidden assumption now for the caller: If bridge is not None, then network ==
bridge.
I guess with the suggestion from line 62, this can be treated.


Line 58:     determines respectively the presence of bridge element
Line 59:     or interface subelement.
Line 60:     """
Line 61: 
Line 62:     if bridge and iface:
It will be cleaner to just have two funcs then, one for bridged and one for iface,
extracting the common code to their own funcs.
Line 63:         raise AttributeError('Cannot set both bridge and iface at once')
Line 64: 
Line 65:     netName = LIBVIRT_NET_PREFIX + network
Line 66: 


https://gerrit.ovirt.org/#/c/65065/8/tests/network/libvirt_test.py
File tests/network/libvirt_test.py:

PS8, Line 41: Unit
How about 'CreateXml' ?


PS8, Line 101: Integration
How about 'CreateNetwork'?


Line 100: @attr(type='integration')
Line 101: class LibvirtIntegrationTests(TestCaseBase):
Line 102: 
Line 103:     @ValidateRunningAsRoot
Line 104:     def _test_create_net(self, **net_attrs):
Private funcs/methods at the bottom please.
Line 105:         net_def = libvirt.createNetworkDef(NETWORK, **net_attrs)
Line 106:         with libvirt_net(NETWORK, net_def):
Line 107:             nets = libvirt.networks()
Line 108:             self.assertIn(NETWORK, nets)


Line 106:         with libvirt_net(NETWORK, net_def):
Line 107:             nets = libvirt.networks()
Line 108:             self.assertIn(NETWORK, nets)
Line 109:             self.assertEqual(nets[NETWORK], net_attrs)
Line 110:         nets = libvirt.networks()
Why do you need to check it is cleaned all the time?
I think a single test that checks that the removal is successful is enough, all others can
assume it is fine.
Line 111:         self.assertNotIn(NETWORK, nets)
Line 112: 
Line 113:     def test_create_net_with_bridge(self):
Line 114:         self._test_create_net(


Line 131: 
Line 132: @contextmanager
Line 133: def libvirt_net(net, net_def):
Line 134:     try:
Line 135:         libvirt.createNetwork(net_def)
If it failed during creation, what is there to remove?
Or can it fail and leave leftovers?
Line 136:         yield
Line 137:     finally:


https://gerrit.ovirt.org/#/c/65065/8/vdsm/virt/vmdevices/graphics.py
File vdsm/virt/vmdevices/graphics.py:

Even with https://gerrit.ovirt.org/#/c/65098 , the displayNetwork will be broken now.
1. We cannot use IP address anymore for binding, as the addresses are different from host
to host.
2. As far as I remember, the libvirt translation between network/bridge name to an IP
address is not working with OVS bridges (please correct me by checking this), so the
display network cannot be resolved when using OVS libvirt network.
It was one of the reasons we convinced ourself that dropping libvirt db is a good step.
Line 1: #
Line 2: # Copyright 2008-2014 Red Hat, Inc.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify


-- 
To view, visit https://gerrit.ovirt.org/65065
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6de49e456d8c49f0309690a7fc2bee995f9846bb
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes