upstart

Merge lp://staging/~xnox/upstart/reload-signal into lp://staging/upstart

reload-signal
Merge into trunk

Proposed by Dimitri John Ledkov on 2013-07-22

Status:

Merged

Merged at revision:

1522

Proposed branch:

lp://staging/~xnox/upstart/reload-signal

Merge into:

lp://staging/upstart

Diff against target:

17650 lines (+17195/-157)

14 files modified

dbus/com.ubuntu.Upstart.Instance.xml (+3/-0)
init/job.c (+48/-0)
init/job.h (+2/-0)
init/job_class.c (+11/-0)
init/job_class.h (+3/-0)
init/man/init.5 (+11/-0)
init/parse_job.c (+92/-0)
init/tests/data/upstart-reload-signal.json (+16580/-0)
init/tests/test_job.c (+142/-0)
init/tests/test_job_class.c (+2/-0)
init/tests/test_parse_job.c (+211/-0)
init/tests/test_state.c (+73/-0)
util/initctl.c (+2/-13)
util/tests/test_initctl.c (+15/-144)

To merge this branch:

bzr merge lp://staging/~xnox/upstart/reload-signal

Undecided

Fix Released

Link a bug report

Reviewer	Date Requested	Status
James Hunt	2013-08-21	Approve on 2013-08-22
Dimitri John Ledkov (community)		Approve on 2013-08-21
Review via email: mp+176099@code.staging.launchpad.net

Description of the change

This branch adds "reload signal" stanza.

Instead of initctl calling kill on the main process, it now calls an instance dbus method Reload.

Reload method, looks up reload_signal and calls kill on the main process with it.

Tests added:
* serialisation/deserialisation
* job configuration parsing
* job_reload() function in init/job.c

Broken tests:
* test_initctl reload_action test currently hangs, and thus is commented out. Help with testing it is appreciated, I guess it now needs to simply test that dbus calls are done in the right order without actually testing that signals were sent to a process, as this is now done in the job_reload() test.

Revision history for this message

James Hunt (jamesodhunt) wrote on 2013-07-23:

Hi Dmitrijs,

Initial comments:

* init/job.c: job_reload(): Unused variable blocked.
* init/man/init.5: Needs update for 'reload' stanza.
* init/tests/test_state.c: We need a new test + json file that includes the new encoded reload signal.
* tests/test_job.c: Unused variables dbus_error, error and blocked.

I'll take a look at the broken test later today hopefully...

Revision history for this message

James Hunt (jamesodhunt) wrote on 2013-08-21:

The test hang appears to be caused because the tests fake the dbus server responses from pid 1. However, with the new API, we would also need to either:

1) Fake the call to job_reload().
2) Call job_reload() explicitly.
3) Change the test to actually fire up an upstart instance to avoid faking anything.

Option (3) is preferred of course, but since test_job.c:test_reload() is already doing (3), it should be sufficient to simply test the client is making the correct type of request over D-Bus for test_initctl.c.

lp://staging/~xnox/upstart/reload-signal updated on 2013-08-21

1514. By Dimitri John Ledkov on 2013-08-21

Do not hang with waitid call waiting for SIGHUP on the child process.

The mock dbus server, doesn't actually call job_reload and thus SIGHUP
is not set to the process under test. It's ok to not test this here,
since sending signals & checking them is otherwise tested in
job_reload's harness.

1515. By Dimitri John Ledkov on 2013-08-21