From acfbd2d490700d616d8050f4ea6a4565bc72fc52 Mon Sep 17 00:00:00 2001 From: Jakub Libosvar Date: Mon, 2 Jan 2017 09:59:35 -0500 Subject: [PATCH] Move ovsdb_nested transaction to ovs_lib The patch introduces new abstract method to the API abstract class. The method is supposed to return a new Transaction object. Each API object is capable of store one nested transaction which is returned by context manager in case some transaction already exists. As there are no projects in OpenStack that use inheritance directly from API abstract class, it's safe to make this new create_transaction() abstract method. Only projects that currenlty use ovsdb API are networking-ovn, dragonflow and networking-l2gw. OVN and Dragonflow use only IDL implementation and L2GW copies the code of API abstract class. Closes-bug 1653517 Change-Id: I55dd417cae7ebbe0668ba5606949ce4ab045d251 --- neutron/agent/ovsdb/api.py | 26 +++- neutron/agent/ovsdb/impl_idl.py | 2 +- neutron/agent/ovsdb/impl_vsctl.py | 2 +- .../openvswitch/agent/trunk_manager.py | 29 +--- neutron/tests/unit/agent/ovsdb/test_api.py | 137 ++++++++++++++++++ .../openvswitch/agent/test_trunk_manager.py | 46 ------ 6 files changed, 168 insertions(+), 74 deletions(-) create mode 100644 neutron/tests/unit/agent/ovsdb/test_api.py diff --git a/neutron/agent/ovsdb/api.py b/neutron/agent/ovsdb/api.py index 553d87e04eb..31f87ca1906 100644 --- a/neutron/agent/ovsdb/api.py +++ b/neutron/agent/ovsdb/api.py @@ -14,6 +14,7 @@ import abc import collections +import contextlib import uuid from oslo_config import cfg @@ -83,6 +84,7 @@ class Transaction(object): class API(object): def __init__(self, context): self.context = context + self._nested_txn = None @staticmethod def get(context, iface_name=None): @@ -92,7 +94,7 @@ class API(object): return iface(context) @abc.abstractmethod - def transaction(self, check_error=False, log_errors=True, **kwargs): + def create_transaction(self, check_error=False, log_errors=True, **kwargs): """Create a transaction :param check_error: Allow the transaction to raise an exception? @@ -103,6 +105,28 @@ class API(object): :rtype: :class:`Transaction` """ + @contextlib.contextmanager + def transaction(self, check_error=False, log_errors=True, **kwargs): + """Create a transaction context. + + :param check_error: Allow the transaction to raise an exception? + :type check_error: bool + :param log_errors: Log an error if the transaction fails? + :type log_errors: bool + :returns: Either a new transaction or an existing one. + :rtype: :class:`Transaction` + """ + if self._nested_txn: + yield self._nested_txn + else: + with self.create_transaction( + check_error, log_errors, **kwargs) as txn: + self._nested_txn = txn + try: + yield txn + finally: + self._nested_txn = None + @abc.abstractmethod def add_manager(self, connection_uri): """Create a command to add a Manager to the OVS switch diff --git a/neutron/agent/ovsdb/impl_idl.py b/neutron/agent/ovsdb/impl_idl.py index f9a9904683a..fcae77d8880 100644 --- a/neutron/agent/ovsdb/impl_idl.py +++ b/neutron/agent/ovsdb/impl_idl.py @@ -207,7 +207,7 @@ class OvsdbIdl(api.API): def _ovs(self): return list(self._tables['Open_vSwitch'].rows.values())[0] - def transaction(self, check_error=False, log_errors=True, **kwargs): + def create_transaction(self, check_error=False, log_errors=True, **kwargs): return NeutronOVSDBTransaction(self, OvsdbIdl.ovsdb_connection, self.context.vsctl_timeout, check_error, log_errors) diff --git a/neutron/agent/ovsdb/impl_vsctl.py b/neutron/agent/ovsdb/impl_vsctl.py index e8fb5409c28..125c2078951 100644 --- a/neutron/agent/ovsdb/impl_vsctl.py +++ b/neutron/agent/ovsdb/impl_vsctl.py @@ -178,7 +178,7 @@ class BrExistsCommand(DbCommand): class OvsdbVsctl(ovsdb.API): - def transaction(self, check_error=False, log_errors=True, **kwargs): + def create_transaction(self, check_error=False, log_errors=True, **kwargs): return Transaction(self.context, check_error, log_errors, **kwargs) def add_manager(self, connection_uri): diff --git a/neutron/services/trunk/drivers/openvswitch/agent/trunk_manager.py b/neutron/services/trunk/drivers/openvswitch/agent/trunk_manager.py index 09515c169a5..ce59442dd62 100644 --- a/neutron/services/trunk/drivers/openvswitch/agent/trunk_manager.py +++ b/neutron/services/trunk/drivers/openvswitch/agent/trunk_manager.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import contextlib - from neutron_lib import constants from neutron_lib import exceptions from oslo_log import log as logging @@ -93,25 +91,6 @@ class TrunkParentPort(object): self.DEV_PREFIX, port_id) self._transaction = None - # TODO(jlibosva): Move nested transaction to ovs_lib - @contextlib.contextmanager - def ovsdb_transaction(self): - """Context manager for ovsdb transaction. - - The object caches whether its already in transaction and if it is, the - original transaction is returned. This behavior enables calling - manager several times while always getting the same transaction. - """ - if self._transaction: - yield self._transaction - else: - with self.bridge.ovsdb.transaction() as txn: - self._transaction = txn - try: - yield txn - finally: - self._transaction = None - def plug(self, br_int): """Plug patch ports between trunk bridge and given bridge. @@ -136,7 +115,7 @@ class TrunkParentPort(object): patch_trunk_attrs = get_patch_peer_attrs(self.patch_port_int_name, self.port_mac, self.port_id) - with self.ovsdb_transaction() as txn: + with ovsdb.transaction() as txn: txn.add(ovsdb.add_port(br_int.br_name, self.patch_port_int_name)) txn.add(ovsdb.db_set('Interface', self.patch_port_int_name, @@ -156,7 +135,7 @@ class TrunkParentPort(object): subport. """ ovsdb = self.bridge.ovsdb - with self.ovsdb_transaction() as txn: + with ovsdb.transaction() as txn: txn.add(ovsdb.del_br(self.bridge.br_name)) txn.add(ovsdb.del_port(self.patch_port_int_name, bridge.br_name)) @@ -190,7 +169,7 @@ class SubPort(TrunkParentPort): will be created. """ ovsdb = self.bridge.ovsdb - with self.ovsdb_transaction() as txn: + with ovsdb.transaction() as txn: super(SubPort, self).plug(br_int) txn.add(ovsdb.db_set( "Port", self.patch_port_trunk_name, @@ -206,7 +185,7 @@ class SubPort(TrunkParentPort): subport. """ ovsdb = self.bridge.ovsdb - with self.ovsdb_transaction() as txn: + with ovsdb.transaction() as txn: txn.add(ovsdb.del_port(self.patch_port_trunk_name, self.bridge.br_name)) txn.add(ovsdb.del_port(self.patch_port_int_name, diff --git a/neutron/tests/unit/agent/ovsdb/test_api.py b/neutron/tests/unit/agent/ovsdb/test_api.py new file mode 100644 index 00000000000..1df441b530f --- /dev/null +++ b/neutron/tests/unit/agent/ovsdb/test_api.py @@ -0,0 +1,137 @@ +# Copyright (c) 2017 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock +import testtools + +from neutron.agent.ovsdb import api +from neutron.tests import base + + +class FakeTransaction(object): + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_val, tb): + self.commit() + + def commit(self): + """Serves just for mock.""" + + +class TestingAPI(api.API): + def create_transaction(self, check_error=False, log_errors=True, **kwargs): + return FakeTransaction() + + def add_manager(self, connection_uri): + pass + + def get_manager(self): + pass + + def remove_manager(self, connection_uri): + pass + + def add_br(self, name, may_exist=True, datapath_type=None): + pass + + def del_br(self, name, if_exists=True): + pass + + def br_exists(self, name): + pass + + def port_to_br(self, name): + pass + + def iface_to_br(self, name): + pass + + def list_br(self): + pass + + def br_get_external_id(self, name, field): + pass + + def db_create(self, table, **col_values): + pass + + def db_destroy(self, table, record): + pass + + def db_set(self, table, record, *col_values): + pass + + def db_add(self, table, record, column, *values): + pass + + def db_clear(self, table, record, column): + pass + + def db_get(self, table, record, column): + pass + + def db_list(self, table, records=None, columns=None, if_exists=False): + pass + + def db_find(self, table, *conditions, **kwargs): + pass + + def set_controller(self, bridge, controllers): + pass + + def del_controller(self, bridge): + pass + + def get_controller(self, bridge): + pass + + def set_fail_mode(self, bridge, mode): + pass + + def add_port(self, bridge, port, may_exist=True): + pass + + def del_port(self, port, bridge=None, if_exists=True): + pass + + def list_ports(self, bridge): + pass + + def list_ifaces(self, bridge): + pass + + +class TransactionTestCase(base.BaseTestCase): + def setUp(self): + super(TransactionTestCase, self).setUp() + self.api = TestingAPI(None) + mock.patch.object(FakeTransaction, 'commit').start() + + def test_transaction_nested(self): + with self.api.transaction() as txn1: + with self.api.transaction() as txn2: + self.assertIs(txn1, txn2) + txn1.commit.assert_called_once_with() + + def test_transaction_no_nested_transaction_after_error(self): + class TestException(Exception): + pass + + with testtools.ExpectedException(TestException): + with self.api.transaction() as txn1: + raise TestException() + + with self.api.transaction() as txn2: + self.assertIsNot(txn1, txn2) diff --git a/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_trunk_manager.py b/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_trunk_manager.py index 2d13506878e..9a7b3c37a2a 100644 --- a/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_trunk_manager.py +++ b/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_trunk_manager.py @@ -16,57 +16,11 @@ import contextlib import mock -from oslo_utils import uuidutils import testtools -from neutron.common import utils as common_utils from neutron.services.trunk.drivers.openvswitch.agent import trunk_manager from neutron.tests import base -NATIVE_OVSDB_CONNECTION = ( - 'neutron.agent.ovsdb.impl_idl.OvsdbIdl.ovsdb_connection') - - -class TrunkParentPortTestCase(base.BaseTestCase): - def setUp(self): - super(TrunkParentPortTestCase, self).setUp() - # Mock out connecting to ovsdb - mock.patch(NATIVE_OVSDB_CONNECTION).start() - trunk_id = uuidutils.generate_uuid() - port_id = uuidutils.generate_uuid() - trunk_mac = common_utils.get_random_mac('fa:16:3e:00:00:00'.split(':')) - self.trunk = trunk_manager.TrunkParentPort( - trunk_id, port_id, trunk_mac) - - def test_multiple_transactions(self): - def method_inner(trunk): - with trunk.ovsdb_transaction() as txn: - return id(txn) - - def method_outer(trunk): - with trunk.ovsdb_transaction() as txn: - return method_inner(trunk), id(txn) - - with self.trunk.ovsdb_transaction() as txn1: - mock_commit = mock.patch.object(txn1, 'commit').start() - txn_inner_id, txn_outer_id = method_outer(self.trunk) - self.assertFalse(mock_commit.called) - self.assertTrue(mock_commit.called) - self.assertTrue(id(txn1) == txn_inner_id == txn_outer_id) - - def test_transaction_raises_error(self): - class MyException(Exception): - pass - - with testtools.ExpectedException(MyException): - with self.trunk.ovsdb_transaction() as txn1: - mock.patch.object(txn1, 'commit').start() - raise MyException() - self.assertIsNone(self.trunk._transaction) - with self.trunk.ovsdb_transaction() as txn2: - mock.patch.object(txn2, 'commit').start() - self.assertIsNot(txn1, txn2) - class TrunkManagerTestCase(base.BaseTestCase): """Tests are aimed to cover negative cases to make sure there is no typo in