Merge lp:~vila/udd/608563-requeue-all-of-type into lp:udd

[Vincent Ladeuil]

Basically the fix is only:

@@ -686,7 +706,7 @@
                             % self.FAILURES_TABLE).fetchall()
                     for row in rows:
                         this_raw_reason = row[1].encode("ascii", "replace")
- this_sig = self.failure_signature(raw_reason)
+ this_sig = self.failure_signature(this_raw_reason)
                         if this_sig == sig:
                             self._retry(c, row[0], sig, row[2],
                                     priority=priority)

I've added some tangentially related tests and found a couple of minor issues while investigating which I'd like to land too.

[lp:~vila/udd/608563-requeue-all-of-type]

420. By Vincent Ladeuil on 2011-03-16

Mark the bug as fixed

[Vincent Ladeuil (vila)]

Hmm, I was so surprised when I fixed the bug that I forgot to write a proper cover letter, here it is.

requeue_package.py --all-of-type <pkg_name> had a bug where all failed imports were requeued instead of only the ones with the same failure signature. This was due to a typo that makes the signature comparison always use the signature from <pkg_name>.

This patches fixes the typo and makes the signature more precise (since the full traceback is stored in the db this should only make things clearer and doesn't require updating the db itself).

There were also some dusty code in failure_signature that I got rid off (there can't be '\n' in the trace lines since they are produced by splitlines(), annotate and some more following across files revealed that this was needed when the traceback was acquired with readlines()).

Finally there was a bogus test against running_sentinel which *contains* a '\n' that should be ignored when comparing the signature from the db.

[lp:~vila/udd/608563-requeue-all-of-type]

421. By Vincent Ladeuil on 2011-03-16

Explain the traceback crude parsing.

[James Westby (james-w)]

Excellent, thanks for fixing this.

Thanks,

James

review: Approve

=== modified file 'icommon.py'
--- icommon.py	2011-03-15 09:00:36 +0000
+++ icommon.py	2011-03-16 10:29:18 +0000
@@ -406,8 +406,8 @@
                       emailed integer default 0,
                       constraint isprimary PRIMARY KEY
                             (package))''' % FAILURES_TABLE
-    FAILURES_TABLE_FIND = '''select * from %s where package=?''' % FAILURES_TABLE
-    FAILURES_TABLE_DELETE = '''delete from %s where package=?''' % FAILURES_TABLE
+    FAILURES_TABLE_FIND = 'select * from %s where package=?' % FAILURES_TABLE
+    FAILURES_TABLE_DELETE = 'delete from %s where package=?' % FAILURES_TABLE
 
     OLD_FAILURES_TABLE = "old_failures"
     OLD_FAILURES_TABLE_CREATE = '''create table if not exists %s
@@ -633,7 +633,7 @@
         try:
             row = c.execute(self.FAILURES_TABLE_FIND, (package,)).fetchone()
             if row is None:
-                return row
+                return None
             return row[1]
         finally:
             self.conn.rollback()
@@ -641,24 +641,49 @@
 
     def failure_signature(self, raw_reason):
         trace = raw_reason.splitlines()
-        sig = ''
         if len(trace) == 1:
-            if trace[0] == running_sentinel:
+            if trace[0] == running_sentinel[:-1]: # Get rid of the final '\n'
                 return None
             # sometimes, Python exceptions do not have file references
             m = re.match('(\w+): ', trace[0])
             if m:
                 return m.group(1)
             else:
-                return trace[0].strip().replace("\n", " ")
+                return trace[0].strip()
         elif len(trace) < 3:
-            return " ".join(trace).strip().replace("\n", " ")
+            return " ".join(trace).strip()
 
+        # If the failure reason is a traceback (which should always be the
+        # case, the running_sentinel check above taking care of the still
+        # running imports), we build the traceback signature and capture the
+        # exception type
+        sig = ''
+        exc_type = ''
+        exc_type_coming = False
+        file_line_seen = False
         for l in trace:
             if l.startswith('  File'):
+                # Keep the method/function name
                 sig += ':' + l.split()[-1]
-
-        return trace[-1].split(':')[0].replace("\n", " ") + sig
+                file_line_seen = True
+            elif file_line_seen:
+                # We've seen the 'File...' line, so we are now seeing the code
+                # line
+                exc_type_coming = True
+                file_line_seen = False
+            elif exc_type_coming:
+                # We've seen the code line so we may find the exception line
+                # itself now.
+
+                # We have no way to know if we are at the last line of the
+                # traceback, but we can't rely on the execption always
+                # displaying a single line either, so the last captured
+                # exception will do.
+                exc_type = l.split(':')[0]
+                exc_type_coming = False
+
+        sig = exc_type + sig
+        return sig
 
     def retry(self, package, force=False, priority=False, auto=False,
             all=False):
@@ -686,7 +711,7 @@
                             % self.FAILURES_TABLE).fetchall()
                     for row in rows:
                         this_raw_reason = row[1].encode("ascii", "replace")
-                        this_sig = self.failure_signature(raw_reason)
+                        this_sig = self.failure_signature(this_raw_reason)
                         if this_sig == sig:
                             self._retry(c, row[0], sig, row[2],
                                     priority=priority)
@@ -705,17 +730,14 @@
                 if row[4] > self.MAX_AUTO_RETRY_COUNT:
                     print ("Warning: %s has failed %d times in the same way"
                             % (package, row[4]))
-                c.execute('update %s set package=?, reason=?, '
-                        'when_failed=?, last_failed=?, failure_count=? '
-                        'where package=?'
-                        % self.OLD_FAILURES_TABLE,
-                        (package, signature, timestamp, timestamp, row[4]+1,
-                         package))
+                failure_count = row[4]+1
             else:
-                c.execute('update %s set package=?, reason=?, when_failed=?, '
-                        'last_failed=?, failure_count=? where package=?'
-                        % self.OLD_FAILURES_TABLE,
-                        (package, signature, timestamp, timestamp, 1, package))
+                failure_count = 1
+            c.execute('update %s set package=?, reason=?, when_failed=?, '
+                      'last_failed=?, failure_count=? where package=?'
+                      % self.OLD_FAILURES_TABLE,
+                      (package, signature, timestamp, timestamp, failure_count,
+                       package))
         else:
             c.execute('insert into %s values (?, ?, ?, ?, ?)'
                     % self.OLD_FAILURES_TABLE,

=== modified file 'tests.py'
--- tests.py	2011-02-23 18:23:37 +0000
+++ tests.py	2011-03-16 10:29:18 +0000
@@ -229,6 +229,85 @@
         self.check_rows(0, 0, 2)
 
 
+class StatusDatabaseTests(tests.TestCase):
+
+    def setUp(self):
+        super(StatusDatabaseTests, self).setUp()
+        self.db = icommon.StatusDatabase(":memory:")
+
+    def test_no_failures_in_empty_db(self):
+        reasons, package_info = self.db.summarise_failures()
+        self.assertEquals({}, reasons)
+        self.assertEquals([], package_info)
+
+    def test_no_unemailed_failures_in_empty_db(self):
+        self.assertEquals([], self.db.unemailed_failures())
+
+    def assertSignature(self, expected, raw):
+        self.assertEquals(expected, self.db.failure_signature(raw))
+
+    def test_running_signature(self):
+        # Running imports use a special failure signature
+        self.assertSignature(None, icommon.running_sentinel)
+
+    def test_one_line_signature(self):
+        self.assertSignature('AssertionError', 'AssertionError: xx')
+
+    def test_multi_line_signature(self):
+        # We use a real-life example here, the apparent duplication of method
+        # names is due to calls to the base class methods
+        self.assertSignature(
+            'OperationalError:do_one_step:do_one_step'
+            ':collect_terminated_threads'
+            ':collect_terminated_threads:collect'
+            ':finish_job:_set_failure',
+            '''Traceback (most recent call last):
+  File "/srv/package-import.canonical.com/new/scripts/mass_import.py", line 234, in do_one_step
+    super(ImportDriver, self).do_one_step()
+  File "/srv/package-import.canonical.com/new/scripts/icommon.py", line 2091, in do_one_step
+    self.collect_terminated_threads()
+  File "/srv/package-import.canonical.com/new/scripts/mass_import.py", line 253, in collect_terminated_threads
+    super(ImportDriver, self).collect_terminated_threads()
+  File "/srv/package-import.canonical.com/new/scripts/icommon.py", line 2119, in collect_terminated_threads
+    t.collect()
+  File "/srv/package-import.canonical.com/new/scripts/mass_import.py", line 162, in collect
+    unicode_output.encode("utf-8", "replace"))
+  File "/srv/package-import.canonical.com/new/scripts/icommon.py", line 552, in finish_job
+    self._set_failure(c, package, output, now)
+  File "/srv/package-import.canonical.com/new/scripts/icommon.py", line 485, in _set_failure
+    % self.FAILURES_TABLE, (package, reason, now))
+OperationalError: table failures has 4 columns but 3 values were supplied
+''')
+
+    def test_regular_traceback(self):
+        self.assertSignature(
+                'bzrlib.errors.NoSuchTag:lookup_tag',
+                                 '''Traceback (most recent call last):
+  File "/usr/lib/python2.5/site-packages/bzrlib/tag.py", line 109, in lookup_tag
+    raise errors.NoSuchTag(tag_name)
+bzrlib.errors.NoSuchTag: No such tag: upstream-4.6.2
+''')
+
+    def test_traceback_with_verbose_execption(self):
+        # make sure we get bzrlib.errors.NoFinalPath without being tricked by:
+        # the final empty line, nor the 'file-id:' and 'root trans-id' lines.
+        self.assertSignature(
+            'bzrlib.errors.NoFinalPath:get_path:_determine_path:final_name',
+            ''''Traceback (most recent call last):
+  File "/usr/lib/python2.5/site-packages/bzrlib/transform.py", line 2368, in get_path
+    self._known_paths[trans_id] = self._determine_path(trans_id)
+  File "/usr/lib/python2.5/site-packages/bzrlib/transform.py", line 2358, in _determine_path
+    name = self.transform.final_name(trans_id)
+  File "/usr/lib/python2.5/site-packages/bzrlib/transform.py", line 470, in final_name
+    raise NoFinalPath(trans_id, self)
+bzrlib.errors.NoFinalPath: No final name for trans_id 'new-18'
+file-id: None
+root trans-id: 'new-0'
+
+''')
+
+
+
 class FindEarliestMergeTests(tests.TestCaseWithTransport):
 
     def test_tip_revision(self):