From 4c9e3edc758699674deed83893cf314481dc6ee1 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Fri, 3 Feb 2017 16:39:24 -0600 Subject: [PATCH] Clean up ovsdb-native's use of verify() When updating an ovsdb set-type column, the existing code does the following: 1. Read the existing column value 2. Call verify() to cause a write failure if something else modifies the column before we commit 3. Append the value to our local copy of the column 4. Assign the local value to the object.column to trigger __setattr__ to write the value to the database If verify() fails, which it *very* often does in a test environment or a busy system, ovsdb-server will respond with a TRY_AGAIN response which will cause the whole process to start over. In the 2.6 cycle, Row.addvalue()/delvalue() methods were added which use OVDB's native 'mutate' methods to handle adding/deleting from a set/map-type column. Using these means calling verify() is no longer required and things will proceed *much* more efficiently. Bug #1627106 where we get frequent TimeoutExceptions appears to be related. Eliminating the frequent TRY_AGAIN responses, in my local testing, also eliminates the TimeoutExceptions. This doesn't mean that the underlying issue is resolved, but it may be avoided. Related-Bug: #1627106 Conflicts: neutron/agent/ovsdb/native/commands.py neutron/tests/functional/agent/test_ovs_lib.py Change-Id: I26c7731f5dbd3bd2955dbfa18a7c41517da63e6e (cherry picked from commit 26766963c6e6b2105718e21cd14dfc6df201154c) (cherry picked from commit 339855ff09916ed3573786395eace0e06011677d) --- neutron/agent/ovsdb/native/commands.py | 93 +++++++++++-------- .../tests/functional/agent/test_ovs_lib.py | 15 +++ 2 files changed, 69 insertions(+), 39 deletions(-) diff --git a/neutron/agent/ovsdb/native/commands.py b/neutron/agent/ovsdb/native/commands.py index 34cd50ce505..deb4cdbfe77 100644 --- a/neutron/agent/ovsdb/native/commands.py +++ b/neutron/agent/ovsdb/native/commands.py @@ -59,8 +59,12 @@ class AddManagerCommand(BaseCommand): def run_idl(self, txn): row = txn.insert(self.api._tables['Manager']) row.target = self.target - self.api._ovs.verify('manager_options') - self.api._ovs.manager_options = self.api._ovs.manager_options + [row] + try: + self.api._ovs.addvalue('manager_options', row) + except AttributeError: # OVS < 2.6 + self.api._ovs.verify('manager_options') + self.api._ovs.manager_options = ( + self.api._ovs.manager_options + [row]) class GetManagerCommand(BaseCommand): @@ -85,11 +89,14 @@ class RemoveManagerCommand(BaseCommand): msg = _("Manager with target %s does not exist") % self.target LOG.error(msg) raise RuntimeError(msg) - self.api._ovs.verify('manager_options') - manager_list = self.api._ovs.manager_options - manager_list.remove(manager) - self.api._ovs.manager_options = manager_list - self.api._tables['Manager'].rows[manager.uuid].delete() + try: + self.api._ovs.delvalue('manager_options', manager) + except AttributeError: # OVS < 2.6 + self.api._ovs.verify('manager_options') + manager_list = self.api._ovs.manager_options + manager_list.remove(manager) + self.api._ovs.manager_options = manager_list + manager.delete() class AddBridgeCommand(BaseCommand): @@ -111,8 +118,11 @@ class AddBridgeCommand(BaseCommand): row.name = self.name if self.datapath_type: row.datapath_type = self.datapath_type - self.api._ovs.verify('bridges') - self.api._ovs.bridges = self.api._ovs.bridges + [row] + try: + self.api._ovs.addvalue('bridges', row) + except AttributeError: # OVS < 2.6 + self.api._ovs.verify('bridges') + self.api._ovs.bridges = self.api._ovs.bridges + [row] # Add the internal bridge port cmd = AddPortCommand(self.api, self.name, self.name, self.may_exist) @@ -140,15 +150,19 @@ class DelBridgeCommand(BaseCommand): msg = _("Bridge %s does not exist") % self.name LOG.error(msg) raise RuntimeError(msg) - self.api._ovs.verify('bridges') + # Clean up cached ports/interfaces for port in br.ports: - cmd = DelPortCommand(self.api, port.name, self.name, - if_exists=True) - cmd.run_idl(txn) - bridges = self.api._ovs.bridges - bridges.remove(br) - self.api._ovs.bridges = bridges - self.api._tables['Bridge'].rows[br.uuid].delete() + for interface in port.interfaces: + interface.delete() + port.delete() + try: + self.api._ovs.delvalue('bridges', br) + except AttributeError: # OVS < 2.6 + self.api._ovs.verify('bridges') + bridges = self.api._ovs.bridges + bridges.remove(br) + self.api._ovs.bridges = bridges + br.delete() class BridgeExistsCommand(BaseCommand): @@ -285,7 +299,7 @@ class SetControllerCommand(BaseCommand): controller = txn.insert(self.api._tables['Controller']) controller.target = target controllers.append(controller) - br.verify('controller') + # Don't need to verify because we unconditionally overwrite br.controller = controllers @@ -306,7 +320,6 @@ class GetControllerCommand(BaseCommand): def run_idl(self, txn): br = idlutils.row_by_value(self.api.idl, 'Bridge', 'name', self.bridge) - br.verify('controller') self.result = [c.target for c in br.controller] @@ -318,7 +331,6 @@ class SetFailModeCommand(BaseCommand): def run_idl(self, txn): br = idlutils.row_by_value(self.api.idl, 'Bridge', 'name', self.bridge) - br.verify('fail_mode') br.fail_mode = self.mode @@ -338,18 +350,19 @@ class AddPortCommand(BaseCommand): return port = txn.insert(self.api._tables['Port']) port.name = self.port - br.verify('ports') - ports = getattr(br, 'ports', []) - ports.append(port) - br.ports = ports + try: + br.addvalue('ports', port) + except AttributeError: # OVS < 2.6 + br.verify('ports') + ports = getattr(br, 'ports', []) + ports.append(port) + br.ports = ports iface = txn.insert(self.api._tables['Interface']) txn.expected_ifaces.add(iface.uuid) iface.name = self.port - port.verify('interfaces') - ifaces = getattr(port, 'interfaces', []) - ifaces.append(iface) - port.interfaces = ifaces + # This is a new port, so it won't have any existing interfaces + port.interfaces = [iface] class DelPortCommand(BaseCommand): @@ -375,24 +388,26 @@ class DelPortCommand(BaseCommand): br = next(b for b in self.api._tables['Bridge'].rows.values() if port in b.ports) - if port.uuid not in br.ports and not self.if_exists: + if port not in br.ports and not self.if_exists: # TODO(twilson) Make real errors across both implementations msg = _("Port %(port)s does not exist on %(bridge)s!") % { - 'port': self.name, 'bridge': self.bridge + 'port': self.port, 'bridge': self.bridge } LOG.error(msg) raise RuntimeError(msg) - br.verify('ports') - ports = br.ports - ports.remove(port) - br.ports = ports + try: + br.delvalue('ports', port) + except AttributeError: # OVS < 2.6 + br.verify('ports') + ports = br.ports + ports.remove(port) + br.ports = ports - # Also remove port/interface directly for indexing? - port.verify('interfaces') - for iface in port.interfaces: - self.api._tables['Interface'].rows[iface.uuid].delete() - self.api._tables['Port'].rows[port.uuid].delete() + # The interface on the port will be cleaned up by ovsdb-server + for interface in port.interfaces: + interface.delete() + port.delete() class ListPortsCommand(BaseCommand): diff --git a/neutron/tests/functional/agent/test_ovs_lib.py b/neutron/tests/functional/agent/test_ovs_lib.py index e1c6ed61296..221838c2c41 100644 --- a/neutron/tests/functional/agent/test_ovs_lib.py +++ b/neutron/tests/functional/agent/test_ovs_lib.py @@ -21,6 +21,7 @@ from neutron_lib import constants as const from neutron.agent.common import ovs_lib from neutron.agent.linux import ip_lib +from neutron.agent.ovsdb.native import idlutils from neutron.common import utils from neutron.tests.common.exclusive_resources import port from neutron.tests.common import net_helpers @@ -336,6 +337,20 @@ class OVSBridgeTestCase(OVSBridgeTestBase): self.assertIsNone(max_rate) self.assertIsNone(burst) + def test_cascading_del_in_txn(self): + ovsdb = self.ovs.ovsdb + port_name, _ = self.create_ovs_port() + + def del_port_mod_iface(): + with ovsdb.transaction(check_error=True) as txn: + txn.add(ovsdb.del_port(port_name, self.br.br_name, + if_exists=False)) + txn.add(ovsdb.db_set('Interface', port_name, + ('type', 'internal'))) + # native gives a more specific exception than vsctl + self.assertRaises((RuntimeError, idlutils.RowNotFound), + del_port_mod_iface) + class OVSLibTestCase(base.BaseOVSLinuxTestCase):