Merge lp:~lifeless/python-oops-datedir-repo/bug-971255 into lp:python-oops-datedir-repo

[Robert Collins]

UniqueFileAllocator lead to lots of race-condition related pain, we've had ages now for everyone to migrate, and the previous release is entirely usable for folk that might not be ready yet. Wake up, time to die.

[Steve Kowalik (stevenk)]

review: Approve (code)

=== modified file 'NEWS'
--- NEWS	2012-09-19 02:21:51 +0000
+++ NEWS	2012-09-26 06:41:19 +0000
@@ -6,6 +6,11 @@
 NEXT
 ----
 
+* The legacy uniquefileallocator code path has been dropped, fixing
+  the bugs associated with its flawed design. As a result the 
+  instance_id parameter to DateDirRepo.__init__ has been dropped.
+  (Robert Collins, #971255)
+
 0.0.19
 ------
 

=== modified file 'README'
--- README	2011-11-11 04:48:53 +0000
+++ README	2012-09-26 06:41:19 +0000
@@ -53,14 +53,11 @@
 functions : an OOPS report can be written to a disk file via the
 serializer_rfc822.write() function, and read via the matching read() function.
 
-The uniquefileallocator module is used by the repository implementation and
-provides a system for allocating file names on disk.
-
 Typical usage::
 
   >>> config = oops.Config()
   >>> with fixtures.TempDir() as tempdir:
-  ...    repo = oops_datedir_repo.DateDirRepo('/tmp/demo', 'servername')
+  ...    repo = oops_datedir_repo.DateDirRepo('/tmp/demo')
   ...    config.publishers.append(repo.publish)
   ...    ids = config.publish({'oops': '!!!'})
 

=== modified file 'oops_datedir_repo/repository.py'
--- oops_datedir_repo/repository.py	2012-09-03 23:13:20 +0000
+++ oops_datedir_repo/repository.py	2012-09-26 06:41:19 +0000
@@ -34,7 +34,6 @@
 import anybson as bson
 import serializer
 import serializer_bson
-from uniquefileallocator import UniqueFileAllocator
 
 
 class DateDirRepo:
@@ -50,21 +49,20 @@
       more OOPS reports. OOPS file names can take various forms, but must not
       end in .tmp - those are considered to be OOPS reports that are currently
       being written.
+
+    * The behaviour of this class is to assign OOPS file names by hashing the
+      serialized OOPS to get a unique file name. Other naming schemes are
+      valid - the code doesn't assume anything other than the .tmp limitation
+      above.
     """
 
-    def __init__(self, error_dir, instance_id=None, serializer=None,
-            inherit_id=False, stash_path=False):
+    def __init__(self, error_dir, serializer=None, inherit_id=False,
+        stash_path=False):
         """Create a DateDirRepo.
 
         :param error_dir: The base directory to write OOPSes into. OOPSes are
             written into a subdirectory this named after the date (e.g.
             2011-12-30).
-        :param instance_id: If None, OOPS file names are named after the OOPS
-            id which is generated by hashing the serialized OOPS (without the
-            id field). Otherwise OOPS file names and ids are created by
-            allocating file names through a UniqueFileAllocator.
-            UniqueFileAllocator has significant performance and concurrency
-            limits and hash based naming is recommended.
         :param serializer: If supplied should be the module (e.g.
             oops_datedir_repo.serializer_rfc822) to use to serialize OOPSes.
             Defaults to using serializer_bson.
@@ -75,14 +73,6 @@
             It is not stored in the OOPS written to disk, only the in-memory
             model.
         """
-        if instance_id is not None:
-            self.log_namer = UniqueFileAllocator(
-                output_root=error_dir,
-                log_type="OOPS",
-                log_subtype=instance_id,
-                )
-        else:
-            self.log_namer = None
         self.root = error_dir
         if serializer is None:
             serializer = serializer_bson
@@ -114,19 +104,16 @@
         # Don't mess with the original report when changing ids etc.
         original_report = report
         report = dict(report)
-        if self.log_namer is not None:
-            oopsid, filename = self.log_namer.newId(now)
-        else:
-            md5hash = md5(serializer_bson.dumps(report)).hexdigest()
-            oopsid = 'OOPS-%s' % md5hash
-            prefix = os.path.join(self.root, now.strftime('%Y-%m-%d'))
-            if not os.path.isdir(prefix):
-                os.makedirs(prefix)
-                # For directories we need to set the x bits too.
-                os.chmod(
-                    prefix, wanted_file_permission | stat.S_IXUSR | stat.S_IXGRP |
-                    stat.S_IXOTH)
-            filename = os.path.join(prefix, oopsid)
+        md5hash = md5(serializer_bson.dumps(report)).hexdigest()
+        oopsid = 'OOPS-%s' % md5hash
+        prefix = os.path.join(self.root, now.strftime('%Y-%m-%d'))
+        if not os.path.isdir(prefix):
+            os.makedirs(prefix)
+            # For directories we need to set the x bits too.
+            os.chmod(
+                prefix, wanted_file_permission | stat.S_IXUSR | stat.S_IXGRP |
+                stat.S_IXOTH)
+        filename = os.path.join(prefix, oopsid)
         if self.inherit_id:
             oopsid = report.get('id') or oopsid
         report['id'] = oopsid

=== modified file 'oops_datedir_repo/tests/__init__.py'
--- oops_datedir_repo/tests/__init__.py	2011-11-11 04:21:05 +0000
+++ oops_datedir_repo/tests/__init__.py	2012-09-26 06:41:19 +0000
@@ -21,7 +21,6 @@
 def test_suite():
     test_mod_names = [
         'repository',
-        'uniquefileallocator',
         'serializer',
         'serializer_bson',
         'serializer_rfc822',

=== modified file 'oops_datedir_repo/tests/test_repository.py'
--- oops_datedir_repo/tests/test_repository.py	2012-09-03 21:04:20 +0000
+++ oops_datedir_repo/tests/test_repository.py	2012-09-26 06:41:19 +0000
@@ -38,7 +38,6 @@
     DateDirRepo,
     serializer_bson,
     )
-from oops_datedir_repo.uniquefileallocator import UniqueFileAllocator
 
 
 class HasUnixPermissions:
@@ -73,27 +72,6 @@
 
 class TestDateDirRepo(testtools.TestCase):
 
-    def test_publish_permissions_lognamer(self):
-        errordir = self.useFixture(TempDir()).path
-        repo = DateDirRepo(errordir, 'T')
-        report = {'id': 'OOPS-91T1'}
-        now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=utc)
-
-        # Set up default file creation mode to rwx------ as some restrictive
-        # servers do.
-        self.useFixture(UMaskFixture(stat.S_IRWXG | stat.S_IRWXO))
-        repo.publish(report, now)
-
-        errorfile = os.path.join(repo.log_namer.output_dir(now), '01800.T1')
-        # Check errorfile and directory are set with the correct permission:
-        # rw-r--r-- and rwxr-xr-x accordingly
-        file_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH
-        has_file_permission = HasUnixPermissions(file_perms)
-        has_dir_permission = HasUnixPermissions(
-            file_perms | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
-        self.assertThat(errorfile, has_file_permission)
-        self.assertThat(repo.log_namer.output_dir(now), has_dir_permission)
-
     def test_publish_permissions_hashnames(self):
         repo = DateDirRepo(self.useFixture(TempDir()).path, stash_path=True)
         report = {'id': 'OOPS-91T1'}
@@ -112,23 +90,15 @@
         self.assertThat(report['datedir_repo_filepath'], has_file_permission)
         self.assertThat(repo.root + '/2006-04-01', has_dir_permission)
 
-    def test_sets_log_namer_to_a_UniqueFileAllocator(self):
-        repo = DateDirRepo(self.useFixture(TempDir()).path, 'T')
-        self.assertIsInstance(repo.log_namer, UniqueFileAllocator)
-
     def test_default_serializer_bson(self):
-        repo = DateDirRepo(self.useFixture(TempDir()).path, 'T')
+        repo = DateDirRepo(self.useFixture(TempDir()).path)
         self.assertEqual(serializer_bson, repo.serializer)
 
     def test_settable_serializer(self):
         an_object = object()
-        repo = DateDirRepo(self.useFixture(TempDir()).path, 'T', an_object)
+        repo = DateDirRepo(self.useFixture(TempDir()).path, an_object)
         self.assertEqual(an_object, repo.serializer)
 
-    def test_no_instance_id_no_log_namer(self):
-        repo = DateDirRepo(self.useFixture(TempDir()).path)
-        self.assertEqual(None, repo.log_namer)
-
     def test_publish_via_hash(self):
         repo = DateDirRepo(self.useFixture(TempDir()).path)
         now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=utc)
@@ -178,7 +148,7 @@
 
     def test_publish_existing_id_lognamer(self):
         # The id reuse and file allocation strategies should be separate.
-        repo = DateDirRepo(self.useFixture(TempDir()).path, 'X',
+        repo = DateDirRepo(self.useFixture(TempDir()).path,
             inherit_id=True, stash_path=True)
         now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=utc)
         report = {'time': now, 'id': '45'}

=== removed file 'oops_datedir_repo/tests/test_uniquefileallocator.py'
--- oops_datedir_repo/tests/test_uniquefileallocator.py	2011-11-11 04:21:05 +0000
+++ oops_datedir_repo/tests/test_uniquefileallocator.py	1970-01-01 00:00:00 +0000
@@ -1,160 +0,0 @@
-# Copyright (c) 2010, 2011, Canonical Ltd
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Lesser General Public License as published by
-# the Free Software Foundation, version 3 only.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Lesser General Public License for more details.
-#
-# You should have received a copy of the GNU Lesser General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-# GNU Lesser General Public License version 3 (see the file LICENSE).
-
-"""Tests for the unique file naming facility."""
-
-__metaclass__ = type
-
-import datetime
-import os
-import stat
-
-from fixtures import TempDir
-import pytz
-import testtools
-
-from oops_datedir_repo.uniquefileallocator import UniqueFileAllocator
-
-
-UTC = pytz.timezone('UTC')
-
-
-class TestUniqueFileAllocator(testtools.TestCase):
-
-    def setUp(self):
-        super(TestUniqueFileAllocator, self).setUp()
-        tempdir = self.useFixture(TempDir())
-        self._tempdir = tempdir.path
-
-    def test_setToken(self):
-        namer = UniqueFileAllocator("/any-old/path/", 'OOPS', 'T')
-        self.assertEqual('T', namer.get_log_infix())
-
-        # Some scripts will append a string token to the prefix.
-        namer.setToken('CW')
-        self.assertEqual('TCW', namer.get_log_infix())
-
-        # Some scripts run multiple processes and append a string number
-        # to the prefix.
-        namer.setToken('1')
-        self.assertEqual('T1', namer.get_log_infix())
-
-    def assertUniqueFileAllocator(self, namer, now, expected_id,
-        expected_last_id, expected_suffix, expected_lastdir):
-        logid, filename = namer.newId(now)
-        self.assertEqual(logid, expected_id)
-        self.assertEqual(filename,
-            os.path.join(namer._output_root, expected_suffix))
-        self.assertEqual(namer._last_serial, expected_last_id)
-        self.assertEqual(namer._last_output_dir,
-            os.path.join(namer._output_root, expected_lastdir))
-
-    def test_newId(self):
-        # TODO: This should return an id, fileobj instead of a file name, to
-        # reduce races with threads that are slow to use what they asked for,
-        # when combined with configuration changes causing disk scans. That
-        # would also permit using a completely stubbed out file system,
-        # reducing IO in tests that use UniqueFileAllocator (such as all the
-        # pagetests in Launchpad. At that point an interface to obtain a
-        # factory of UniqueFileAllocator's would be useful to parameterise the
-        # entire test suite.
-        namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T')
-        # first name of the day
-        self.assertUniqueFileAllocator(namer,
-            datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC),
-            'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01')
-        # second name of the day
-        self.assertUniqueFileAllocator(namer,
-            datetime.datetime(2006, 04, 01, 12, 00, 00, tzinfo=UTC),
-            'OOPS-91T2', 2, '2006-04-01/43200.T2', '2006-04-01')
-
-        # first name of the following day sets a new dir and the id starts
-        # over.
-        self.assertUniqueFileAllocator(namer,
-            datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC),
-            'OOPS-92T1', 1, '2006-04-02/01800.T1', '2006-04-02')
-
-        # Setting a token inserts the token into the filename.
-        namer.setToken('YYY')
-        logid, filename = namer.newId(
-            datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC))
-        self.assertEqual(logid, 'OOPS-92TYYY2')
-
-        # Setting a type controls the log id:
-        namer.setToken('')
-        namer._log_type = "PROFILE"
-        logid, filename = namer.newId(
-            datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC))
-        self.assertEqual(logid, 'PROFILE-92T3')
-
-        # Native timestamps are not permitted - UTC only.
-        now = datetime.datetime(2006, 04, 02, 00, 30, 00)
-        self.assertRaises(ValueError, namer.newId, now)
-
-    def test_changeErrorDir(self):
-        """Test changing the log output dir."""
-        namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T')
-
-        # First an id in the original error directory.
-        self.assertUniqueFileAllocator(namer,
-            datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC),
-            'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01')
-
-        # UniqueFileAllocator uses the _output_root attribute to get the
-        # current output directory.
-        new_output_dir = self.useFixture(TempDir()).path
-        namer._output_root = new_output_dir
-
-        # Now an id on the same day, in the new directory.
-        now = datetime.datetime(2006, 04, 01, 12, 00, 00, tzinfo=UTC)
-        log_id, filename = namer.newId(now)
-
-        # Since it's a new directory, with no previous logs, the id is 1
-        # again, rather than 2.
-        self.assertEqual(log_id, 'OOPS-91T1')
-        self.assertEqual(namer._last_serial, 1)
-        self.assertEqual(namer._last_output_dir,
-            os.path.join(new_output_dir, '2006-04-01'))
-
-    def test_findHighestSerial(self):
-        namer = UniqueFileAllocator(self._tempdir, "OOPS", "T")
-        # Creates the dir using now as the timestamp.
-        output_dir = namer.output_dir()
-        # write some files, in non-serial order.
-        open(os.path.join(output_dir, '12343.T1'), 'w').close()
-        open(os.path.join(output_dir, '12342.T2'), 'w').close()
-        open(os.path.join(output_dir, '12345.T3'), 'w').close()
-        open(os.path.join(output_dir, '1234567.T0010'), 'w').close()
-        open(os.path.join(output_dir, '12346.A42'), 'w').close()
-        open(os.path.join(output_dir, '12346.B100'), 'w').close()
-        # The namer should figure out the right highest serial.
-        self.assertEqual(namer._findHighestSerial(output_dir), 10)
-
-    def test_output_dir_permission(self):
-        # Set up default dir creation mode to rwx------.
-        umask_permission = stat.S_IRWXG | stat.S_IRWXO
-        old_umask = os.umask(umask_permission)
-        namer = UniqueFileAllocator(self._tempdir, "OOPS", "T")
-        output_dir = namer.output_dir()
-        st = os.stat(output_dir)
-        # Permission we want here is: rwxr-xr-x
-        wanted_permission = (
-            stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH |
-            stat.S_IXOTH)
-        # Get only the permission bits for this directory.
-        dir_permission = stat.S_IMODE(st.st_mode)
-        self.assertEqual(dir_permission, wanted_permission)
-        # Restore the umask to the original value.
-        ignored = os.umask(old_umask)

=== removed file 'oops_datedir_repo/uniquefileallocator.py'
--- oops_datedir_repo/uniquefileallocator.py	2011-11-11 04:21:05 +0000
+++ oops_datedir_repo/uniquefileallocator.py	1970-01-01 00:00:00 +0000
@@ -1,212 +0,0 @@
-# Copyright (c) 2010, 2011, Canonical Ltd
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Lesser General Public License as published by
-# the Free Software Foundation, version 3 only.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Lesser General Public License for more details.
-#
-# You should have received a copy of the GNU Lesser General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-# GNU Lesser General Public License version 3 (see the file LICENSE).
-
-
-"""Create uniquely named log files on disk."""
-
-
-__all__ = ['UniqueFileAllocator']
-
-__metaclass__ = type
-
-
-import datetime
-import errno
-import os.path
-import stat
-import threading
-
-import pytz
-
-
-UTC = pytz.utc
-
-# the section of the ID before the instance identifier is the
-# days since the epoch, which is defined as the start of 2006.
-epoch = datetime.datetime(2006, 01, 01, 00, 00, 00, tzinfo=UTC)
-
-
-class UniqueFileAllocator:
-    """Assign unique file names to logs being written from an app/script.
-
-    UniqueFileAllocator causes logs written from one process to be uniquely
-    named. It is not safe for use in multiple processes with the same output
-    root - each process must have a unique output root.
-    """
-
-    def __init__(self, output_root, log_type, log_subtype):
-        """Create a UniqueFileAllocator.
-
-        :param output_root: The root directory that logs should be placed in.
-        :param log_type: A string to use as a prefix in the ID assigned to new
-            logs. For instance, "OOPS".
-        :param log_subtype: A string to insert in the generate log filenames
-            between the day number and the serial. For instance "T" for
-            "Testing".
-        """
-        self._lock = threading.Lock()
-        self._output_root = output_root
-        self._last_serial = 0
-        self._last_output_dir = None
-        self._log_type = log_type
-        self._log_subtype = log_subtype
-        self._log_token = ""
-
-    def _findHighestSerialFilename(self, directory=None, time=None):
-        """Find details of the last log present in the given directory.
-
-        This function only considers logs with the currently
-        configured log_subtype.
-
-        One of directory, time must be supplied.
-
-        :param directory: Look in this directory.
-        :param time: Look in the directory that a log written at this time
-            would have been written to. If supplied, supercedes directory.
-        :return: a tuple (log_serial, log_filename), which will be (0,
-            None) if no logs are found. log_filename is a usable path, not
-            simply the basename.
-        """
-        if directory is None:
-            directory = self.output_dir(time)
-        prefix = self.get_log_infix()
-        lastid = 0
-        lastfilename = None
-        for filename in os.listdir(directory):
-            logid = filename.rsplit('.', 1)[1]
-            if not logid.startswith(prefix):
-                continue
-            logid = logid[len(prefix):]
-            if logid.isdigit() and (lastid is None or int(logid) > lastid):
-                lastid = int(logid)
-                lastfilename = filename
-        if lastfilename is not None:
-            lastfilename = os.path.join(directory, lastfilename)
-        return lastid, lastfilename
-
-    def _findHighestSerial(self, directory):
-        """Find the last serial actually applied to disk in directory.
-
-        The purpose of this function is to not repeat sequence numbers
-        if the logging application is restarted.
-
-        This method is not thread safe, and only intended to be called
-        from the constructor (but it is called from other places in
-        integration tests).
-        """
-        return self._findHighestSerialFilename(directory)[0]
-
-    def getFilename(self, log_serial, time):
-        """Get the filename for a given log serial and time."""
-        log_subtype = self.get_log_infix()
-        # TODO: Calling output_dir causes a global lock to be taken and a
-        # directory scan, which is bad for performance. It would be better
-        # to have a split out 'directory name for time' function which the
-        # 'want to use this directory now' function can call.
-        output_dir = self.output_dir(time)
-        second_in_day = time.hour * 3600 + time.minute * 60 + time.second
-        return os.path.join(
-            output_dir, '%05d.%s%s' % (
-            second_in_day, log_subtype, log_serial))
-
-    def get_log_infix(self):
-        """Return the current log infix to use in ids and file names."""
-        return self._log_subtype + self._log_token
-
-    def newId(self, now=None):
-        """Returns an (id, filename) pair for use by the caller.
-
-        The ID is composed of a short string to identify the Launchpad
-        instance followed by an ID that is unique for the day.
-
-        The filename is composed of the zero padded second in the day
-        followed by the ID.  This ensures that reports are in date order when
-        sorted lexically.
-        """
-        if now is not None:
-            now = now.astimezone(UTC)
-        else:
-            now = datetime.datetime.now(UTC)
-        # We look up the error directory before allocating a new ID,
-        # because if the day has changed, errordir() will reset the ID
-        # counter to zero.
-        self.output_dir(now)
-        self._lock.acquire()
-        try:
-            self._last_serial += 1
-            newid = self._last_serial
-        finally:
-            self._lock.release()
-        subtype = self.get_log_infix()
-        day_number = (now - epoch).days + 1
-        log_id = '%s-%d%s%d' % (self._log_type, day_number, subtype, newid)
-        filename = self.getFilename(newid, now)
-        return log_id, filename
-
-    def output_dir(self, now=None):
-        """Find or make the directory to allocate log names in.
-
-        Log names are assigned within subdirectories containing the date the
-        assignment happened.
-        """
-        if now is not None:
-            now = now.astimezone(UTC)
-        else:
-            now = datetime.datetime.now(UTC)
-        date = now.strftime('%Y-%m-%d')
-        result = os.path.join(self._output_root, date)
-        if result != self._last_output_dir:
-            self._lock.acquire()
-            try:
-                self._last_output_dir = result
-                # make sure the directory exists
-                try:
-                    os.makedirs(result)
-                except OSError, e:
-                    if e.errno != errno.EEXIST:
-                        raise
-                # Make sure the directory permission is set to: rwxr-xr-x
-                permission = (
-                    stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP |
-                    stat.S_IROTH | stat.S_IXOTH)
-                os.chmod(result, permission)
-                # TODO: Note that only one process can do this safely: its not
-                # cross-process safe, and also not entirely threadsafe:
-                # another # thread that has a new log and hasn't written it
-                # could then use that serial number. We should either make it
-                # really safe, or remove the contention entirely and log
-                # uniquely per thread of execution.
-                self._last_serial = self._findHighestSerial(result)
-            finally:
-                self._lock.release()
-        return result
-
-    def listRecentReportFiles(self):
-        now = datetime.datetime.now(UTC)
-        yesterday = now - datetime.timedelta(days=1)
-        directories = [self.output_dir(now), self.output_dir(yesterday)]
-        for directory in directories:
-            report_names = os.listdir(directory)
-            for name in sorted(report_names, reverse=True):
-                yield directory, name
-
-    def setToken(self, token):
-        """Append a string to the log subtype in filenames and log ids.
-
-        :param token: a string to append..
-            Scripts that run multiple processes can use this to create a
-            unique identifier for each process.
-        """
-        self._log_token = token