Merge lp:~jamesodhunt/upstart/fix-system-reexec-test into lp:upstart

[James Hunt]

Various fixes and improvements to the system and session tests:

* scripts/pyupstart.py:
  - dbus_encode(): Comments.
  - Upstart::_idle_create_job_cb(): Pass retain option.
  - Upstart::job_create(): Allow specification of retain option to keep
    job configuration file on object destruction.
  - Upstart::job_recreate(): New method to vivify a Job object using an
    existing job configuration file.
  - Job::__init__(): New 'retain' and 'reuse_path' options.
  - Job::get_instance(): New method to obtain a Jobs JobInstance.
  - Job::get_dbus_instance(): Renamed from get_instance().
  - JobInstance::destroy(): NOP for 'retain'ed jobs.
* scripts/tests/test_pyupstart_system_init.py:
  - test_pid1_reexec():
    - Retain the job configuration to allow the object to be recreated
      after re-exec.
* scripts/tests/test_pyupstart_session_init.py:
  - test_session_init_reexec: Add job creation for parity with
    test_pid1_reexec().

[Dimitri John Ledkov (xnox)]

What "::" is in C++ is "." in python =)

pyflakes 3 .
pep8 .

give plenty of warnings, but that's a pre-existing (health) condition.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2013-09-05 16:19:06 +0000
+++ ChangeLog	2013-09-12 10:58:02 +0000
@@ -1,3 +1,24 @@
[phone]  James Hunt  <[e-mail]>
+
+	* scripts/pyupstart.py:
+	  - dbus_encode(): Comments.
+	  - Upstart::_idle_create_job_cb(): Pass retain option.
+	  - Upstart::job_create(): Allow specification of retain option to keep
+	    job configuration file on object destruction.
+	  - Upstart::job_recreate(): New method to vivify a Job object using an
+	    existing job configuration file.
+	  - Job::__init__(): New 'retain' and 'reuse_path' options.
+	  - Job::get_instance(): New method to obtain a Jobs JobInstance.
+	  - Job::get_dbus_instance(): Renamed from get_instance().
+	  - JobInstance::destroy(): NOP for 'retain'ed jobs.
+	* scripts/tests/test_pyupstart_system_init.py:
+	  - test_pid1_reexec():
+	    - Retain the job configuration to allow the object to be recreated
+	      after re-exec.
+	* scripts/tests/test_pyupstart_session_init.py:
+	  - test_session_init_reexec: Add job creation for parity with
+	    test_pid1_reexec().
+
 2013-09-05  James Hunt  <[e-mail]>
 
 	* util/tests/test_initctl.c: test_quiesce():
 
=== modified file 'scripts/pyupstart.py'
--- scripts/pyupstart.py	2013-07-30 08:25:59 +0000
+++ scripts/pyupstart.py	2013-09-12 10:58:02 +0000
@@ -91,8 +91,13 @@
     Note that in the special case of the specified string being None
     or the nul string, it is encoded as '_'.
 
-    Example: 'hello-world' would be encoded as 'hello_2dworld' since
-    '-' is 2d in hex.
+    Examples:
+
+    'hello-world' would be encoded as 'hello_2dworld' since
+    '-' is 2d in hex (resulting in '_2d').
+
+    Similarly, '_2f' would be the encoding for '/' since that character
+    has hex value 2f.
 
     """
     if not str:
@@ -244,7 +249,8 @@
         the main loop starts.
 
         """
-        self.new_job = Job(self, self.test_dir, self.test_dir_name, name, body=body)
+        self.new_job = Job(self, self.test_dir, self.test_dir_name,
+            name, body=body, retain=self.retain)
 
         # deregister
         return False
@@ -377,13 +383,15 @@
         """
         raise NotImplementedError('method must be implemented by subclass')
 
-    def job_create(self, name, body):
+    def job_create(self, name, body, retain=False):
         """
         Create a Job Configuration File.
 
         @name: Name to give the job.
         @body: String representation of configuration file, or list of
          strings.
+        @retain: if True, don't remove the Job Configuration File when
+         object is cleaned up.
 
         Strategy:
 
@@ -421,6 +429,8 @@
         self.job_seen = False
         self.new_job = None
 
+        self.retain = retain
+
         # construct the D-Bus path for the new job
         job_path = '{}/{}'.format(self.test_dir_name, name)
 
@@ -452,6 +462,27 @@
 
         return self.new_job
 
+    def job_recreate(self, name, conf_path):
+        """
+        Create a job object from an existing Job Configuration File.
+
+        @name: Name prefix of existing job configuration file.
+        @conf_path: Full path to *existing* Job Configuration File.
+        """
+
+        assert (name)
+        assert (conf_path)
+
+        job_path = '{}/{}'.format(self.test_dir_name, name)
+        self.job_object_path = '{}/{}/{}'.format(
+            OBJECT_PATH, 'jobs', dbus_encode(job_path)
+        )
+
+        self.new_job = Job(self, self.test_dir, self.test_dir_name,
+                name, body=None, reuse_path=conf_path)
+        self.jobs.append(self.new_job)
+        return self.new_job
+
 
 class Job:
     """
@@ -470,7 +501,8 @@
 
     """
 
-    def __init__(self, upstart, dir_name, subdir_name, job_name, body=None):
+    def __init__(self, upstart, dir_name, subdir_name, job_name,
+            body=None, reuse_path=None, retain=False):
         """
         @upstart: Upstart() parent object.
         @dir_name: Full path to job configuration files directory.
@@ -479,6 +511,10 @@
         @job_name: Name of job.
         @body: Contents of job configuration file (either a string, or a
          list of strings).
+        @reuse_path: If set and @body is None, (re)create the job object
+         using the existing specified job configuration file path.
+        @retain: If True, don't delete the Job Configuration File on
+         object destruction.
         """
 
         self.logger = logging.getLogger(self.__class__.__name__)
@@ -491,6 +527,8 @@
         self.name = job_name
         self.job_dir = dir_name
         self.body = body
+        self.reuse_path = reuse_path
+        self.retain = retain
 
         self.instance_name = None
 
@@ -499,19 +537,30 @@
 
         self.properties = None
 
-        if not self.body:
-            raise UpstartException('No body specified')
+        # need some way to create the job
+        if not self.body and not self.reuse_path:
+            raise UpstartException('No body or reusable path specified')
 
-        self.conffile = os.path.join(self.job_dir, self.name + '.conf')
+        if self.reuse_path:
+            self.conffile = self.reuse_path
+        else:
+            self.conffile = os.path.join(self.job_dir, self.name + '.conf')
 
         if self.body and isinstance(self.body, str):
             # Assume body cannot be a bytes object.
             body = body.splitlines()
 
-        with open(self.conffile, 'w', encoding='utf-8') as fh:
-            for line in body:
-                print(line.strip(), file=fh)
-            print(file=fh)
+        if not self.body and self.reuse_path:
+            # Just check conf file exists
+            if not os.path.exists(self.conffile):
+                raise UpstartException(
+                        'File {} does not exist for reuse'.format(self.conffile))
+        else:
+            # Create conf file
+            with open(self.conffile, 'w', encoding='utf-8') as fh:
+                for line in body:
+                    print(line.strip(), file=fh)
+                print(file=fh)
 
         self.valid = True
 
@@ -533,7 +582,8 @@
             for instance in self.instances:
                 instance.destroy()
 
-            os.remove(self.conffile)
+            if not self.retain:
+                os.remove(self.conffile)
         except FileNotFoundError:
             pass
 
@@ -551,19 +601,44 @@
         if env is None:
             env = []
         instance_path = self.interface.Start(dbus.Array(env, 's'), wait)
-        instance_name = instance_path.replace("%s/" % self.object_path, '')
-
-        # store the D-Bus encoded instance name ('_' for single-instance jobs)
-        if instance_name not in self.instance_names:
-            self.instance_names.append(instance_name)
-
-        instance = JobInstance(self, instance_name, instance_path)
-        self.instances.append(instance)
-        return instance
-
-    def _get_instance(self, name):
-        """
-        Retrieve job instance and its properties.
+
+        instance_name = instance_path.replace("%s/" % self.object_path, '')
+
+        # store the D-Bus encoded instance name ('_' for single-instance jobs)
+        if instance_name not in self.instance_names:
+            self.instance_names.append(instance_name)
+
+        instance = JobInstance(self, instance_name, instance_path)
+        self.instances.append(instance)
+        return instance
+
+    def get_instance(self):
+        """
+        Returns: JobInstance of calling Job.
+        """
+
+        # construct the D-Bus path for the new job
+        job_path = '{}/{}'.format(self.upstart.test_dir_name, self.name)
+
+        instance_path = '{}/{}/{}/{}'.format(
+            OBJECT_PATH, 'jobs', dbus_encode(job_path),
+
+            # XXX: LIMITATION - only support default instance.
+            '_'
+        )
+        instance_name = instance_path.replace("%s/" % self.object_path, '')
+
+        # store the D-Bus encoded instance name ('_' for single-instance jobs)
+        if instance_name not in self.instance_names:
+            self.instance_names.append(instance_name)
+
+        instance = JobInstance(self, instance_name, instance_path)
+        self.instances.append(instance)
+        return instance
+
+    def _get_dbus_instance(self, name):
+        """
+        Retrieve D-Bus job instance and its properties.
 
         @name: D-Bus encoded instance name.
 
@@ -586,7 +661,7 @@
         """
 
         for name in self.instance_names:
-            instance = self._get_instance(name)
+            instance = self._get_dbus_instance(name)
             try:
                 instance.Stop(wait)
             except dbus.exceptions.DBusException:
@@ -600,7 +675,7 @@
         @wait: if False, stop job instances asynchronously.
         """
         for name in self.instance_names:
-            instance = self._get_instance(name)
+            instance = self._get_dbus_instance(name)
             instance.Restart(wait)
 
     def instance_object_paths(self):
@@ -629,9 +704,13 @@
 
         assert(name in self.instance_names)
 
-        instance = self._get_instance(name)
+        instance = self._get_dbus_instance(name)
+        assert (instance)
 
         properties = dbus.Interface(instance, FREEDESKTOP_PROPERTIES)
+        assert (properties)
+
+        # don't assert as there may not be any processes
         procs = properties.Get(INSTANCE_INTERFACE_NAME, 'processes')
 
         pid_map = {}
@@ -845,10 +924,13 @@
     def destroy(self):
         """
         Stop the instance and cleanup.
+
+	Note: If the instance specified retain when created, this will
+	be a NOP.
         """
-        self.stop()
-        self.logfile.destroy()
-
+        if not self.job.retain:
+            self.stop()
+            self.logfile.destroy()
 
 class SystemInit(Upstart):
 
 
=== modified file 'scripts/tests/test_pyupstart_session_init.py'
--- scripts/tests/test_pyupstart_session_init.py	2013-08-06 16:54:28 +0000
+++ scripts/tests/test_pyupstart_session_init.py	2013-09-12 10:58:02 +0000
@@ -229,6 +229,23 @@
         # Ensure no stateful-reexec already performed.
         self.assertFalse(re.search('state-fd', output))
 
+        version = self.upstart.version()
+        self.assertTrue(version)
+
+        # create a job and start it, marking it such that the .conf file
+        # will be retained when object becomes unusable (after re-exec).
+        job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
+        self.assertTrue(job)
+
+        # Used when recreating the job
+        conf_path = job.conffile
+
+        inst = job.start()
+        self.assertTrue(inst)
+        pids = job.pids()
+        self.assertEqual(len(pids), 1)
+        pid = pids['main']
+
         # Trigger re-exec and catch the D-Bus exception resulting
         # from disconnection from Session Init when it severs client
         # connections.
@@ -248,8 +265,40 @@
         # instantaneous, try a few times.
         self.upstart.polling_connect(force=True)
 
+        # Since the parent job was created with 'retain', this is actually
+        # a NOP but is added to denote that the old instance is dead.
+        inst.destroy()
+
         # check that we can still operate on the re-exec'd Upstart
-        self.assertTrue(self.upstart.version())
+        version_postexec = self.upstart.version()
+        self.assertTrue(version_postexec)
+        self.assertEqual(version, version_postexec)
+
+        # Ensure the job is still running with the same PID
+        os.kill(pid, 0)
+
+        # XXX: The re-exec will have severed the D-Bus connection to
+        # Upstart. Hence, revivify the job with some magic.
+        job = self.upstart.job_recreate('sleeper', conf_path)
+        self.assertTrue(job)
+
+        # Recreate the instance
+        inst = job.get_instance()
+        self.assertTrue(inst)
+
+        self.assertTrue(job.running('_'))
+        pids = job.pids()
+        self.assertEqual(len(pids), 1)
+        self.assertTrue(pids['main'])
+
+        # The pid should not have changed after a restart
+        self.assertEqual(pid, pids['main'])
+
+        job.stop()
+
+        # Ensure the pid has gone
+        with self.assertRaises(ProcessLookupError):
+            os.kill(pid, 0)
 
         self.stop_session_init()
 
 
=== modified file 'scripts/tests/test_pyupstart_system_init.py'
--- scripts/tests/test_pyupstart_system_init.py	2013-08-06 16:54:28 +0000
+++ scripts/tests/test_pyupstart_system_init.py	2013-09-12 10:58:02 +0000
@@ -63,9 +63,12 @@
 
       # create a job and start it, marking it such that the .conf file
       # will be retained when object becomes unusable (after re-exec).
-      job = self.upstart.job_create('sleeper', 'exec sleep 123')
+      job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
       self.assertTrue(job)
 
+      # Used when recreating the job
+      conf_path = job.conffile
+
       inst = job.start()
       self.assertTrue(inst)
       pids = job.pids()
@@ -80,6 +83,10 @@
       # try a few times.
       self.upstart.polling_connect(force=True)
 
+      # Since the parent job was created with 'retain', this is actually
+      # a NOP but is added to denote that the old instance is dead.
+      inst.destroy()
+
       # check that we can still operate on the re-exec'd Upstart
       version_postexec = self.upstart.version()
       self.assertTrue(version_postexec)
@@ -88,32 +95,29 @@
       # Ensure the job is still running with the same PID
       os.kill(pid, 0)
 
+      # XXX: The re-exec will have severed the D-Bus connection to
+      # Upstart. Hence, revivify the job with some magic.
+      job = self.upstart.job_recreate('sleeper', conf_path)
+      self.assertTrue(job)
+
+      # Recreate the instance
+      inst = job.get_instance()
+      self.assertTrue(inst)
+
       self.assertTrue(job.running('_'))
-
       pids = job.pids()
       self.assertEqual(len(pids), 1)
       self.assertTrue(pids['main'])
 
-      # Ensure pid remains the same
+      # The pid should not have changed after a restart
       self.assertEqual(pid, pids['main'])
 
-      # Exceptions will be caught by the unittest framework
-      inst.stop()
-
-      job.start()
-
-      pids = job.pids()
-      self.assertEqual(len(pids), 1)
-      self.assertTrue(pids['main'])
-
-      os.kill(pids['main'], 0)
-      self.assertTrue(job.running('_'))
-
-      # The pid should have changed after a restart
-      self.assertNotEqual(pid, pids['main'])
-
       job.stop()
 
+      # Ensure the pid has gone
+      with self.assertRaises(ProcessLookupError):
+          os.kill(pid, 0)
+
       # Clean up
       self.upstart.destroy()