Edward Haas has uploaded a new change for review.

Change subject: net: Split OVS setup transaction to adjust for bond setup
......................................................................

net: Split OVS setup transaction to adjust for bond setup

When using Linux bonds (instead of OVS bonds), the OVS setup transaction
needs to be split into two.
First one for removing networks and a second for adding networks.
In between, the bonds setup is placed.

These three steps contain, in addition to the actual devices setup, the
acquire and running-config update, allowing proper rollback in case of
setup failure.

This patch, fixies a scenario that fails with the previous patch, where
the Linux bonds have been introduced in the setup flow.
The scenario: Move a network nic from a network to a bond slave.

Change-Id: I0bd351e755be9d5dcaf5a05e5b79345e6f7bcfe8
Signed-off-by: Edward Haas <edwa...@redhat.com>
---
M lib/vdsm/network/netswitch.py
M lib/vdsm/network/ovs/switch.py
M tests/network/ovs_switch_test.py
M tests/network/ovs_test.py
4 files changed, 106 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/18/64118/1

diff --git a/lib/vdsm/network/netswitch.py b/lib/vdsm/network/netswitch.py
index 70d2cd6..529957a 100644
--- a/lib/vdsm/network/netswitch.py
+++ b/lib/vdsm/network/netswitch.py
@@ -172,37 +172,40 @@
 
     # FIXME: we are not able to move a nic from bond to network in one setup
     with Transaction(in_rollback=in_rollback) as config:
-        setup_bonds = SetupBonds(bonds2add, bonds2edit, bonds2remove, config)
         with ifacquire.Transaction(ovs_netinfo['networks']) as acq:
-            with ovs_switch.create_setup(_ovs_info) as setup_ovs:
-                setup_ovs.remove_nets(nets2remove)
-                setup_bonds.remove_bonds()
-                acq.acquire(setup_bonds.ifaces_for_acquirement)
-                setup_bonds.edit_bonds()
-                setup_bonds.add_bonds()
-                setup_ovs.add_nets(nets2add)
-                acq.acquire(setup_ovs.acquired_ifaces)
-            _update_networks_running_config(networks, config)
+            _remove_networks(nets2remove, _ovs_info, config)
+            _setup_bonds(bonds2add, bonds2edit, bonds2remove, config, acq)
+            _add_networks(nets2add, _ovs_info, config, acq)
+
             ovs_switch.cleanup()
             setup_ipv6autoconf(networks)
             set_ovs_links_up(nets2add, bonds2add, bonds2edit)
             setup_ovs_ip_config(nets2add, nets2remove)
+
             connectivity.check(options)
 
 
-# TODO: We should use KernelConfig when it will be fully reliable.
-def _update_networks_running_config(networks, running_config):
-    """
-    Update running_config with the networks configuration.
+def _remove_networks(nets2remove, ovs_info, config):
+    net_rem_setup = ovs_switch.create_network_removal_setup(ovs_info)
+    net_rem_setup.remove(nets2remove)
+    for net, attrs in six.iteritems(nets2remove):
+        config.removeNetwork(net)
 
-    This step has to be done as soon as we apply the changes in the system.
-    The running config will be used to generate the rollback query.
-    """
-    for net, attrs in six.iteritems(networks):
-        if 'remove' in attrs:
-            running_config.removeNetwork(net)
-        else:
-            running_config.setNetwork(net, attrs)
+
+def _setup_bonds(bonds2add, bonds2edit, bonds2remove, config, acq):
+    setup_bonds = SetupBonds(bonds2add, bonds2edit, bonds2remove, config)
+    setup_bonds.remove_bonds()
+    acq.acquire(setup_bonds.ifaces_for_acquirement)
+    setup_bonds.edit_bonds()
+    setup_bonds.add_bonds()
+
+
+def _add_networks(nets2add, ovs_info, config, acq):
+    net_add_setup = ovs_switch.create_network_addition_setup(ovs_info)
+    net_add_setup.add(nets2add)
+    acq.acquire(net_add_setup.acquired_ifaces)
+    for net, attrs in six.iteritems(nets2add):
+        config.setNetwork(net, attrs)
 
 
 def setup_ovs_ip_config(nets2add, nets2remove):
diff --git a/lib/vdsm/network/ovs/switch.py b/lib/vdsm/network/ovs/switch.py
index 1b47315..1e16a88 100644
--- a/lib/vdsm/network/ovs/switch.py
+++ b/lib/vdsm/network/ovs/switch.py
@@ -53,84 +53,96 @@
         t.add(*_cleanup_unused_bridges(ovsdb))
 
 
-def create_setup(ovs_info):
+def create_network_removal_setup(ovs_info):
     ovsdb = driver.create()
-    return Setup(ovsdb, ovs_info)
+    return NetsRemovalSetup(ovsdb, ovs_info)
 
 
-# TODO: We could move all setup() code into __init__ and __exit__.
-class Setup(object):
+def create_network_addition_setup(ovs_info):
+    ovsdb = driver.create()
+    return NetsAdditionSetup(ovsdb, ovs_info)
+
+
+class NetsRemovalSetup(object):
     def __init__(self, ovsdb, ovs_info):
         self._ovsdb = ovsdb
-        self._transaction = self._ovsdb.transaction()
         self._ovs_info = ovs_info
-        self._bridges_by_sb = ovs_info.bridges_by_sb
-        self._northbounds_by_sb = ovs_info.northbounds_by_sb
+        self._transaction = ovsdb.transaction()
 
+    def remove(self, nets):
+        ovs_netinfo = info.create_netinfo(self._ovs_info)
+        running_networks = ovs_netinfo['networks']
+        with self._transaction:
+            for net in nets:
+                sb = self._get_southbound(net, running_networks)
+                self._remove_northbound(net, sb)
+                self._detach_unused_southbound(sb)
+
+    def _remove_northbound(self, net, sb):
+        self._ovs_info.northbounds_by_sb[sb].discard(net)
+        self._transaction.add(self._ovsdb.del_port(net))
+
+    def _detach_unused_southbound(self, sb):
+        if sb and not self._ovs_info.northbounds_by_sb[sb]:
+            self._ovs_info.northbounds_by_sb.pop(sb)
+            self._ovs_info.bridges_by_sb.pop(sb)
+
+            self._transaction.add(self._ovsdb.del_port(sb))
+
+    @staticmethod
+    def _get_southbound(net, running_networks):
+        running_attrs = running_networks[net]
+        bond = running_attrs['bond']
+        nic = running_attrs['nics'][0] if not bond else None
+        return nic or bond
+
+
+class NetsAdditionSetup(object):
+    def __init__(self, ovsdb, ovs_info):
+        self._ovsdb = ovsdb
+        self._ovs_info = ovs_info
+        self._transaction = ovsdb.transaction()
         self._acquired_ifaces = set()
+
+    def add(self, nets):
+        with self._transaction:
+            for net, attrs in six.iteritems(nets):
+                nic = attrs.get('nic')
+                bond = attrs.get('bonding')
+                sb = nic or bond
+                self._acquired_ifaces.add(sb)
+
+                bridge = self._get_ovs_bridge(sb)
+                self._create_nb(bridge, net)
+
+                vlan = attrs.get('vlan')
+                if vlan is not None:
+                    self._set_vlan(net, vlan)
+
+                # FIXME: What about an existing bond?
+                if nic is not None and vlan is None:
+                    self._copy_nic_hwaddr_to_nb(net, nic)
+
+                self._ovs_info.northbounds_by_sb.setdefault(sb, set())
+                self._ovs_info.northbounds_by_sb[sb].add(net)
 
     @property
     def acquired_ifaces(self):
         """
-        Report the interfaces that have been added to networks/bondings, either
+        Report the interfaces that have been added to networks, either
         by add or edit actions, including ifaces that have been removed and
-        re-added to a different network/bonding.
+        re-added to a different network.
         """
         return self._acquired_ifaces
 
-    def __enter__(self):
-        return self
-
-    def __exit__(self, type, value, traceback):
-        if type is None:
-            self._transaction.commit()
+    def _get_ovs_bridge(self, sb):
+        if sb in self._ovs_info.bridges_by_sb:
+            bridge = self._ovs_info.bridges_by_sb[sb]
         else:
-            six.reraise(type, value, traceback)
-
-    def remove_nets(self, nets):
-        ovs_netinfo = info.create_netinfo(self._ovs_info)
-        running_networks = ovs_netinfo['networks']
-        for net in nets:
-            running_attrs = running_networks[net]
-            bond = running_attrs['bond']
-            nic = running_attrs['nics'][0] if not bond else None
-            sb = nic or bond
-
-            self._northbounds_by_sb[sb].discard(net)
-
-            # Detach NIC if not used anymore.
-            if nic and not self._northbounds_by_sb[nic]:
-                self._detach_sb_nic(nic)
-
-            self._transaction.add(self._ovsdb.del_port(net))
-
-    def _detach_sb_nic(self, nic):
-        self._northbounds_by_sb.pop(nic)
-        self._bridges_by_sb.pop(nic)
-        self._transaction.add(self._ovsdb.del_port(nic))
-
-    def add_nets(self, nets):
-        for net, attrs in six.iteritems(nets):
-            nic = attrs.get('nic')
-            bond = attrs.get('bonding')
-            sb = nic or bond
-            self._acquired_ifaces.add(sb)
-            if sb in self._bridges_by_sb:
-                bridge = self._bridges_by_sb[sb]
-            else:
-                bridge = self._create_bridge()
-                self._bridges_by_sb[sb] = bridge
-                self._create_sb_nic(bridge, sb)
-
-            self._create_nb(bridge, net)
-            vlan = attrs.get('vlan')
-            if vlan is not None:
-                self._set_vlan(net, vlan)
-            # FIXME: What about an existing bond?
-            if nic is not None and vlan is None:
-                self._copy_nic_hwaddr_to_nb(net, nic)
-
-            self._northbounds_by_sb.setdefault(sb, set()).add(net)
+            bridge = self._create_bridge()
+            self._ovs_info.bridges_by_sb[sb] = bridge
+            self._create_sb_nic(bridge, sb)
+        return bridge
 
     def _create_nb(self, bridge, port):
         self._transaction.add(self._ovsdb.add_port(bridge, port))
diff --git a/tests/network/ovs_switch_test.py b/tests/network/ovs_switch_test.py
index c899b4c..178dab6 100644
--- a/tests/network/ovs_switch_test.py
+++ b/tests/network/ovs_switch_test.py
@@ -55,10 +55,10 @@
             mock.patch('vdsm.network.ovs.switch.link.get_link',
                        return_value={'address': '01:23:45:67:89:ab'}):
 
-            with switch.Setup(ovsdb, _ovs_info) as s:
-                s.add_nets(nets2add)
+            setup = switch.NetsAdditionSetup(ovsdb, _ovs_info)
+            setup.add(nets2add)
 
-                self.assertEqual(s.acquired_ifaces, expected_ifaces)
+            self.assertEqual(setup.acquired_ifaces, expected_ifaces)
 
 
 def _init_ovs_info(mock_ovs_info):
diff --git a/tests/network/ovs_test.py b/tests/network/ovs_test.py
index 4c7cc3a..f6d589e 100644
--- a/tests/network/ovs_test.py
+++ b/tests/network/ovs_test.py
@@ -212,6 +212,8 @@
 
     def test_dry_run(self):
         ovs_info = MockedOvsInfo()
-        with ovs_switch.Setup(self.ovsdb, ovs_info) as s:
-            s.remove_nets({})
-            s.add_nets({})
+        net_rem_setup = ovs_switch.NetsRemovalSetup(self.ovsdb, ovs_info)
+        net_rem_setup.remove({})
+
+        net_add_setup = ovs_switch.NetsAdditionSetup(self.ovsdb, ovs_info)
+        net_add_setup.add({})


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0bd351e755be9d5dcaf5a05e5b79345e6f7bcfe8
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwa...@redhat.com>
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to