x/quark
Code Issues Proposed changes

Compare-and-swap: MAC address reallocations

Browse Source 
* Removed ORDER BY clause since SQLAlchemy doesn't support ORDER BY
on UPDATEs.
* Added migration for addding transaction_id column to quark_mac_addresses.

RM11013
This commit is contained in:
Amir Sadoughi
2015-04-14 01:50:10 -05:00
parent b54b815359
commit c82fa361b8
7 changed files with 157 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
quark/db/api.py
View File

@@ -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)

30
quark/db/migration/alembic/versions/356d6c0623c8_add_transaction_id_to_quark_mac_.py Normal file
View File

@@ -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')

2
quark/db/migration/alembic/versions/HEAD
View File

@@ -1 +1 @@
5632aa202d89
356d6c0623c8

3
quark/db/models.py
View File

@@ -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):

65
quark/ipam.py
View File

@@ -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())

38
quark/tests/functional/mysql/test_ipam.py
View File

@@ -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)

89
quark/tests/test_ipam.py
View File

@@ -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"])