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

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

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)

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

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)

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)