From 37b27576a11d394400de4a7376639b03e8399936 Mon Sep 17 00:00:00 2001 From: Matt Dietz Date: Thu, 15 Jan 2015 04:26:22 +0000 Subject: [PATCH] Implement do_not_use and Delete MACs if do_not_use RM11043 Implements use of the do_not_use column on mac_address_ranges. Additionally, sets a rule in place that if a MAC is deallocated in a range marked as do_not_use, it's deleted instead of deallocated. The reason for this is two fold: * We needed a fast fix to unblock other work being done with Neutron * Introducing the logic necessary to handle deallocating and not reusing MACs in the current implementation would also introduce a new chance for deadlocks as MAC address ranges would have to be included in all queries for MAC address reallocation. * I have an in-progress fix for removing the SELECT FOR UPDATE when reallocating MAC addresses, but there are flaws I have yet to work around. Rather than rush the fix, I decided to create a less than optimal path so we could move the ball forward. --- quark/db/api.py | 5 ++ quark/ipam.py | 25 ++++++- quark/tests/functional/db/test_api.py | 36 ++++++++++ quark/tests/test_ipam.py | 94 ++++++++++++++++++++++----- 4 files changed, 143 insertions(+), 17 deletions(-) diff --git a/quark/db/api.py b/quark/db/api.py index c0f14af..8c00a33 100644 --- a/quark/db/api.py +++ b/quark/db/api.py @@ -325,6 +325,10 @@ def mac_address_find(context, lock_mode=False, **filters): return query.filter(*model_filters) +def mac_address_delete(context, mac_address): + context.session.delete(mac_address) + + def mac_address_range_find_allocation_counts(context, address=None): count = sql_func.count(models.MacAddress.address) query = context.session.query(models.MacAddressRange, @@ -336,6 +340,7 @@ def mac_address_range_find_allocation_counts(context, address=None): query = query.filter(models.MacAddressRange.last_address >= address) query = query.filter(models.MacAddressRange.first_address <= address) query = query.filter(models.MacAddressRange.next_auto_assign_mac != -1) + query = query.filter(models.MacAddressRange.do_not_use == False) # noqa query = query.limit(1) return query.first() diff --git a/quark/ipam.py b/quark/ipam.py index 2cff00f..175471e 100644 --- a/quark/ipam.py +++ b/quark/ipam.py @@ -113,6 +113,18 @@ def generate_v6(mac, port_id, cidr): 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): @@ -136,6 +148,13 @@ class QuarkIpam(object): 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))) @@ -655,8 +674,10 @@ class QuarkIpam(object): if not mac: raise exceptions.NotFound( message="No MAC address %s found" % netaddr.EUI(address)) - db_api.mac_address_update(context, mac, deallocated=True, - deallocated_at=timeutils.utcnow()) + + if not self._delete_mac_if_do_not_use(context, mac): + db_api.mac_address_update(context, mac, deallocated=True, + deallocated_at=timeutils.utcnow()) # RM6180(roaet): # - removed session.begin due to deadlocks diff --git a/quark/tests/functional/db/test_api.py b/quark/tests/functional/db/test_api.py index 76273fd..0d02848 100644 --- a/quark/tests/functional/db/test_api.py +++ b/quark/tests/functional/db/test_api.py @@ -187,3 +187,39 @@ class QuarkFindSubnetAllocationCount(QuarkIpamBaseFunctionalTest): self.assertEqual( netaddr.IPAddress(subnet["next_auto_assign_ip"]).ipv4(), net4[1]) + + +class QuarkFindMacAddressRangeAllocationCount(QuarkIpamBaseFunctionalTest): + @contextlib.contextmanager + def _fixtures(self, mac_ranges): + self.ipam = quark.ipam.QuarkIpamANY() + for mar in mac_ranges: + with self.context.session.begin(): + db_api.mac_address_range_create(self.context, + **mar) + yield + + def test_mac_address_ranges(self): + mr1_mac = netaddr.EUI("AA:AA:AA:00:00:00") + mr1 = {"cidr": "AA:AA:AA/24", "do_not_use": False, + "first_address": mr1_mac.value, + "last_address": netaddr.EUI("AA:AA:AA:FF:FF:FF").value, + "next_auto_assign_mac": mr1_mac.value} + with self._fixtures([mr1]): + with self.context.session.begin(): + ranges = db_api.mac_address_range_find_allocation_counts( + self.context) + self.assertTrue(ranges[0]["cidr"], mr1["cidr"]) + + def test_mac_address_ranges_do_not_use_returns_nothing(self): + mr1_mac = netaddr.EUI("AA:AA:AA:00:00:00") + mr1 = {"cidr": "AA:AA:AA/24", "do_not_use": True, + "first_address": mr1_mac.value, + "last_address": netaddr.EUI("AA:AA:AA:FF:FF:FF").value, + "next_auto_assign_mac": mr1_mac.value} + + with self._fixtures([mr1]): + with self.context.session.begin(): + ranges = db_api.mac_address_range_find_allocation_counts( + self.context) + self.assertTrue(ranges is None) diff --git a/quark/tests/test_ipam.py b/quark/tests/test_ipam.py index 73c1d98..aecc0a2 100644 --- a/quark/tests/test_ipam.py +++ b/quark/tests/test_ipam.py @@ -99,17 +99,21 @@ class QuarkIpamBaseTest(test_base.TestBase): class QuarkMacAddressAllocateDeallocated(QuarkIpamBaseTest): @contextlib.contextmanager - def _stubs(self, mac_find=True): + def _stubs(self, mac_find=True, do_not_use=False): address = dict(address=0) mac_range = dict(id=1, first_address=0, last_address=255, - next_auto_assign_mac=0) + 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_range_find_allocation_counts"), mock.patch("quark.db.api.mac_address_update"), - mock.patch("quark.db.api.mac_address_create") - ) as (addr_find, mac_range_count, mac_update, mac_create): + 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) range_mod = models.MacAddressRange(**mac_range) if mac_find: @@ -118,19 +122,66 @@ class QuarkMacAddressAllocateDeallocated(QuarkIpamBaseTest): addr_find.side_effect = [None, None] mac_range_count.return_value = (range_mod, 0) mac_create.return_value = address_mod - yield mac_update, mac_create + range_find.return_value = range_mod + yield mac_update, mac_create, mac_delete def test_allocate_mac_address_find_deallocated(self): - with self._stubs(True) as (mac_update, mac_create): + with self._stubs(True) as (mac_update, mac_create, mac_delete): self.ipam.allocate_mac_address(self.context, 0, 0, 0) self.assertTrue(mac_update.called) self.assertFalse(mac_create.called) + self.assertFalse(mac_delete.called) def test_allocate_mac_address_creates_new_mac(self): - with self._stubs(False) as (mac_update, mac_create): + with self._stubs(False) as (mac_update, mac_create, mac_delete): self.ipam.allocate_mac_address(self.context, 0, 0, 0) self.assertFalse(mac_update.called) self.assertTrue(mac_create.called) + self.assertFalse(mac_delete.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): @@ -260,24 +311,37 @@ class QuarkNewMacAddressReallocationDeadlocks(QuarkIpamBaseTest): class QuarkMacAddressDeallocation(QuarkIpamBaseTest): @contextlib.contextmanager - def _stubs(self, mac): + def _stubs(self, mac, mac_range): with contextlib.nested( mock.patch("quark.db.api.mac_address_find"), - mock.patch("quark.db.api.mac_address_update") - ) as (mac_find, - mac_update): + mock.patch("quark.db.api.mac_address_update"), + mock.patch("quark.db.api.mac_address_range_find"), + mock.patch("quark.db.api.mac_address_delete") + ) as (mac_find, mac_update, range_find, mac_delete): mac_update.return_value = mac mac_find.return_value = mac - yield mac_update + range_find.return_value = mac_range + yield mac_update, mac_delete def test_deallocate_mac(self): - mac = dict(id=1, address=1) - with self._stubs(mac=mac) as mac_update: + mac_range = dict(id=2, do_not_use=False) + mac = dict(id=1, address=1, mac_address_range_id=mac_range["id"]) + with self._stubs(mac=mac, mac_range=mac_range) as (mac_update, + mac_delete): self.ipam.deallocate_mac_address(self.context, mac["address"]) self.assertTrue(mac_update.called) + 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"]) + with self._stubs(mac=mac, mac_range=mac_range) as (mac_update, + mac_delete): + self.ipam.deallocate_mac_address(self.context, mac["address"]) + self.assertFalse(mac_update.called) + self.assertTrue(mac_delete.called) + def test_deallocate_mac_mac_not_found_fails(self): - with self._stubs(mac=None) as mac_update: + with self._stubs(mac=None, mac_range=None) as (mac_update, mac_delete): self.assertRaises(exceptions.NotFound, self.ipam.deallocate_mac_address, self.context, 0)