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

Proposed by James Hunt on 2013-09-12
Status: Merged
Merged at revision: 1550
Proposed branch: lp:~jamesodhunt/upstart/fix-system-reexec-test
Merge into: lp:upstart
Diff against target: 443 lines (+207/-51)
4 files modified
ChangeLog (+21/-0)
scripts/pyupstart.py (+114/-32)
scripts/tests/test_pyupstart_session_init.py (+50/-1)
scripts/tests/test_pyupstart_system_init.py (+22/-18)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/fix-system-reexec-test
Reviewer Review Type Date Requested Status
Dimitri John Ledkov 2013-09-12 Approve on 2013-11-03
Review via email: mp+185238@code.launchpad.net

Description of the change

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().

To post a comment you must log in.
Dimitri John Ledkov (xnox) wrote :

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

pyflakes 3 .
pep8 .

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2013-09-05 16:19:06 +0000
3+++ ChangeLog 2013-09-12 10:58:02 +0000
4@@ -1,3 +1,24 @@
5+2013-09-12 James Hunt <james.hunt@ubuntu.com>
6+
7+ * scripts/pyupstart.py:
8+ - dbus_encode(): Comments.
9+ - Upstart::_idle_create_job_cb(): Pass retain option.
10+ - Upstart::job_create(): Allow specification of retain option to keep
11+ job configuration file on object destruction.
12+ - Upstart::job_recreate(): New method to vivify a Job object using an
13+ existing job configuration file.
14+ - Job::__init__(): New 'retain' and 'reuse_path' options.
15+ - Job::get_instance(): New method to obtain a Jobs JobInstance.
16+ - Job::get_dbus_instance(): Renamed from get_instance().
17+ - JobInstance::destroy(): NOP for 'retain'ed jobs.
18+ * scripts/tests/test_pyupstart_system_init.py:
19+ - test_pid1_reexec():
20+ - Retain the job configuration to allow the object to be recreated
21+ after re-exec.
22+ * scripts/tests/test_pyupstart_session_init.py:
23+ - test_session_init_reexec: Add job creation for parity with
24+ test_pid1_reexec().
25+
26 2013-09-05 James Hunt <james.hunt@ubuntu.com>
27
28 * util/tests/test_initctl.c: test_quiesce():
29
30=== modified file 'scripts/pyupstart.py'
31--- scripts/pyupstart.py 2013-07-30 08:25:59 +0000
32+++ scripts/pyupstart.py 2013-09-12 10:58:02 +0000
33@@ -91,8 +91,13 @@
34 Note that in the special case of the specified string being None
35 or the nul string, it is encoded as '_'.
36
37- Example: 'hello-world' would be encoded as 'hello_2dworld' since
38- '-' is 2d in hex.
39+ Examples:
40+
41+ 'hello-world' would be encoded as 'hello_2dworld' since
42+ '-' is 2d in hex (resulting in '_2d').
43+
44+ Similarly, '_2f' would be the encoding for '/' since that character
45+ has hex value 2f.
46
47 """
48 if not str:
49@@ -244,7 +249,8 @@
50 the main loop starts.
51
52 """
53- self.new_job = Job(self, self.test_dir, self.test_dir_name, name, body=body)
54+ self.new_job = Job(self, self.test_dir, self.test_dir_name,
55+ name, body=body, retain=self.retain)
56
57 # deregister
58 return False
59@@ -377,13 +383,15 @@
60 """
61 raise NotImplementedError('method must be implemented by subclass')
62
63- def job_create(self, name, body):
64+ def job_create(self, name, body, retain=False):
65 """
66 Create a Job Configuration File.
67
68 @name: Name to give the job.
69 @body: String representation of configuration file, or list of
70 strings.
71+ @retain: if True, don't remove the Job Configuration File when
72+ object is cleaned up.
73
74 Strategy:
75
76@@ -421,6 +429,8 @@
77 self.job_seen = False
78 self.new_job = None
79
80+ self.retain = retain
81+
82 # construct the D-Bus path for the new job
83 job_path = '{}/{}'.format(self.test_dir_name, name)
84
85@@ -452,6 +462,27 @@
86
87 return self.new_job
88
89+ def job_recreate(self, name, conf_path):
90+ """
91+ Create a job object from an existing Job Configuration File.
92+
93+ @name: Name prefix of existing job configuration file.
94+ @conf_path: Full path to *existing* Job Configuration File.
95+ """
96+
97+ assert (name)
98+ assert (conf_path)
99+
100+ job_path = '{}/{}'.format(self.test_dir_name, name)
101+ self.job_object_path = '{}/{}/{}'.format(
102+ OBJECT_PATH, 'jobs', dbus_encode(job_path)
103+ )
104+
105+ self.new_job = Job(self, self.test_dir, self.test_dir_name,
106+ name, body=None, reuse_path=conf_path)
107+ self.jobs.append(self.new_job)
108+ return self.new_job
109+
110
111 class Job:
112 """
113@@ -470,7 +501,8 @@
114
115 """
116
117- def __init__(self, upstart, dir_name, subdir_name, job_name, body=None):
118+ def __init__(self, upstart, dir_name, subdir_name, job_name,
119+ body=None, reuse_path=None, retain=False):
120 """
121 @upstart: Upstart() parent object.
122 @dir_name: Full path to job configuration files directory.
123@@ -479,6 +511,10 @@
124 @job_name: Name of job.
125 @body: Contents of job configuration file (either a string, or a
126 list of strings).
127+ @reuse_path: If set and @body is None, (re)create the job object
128+ using the existing specified job configuration file path.
129+ @retain: If True, don't delete the Job Configuration File on
130+ object destruction.
131 """
132
133 self.logger = logging.getLogger(self.__class__.__name__)
134@@ -491,6 +527,8 @@
135 self.name = job_name
136 self.job_dir = dir_name
137 self.body = body
138+ self.reuse_path = reuse_path
139+ self.retain = retain
140
141 self.instance_name = None
142
143@@ -499,19 +537,30 @@
144
145 self.properties = None
146
147- if not self.body:
148- raise UpstartException('No body specified')
149+ # need some way to create the job
150+ if not self.body and not self.reuse_path:
151+ raise UpstartException('No body or reusable path specified')
152
153- self.conffile = os.path.join(self.job_dir, self.name + '.conf')
154+ if self.reuse_path:
155+ self.conffile = self.reuse_path
156+ else:
157+ self.conffile = os.path.join(self.job_dir, self.name + '.conf')
158
159 if self.body and isinstance(self.body, str):
160 # Assume body cannot be a bytes object.
161 body = body.splitlines()
162
163- with open(self.conffile, 'w', encoding='utf-8') as fh:
164- for line in body:
165- print(line.strip(), file=fh)
166- print(file=fh)
167+ if not self.body and self.reuse_path:
168+ # Just check conf file exists
169+ if not os.path.exists(self.conffile):
170+ raise UpstartException(
171+ 'File {} does not exist for reuse'.format(self.conffile))
172+ else:
173+ # Create conf file
174+ with open(self.conffile, 'w', encoding='utf-8') as fh:
175+ for line in body:
176+ print(line.strip(), file=fh)
177+ print(file=fh)
178
179 self.valid = True
180
181@@ -533,7 +582,8 @@
182 for instance in self.instances:
183 instance.destroy()
184
185- os.remove(self.conffile)
186+ if not self.retain:
187+ os.remove(self.conffile)
188 except FileNotFoundError:
189 pass
190
191@@ -551,19 +601,44 @@
192 if env is None:
193 env = []
194 instance_path = self.interface.Start(dbus.Array(env, 's'), wait)
195- instance_name = instance_path.replace("%s/" % self.object_path, '')
196-
197- # store the D-Bus encoded instance name ('_' for single-instance jobs)
198- if instance_name not in self.instance_names:
199- self.instance_names.append(instance_name)
200-
201- instance = JobInstance(self, instance_name, instance_path)
202- self.instances.append(instance)
203- return instance
204-
205- def _get_instance(self, name):
206- """
207- Retrieve job instance and its properties.
208+
209+ instance_name = instance_path.replace("%s/" % self.object_path, '')
210+
211+ # store the D-Bus encoded instance name ('_' for single-instance jobs)
212+ if instance_name not in self.instance_names:
213+ self.instance_names.append(instance_name)
214+
215+ instance = JobInstance(self, instance_name, instance_path)
216+ self.instances.append(instance)
217+ return instance
218+
219+ def get_instance(self):
220+ """
221+ Returns: JobInstance of calling Job.
222+ """
223+
224+ # construct the D-Bus path for the new job
225+ job_path = '{}/{}'.format(self.upstart.test_dir_name, self.name)
226+
227+ instance_path = '{}/{}/{}/{}'.format(
228+ OBJECT_PATH, 'jobs', dbus_encode(job_path),
229+
230+ # XXX: LIMITATION - only support default instance.
231+ '_'
232+ )
233+ instance_name = instance_path.replace("%s/" % self.object_path, '')
234+
235+ # store the D-Bus encoded instance name ('_' for single-instance jobs)
236+ if instance_name not in self.instance_names:
237+ self.instance_names.append(instance_name)
238+
239+ instance = JobInstance(self, instance_name, instance_path)
240+ self.instances.append(instance)
241+ return instance
242+
243+ def _get_dbus_instance(self, name):
244+ """
245+ Retrieve D-Bus job instance and its properties.
246
247 @name: D-Bus encoded instance name.
248
249@@ -586,7 +661,7 @@
250 """
251
252 for name in self.instance_names:
253- instance = self._get_instance(name)
254+ instance = self._get_dbus_instance(name)
255 try:
256 instance.Stop(wait)
257 except dbus.exceptions.DBusException:
258@@ -600,7 +675,7 @@
259 @wait: if False, stop job instances asynchronously.
260 """
261 for name in self.instance_names:
262- instance = self._get_instance(name)
263+ instance = self._get_dbus_instance(name)
264 instance.Restart(wait)
265
266 def instance_object_paths(self):
267@@ -629,9 +704,13 @@
268
269 assert(name in self.instance_names)
270
271- instance = self._get_instance(name)
272+ instance = self._get_dbus_instance(name)
273+ assert (instance)
274
275 properties = dbus.Interface(instance, FREEDESKTOP_PROPERTIES)
276+ assert (properties)
277+
278+ # don't assert as there may not be any processes
279 procs = properties.Get(INSTANCE_INTERFACE_NAME, 'processes')
280
281 pid_map = {}
282@@ -845,10 +924,13 @@
283 def destroy(self):
284 """
285 Stop the instance and cleanup.
286+
287+ Note: If the instance specified retain when created, this will
288+ be a NOP.
289 """
290- self.stop()
291- self.logfile.destroy()
292-
293+ if not self.job.retain:
294+ self.stop()
295+ self.logfile.destroy()
296
297 class SystemInit(Upstart):
298
299
300=== modified file 'scripts/tests/test_pyupstart_session_init.py'
301--- scripts/tests/test_pyupstart_session_init.py 2013-08-06 16:54:28 +0000
302+++ scripts/tests/test_pyupstart_session_init.py 2013-09-12 10:58:02 +0000
303@@ -229,6 +229,23 @@
304 # Ensure no stateful-reexec already performed.
305 self.assertFalse(re.search('state-fd', output))
306
307+ version = self.upstart.version()
308+ self.assertTrue(version)
309+
310+ # create a job and start it, marking it such that the .conf file
311+ # will be retained when object becomes unusable (after re-exec).
312+ job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
313+ self.assertTrue(job)
314+
315+ # Used when recreating the job
316+ conf_path = job.conffile
317+
318+ inst = job.start()
319+ self.assertTrue(inst)
320+ pids = job.pids()
321+ self.assertEqual(len(pids), 1)
322+ pid = pids['main']
323+
324 # Trigger re-exec and catch the D-Bus exception resulting
325 # from disconnection from Session Init when it severs client
326 # connections.
327@@ -248,8 +265,40 @@
328 # instantaneous, try a few times.
329 self.upstart.polling_connect(force=True)
330
331+ # Since the parent job was created with 'retain', this is actually
332+ # a NOP but is added to denote that the old instance is dead.
333+ inst.destroy()
334+
335 # check that we can still operate on the re-exec'd Upstart
336- self.assertTrue(self.upstart.version())
337+ version_postexec = self.upstart.version()
338+ self.assertTrue(version_postexec)
339+ self.assertEqual(version, version_postexec)
340+
341+ # Ensure the job is still running with the same PID
342+ os.kill(pid, 0)
343+
344+ # XXX: The re-exec will have severed the D-Bus connection to
345+ # Upstart. Hence, revivify the job with some magic.
346+ job = self.upstart.job_recreate('sleeper', conf_path)
347+ self.assertTrue(job)
348+
349+ # Recreate the instance
350+ inst = job.get_instance()
351+ self.assertTrue(inst)
352+
353+ self.assertTrue(job.running('_'))
354+ pids = job.pids()
355+ self.assertEqual(len(pids), 1)
356+ self.assertTrue(pids['main'])
357+
358+ # The pid should not have changed after a restart
359+ self.assertEqual(pid, pids['main'])
360+
361+ job.stop()
362+
363+ # Ensure the pid has gone
364+ with self.assertRaises(ProcessLookupError):
365+ os.kill(pid, 0)
366
367 self.stop_session_init()
368
369
370=== modified file 'scripts/tests/test_pyupstart_system_init.py'
371--- scripts/tests/test_pyupstart_system_init.py 2013-08-06 16:54:28 +0000
372+++ scripts/tests/test_pyupstart_system_init.py 2013-09-12 10:58:02 +0000
373@@ -63,9 +63,12 @@
374
375 # create a job and start it, marking it such that the .conf file
376 # will be retained when object becomes unusable (after re-exec).
377- job = self.upstart.job_create('sleeper', 'exec sleep 123')
378+ job = self.upstart.job_create('sleeper', 'exec sleep 123', retain=True)
379 self.assertTrue(job)
380
381+ # Used when recreating the job
382+ conf_path = job.conffile
383+
384 inst = job.start()
385 self.assertTrue(inst)
386 pids = job.pids()
387@@ -80,6 +83,10 @@
388 # try a few times.
389 self.upstart.polling_connect(force=True)
390
391+ # Since the parent job was created with 'retain', this is actually
392+ # a NOP but is added to denote that the old instance is dead.
393+ inst.destroy()
394+
395 # check that we can still operate on the re-exec'd Upstart
396 version_postexec = self.upstart.version()
397 self.assertTrue(version_postexec)
398@@ -88,32 +95,29 @@
399 # Ensure the job is still running with the same PID
400 os.kill(pid, 0)
401
402+ # XXX: The re-exec will have severed the D-Bus connection to
403+ # Upstart. Hence, revivify the job with some magic.
404+ job = self.upstart.job_recreate('sleeper', conf_path)
405+ self.assertTrue(job)
406+
407+ # Recreate the instance
408+ inst = job.get_instance()
409+ self.assertTrue(inst)
410+
411 self.assertTrue(job.running('_'))
412-
413 pids = job.pids()
414 self.assertEqual(len(pids), 1)
415 self.assertTrue(pids['main'])
416
417- # Ensure pid remains the same
418+ # The pid should not have changed after a restart
419 self.assertEqual(pid, pids['main'])
420
421- # Exceptions will be caught by the unittest framework
422- inst.stop()
423-
424- job.start()
425-
426- pids = job.pids()
427- self.assertEqual(len(pids), 1)
428- self.assertTrue(pids['main'])
429-
430- os.kill(pids['main'], 0)
431- self.assertTrue(job.running('_'))
432-
433- # The pid should have changed after a restart
434- self.assertNotEqual(pid, pids['main'])
435-
436 job.stop()
437
438+ # Ensure the pid has gone
439+ with self.assertRaises(ProcessLookupError):
440+ os.kill(pid, 0)
441+
442 # Clean up
443 self.upstart.destroy()
444

Subscribers

People subscribed via source and target branches