diff --git a/quark/db/api.py b/quark/db/api.py index 54f8617..ca89db1 100644 --- a/quark/db/api.py +++ b/quark/db/api.py @@ -86,7 +86,7 @@ def _model_query(context, model, filters, fields=None): filters = filters or {} model_filters = [] eq_filters = ["address", "cidr", "deallocated", "ip_version", - "mac_address_range_id"] + "mac_address_range_id", "transaction_id"] in_filters = ["device_id", "device_owner", "group_id", "id", "mac_address", "name", "network_id", "segment_id", "subnet_id", "used_by_tenant_id", "version"] @@ -316,10 +316,6 @@ def ip_address_find(context, lock_mode=False, **filters): model_filters.append( models.IPAddress.address_type == filters['address_type']) - if filters.get("transaction_id"): - model_filters.append( - models.IPAddress.transaction_id == filters['transaction_id']) - return query.filter(*model_filters) @@ -394,6 +390,37 @@ def mac_address_delete(context, mac_address): context.session.delete(mac_address) +@scoped +def mac_address_reallocate(context, update_kwargs, **filters): + LOG.debug("mac_address_reallocate %s", filters) + query = context.session.query(models.MacAddress) + model_filters = _model_query(context, models.MacAddress, filters) + query = query.filter(*model_filters) + row_count = quark_sa.update( + query, update_kwargs, + update_args={"mysql_limit": 1}) + return row_count == 1 + + +def mac_address_reallocate_find(context, transaction_id): + mac = mac_address_find(context, transaction_id=transaction_id, + scope=ONE) + if not mac: + LOG.warn("Couldn't find MAC address with transaction_id %s", + transaction_id) + return + + # NOTE(mdietz): This is a HACK. Please see RM11043 for details + if mac["mac_address_range"] and mac["mac_address_range"]["do_not_use"]: + mac_address_delete(context, mac) + LOG.debug("Found a deallocated MAC in a do_not_use" + " mac_address_range and deleted it. " + "Retrying...") + return + + return mac + + def mac_address_range_find_allocation_counts(context, address=None, use_forbidden_mac_range=False): count = sql_func.count(models.MacAddress.address) diff --git a/quark/db/migration/alembic/versions/356d6c0623c8_add_transaction_id_to_quark_mac_.py b/quark/db/migration/alembic/versions/356d6c0623c8_add_transaction_id_to_quark_mac_.py new file mode 100644 index 0000000..94ba699 --- /dev/null +++ b/quark/db/migration/alembic/versions/356d6c0623c8_add_transaction_id_to_quark_mac_.py @@ -0,0 +1,30 @@ +"""Add transaction_id to quark_mac_addresses + +Revision ID: 356d6c0623c8 +Revises: 5632aa202d89 +Create Date: 2015-04-14 01:53:48.233241 + +""" + +# revision identifiers, used by Alembic. +revision = '356d6c0623c8' +down_revision = '5632aa202d89' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('quark_mac_addresses', + sa.Column('transaction_id', sa.Integer(), nullable=True)) + op.create_foreign_key( + 'fk_quark_macs_transaction_id', + 'quark_mac_addresses', 'quark_transactions', + ['transaction_id'], ['id']) + + +def downgrade(): + op.drop_constraint( + 'fk_quark_macs_transaction_id', 'quark_mac_addresses', + type_='foreignkey') + op.drop_column('quark_mac_addresses', 'transaction_id') diff --git a/quark/db/migration/alembic/versions/HEAD b/quark/db/migration/alembic/versions/HEAD index 31df80e..7b01d0a 100644 --- a/quark/db/migration/alembic/versions/HEAD +++ b/quark/db/migration/alembic/versions/HEAD @@ -1 +1 @@ -5632aa202d89 +356d6c0623c8 \ No newline at end of file diff --git a/quark/db/models.py b/quark/db/models.py index 3ea974d..30784a0 100644 --- a/quark/db/models.py +++ b/quark/db/models.py @@ -407,6 +407,9 @@ class MacAddress(BASEV2, models.HasTenant): deallocated = sa.Column(sa.Boolean(), index=True) deallocated_at = sa.Column(sa.DateTime(), index=True) orm.relationship(Port, backref="mac_address") + transaction_id = sa.Column(sa.Integer(), + sa.ForeignKey("quark_transactions.id"), + nullable=True) class MacAddressRange(BASEV2, models.HasId): diff --git a/quark/ipam.py b/quark/ipam.py index 65924db..4e11100 100644 --- a/quark/ipam.py +++ b/quark/ipam.py @@ -187,18 +187,6 @@ class QuarkIPAMLogEntry(object): class QuarkIpam(object): - def _delete_mac_if_do_not_use(self, context, mac): - # NOTE(mdietz): This is a HACK. Please see RM11043 for details - admin_ctx = context.elevated() - mac_range = db_api.mac_address_range_find( - admin_ctx, id=mac["mac_address_range_id"], scope=db_api.ONE) - - if mac_range and mac_range["do_not_use"]: - # NOTE(mdietz): This is a HACK. Please see RM11043 for details - db_api.mac_address_delete(context, mac) - return True - return False - @synchronized(named("allocate_mac_address")) def allocate_mac_address(self, context, net_id, port_id, reuse_after, mac_address=None, @@ -218,31 +206,32 @@ class QuarkIpam(object): retry + 1, CONF.QUARK.mac_address_retry_max)) try: with context.session.begin(): - deallocated_mac = db_api.mac_address_find( - context, lock_mode=True, reuse_after=reuse_after, - deallocated=True, scope=db_api.ONE, - address=mac_address, order_by="address ASC") - - if deallocated_mac: - if self._delete_mac_if_do_not_use(context, - deallocated_mac): - LOG.debug("Found a deallocated MAC in a do_not_use" - " mac_address_range and deleted it. " - "Retrying...") - continue - - dealloc = netaddr.EUI(deallocated_mac["address"]) - LOG.info("Found a suitable deallocated MAC {0}".format( - str(dealloc))) - - address = db_api.mac_address_update( - context, deallocated_mac, deallocated=False, - deallocated_at=None) - - LOG.info("MAC assignment for port ID {0} completed " - "with address {1}".format(port_id, dealloc)) - return address + transaction = db_api.transaction_create(context) + update_kwargs = { + "deallocated": False, + "deallocated_at": None, + "transaction_id": transaction.id + } + filter_kwargs = { + "reuse_after": reuse_after, + "deallocated": True, + "address": mac_address + } + elevated = context.elevated() + result = db_api.mac_address_reallocate( + elevated, update_kwargs, **filter_kwargs) + if not result: break + + reallocated_mac = db_api.mac_address_reallocate_find( + elevated, transaction.id) + if reallocated_mac: + dealloc = netaddr.EUI(reallocated_mac["address"]) + LOG.info("Found a suitable deallocated MAC {0}".format( + str(dealloc))) + LOG.info("MAC assignment for port ID {0} completed " + "with address {1}".format(port_id, dealloc)) + return reallocated_mac except Exception: LOG.exception("Error in mac reallocate...") continue @@ -747,7 +736,9 @@ class QuarkIpam(object): raise exceptions.NotFound( message="No MAC address %s found" % netaddr.EUI(address)) - if not self._delete_mac_if_do_not_use(context, mac): + if mac["mac_address_range"]["do_not_use"]: + db_api.mac_address_delete(context, mac) + else: db_api.mac_address_update(context, mac, deallocated=True, deallocated_at=timeutils.utcnow()) diff --git a/quark/tests/functional/mysql/test_ipam.py b/quark/tests/functional/mysql/test_ipam.py index 59844cb..bc7d717 100644 --- a/quark/tests/functional/mysql/test_ipam.py +++ b/quark/tests/functional/mysql/test_ipam.py @@ -88,3 +88,41 @@ class QuarkIPAddressReallocate(QuarkIpamBaseFunctionalTest): with self.assertRaises(exceptions.IpAddressGenerationFailure): self.ipam.allocate_ip_address(self.context, [], net["id"], 0, 0) + + +class MacAddressReallocate(QuarkIpamBaseFunctionalTest): + @contextlib.contextmanager + def _stubs(self, do_not_use): + self.ipam = quark.ipam.QuarkIpamANY() + mar = db_api.mac_address_range_create( + self.context, + cidr="00:00:00:00:00:00/40", + first_address=0, last_address=255, + next_auto_assign_mac=6, + do_not_use=do_not_use) + mac = db_api.mac_address_create( + self.context, + address=1, + mac_address_range=mar) + db_api.mac_address_update( + self.context, mac, + deallocated=True, + deallocated_at=datetime.datetime(1970, 1, 1)) + self.context.session.flush() + yield mar + + def test_reallocate_mac(self): + with self._stubs(do_not_use=False): + realloc_mac = self.ipam.allocate_mac_address(self.context, 0, 0, 0) + self.assertEqual(realloc_mac["address"], 1) + + def test_delete_mac_with_mac_range_do_not_use(self): + macs = lambda mar: db_api.mac_address_find( + self.context, + mac_address_range_id=mar["id"], + scope=db_api.ALL) + with self._stubs(do_not_use=True) as mar: + self.assertEqual(len(macs(mar)), 1) + with self.assertRaises(exceptions.MacAddressGenerationFailure): + self.ipam.allocate_mac_address(self.context, 0, 0, 0) + self.assertEqual(len(macs(mar)), 0) diff --git a/quark/tests/test_ipam.py b/quark/tests/test_ipam.py index dc52282..8a7aecb 100644 --- a/quark/tests/test_ipam.py +++ b/quark/tests/test_ipam.py @@ -106,24 +106,27 @@ class QuarkMacAddressAllocateDeallocated(QuarkIpamBaseTest): next_auto_assign_mac=0, do_not_use=do_not_use, cidr="AA:BB:CC/24") with contextlib.nested( - mock.patch("quark.db.api.mac_address_find"), + mock.patch("quark.db.api.mac_address_reallocate"), + mock.patch("quark.db.api.mac_address_reallocate_find"), mock.patch("quark.db.api." "mac_address_range_find_allocation_counts"), - mock.patch("quark.db.api.mac_address_update"), mock.patch("quark.db.api.mac_address_create"), mock.patch("quark.db.api.mac_address_range_find"), mock.patch("quark.db.api.mac_address_delete"), mock.patch("quark.db.api.mac_range_update_next_auto_assign_mac"), mock.patch("quark.db.api.mac_range_update_set_full"), mock.patch("sqlalchemy.orm.session.Session.refresh") - ) as (addr_find, mac_range_count, mac_update, mac_create, - range_find, mac_delete, mac_auto_assign, set_full, refresh): + ) as (addr_realloc, addr_realloc_find, mac_range_count, + mac_create, range_find, mac_delete, mac_auto_assign, set_full, + refresh): address_mod = models.MacAddress(**address) range_mod = models.MacAddressRange(**mac_range) if mac_find: - addr_find.return_value = address_mod + addr_realloc.return_value = True + addr_realloc_find.return_value = address_mod else: - addr_find.side_effect = [None, None] + addr_realloc.return_value = False + addr_realloc_find.side_effect = [None, None] mac_range_count.return_value = (range_mod, 0) mac_create.return_value = address_mod range_find.return_value = range_mod @@ -138,71 +141,27 @@ class QuarkMacAddressAllocateDeallocated(QuarkIpamBaseTest): refresh.side_effect = refresh_mock set_full.side_effect = set_full_mock - yield mac_update, mac_create, mac_delete, mac_auto_assign + yield addr_realloc_find, mac_create, mac_delete, mac_auto_assign def test_allocate_mac_address_find_deallocated(self): - with self._stubs(True) as (mac_update, mac_create, mac_delete, + with self._stubs(True) as (addr_realloc_find, mac_create, mac_delete, mac_auto_assign): self.ipam.allocate_mac_address(self.context, 0, 0, 0) - self.assertTrue(mac_update.called) + self.assertTrue(addr_realloc_find.called) self.assertFalse(mac_create.called) self.assertFalse(mac_delete.called) self.assertFalse(mac_auto_assign.called) def test_allocate_mac_address_creates_new_mac(self): - with self._stubs(False) as (mac_update, mac_create, mac_delete, + with self._stubs(False) as (addr_realloc_find, mac_create, mac_delete, mac_auto_assign): self.ipam.allocate_mac_address(self.context, 0, 0, 0) - self.assertFalse(mac_update.called) + self.assertFalse(addr_realloc_find.called) self.assertTrue(mac_create.called) self.assertFalse(mac_delete.called) self.assertTrue(mac_auto_assign.called) -class QuarkMacAddressAllocateDeallocatedDoNotUse(QuarkIpamBaseTest): - @contextlib.contextmanager - def _stubs(self, address, address2, mac_range, mac_range2): - - with contextlib.nested( - mock.patch("quark.db.api.mac_address_find"), - mock.patch("quark.db.api." - "mac_address_range_find_allocation_counts"), - mock.patch("quark.db.api.mac_address_update"), - mock.patch("quark.db.api.mac_address_create"), - mock.patch("quark.db.api.mac_address_range_find"), - mock.patch("quark.db.api.mac_address_delete") - ) as (addr_find, mac_range_count, mac_update, mac_create, - range_find, mac_delete): - address_mod = models.MacAddress(**address) - address_mod2 = models.MacAddress(**address2) - range_mod = models.MacAddressRange(**mac_range) - range_mod2 = models.MacAddressRange(**mac_range2) - - addr_find.side_effect = [address_mod, address_mod2] - mac_range_count.return_value = (range_mod, 0) - mac_create.return_value = address_mod - range_find.side_effect = [range_mod, range_mod2] - mac_update.return_value = address2 - yield mac_update, mac_create, mac_delete - - def test_allocate_deallocated_do_not_use_range_deletes(self): - address = dict(address=0) - mac_range = dict(id=1, first_address=0, last_address=255, - next_auto_assign_mac=0, do_not_use=True, - cidr="AA:BB:CC/24") - address2 = dict(address=1) - mac_range2 = dict(id=1, first_address=0, last_address=255, - next_auto_assign_mac=0, do_not_use=False, - cidr="AA:BB:CC/24") - with self._stubs(address, address2, mac_range, mac_range2) as ( - mac_update, mac_create, mac_delete): - addr = self.ipam.allocate_mac_address(self.context, 0, 0, 0) - self.assertTrue(mac_update.called) - self.assertFalse(mac_create.called) - self.assertTrue(mac_delete.called) - self.assertEqual(addr["address"], 1) - - class QuarkNewMacAddressAllocation(QuarkIpamBaseTest): @contextlib.contextmanager def _stubs(self, addresses=None, ranges=None): @@ -330,26 +289,26 @@ class QuarkNewMacAddressReallocationDeadlocks(QuarkIpamBaseTest): old_override = cfg.CONF.QUARK.mac_address_retry_max cfg.CONF.set_override('mac_address_retry_max', 1, 'QUARK') with contextlib.nested( - mock.patch("quark.db.api.mac_address_find"), + mock.patch("quark.db.api.mac_address_reallocate"), mock.patch("quark.db.api.mac_address_create"), mock.patch("quark.db.api." "mac_address_range_find_allocation_counts"), - ) as (mac_find, mac_create, mac_range_count): - mac_find.side_effect = [Exception, None] + ) as (mac_realloc, mac_create, mac_range_count): + mac_realloc.side_effect = [Exception, None] mac_create.side_effect = addresses mac_range_count.return_value = ranges - yield mac_find + yield mac_realloc cfg.CONF.set_override('mac_address_retry_max', old_override, 'QUARK') - def test_reallocate_mac_deadlock_raises_retry(self): + def test_reallocate_mac_exception_raises_retry(self): mar = dict(id=1, first_address=0, last_address=255, next_auto_assign_mac=0) mac = dict(id=1, address=254) with self._stubs(ranges=(mar, 0), addresses=[Exception, mac]) as ( - mac_find): + mac_realloc): with self.assertRaises(exceptions.MacAddressGenerationFailure): self.ipam.allocate_mac_address(self.context, 0, 0, 0) - self.assertEqual(mac_find.call_count, 1) + self.assertEqual(mac_realloc.call_count, 1) class QuarkMacAddressDeallocation(QuarkIpamBaseTest): @@ -368,7 +327,8 @@ class QuarkMacAddressDeallocation(QuarkIpamBaseTest): def test_deallocate_mac(self): mac_range = dict(id=2, do_not_use=False) - mac = dict(id=1, address=1, mac_address_range_id=mac_range["id"]) + mac = dict(id=1, address=1, mac_address_range_id=mac_range["id"], + mac_address_range=mac_range) with self._stubs(mac=mac, mac_range=mac_range) as (mac_update, mac_delete): self.ipam.deallocate_mac_address(self.context, mac["address"]) @@ -376,7 +336,8 @@ class QuarkMacAddressDeallocation(QuarkIpamBaseTest): def test_deallocate_mac_do_not_use_range_deletes_mac(self): mac_range = dict(id=2, do_not_use=True) - mac = dict(id=1, address=1, mac_address_range_id=mac_range["id"]) + mac = dict(id=1, address=1, mac_address_range_id=mac_range["id"], + mac_address_range=mac_range) with self._stubs(mac=mac, mac_range=mac_range) as (mac_update, mac_delete): self.ipam.deallocate_mac_address(self.context, mac["address"])