Merge lp:~zyga/checkbox/fix-session-init into lp:checkbox

[Zygmunt Krynicki]

Fix for bug lp: #1157264 affecting SessionState.__init__()

[Zygmunt Krynicki (zyga)]

This is how it looks like now:

vagrant@vagrant-ubuntu-raring-32:/usr/share/checkbox/jobs$ plainbox run -i cpu/scaling_test
===============================[ Analyzing Jobs ]===============================
The job database you are currently using is broken
At least two jobs contend for the name keys/volume
First job defined in: /usr/share/checkbox/jobs/test.txt:1-9
Second job defined in: /usr/share/checkbox/jobs/keys.txt:12-26
duplicate job name: 'keys/volume'

[lp:~zyga/checkbox/fix-session-init]

2001. By Zygmunt Krynicki on 2013-03-19

session: properly handle SessionState() called with duplicates

This is a part of fix to lp: #1157264

When SessionState() is initialized with a list containing duplicates
bad things happen. The error recovery code in update_desired_jobs()
is not prepared for any duplicates and things break in odd places.

To fix that, have SessionState reject duplicate jobs in the same way
that DependencySolver does. As a small exception to this rule, jobs
that are prefect clones are silently coalesced.

This makes the constructor of SessionState throw DependencyDuplicateError
exception when real problems have occurred and prevent any testing from
being started. Unlike local jobs that can also silently produce duplicate
jobs, having initial name clashes cannot be solved at this layer

Signed-off-by: Zygmunt Krynicki <email address hidden>
Reported-by: Sean Feole <email address hidden>

2002. By Zygmunt Krynicki on 2013-03-19

run: handle duplicate jobs when constructing SessionState

This fixes lp: #1157264

This builds on earlier patches, when the job database is wrong and
contains duplicate jobs that are not identical then tell the operator
about where each job came from.

Signed-off-by: Zygmunt Krynicki <email address hidden>
Reported-by: Sean Feole <email address hidden>
Tested-by: Zygmunt Krynicki <email address hidden>

[Daniel Manrique (roadmr)]

Looks good, thanks!

review: Approve

=== modified file 'plainbox/plainbox/impl/commands/run.py'
--- plainbox/plainbox/impl/commands/run.py	2013-03-14 17:06:41 +0000
+++ plainbox/plainbox/impl/commands/run.py	2013-03-19 19:07:23 +0000
@@ -33,6 +33,7 @@
 
 from plainbox.impl.commands import PlainBoxCommand
 from plainbox.impl.commands.checkbox import CheckBoxCommandMixIn
+from plainbox.impl.depmgr import DependencyDuplicateError
 from plainbox.impl.exporter import get_all_exporters
 from plainbox.impl.result import JobResult
 from plainbox.impl.runner import JobRunner
@@ -127,7 +128,18 @@
         matching_job_list = self._get_matching_job_list(ns, job_list)
         print("[ Analyzing Jobs ]".center(80, '='))
         # Create a session that handles most of the stuff needed to run jobs
-        session = SessionState(job_list)
+        try:
+            session = SessionState(job_list)
+        except DependencyDuplicateError as exc:
+            # Handle possible DependencyDuplicateError that can happen if
+            # someone is using plainbox for job development.
+            print("The job database you are currently using is broken")
+            print("At least two jobs contend for the name {0}".format(
+                exc.job.name))
+            print("First job defined in: {0}".format(exc.job.origin))
+            print("Second job defined in: {0}".format(
+                exc.duplicate_job.origin))
+            raise SystemExit(exc)
         with session.open():
             if session.previous_session_file():
                 if self.ask_for_resume():

=== modified file 'plainbox/plainbox/impl/depmgr.py'
--- plainbox/plainbox/impl/depmgr.py	2013-02-25 11:03:02 +0000
+++ plainbox/plainbox/impl/depmgr.py	2013-03-19 19:07:23 +0000
@@ -157,14 +157,21 @@
         graph while still having access and knowledge of all jobs.
 
         :returns list: the solution (a list of jobs to execute in order)
-        :raises DependencyCycleError: if a cyclic dependency is present.
-        :raises DependencyMissingErorr: if a required job does not exist.
+        :raises DependencyDuplicateError:
+            if a duplicate job definition is present
+        :raises DependencyCycleError:
+            if a cyclic dependency is present.
+        :raises DependencyMissingErorr:
+            if a required job does not exist.
         """
         return cls(job_list)._solve(visit_list)
 
     def __init__(self, job_list):
         """
         Instantiate a new dependency solver with the specified list of jobs
+
+        :raises DependencyDuplicateError:
+            if the initial job_list has any duplicate jobs
         """
         # Remember the jobs that were passed
         self._job_list = job_list

=== modified file 'plainbox/plainbox/impl/session.py'
--- plainbox/plainbox/impl/session.py	2013-02-28 11:06:34 +0000
+++ plainbox/plainbox/impl/session.py	2013-03-19 19:07:23 +0000
@@ -1,6 +1,6 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012 Canonical Ltd.
+# Copyright 2012, 2013 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
 #
@@ -31,7 +31,7 @@
 import shutil
 import tempfile
 
-from plainbox.impl.depmgr import DependencyError
+from plainbox.impl.depmgr import DependencyError, DependencyDuplicateError
 from plainbox.impl.depmgr import DependencySolver
 from plainbox.impl.job import JobDefinition
 from plainbox.impl.resource import ExpressionCannotEvaluateError
@@ -297,6 +297,12 @@
         changes this will start out empty and will be changeable dynamically.
         It can still change due to local jobs but there is no API yes.
 
+        This list cannot have any duplicates, if that is the case a
+        DependencyDuplicateError is raised. This has to be handled externally
+        and is a sign that the job database is corrupted or has wrong data. As
+        an exception if duplicates are perfectly identical this error is
+        silently corrected.
+
     :ivar dict job_state_map: mapping that tracks the state of each job
 
         Mapping from job name to :class:`JobState`. This basically has the test
@@ -340,6 +346,38 @@
     session_data_filename = 'session.json'
 
     def __init__(self, job_list):
+        # Start by making a copy of job_list as we may modify it below
+        job_list = job_list[:]
+        while True:
+            try:
+                # Construct a solver with the job list as passed by the caller.
+                # This will do a little bit of validation and might raise
+                # DepdendencyDuplicateError if there are any duplicates at this
+                # stage.
+                #
+                # There's a single case that is handled here though, if both
+                # jobs are identical this problem is silently fixed. This
+                # should not happen in normal circumstances but is non the less
+                # harmless (as long as both jobs are perfectly identical)
+                #
+                # Since this problem can happen any number of times (many
+                # duplicates) this is performed in a loop. The loop breaks when
+                # we cannot solve the problem _OR_ when no error occurs.
+                DependencySolver(job_list)
+            except DependencyDuplicateError as exc:
+                # If both jobs are identical then silently fix the problem by
+                # removing one of the jobs (here the second one we've seen but
+                # it's not relevant as they are possibly identical) and try
+                # again
+                if exc.job == exc.duplicate_job:
+                    job_list.remove(exc.duplicate_job)
+                    continue
+                else:
+                    # If the jobs differ report this back to the caller
+                    raise
+            else:
+                # If there are no problems then break the loop
+                break
         self._job_list = job_list
         self._job_state_map = {job.name: JobState(job)
                                for job in self._job_list}

=== modified file 'plainbox/plainbox/impl/test_depmgr.py'
--- plainbox/plainbox/impl/test_depmgr.py	2013-03-14 10:36:25 +0000
+++ plainbox/plainbox/impl/test_depmgr.py	2013-03-19 19:07:23 +0000
@@ -226,6 +226,16 @@
         observed = DependencySolver.resolve_dependencies(job_list)
         self.assertEqual(expected, observed)
 
+    def test_duplicate_error(self):
+        A = make_job('A')
+        another_A = make_job('A')
+        job_list = [A, another_A]
+        with self.assertRaises(DependencyDuplicateError) as call:
+            DependencySolver.resolve_dependencies(job_list)
+        self.assertIs(call.exception.job, A)
+        self.assertIs(call.exception.duplicate_job, another_A)
+
+
     def test_missing_direct_dependency(self):
         # This tests missing dependencies
         # A -> (inexisting B)

=== modified file 'plainbox/plainbox/impl/test_session.py'
--- plainbox/plainbox/impl/test_session.py	2013-03-14 10:36:25 +0000
+++ plainbox/plainbox/impl/test_session.py	2013-03-19 19:07:23 +0000
@@ -1,6 +1,6 @@
 # This file is part of Checkbox.
 #
-# Copyright 2012 Canonical Ltd.
+# Copyright 2012, 2013 Canonical Ltd.
 # Written by:
 #   Zygmunt Krynicki <[e-mail]>
 #
@@ -32,6 +32,7 @@
 from tempfile import TemporaryDirectory
 from unittest import TestCase
 
+from plainbox.impl.depmgr import DependencyDuplicateError
 from plainbox.impl.depmgr import DependencyMissingError
 from plainbox.impl.resource import Resource
 from plainbox.impl.result import JobResult
@@ -293,6 +294,28 @@
         self.assertIsInstance(problems[0], DependencyMissingError)
         self.assertIs(problems[0].affected_job, A)
 
+    def test_init_with_identical_jobs(self):
+        A = make_job("A")
+        second_A = make_job("A")
+        third_A = make_job("A")
+        # Identical jobs are folded for backwards compatibility with some local
+        # jobs that re-added existing jobs
+        session = SessionState([A, second_A, third_A])
+        # But we don't really store both, just the first one
+        self.assertEqual(session.job_list, [A])
+
+    def test_init_with_colliding_jobs(self):
+        # This is similar to the test above but the jobs actually differ In
+        # this case the _second_ job is rejected but it really signifies a
+        # deeper problem that should only occur during development of jobs
+        A = make_job("A")
+        different_A = make_job("A", plugin="resource")
+        with self.assertRaises(DependencyDuplicateError) as call:
+            SessionState([A, different_A])
+            self.assertIs(call.exception.job, A)
+            self.assertIs(call.exception.duplicate_job, different_A)
+            self.assertIs(call.exception.affected_job, different_A)
+
 
 class SessionStateSpecialTests(TestCase):