From ed34f18916c62d77358d4c5910ff17694828576a Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Wed, 13 Jul 2016 16:06:06 -0400 Subject: [PATCH] DVR: Fix ItemAllocator class to handle exceptions The ItemAllocator class blindly reads from a file when initialized, assuming each line is always in "key,value" syntax. It's possible that the file may become corrupted, leading to ValueErrors being thrown which it doesn't handle. This was found running the unit tests, more specifically those doing DVR FIP namespace operations, since they were always reading/writing the same files in /tmp. Typically the failure was random and would go away with a recheck, but could be the cause of check job instability. All occurrences were fixed to use a test-created temporary directory to avoid collision with each other. Change-Id: I39d116aa8261b50bfcb3269416c1a307cafa134e Closes-bug: #1602794 --- neutron/agent/l3/item_allocator.py | 20 +++++++++++++++++-- neutron/tests/unit/agent/l3/test_agent.py | 6 ++---- .../tests/unit/agent/l3/test_dvr_fip_ns.py | 3 ++- .../unit/agent/l3/test_dvr_local_router.py | 5 ++--- .../unit/agent/l3/test_item_allocator.py | 15 ++++++++++++++ 5 files changed, 39 insertions(+), 10 deletions(-) diff --git a/neutron/agent/l3/item_allocator.py b/neutron/agent/l3/item_allocator.py index e7b96ab2860..b2c8d95e176 100644 --- a/neutron/agent/l3/item_allocator.py +++ b/neutron/agent/l3/item_allocator.py @@ -14,6 +14,12 @@ import os +from oslo_log import log as logging + +from neutron._i18n import _LW + +LOG = logging.getLogger(__name__) + class ItemAllocator(object): """Manages allocation of items from a pool @@ -42,11 +48,21 @@ class ItemAllocator(object): self.remembered = {} self.pool = item_pool + read_error = False for line in self._read(): - key, saved_value = line.strip().split(delimiter) - self.remembered[key] = self.ItemClass(saved_value) + try: + key, saved_value = line.strip().split(delimiter) + self.remembered[key] = self.ItemClass(saved_value) + except ValueError: + read_error = True + LOG.warning(_LW("Invalid line in %(file)s, " + "ignoring: %(line)s"), + {'file': state_file, 'line': line}) self.pool.difference_update(self.remembered.values()) + if read_error: + LOG.debug("Re-writing file %s due to read error", state_file) + self._write_allocations() def allocate(self, key): """Try to allocate an item of ItemClass type. diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 6fda2ead05a..eaa8dceea27 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -22,6 +22,7 @@ import mock import netaddr from neutron_lib import constants as l3_constants from neutron_lib import exceptions as exc +from oslo_config import cfg from oslo_log import log import oslo_messaging from oslo_utils import timeutils @@ -83,8 +84,7 @@ class BasicRouterOperationsFramework(base.BaseTestCase): self.conf.set_override('interface_driver', 'neutron.agent.linux.interface.NullDriver') self.conf.set_override('send_arp_for_ha', 1) - self.conf.set_override('state_path', '/tmp') - self.conf.set_override('ra_confs', '/tmp') + self.conf.set_override('state_path', cfg.CONF.state_path) self.conf.set_override('pd_dhcp_driver', '') self.device_exists_p = mock.patch( @@ -2233,8 +2233,6 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): self.assertEqual(tuple(), agent.neutron_service_plugins) def test_external_gateway_removed_ext_gw_port_no_fip_ns(self): - self.conf.set_override('state_path', '/tmp') - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) agent.conf.agent_mode = 'dvr_snat' router = l3_test_common.prepare_router_data(num_internal_ports=2) 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 089b3c93e6f..47be9f7f38f 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py @@ -14,6 +14,7 @@ import copy import mock +from oslo_config import cfg from oslo_utils import uuidutils from neutron.agent.common import utils @@ -30,7 +31,7 @@ class TestDvrFipNs(base.BaseTestCase): def setUp(self): super(TestDvrFipNs, self).setUp() self.conf = mock.Mock() - self.conf.state_path = '/tmp' + self.conf.state_path = cfg.CONF.state_path self.driver = mock.Mock() self.driver.DEV_NAME_LEN = 14 self.net_id = _uuid() diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py index f8f67016a2e..332fb657921 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py +++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py @@ -15,6 +15,7 @@ import mock import netaddr from neutron_lib import constants as l3_constants +from oslo_config import cfg from oslo_log import log from oslo_utils import uuidutils @@ -58,7 +59,7 @@ class TestDvrRouterOperations(base.BaseTestCase): self.conf.set_override('interface_driver', 'neutron.agent.linux.interface.NullDriver') self.conf.set_override('send_arp_for_ha', 1) - self.conf.set_override('state_path', '') + self.conf.set_override('state_path', cfg.CONF.state_path) self.device_exists_p = mock.patch( 'neutron.agent.linux.ip_lib.device_exists') @@ -600,8 +601,6 @@ class TestDvrRouterOperations(base.BaseTestCase): 'dvr', 0) def test_external_gateway_removed_ext_gw_port_and_fip(self): - self.conf.set_override('state_path', '/tmp') - agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) agent.conf.agent_mode = 'dvr' router = l3_test_common.prepare_router_data(num_internal_ports=2) diff --git a/neutron/tests/unit/agent/l3/test_item_allocator.py b/neutron/tests/unit/agent/l3/test_item_allocator.py index f920c6bfbb5..d350e458e1c 100644 --- a/neutron/tests/unit/agent/l3/test_item_allocator.py +++ b/neutron/tests/unit/agent/l3/test_item_allocator.py @@ -51,6 +51,21 @@ class TestItemAllocator(base.BaseTestCase): self.assertIn('da873ca2', a.remembered) self.assertEqual({}, a.allocations) + def test__init__readfile_error(self): + test_pool = set(TestObject(s) for s in range(32768, 40000)) + with mock.patch.object(ia.ItemAllocator, '_read') as read,\ + mock.patch.object(ia.ItemAllocator, '_write') as write: + read.return_value = ["da873ca2,10\n", + "corrupt_entry_no_delimiter\n", + "42c9daf7,11\n"] + a = ia.ItemAllocator('/file', TestObject, test_pool) + + self.assertIn('da873ca2', a.remembered) + self.assertIn('42c9daf7', a.remembered) + self.assertNotIn('corrupt_entry_no_delimiter', a.remembered) + self.assertEqual({}, a.allocations) + self.assertTrue(write.called) + def test_allocate(self): test_pool = set([TestObject(33000), TestObject(33001)]) a = ia.ItemAllocator('/file', TestObject, test_pool)