Autopkgtest: Reorganize results map

- Invert the map to go from triggers to tested versions, instead of from
   tested versions to triggers. This is the lookup and update mode that we
   usually want (except for determining "ever passed"), thus this simplifies
   the code and speeds up lookups.

 - Drop "latest_stamp" in favor of tracking individual run IDs for every
   result. This allows us in the future to directly expose the run IDs on
   excuses.{yaml,html}, e. g. by providing direct links to result logs.

 - Drop "ever_passed" flag as we can compute this from the individual results.

 - Don't track multiple package versions for a package and a particular
   trigger. We are only interested in the latest (passing) version and don't
   otherwise use the tested version except for displaying.

This requires adjusting the test_dkms_results_per_kernel_old_results test, as
we now consistently ignore "ever passed" for kernel tests also for "RUNNING"
vs.  "RUNNING-ALWAYSFAILED", not just for "PASS" vs. "ALWAYSFAIL".

Also fix a bug in results() when checking if a test for which we don't have
results yet is currently running: Check for correct trigger, not for the
current source package version. This most probably already fixes LP: #1494786.
Also upgrade the warning about "result is neither known nor pending" to a grave
error, for making it more obvious to debug remaining errors with this.

ATTENTION: This changes the on-disk format of results.cache, and thus this
needs to be dropped and rebuilt when rolling this out.

autopkgtest.py

@@ -69,19 +69,15 @@ class AutoPackageTest(object):
             os.mkdir(self.test_state_dir)
         self.read_pending_tests()
 
-        # results map: src -> arch -> [latest_stamp, ver -> trigger -> passed, ever_passed]
+        # results map: trigger -> src -> arch -> [passed, version, run_id]
-        # - It's tempting to just use a global "latest" time stamp, but due to
-        #   swift's "eventual consistency" we might miss results with older time
-        #   stamps from other packages that we don't see in the current run, but
-        #   will in the next one. This doesn't hurt for older results of the same
-        #   package.
         # - trigger is "source/version" of an unstable package that triggered
-        #   this test run. We need to track this to avoid unnecessarily
+        #   this test run.
-        #   re-running tests.
         # - "passed" is a bool
-        # - ever_passed is a bool whether there is any successful test of
+        # - "version" is the package version  of "src" of that test
-        #   src/arch of any version. This is used for detecting "regression"
+        # - "run_id" is an opaque ID that identifies a particular test run for
-        #   vs. "always failed"
+        #   a given src/arch. It's usually a time stamp like "20150120_125959".
+        #   This is also used for tracking the latest seen time stamp for
+        #   requesting only newer results.
         self.test_results = {}
         self.results_cache_file = os.path.join(self.test_state_dir, 'results.cache')
 
@@ -298,31 +294,23 @@ class AutoPackageTest(object):
     def add_test_request(self, src, ver, arch, trigsrc, trigver):
         '''Add one test request to the local self.requested_tests queue
 
+        trigger is "pkgname/version" of the package that triggers the testing
+        of src.
+
         This will only be done if that test wasn't already requested in a
         previous run (i. e. not already in self.pending_tests) or there already
         is a result for it.
         '''
-        # check for existing results for both the requested and the current
+        # Don't re-request if we already have a result
-        # unstable version: test runs might see newly built versions which we
-        # didn't see in britney yet
-        ver_trig_results = self.test_results.get(src, {}).get(arch, [None, {}, None])[1]
-        unstable_ver = self.britney.sources['unstable'][src][VERSION]
         try:
-            testing_ver = self.britney.sources['testing'][src][VERSION]
+            self.test_results[trigsrc + '/' + trigver][src][arch]
+            self.log_verbose('There already is a result for %s/%s triggered by %s/%s' %
+                             (src, arch, trigsrc, trigver))
+            return
         except KeyError:
-            testing_ver = unstable_ver
+            pass
-        for result_ver in set([testing_ver, ver, unstable_ver]):
-            # result_ver might be < ver here; that's okay, if we already have a
-            # result for trigsrc/trigver we don't need to re-run it again
-            if result_ver not in ver_trig_results:
-                continue
-            for trigger in ver_trig_results[result_ver]:
-                (tsrc, tver) = trigger.split('/', 1)
-                if tsrc == trigsrc and apt_pkg.version_compare(tver, trigver) >= 0:
-                    self.log_verbose('There already is a result for %s/%s/%s triggered by %s/%s' %
-                                     (src, result_ver, arch, tsrc, tver))
-                    return
 
+        # Don't re-request if it's already pending
         if (trigsrc, trigver) in self.pending_tests.get(src, {}).get(
                 ver, {}).get(arch, set()):
             self.log_verbose('test %s/%s/%s for %s/%s is already pending, not queueing' %
@@ -331,20 +319,32 @@ class AutoPackageTest(object):
         self.requested_tests.setdefault(src, {}).setdefault(
             ver, {}).setdefault(arch, set()).add((trigsrc, trigver))
 
-    def fetch_swift_results(self, swift_url, src, arch):
+    def fetch_swift_results(self, swift_url, src, arch, triggers):
-        '''Download new results for source package/arch from swift'''
+        '''Download new results for source package/arch from swift
 
-        # prepare query: get all runs with a timestamp later than latest_stamp
+        triggers is an iterable of (trigsrc, trigver) for which to check
-        # for this package/arch; '@' is at the end of each run timestamp, to
+        results.
+        '''
+        # prepare query: get all runs with a timestamp later than the latest
+        # run_id for this package/arch; '@' is at the end of each run id, to
         # mark the end of a test run directory path
         # example: <autopkgtest-wily>wily/amd64/libp/libpng/20150630_054517@/result.tar
         query = {'delimiter': '@',
                  'prefix': '%s/%s/%s/%s/' % (self.series, arch, srchash(src), src)}
-        try:
+
-            query['marker'] = query['prefix'] + self.test_results[src][arch][0]
+        # determine latest run_id from results
-        except KeyError:
+        # FIXME: consider dropping "triggers" arg again and iterate over all
-            # no stamp yet, download all results
+        # results, if that's not too slow; cache the results?
-            pass
+        latest_run_id = ''
+        for trigsrc, trigver in triggers:
+            try:
+                run_id = self.test_results[trigsrc + '/' + trigver][src][arch][2]
+            except KeyError:
+                continue
+            if run_id > latest_run_id:
+                latest_run_id = run_id
+        if latest_run_id:
+            query['marker'] = query['prefix'] + latest_run_id
 
         # request new results from swift
         url = os.path.join(swift_url, 'autopkgtest-' + self.series)
@@ -437,47 +437,45 @@ class AutoPackageTest(object):
                                          (src, pending_ver, arch))
 
         # add this result
-        src_arch_results = self.test_results.setdefault(src, {}).setdefault(arch, [stamp, {}, False])
+        for (trigsrc, trigver) in result_triggers:
-        trigmap = src_arch_results[1].setdefault(ver, {})
-        for trig in result_triggers:
-            trig_idx = trig[0] + '/' + trig[1]
-
             # If a test runs because of its own package (newer version), ensure
             # that we got a new enough version; FIXME: this should be done more
             # generically by matching against testpkg-versions
-            if trig[0] == src and apt_pkg.version_compare(ver, trig[1]) < 0:
+            if trigsrc == src and apt_pkg.version_compare(ver, trigver) < 0:
-                self.log_error('test trigger %s, but run for older version %s, ignoring' %
+                self.log_error('test trigger %s/%s, but run for older version %s, ignoring' %
-                               (trig_idx, ver))
+                               (trigsrc, trigver, ver))
                 continue
 
-            # passed results are always good, but don't clobber existing passed
+            result = self.test_results.setdefault(trigsrc + '/' + trigver, {}).setdefault(
-            # results with failures from re-runs
+                src, {}).setdefault(arch, [False, None, ''])
-            if passed or trig_idx not in trigmap:
-                trigmap[trig_idx] = passed
 
-        # update ever_passed field, unless we got triggered from
+            # don't clobber existing passed results with failures from re-runs
-        # linux-meta*: we trigger separate per-kernel tests for reverse
+            result[0] = result[0] or passed
-        # test dependencies, and we don't want to track per-trigger
+            result[1] = ver
-        # ever_passed. This would be wrong for everything except the
+            if stamp > result[2]:
-        # kernel, and the kernel team tracks per-kernel regressions already
+                result[2] = stamp
-        if passed and not result_triggers[0][0].startswith('linux-meta'):
-            src_arch_results[2] = True
-
-        # update latest_stamp
-        if stamp > src_arch_results[0]:
-            src_arch_results[0] = stamp
 
     def failed_tests_for_trigger(self, trigsrc, trigver):
         '''Return (src, arch) set for failed tests for given trigger pkg'''
 
-        result = set()
+        failed = set()
-        trigger = trigsrc + '/' + trigver
+        for src, srcinfo in self.test_results.get(trigsrc + '/' + trigver, {}).items():
-        for src, srcinfo in self.test_results.items():
+            for arch, result in srcinfo.items():
-            for arch, (stamp, vermap, ever_passed) in srcinfo.items():
+                if not result[0]:
-                for ver, trig_results in vermap.items():
+                    failed.add((src, arch))
-                    if trig_results.get(trigger) is False:
+        return failed
-                        result.add((src, arch))
+
-        return result
+    def check_ever_passed(self, src, arch):
+        '''Check if tests for src ever passed on arch'''
+
+        # FIXME: add caching
+        for srcmap in self.test_results.values():
+            try:
+                if srcmap[src][arch][0]:
+                    return True
+            except KeyError:
+                pass
+        return False
 
     #
     # Public API
@@ -578,8 +576,8 @@ class AutoPackageTest(object):
         '''
         for pkg, verinfo in copy.deepcopy(self.requested_tests).items():
             for archinfo in verinfo.values():
-                for arch in archinfo:
+                for arch, triggers in archinfo.items():
-                    self.fetch_swift_results(self.britney.options.adt_swift_url, pkg, arch)
+                    self.fetch_swift_results(self.britney.options.adt_swift_url, pkg, arch, triggers)
 
     def collect(self, packages):
         '''Update results from swift for all pending packages
@@ -588,8 +586,8 @@ class AutoPackageTest(object):
         '''
         for pkg, verinfo in copy.deepcopy(self.pending_tests).items():
             for archinfo in verinfo.values():
-                for arch in archinfo:
+                for arch, triggers in archinfo.items():
-                    self.fetch_swift_results(self.britney.options.adt_swift_url, pkg, arch)
+                    self.fetch_swift_results(self.britney.options.adt_swift_url, pkg, arch, triggers)
         # also update results for excuses whose tests failed, in case a
         # manual retry worked
         for (trigpkg, trigver) in packages:
@@ -597,7 +595,7 @@ class AutoPackageTest(object):
                 if arch not in self.pending_tests.get(trigpkg, {}).get(trigver, {}):
                     self.log_verbose('Checking for new results for failed %s on %s for trigger %s/%s' %
                                      (pkg, arch, trigpkg, trigver))
-                    self.fetch_swift_results(self.britney.options.adt_swift_url, pkg, arch)
+                    self.fetch_swift_results(self.britney.options.adt_swift_url, pkg, arch, [(trigpkg, trigver)])
 
         # update the results cache
         with open(self.results_cache_file + '.new', 'w') as f:
@@ -620,62 +618,39 @@ class AutoPackageTest(object):
 
         for arch in self.britney.options.adt_arches:
             for testsrc, testver in self.tests_for_source(trigsrc, trigver, arch):
-                # by default, assume that we has never passed (i.e. this is the first run)
+                ever_passed = self.check_ever_passed(testsrc, arch)
-                ever_passed = False
+
+                # Do we have a result already? (possibly for an older or newer
+                # version, that's okay)
                 try:
-                    (_, ver_map, ever_passed) = self.test_results[testsrc][arch]
+                    r = self.test_results[trigger][testsrc][arch]
-
+                    testver = r[1]
-                    # check if we have a result for any version of testsrc that
+                    if r[0]:
-                    # was triggered for trigsrc/trigver; we prefer PASSes, as
-                    # it could be that an unrelated package upload could break
-                    # testsrc's tests at a later point
-                    status = None
-                    for ver, trigger_results in ver_map.items():
-                        try:
-                            status = trigger_results[trigger]
-                            testver = ver
-                            # if we found a PASS, we can stop searching
-                            if status is True:
-                                break
-                        except KeyError:
-                            pass
-
-                    if status is None:
-                        # no result? go to "still running" below
-                        raise KeyError
-
-                    if status:
                         result = 'PASS'
                     else:
-                        # test failed, check ever_passed flag for that src/arch
+                        # Special-case triggers from linux-meta*: we cannot compare
-                        # unless we got triggered from linux-meta*: we trigger
+                        # results against different kernels, as e. g. a DKMS module
-                        # separate per-kernel tests for reverse test
+                        # might work against the default kernel but fail against a
-                        # dependencies, and we don't want to track per-trigger
+                        # different flavor; so for those, ignore the "ever
-                        # ever_passed. This would be wrong for everything
+                        # passed" check; FIXME: check against trigsrc only
-                        # except the kernel, and the kernel team tracks
+                        if trigsrc.startswith('linux-meta') or trigsrc == 'linux':
-                        # per-kernel regressions already
+                            ever_passed = False
-                        if ever_passed and not trigsrc.startswith('linux-meta') and trigsrc != 'linux':
-                            result = 'REGRESSION'
-                        else:
-                            result = 'ALWAYSFAIL'
-                except KeyError:
-                    # no result for testsrc/testver/arch; still running?
-                    try:
-                        self.pending_tests[testsrc][testver][arch]
 
-                        if ever_passed:
+                        result = ever_passed and 'REGRESSION' or 'ALWAYSFAIL'
-                            result = 'RUNNING'
+                except KeyError:
-                        else:
+                    # no result for testsrc/arch; still running?
-                            result = 'RUNNING-ALWAYSFAIL'
+                    result = None
-                    except KeyError:
+                    for arch_map in self.pending_tests.get(testsrc, {}).values():
+                        if (trigsrc, trigver) in arch_map.get(arch, set()):
+                            result = ever_passed and 'RUNNING' or 'RUNNING-ALWAYSFAIL'
+                            break
+                    else:
                         # ignore if adt or swift results are disabled,
                         # otherwise this is unexpected
                         if not hasattr(self.britney.options, 'adt_swift_url'):
                             continue
-                        # FIXME: Ignore this error for now as it crashes britney, but investigate!
+                        raise RuntimeError('Result for %s/%s/%s (triggered by %s) is neither known nor pending!' %
-                        self.log_error('FIXME: Result for %s/%s/%s (triggered by %s) is neither known nor pending!' %
+                                           (testsrc, testver, arch, trigger))
-                                       (testsrc, testver, arch, trigger))
-                        continue
 
                 pkg_arch_result.setdefault((testsrc, testver), {})[arch] = result
 

tests/test_autopkgtest.py

@@ -301,14 +301,11 @@ lightgreen 1 i386 green 2
         # caches the results and triggers
         with open(os.path.join(self.data.path, 'data/series-proposed/autopkgtest/results.cache')) as f:
             res = json.load(f)
-        self.assertEqual(res['green']['i386'],
+        self.assertEqual(res['green/1']['green']['amd64'],
-                         ['20150101_100200@',
+                         [False, '1', '20150101_020000@'])
-                          {'1': {'passedbefore/1': True}, '2': {'green/2': True}},
+        self.assertEqual(set(res['green/2']), {'darkgreen', 'green', 'lightgreen'})
-                          True])
+        self.assertEqual(res['green/2']['lightgreen']['i386'],
-        self.assertEqual(res['lightgreen']['amd64'],
+                         [True, '1', '20150101_100100@'])
-                         ['20150101_100101@',
-                          {'1': {'green/2': True}},
-                          True])
 
         # third run should not trigger any new tests, should all be in the
         # cache
@@ -1415,7 +1412,7 @@ fancy 1 i386 linux-meta-lts-grumpy 1
             ],
             {'linux-meta': (True, {'fancy 1': {'amd64': 'PASS', 'i386': 'PASS'}}),
              # we don't have an explicit result for amd64
-             'linux-meta-lts-grumpy': (True, {'fancy 1': {'amd64': 'RUNNING-ALWAYSFAIL', 'i386': 'ALWAYSFAIL'}}),
+             'linux-meta-lts-grumpy': (False, {'fancy 1': {'amd64': 'RUNNING', 'i386': 'ALWAYSFAIL'}}),
              'linux-meta-64only': (True, {'fancy 1': {'amd64': 'PASS'}}),
             })