Merge lp:~jamesodhunt/upstart/test-quiesce-cleanup into lp:upstart

[James Hunt]

* util/tests/test_initctl.c: test_quiesce(): Clean up any processes that
  the Session Init couldn't before it shut down.

Note that such rogues can cause issues for pbuilder.

[Steve Langasek (vorlon)]

Hi James,

Some of these changes appear to be for cases where we launch a session init with a single job, and then shut down the session init. Why should the session init ever fail to kill the job's process in this case? Isn't this a bug in the session init itself, not in the test case?

If a session init can fail to kill -9 its subprocesses under test, it can also fail to do so in the real world; that sounds like a bug to me that we should fix, not work around in the test.

review: Needs Information

[lp:~jamesodhunt/upstart/test-quiesce-cleanup]

1528. By James Hunt on 2013-09-05

* util/tests/test_initctl.c: test_quiesce():
  - Improve kill checks on job processes.
  - Assert precisely which job processes are expected to be running
    after the Session Init has exited (particularly important for jobs
    that 'start on session-end' since they may be running in a System
    Shutdown scenario).

[James Hunt (jamesodhunt)]

Hi Steve,

The tests are simulating both System Shutdown and Session Shutdown.

For the Session Shutdown scenario, the Session Init will wait for the jobs, hence we expect all job processes (even those that block SIGTERM) to be dead by the time the Session Init exits since it will have sent such jobs SIGKILL.

However, in the System Shutdown scenario, when the Display Manager receives SIGTERM to denote system shutdown, it will send the same signal to all its clients, one of which is the Session Init. The Session Init will then send all job processes the job->class->kill_signal signal. However, since the Session Init should not hold up the system shutdown, although it attempts to wait for 5 seconds after sending SIGTERM before sending the final SIGKILL to each job process, it may already have been SIGKILL'd by the Display Manager: on an Ubuntu system, the DM waits 5 seconds between sending SIGTERM and SIGKILL... and the Session Init waits 5 seconds between the time it sent SIGTERM to each job process and the time it sends SIGKILL. The tests were attempting to simulate this exact behaviour but that does admittedly look "odd". I've now updated the branch to avoid such absolute mimicry but at the same time be more careful wrt checking which pids we expect to have stopped and which could still be running.

What this branch does highlight (and which the tests now comment on explicitly) is that any job that specifies "start on session-end" could still be running after the Session Init has exited but only in a System Shutdown context (since the "session-end" event is only emitted in the next iteration of the main loop - after Upstart has sent SIGTERM to all running job processes). As above, on an actual system those processes would get killed by the System Init.

[Steve Langasek (vorlon)]

The updated branch looks good to me - that precisely addresses my concerns about inconsistent/unreliable tests, thanks.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2013-08-23 14:49:09 +0000
+++ ChangeLog	2013-09-05 16:19:52 +0000
@@ -1,3 +1,17 @@
[phone]  James Hunt  <[e-mail]>
+
+	* util/tests/test_initctl.c: test_quiesce():
+	  - Improve kill checks on job processes.
+	  - Assert precisely which job processes are expected to be running
+	    after the Session Init has exited (particularly important for jobs
+	    that 'start on session-end' since they may be running in a System
+	    Shutdown scenario).
+
[phone]  James Hunt  <[e-mail]>
+
+	* util/tests/test_initctl.c: test_quiesce(): Clean up any
+	  processes that the Session Init couldn't before it shut down.
+
 2013-08-23  James Hunt  <[e-mail]>
 
 	* NEWS: Release 1.10

=== modified file 'util/tests/test_initctl.c'
--- util/tests/test_initctl.c	2013-08-23 11:23:19 +0000
+++ util/tests/test_initctl.c	2013-09-05 16:19:52 +0000
@@ -11125,6 +11125,7 @@
 {
 	char                      confdir[PATH_MAX];
 	char                      logdir[PATH_MAX];
+	char                      pid_file[PATH_MAX];
 	char                      sessiondir[PATH_MAX];
 	nih_local char           *cmd = NULL;
 	pid_t                     upstart_pid = 0;
@@ -11135,6 +11136,8 @@
 	nih_local NihDBusProxy   *upstart = NULL;
 	nih_local char           *orig_xdg_runtime_dir = NULL;
 	nih_local char           *session_file = NULL;
+	nih_local char           *job = NULL;
+	pid_t                     job_pid;
 
 	TEST_GROUP ("Session Init quiesce");
 
@@ -11204,21 +11207,64 @@
 	TEST_EQ (lines, 1);
 	nih_free (output);
 
+	job_pid = job_to_pid ("long-running");
+	TEST_NE (job_pid, -1);
+
 	/* Trigger shutdown */
 	assert0 (kill (upstart_pid, SIGTERM));
 
 	/* Force reset */
 	test_user_mode = FALSE;
 
-	TEST_EQ (timed_waitpid (upstart_pid, TEST_QUIESCE_KILL_PHASE), upstart_pid);
+	/* Wait for longer than we expect the Session Init to take to
+	 * shutdown to give it time to send SIGKILL to all job
+	 * processes. This is unrealistic, but safer for the tests since
+	 * the exact behaviour can be checked.
+	 *
+	 * In reality, the following steps either side of the markers *will*
+	 * occur and those within the markers *may* occur:
+	 *
+	 * 1) A System Shutdown is triggered.
+	 * 2) The Display Manager receives SIGTERM.
+	 * 3) The Display Manager sends SIGTERM to all its clients.
+	 *    (including the Session Init).
+	 * 4) The Session Init sends SIGTERM to all running job
+	 *    processes.
+	 *
+	 *  --- :XXX: START MARKER :XXX: ---
+	 *
+	 * 5) The Session Init will attempt to wait for
+	 *    MAX(kill_timeout) seconds.
+	 * 6) The Session Init will send all running job processes
+	 *    SIGKILL.
+	 * 7) The Session Init will wait for all remaining job processes
+	 *    to end.
+	 * 8) The Session Init will exit.
+	 *
+	 *  --- :XXX: END MARKER :XXX: ---
+	 *
+	 * 9) The Display Manager sends SIGKILL to all its clients.
+	 * 10) If still running, the Session Init is killed and exits.
+	 *
+	 * The problem is that the Session Init cannot know when the
+	 * Display Manager will kill *it* so it may be that the Session
+	 * Init cannot send SIGKILL to each job process instead relying
+	 * on the System Init to clean up.
+	 */
+	TEST_EQ (timed_waitpid (upstart_pid, 1+TEST_QUIESCE_KILL_PHASE), upstart_pid);
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
 	DELETE_FILE (confdir, "long-running.conf");
 
 	/*******************************************************************/
@@ -11243,33 +11289,45 @@
 	TEST_EQ (lines, 1);
 	nih_free (output);
 
+	job_pid = job_to_pid ("long-running-term");
+	TEST_NE (job_pid, -1);
+
 	/* Trigger shutdown */
 	assert0 (kill (upstart_pid, SIGTERM));
 
 	/* Force reset */
 	test_user_mode = FALSE;
 
-	TEST_EQ (timed_waitpid (upstart_pid, TEST_QUIESCE_KILL_PHASE), upstart_pid);
+	TEST_EQ (timed_waitpid (upstart_pid, 1+TEST_QUIESCE_KILL_PHASE), upstart_pid);
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
 	DELETE_FILE (confdir, "long-running-term.conf");
 
 	/*******************************************************************/
 	TEST_FEATURE ("system shutdown: one job which starts on session-end");
 
-	CREATE_FILE (confdir, "session-end.conf",
-			"start on session-end\n"
-			"\n"
-			"script\n"
-			"  echo hello\n"
-			"  sleep 999\n"
-			"end script");
+	TEST_FILENAME (pid_file);
+
+	job = NIH_MUST (nih_sprintf (NULL, "start on session-end\n"
+				"\n"
+				"script\n"
+				"  echo hello\n"
+				"  echo $$ >\"%s\"\n"
+				"  exec sleep 999\n"
+				"end script", pid_file));
+
+	CREATE_FILE (confdir, "session-end.conf", job);
 
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -11282,10 +11340,11 @@
 	/* Force reset */
 	test_user_mode = FALSE;
 
-	TEST_EQ (timed_waitpid (upstart_pid, TEST_QUIESCE_KILL_PHASE), upstart_pid);
+	TEST_EQ (timed_waitpid (upstart_pid, 1+TEST_QUIESCE_KILL_PHASE), upstart_pid);
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
 				logdir,
@@ -11302,19 +11361,36 @@
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	file = fopen (pid_file, "r");
+	TEST_NE_P (file, NULL);
+	TEST_EQ (fscanf (file, "%d", &job_pid), 1);
+	fclose (file);
+
+	/* pid should be running since Upstart won't have signalled it
+	 * to stop (since it started as a result of session-end being
+	 * emitted _after_ the job pids were sent SIGTERM).
+	 */
+	TEST_EQ (kill (job_pid, SIGKILL), 0);
+
+	assert0 (unlink (pid_file));
+
 	DELETE_FILE (confdir, "session-end.conf");
 
 	/*******************************************************************/
 	TEST_FEATURE ("system shutdown: one job which starts on session-end and ignores SIGTERM");
 
-	CREATE_FILE (confdir, "session-end-term.conf",
-			"start on session-end\n"
-			"\n"
-			"script\n"
-			"  trap '' TERM\n"
-			"  echo hello\n"
-			"  sleep 999\n"
-			"end script");
+	TEST_FILENAME (pid_file);
+
+	job = NIH_MUST (nih_sprintf (NULL, "start on session-end\n"
+				"\n"
+				"script\n"
+				"  trap '' TERM\n"
+				"  echo hello\n"
+				"  echo $$ >\"%s\"\n"
+				"  exec sleep 999\n"
+				"end script", pid_file));
+
+	CREATE_FILE (confdir, "session-end-term.conf", job);
 
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -11327,10 +11403,11 @@
 	/* Force reset */
 	test_user_mode = FALSE;
 
-	TEST_EQ (timed_waitpid (upstart_pid, TEST_QUIESCE_KILL_PHASE), upstart_pid);
+	TEST_EQ (timed_waitpid (upstart_pid, 1+TEST_QUIESCE_KILL_PHASE), upstart_pid);
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	logfile = NIH_MUST (nih_sprintf (NULL, "%s/%s",
 				logdir,
@@ -11347,6 +11424,17 @@
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* kill job pid if not already dead */
+	file = fopen (pid_file, "r");
+	TEST_NE_P (file, NULL);
+	TEST_EQ (fscanf (file, "%d", &job_pid), 1);
+	fclose (file);
+
+	/* pid should still be running */
+	TEST_EQ (kill (job_pid, SIGKILL), 0);
+
+	assert0 (unlink (pid_file));
+
 	DELETE_FILE (confdir, "session-end-term.conf");
 
 	/*******************************************************************/
@@ -11357,17 +11445,20 @@
 	CREATE_FILE (confdir, "long-running-term.conf",
 			"script\n"
 			"  trap '' TERM\n"
-		        "  sleep 999\n"
-			"end script");
-
-	CREATE_FILE (confdir, "session-end-term.conf",
-			"start on session-end\n"
-			"\n"
-			"script\n"
-			"  trap '' TERM\n"
-			"  sleep 999\n"
-			"end script");
-
+		        "  exec sleep 999\n"
+			"end script");
+
+	TEST_FILENAME (pid_file);
+
+	job = NIH_MUST (nih_sprintf (NULL, "start on session-end\n"
+				"\n"
+				"script\n"
+				"  trap '' TERM\n"
+				"  echo $$ >\"%s\"\n"
+				"  exec sleep 999\n"
+				"end script", pid_file));
+
+	CREATE_FILE (confdir, "session-end-term.conf", job);
 
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -11382,21 +11473,39 @@
 	TEST_EQ (lines, 1);
 	nih_free (output);
 
+	job_pid = job_to_pid ("long-running-term");
+	TEST_NE (job_pid, -1);
+
 	/* Trigger shutdown */
 	assert0 (kill (upstart_pid, SIGTERM));
 
 	/* Force reset */
 	test_user_mode = FALSE;
 
-	TEST_EQ (timed_waitpid (upstart_pid, TEST_QUIESCE_KILL_PHASE), upstart_pid);
+	TEST_EQ (timed_waitpid (upstart_pid, 1+TEST_QUIESCE_KILL_PHASE), upstart_pid);
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* the long-running job pid should no longer exist */
+	kill (job_pid, SIGKILL);
+	TEST_EQ (errno, ESRCH);
+
+	file = fopen (pid_file, "r");
+	TEST_NE_P (file, NULL);
+	TEST_EQ (fscanf (file, "%d", &job_pid), 1);
+	fclose (file);
+
+	/* .... but the session-end job pid should still be running */
+	TEST_EQ (kill (job_pid, SIGKILL), 0);
+
+	assert0 (unlink (pid_file));
+
 	DELETE_FILE (confdir, "long-running-term.conf");
 	DELETE_FILE (confdir, "session-end-term.conf");
 
@@ -11449,6 +11558,9 @@
 	TEST_EQ (lines, 1);
 	nih_free (output);
 
+	job_pid = job_to_pid ("long-running");
+	TEST_NE (job_pid, -1);
+
 	upstart = upstart_open (NULL);
 	TEST_NE_P (upstart, NULL);
 
@@ -11465,11 +11577,16 @@
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
 	DELETE_FILE (confdir, "long-running.conf");
 
 	/*******************************************************************/
@@ -11484,13 +11601,16 @@
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
 	cmd = nih_sprintf (NULL, "%s start %s 2>&1",
-			get_initctl (), "long-running");
+			get_initctl (), "long-running-term");
 	TEST_NE_P (cmd, NULL);
 
 	RUN_COMMAND (NULL, cmd, &output, &lines);
 	TEST_EQ (lines, 1);
 	nih_free (output);
 
+	job_pid = job_to_pid ("long-running-term");
+	TEST_NE (job_pid, -1);
+
 	upstart = upstart_open (NULL);
 	TEST_NE_P (upstart, NULL);
 
@@ -11507,23 +11627,32 @@
 
 	/* Should not now be running */
 	TEST_EQ (kill (upstart_pid, 0), -1);
+	TEST_EQ (errno, ESRCH);
 
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
 	DELETE_FILE (confdir, "long-running-term.conf");
 
 	/*******************************************************************/
 	TEST_FEATURE ("session shutdown: one job which starts on session-end");
 
-	CREATE_FILE (confdir, "session-end.conf",
-			"start on session-end\n"
-			"\n"
-			"script\n"
-			"  echo hello\n"
-			"  sleep 999\n"
-			"end script");
+	TEST_FILENAME (pid_file);
+
+	job = NIH_MUST (nih_sprintf (NULL, "start on session-end\n"
+				"\n"
+				"script\n"
+				"  echo hello\n"
+				"  echo $$ >\"%s\"\n"
+				"  exec sleep 999\n"
+				"end script", pid_file));
+
+	CREATE_FILE (confdir, "session-end.conf", job);
 
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -11555,6 +11684,17 @@
 	TEST_EQ (fclose (file), 0);
 	assert0 (unlink (logfile));
 
+	file = fopen (pid_file, "r");
+	TEST_NE_P (file, NULL);
+	TEST_EQ (fscanf (file, "%d", &job_pid), 1);
+	fclose (file);
+
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
+	assert0 (unlink (pid_file));
+
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
@@ -11564,14 +11704,18 @@
 	/*******************************************************************/
 	TEST_FEATURE ("session shutdown: one job which starts on session-end");
 
-	CREATE_FILE (confdir, "session-end-term.conf",
-			"start on session-end\n"
-			"\n"
-			"script\n"
-			"  trap '' TERM\n"
-			"  echo hello\n"
-			"  sleep 999\n"
-			"end script");
+	TEST_FILENAME (pid_file);
+
+	job = NIH_MUST (nih_sprintf (NULL, "start on session-end\n"
+				"\n"
+				"script\n"
+				"  trap '' TERM\n"
+				"  echo hello\n"
+				"  echo $$ >\"%s\"\n"
+				"  exec sleep 999\n"
+				"end script", pid_file));
+
+	CREATE_FILE (confdir, "session-end-term.conf", job);
 
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -11603,6 +11747,17 @@
 	TEST_EQ (fclose (file), 0);
 	assert0 (unlink (logfile));
 
+	file = fopen (pid_file, "r");
+	TEST_NE_P (file, NULL);
+	TEST_EQ (fscanf (file, "%d", &job_pid), 1);
+	fclose (file);
+
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
+	assert0 (unlink (pid_file));
+
 	session_file = NIH_MUST (nih_sprintf (NULL, "%s/upstart/sessions/%d.session",
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
@@ -11620,13 +11775,17 @@
 		        "  sleep 999\n"
 			"end script");
 
-	CREATE_FILE (confdir, "session-end-term.conf",
-			"start on session-end\n"
-			"\n"
-			"script\n"
-			"  trap '' TERM\n"
-			"  sleep 999\n"
-			"end script");
+	TEST_FILENAME (pid_file);
+
+	job = NIH_MUST (nih_sprintf (NULL, "start on session-end\n"
+				"\n"
+				"script\n"
+				"  trap '' TERM\n"
+				"  echo $$ >\"%s\"\n"
+				"  sleep 999\n"
+				"end script", pid_file));
+
+	CREATE_FILE (confdir, "session-end-term.conf", job);
 
 	start_upstart_common (&upstart_pid, TRUE, confdir, logdir, NULL);
 
@@ -11638,6 +11797,9 @@
 	TEST_EQ (lines, 1);
 	nih_free (output);
 
+	job_pid = job_to_pid ("long-running-term");
+	TEST_NE (job_pid, -1);
+
 	upstart = upstart_open (NULL);
 	TEST_NE_P (upstart, NULL);
 
@@ -11659,6 +11821,21 @@
 				sessiondir, (int)upstart_pid));
 	unlink (session_file);
 
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
+	file = fopen (pid_file, "r");
+	TEST_NE_P (file, NULL);
+	TEST_EQ (fscanf (file, "%d", &job_pid), 1);
+	fclose (file);
+
+	/* pid should no longer exist */
+	TEST_EQ (kill (job_pid, SIGKILL), -1);
+	TEST_EQ (errno, ESRCH);
+
+	assert0 (unlink (pid_file));
+
 	DELETE_FILE (confdir, "long-running-term.conf");
 	DELETE_FILE (confdir, "session-end-term.conf");