From de81ab8385e8490b2320e23ee7dd86b43e22fd32 Mon Sep 17 00:00:00 2001 From: Adolfo Duarte Date: Thu, 18 Jun 2015 19:50:13 -0700 Subject: [PATCH] Preserve DVR FIP rule priority over Agent restarts IP rule priorities assigned to DVR floating IPs need to be preserved over L3 agent restarts. Reuse the ItemAllocator class decomposed from Link Local IP address allocation. Also move commn unit tests to ItemAllocator class. Closes-Bug: #1414779 Change-Id: I6a75aa8ad612ee80b391f0a27a8a7e29519c3f8d Co-Authored-By: Rajeev Grover Co-Authored-By: Ryan Moats --- neutron/agent/l3/dvr_fip_ns.py | 21 +++-- neutron/agent/l3/dvr_local_router.py | 4 +- .../agent/l3/fip_rule_priority_allocator.py | 53 ++++++++++++ .../tests/functional/agent/test_l3_agent.py | 25 ++++++ .../tests/unit/agent/l3/test_dvr_fip_ns.py | 14 +-- .../l3/test_fip_rule_priority_allocator.py | 61 +++++++++++++ .../unit/agent/l3/test_item_allocator.py | 85 +++++++++++++++++-- .../agent/l3/test_link_local_allocator.py | 67 --------------- 8 files changed, 244 insertions(+), 86 deletions(-) create mode 100644 neutron/agent/l3/fip_rule_priority_allocator.py create mode 100644 neutron/tests/unit/agent/l3/test_fip_rule_priority_allocator.py diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 90e24d129d9..839ee87ff99 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -14,13 +14,13 @@ import os -from oslo_log import log as logging - +from neutron.agent.l3 import fip_rule_priority_allocator as frpa from neutron.agent.l3 import link_local_allocator as lla from neutron.agent.l3 import namespaces from neutron.agent.linux import ip_lib from neutron.agent.linux import iptables_manager from neutron.common import utils as common_utils +from oslo_log import log as logging LOG = logging.getLogger(__name__) @@ -49,7 +49,10 @@ class FipNamespace(namespaces.Namespace): self.use_ipv6 = use_ipv6 self.agent_gateway_port = None self._subscribers = set() - self._rule_priorities = set(range(FIP_PR_START, FIP_PR_END)) + path = os.path.join(agent_conf.state_path, 'fip-priorities') + self._rule_priorities = frpa.FipRulePriorityAllocator(path, + FIP_PR_START, + FIP_PR_END) self._iptables_manager = iptables_manager.IptablesManager( namespace=self.get_name(), use_ipv6=self.use_ipv6) @@ -85,11 +88,11 @@ class FipNamespace(namespaces.Namespace): self._subscribers.discard(router_id) return not self.has_subscribers() - def allocate_rule_priority(self): - return self._rule_priorities.pop() + def allocate_rule_priority(self, floating_ip): + return self._rule_priorities.allocate(floating_ip) - def deallocate_rule_priority(self, rule_pr): - self._rule_priorities.add(rule_pr) + def deallocate_rule_priority(self, floating_ip): + self._rule_priorities.release(floating_ip) def _gateway_added(self, ex_gw_port, interface_name): """Add Floating IP gateway port.""" @@ -232,4 +235,8 @@ class FipNamespace(namespaces.Namespace): existing_cidrs = [addr['cidr'] for addr in device.addr.list()] fip_cidrs = [c for c in existing_cidrs if common_utils.is_cidr_host(c)] + for fip_cidr in fip_cidrs: + fip_ip = fip_cidr.split('/')[0] + rule_pr = self._rule_priorities.allocate(fip_ip) + ri.floating_ips_dict[fip_ip] = rule_pr ri.dist_fip_count = len(fip_cidrs) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index e14fc2d172a..ce899f29ca3 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -74,7 +74,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): """Add floating IP to FIP namespace.""" floating_ip = fip['floating_ip_address'] fixed_ip = fip['fixed_ip_address'] - rule_pr = self.fip_ns.allocate_rule_priority() + rule_pr = self.fip_ns.allocate_rule_priority(floating_ip) self.floating_ips_dict[floating_ip] = rule_pr fip_2_rtr_name = self.fip_ns.get_int_device_name(self.router_id) ip_rule = ip_lib.IPRule(namespace=self.ns_name) @@ -113,7 +113,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): ip_rule.rule.delete(ip=floating_ip, table=dvr_fip_ns.FIP_RT_TBL, priority=rule_pr) - self.fip_ns.deallocate_rule_priority(rule_pr) + self.fip_ns.deallocate_rule_priority(floating_ip) #TODO(rajeev): Handle else case - exception/log? device = ip_lib.IPDevice(fip_2_rtr_name, namespace=fip_ns_name) diff --git a/neutron/agent/l3/fip_rule_priority_allocator.py b/neutron/agent/l3/fip_rule_priority_allocator.py new file mode 100644 index 00000000000..016f12cd317 --- /dev/null +++ b/neutron/agent/l3/fip_rule_priority_allocator.py @@ -0,0 +1,53 @@ +# Copyright 2015 IBM Corporation +# +# 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. + +from neutron.agent.l3.item_allocator import ItemAllocator + + +class FipPriority(object): + def __init__(self, index): + self.index = index + + def __repr__(self): + return str(self.index) + + def __hash__(self): + return hash(self.__repr__()) + + def __eq__(self, other): + if isinstance(other, FipPriority): + return (self.index == other.index) + else: + return False + + +class FipRulePriorityAllocator(ItemAllocator): + """Manages allocation of floating ips rule priorities. + IP rule priorities assigned to DVR floating IPs need + to be preserved over L3 agent restarts. + This class provides an allocator which saves the prirorities + to a datastore which will survive L3 agent restarts. + """ + def __init__(self, data_store_path, priority_rule_start, + priority_rule_end): + """Create the necessary pool and create the item allocator + using ',' as the delimiter and FipRulePriorityAllocator as the + class type + """ + pool = set(FipPriority(str(s)) for s in range(priority_rule_start, + priority_rule_end)) + + super(FipRulePriorityAllocator, self).__init__(data_store_path, + FipPriority, + pool) diff --git a/neutron/tests/functional/agent/test_l3_agent.py b/neutron/tests/functional/agent/test_l3_agent.py index 18b8c3347e2..986da2c1a06 100644 --- a/neutron/tests/functional/agent/test_l3_agent.py +++ b/neutron/tests/functional/agent/test_l3_agent.py @@ -1285,6 +1285,31 @@ class TestDvrRouter(L3AgentTestFramework): self._assert_dvr_snat_gateway(router1) self.assertFalse(self._namespace_exists(fip_ns)) + def test_dvr_router_add_fips_on_restarted_agent(self): + self.agent.conf.agent_mode = 'dvr' + router_info = self.generate_dvr_router_info() + router = self.manage_router(self.agent, router_info) + floating_ips = router.router[l3_constants.FLOATINGIP_KEY] + router_ns = router.ns_name + fip_rule_prio_1 = self._get_fixed_ip_rule_priority( + router_ns, floating_ips[0]['fixed_ip_address']) + restarted_agent = neutron_l3_agent.L3NATAgent( + self.agent.host, self.agent.conf) + floating_ips[0]['floating_ip_address'] = '21.4.4.2' + floating_ips[0]['fixed_ip_address'] = '10.0.0.2' + self.manage_router(restarted_agent, router_info) + fip_rule_prio_2 = self._get_fixed_ip_rule_priority( + router_ns, floating_ips[0]['fixed_ip_address']) + self.assertNotEqual(fip_rule_prio_1, fip_rule_prio_2) + + def _get_fixed_ip_rule_priority(self, namespace, fip): + iprule = ip_lib.IPRule(namespace) + lines = iprule.rule._as_root([4], ['show']).splitlines() + for line in lines: + if fip in line: + info = iprule.rule._parse_line(4, line) + return info['priority'] + def test_dvr_router_add_internal_network_set_arp_cache(self): # Check that, when the router is set up and there are # existing ports on the the uplinked subnet, the ARP diff --git a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py index db3423f6c3e..951149f39fd 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py @@ -57,13 +57,15 @@ class TestDvrFipNs(base.BaseTestCase): self.assertFalse(is_last) def test_allocate_rule_priority(self): - pr = self.fip_ns.allocate_rule_priority() - self.assertNotIn(pr, self.fip_ns._rule_priorities) + pr = self.fip_ns.allocate_rule_priority('20.0.0.30') + self.assertIn('20.0.0.30', self.fip_ns._rule_priorities.allocations) + self.assertNotIn(pr, self.fip_ns._rule_priorities.pool) def test_deallocate_rule_priority(self): - pr = self.fip_ns.allocate_rule_priority() - self.fip_ns.deallocate_rule_priority(pr) - self.assertIn(pr, self.fip_ns._rule_priorities) + pr = self.fip_ns.allocate_rule_priority('20.0.0.30') + self.fip_ns.deallocate_rule_priority('20.0.0.30') + self.assertNotIn('20.0.0.30', self.fip_ns._rule_priorities.allocations) + self.assertIn(pr, self.fip_ns._rule_priorities.pool) @mock.patch.object(ip_lib, 'IPWrapper') @mock.patch.object(ip_lib, 'IPDevice') @@ -179,6 +181,7 @@ class TestDvrFipNs(base.BaseTestCase): device_exists.return_value = True ri = mock.Mock() ri.dist_fip_count = None + ri.floating_ips_dict = {} ip_list = [{'cidr': '111.2.3.4/32'}, {'cidr': '111.2.3.5/32'}] self._test_scan_fip_ports(ri, ip_list) self.assertEqual(2, ri.dist_fip_count) @@ -188,6 +191,7 @@ class TestDvrFipNs(base.BaseTestCase): device_exists.return_value = True ri = mock.Mock() ri.dist_fip_count = None + ri.floating_ips_dict = {} self._test_scan_fip_ports(ri, []) self.assertEqual(0, ri.dist_fip_count) diff --git a/neutron/tests/unit/agent/l3/test_fip_rule_priority_allocator.py b/neutron/tests/unit/agent/l3/test_fip_rule_priority_allocator.py new file mode 100644 index 00000000000..b7d606d5865 --- /dev/null +++ b/neutron/tests/unit/agent/l3/test_fip_rule_priority_allocator.py @@ -0,0 +1,61 @@ +# Copyright 2014 Hewlett-Packard Development Company, L.P. +# +# 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. + +from neutron.agent.l3 import fip_rule_priority_allocator as frpa +from neutron.tests import base + + +class TestFipPriority(base.BaseTestCase): + def setUp(self): + super(TestFipPriority, self).setUp() + + def test__init__(self): + test_pr = frpa.FipPriority(10) + self.assertEqual(10, test_pr.index) + + def test__repr__(self): + test_pr = frpa.FipPriority(20) + self.assertEqual("20", str(test_pr)) + + def test__eq__(self): + left_pr = frpa.FipPriority(10) + right_pr = frpa.FipPriority(10) + other_pr = frpa.FipPriority(20) + self.assertEqual(left_pr, right_pr) + self.assertNotEqual(left_pr, other_pr) + self.assertNotEqual(right_pr, other_pr) + + def test__hash__(self): + left_pr = frpa.FipPriority(10) + right_pr = frpa.FipPriority(10) + other_pr = frpa.FipPriority(20) + self.assertEqual(hash(left_pr), hash(right_pr)) + self.assertNotEqual(hash(left_pr), hash(other_pr)) + self.assertNotEqual(hash(other_pr), hash(right_pr)) + + +class TestFipRulePriorityAllocator(base.BaseTestCase): + def setUp(self): + super(TestFipRulePriorityAllocator, self).setUp() + self.priority_rule_start = 100 + self.priority_rule_end = 200 + self.data_store_path = '/data_store_path_test' + + def test__init__(self): + _frpa = frpa.FipRulePriorityAllocator(self.data_store_path, + self.priority_rule_start, + self.priority_rule_end) + self.assertEqual(self.data_store_path, _frpa.state_file) + self.assertEqual(frpa.FipPriority, _frpa.ItemClass) + self.assertEqual(100, len(_frpa.pool)) diff --git a/neutron/tests/unit/agent/l3/test_item_allocator.py b/neutron/tests/unit/agent/l3/test_item_allocator.py index 767ad8d5c52..c1142bbc449 100644 --- a/neutron/tests/unit/agent/l3/test_item_allocator.py +++ b/neutron/tests/unit/agent/l3/test_item_allocator.py @@ -12,18 +12,93 @@ # License for the specific language governing permissions and limitations # under the License. +import mock + from neutron.agent.l3 import item_allocator as ia from neutron.tests import base +class TestObject(object): + def __init__(self, value): + super(TestObject, self).__init__() + self._value = value + + def __str__(self): + return str(self._value) + + class TestItemAllocator(base.BaseTestCase): def setUp(self): super(TestItemAllocator, self).setUp() def test__init__(self): - test_pool = set(s for s in range(32768, 40000)) - a = ia.ItemAllocator('/file', object, test_pool) - self.assertEqual('/file', a.state_file) + test_pool = set(TestObject(s) for s in range(32768, 40000)) + with mock.patch.object(ia.ItemAllocator, '_write') as write: + a = ia.ItemAllocator('/file', TestObject, test_pool) + test_object = a.allocate('test') + + self.assertTrue('test' in a.allocations) + self.assertTrue(test_object in a.allocations.values()) + self.assertTrue(test_object not in a.pool) + self.assertTrue(write.called) + + def test__init__readfile(self): + test_pool = set(TestObject(s) for s in range(32768, 40000)) + with mock.patch.object(ia.ItemAllocator, '_read') as read: + read.return_value = ["da873ca2,10\n"] + a = ia.ItemAllocator('/file', TestObject, test_pool) + + self.assertTrue('da873ca2' in a.remembered) self.assertEqual({}, a.allocations) - self.assertEqual(object, a.ItemClass) - self.assertEqual(test_pool, a.pool) + + def test_allocate(self): + test_pool = set([TestObject(33000), TestObject(33001)]) + a = ia.ItemAllocator('/file', TestObject, test_pool) + with mock.patch.object(ia.ItemAllocator, '_write') as write: + test_object = a.allocate('test') + + self.assertTrue('test' in a.allocations) + self.assertTrue(test_object in a.allocations.values()) + self.assertTrue(test_object not in a.pool) + self.assertTrue(write.called) + + def test_allocate_from_file(self): + test_pool = set([TestObject(33000), TestObject(33001)]) + with mock.patch.object(ia.ItemAllocator, '_read') as read: + read.return_value = ["deadbeef,33000\n"] + a = ia.ItemAllocator('/file', TestObject, test_pool) + + with mock.patch.object(ia.ItemAllocator, '_write') as write: + t_obj = a.allocate('deadbeef') + + self.assertEqual('33000', t_obj._value) + self.assertTrue('deadbeef' in a.allocations) + self.assertTrue(t_obj in a.allocations.values()) + self.assertTrue(33000 not in a.pool) + self.assertFalse(write.called) + + def test_allocate_exhausted_pool(self): + test_pool = set([TestObject(33000)]) + with mock.patch.object(ia.ItemAllocator, '_read') as read: + read.return_value = ["deadbeef,33000\n"] + a = ia.ItemAllocator('/file', TestObject, test_pool) + + with mock.patch.object(ia.ItemAllocator, '_write') as write: + allocation = a.allocate('abcdef12') + + self.assertFalse('deadbeef' in a.allocations) + self.assertTrue(allocation not in a.pool) + self.assertTrue(write.called) + + def test_release(self): + test_pool = set([TestObject(33000), TestObject(33001)]) + with mock.patch.object(ia.ItemAllocator, '_write') as write: + a = ia.ItemAllocator('/file', TestObject, test_pool) + allocation = a.allocate('deadbeef') + write.reset_mock() + a.release('deadbeef') + + self.assertTrue('deadbeef' not in a.allocations) + self.assertTrue(allocation in a.pool) + self.assertEqual({}, a.allocations) + write.assert_called_once_with([]) diff --git a/neutron/tests/unit/agent/l3/test_link_local_allocator.py b/neutron/tests/unit/agent/l3/test_link_local_allocator.py index 89ad856f1be..e33b6769d97 100644 --- a/neutron/tests/unit/agent/l3/test_link_local_allocator.py +++ b/neutron/tests/unit/agent/l3/test_link_local_allocator.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import mock import netaddr from neutron.agent.l3 import link_local_allocator as lla @@ -28,69 +27,3 @@ class TestLinkLocalAddrAllocator(base.BaseTestCase): a = lla.LinkLocalAllocator('/file', self.subnet.cidr) self.assertEqual('/file', a.state_file) self.assertEqual({}, a.allocations) - - def test__init__readfile(self): - with mock.patch.object(lla.LinkLocalAllocator, '_read') as read: - read.return_value = ["da873ca2,169.254.31.28/31\n"] - a = lla.LinkLocalAllocator('/file', self.subnet.cidr) - - self.assertTrue('da873ca2' in a.remembered) - self.assertEqual({}, a.allocations) - - def test_allocate(self): - a = lla.LinkLocalAllocator('/file', self.subnet.cidr) - with mock.patch.object(lla.LinkLocalAllocator, '_write') as write: - subnet = a.allocate('deadbeef') - - self.assertTrue('deadbeef' in a.allocations) - self.assertTrue(subnet not in a.pool) - self._check_allocations(a.allocations) - write.assert_called_once_with(['deadbeef,%s\n' % subnet.cidr]) - - def test_allocate_from_file(self): - with mock.patch.object(lla.LinkLocalAllocator, '_read') as read: - read.return_value = ["deadbeef,169.254.31.88/31\n"] - a = lla.LinkLocalAllocator('/file', self.subnet.cidr) - - with mock.patch.object(lla.LinkLocalAllocator, '_write') as write: - subnet = a.allocate('deadbeef') - - self.assertEqual(netaddr.IPNetwork('169.254.31.88/31'), subnet) - self.assertTrue(subnet not in a.pool) - self._check_allocations(a.allocations) - self.assertFalse(write.called) - - def test_allocate_exhausted_pool(self): - subnet = netaddr.IPNetwork('169.254.31.0/31') - with mock.patch.object(lla.LinkLocalAllocator, '_read') as read: - read.return_value = ["deadbeef,169.254.31.0/31\n"] - a = lla.LinkLocalAllocator('/file', subnet.cidr) - - with mock.patch.object(lla.LinkLocalAllocator, '_write') as write: - allocation = a.allocate('abcdef12') - - self.assertEqual(subnet, allocation) - self.assertFalse('deadbeef' in a.allocations) - self.assertTrue('abcdef12' in a.allocations) - self.assertTrue(allocation not in a.pool) - self._check_allocations(a.allocations) - write.assert_called_once_with(['abcdef12,%s\n' % allocation.cidr]) - - self.assertRaises(RuntimeError, a.allocate, 'deadbeef') - - def test_release(self): - with mock.patch.object(lla.LinkLocalAllocator, '_write') as write: - a = lla.LinkLocalAllocator('/file', self.subnet.cidr) - subnet = a.allocate('deadbeef') - write.reset_mock() - a.release('deadbeef') - - self.assertTrue('deadbeef' not in a.allocations) - self.assertTrue(subnet in a.pool) - self.assertEqual({}, a.allocations) - write.assert_called_once_with([]) - - def _check_allocations(self, allocations): - for key, subnet in allocations.items(): - self.assertTrue(subnet in self.subnet) - self.assertEqual(subnet.prefixlen, 31)