Merge lp:~jamesodhunt/upstart/bug-901038 into lp:upstart

[James Hunt]

[Stéphane Graber (stgraber)]

Should there be some kind of timeout, say a minute or so to catch the unlikely case where upstart is messed up an never starts listening again on dbus?

Also, you mention that a rejected reexec request is unlikely, but what happens if a non-root user does it, wouldn't that fail and then have the rest of the code return success anyway?

[James Hunt (jamesodhunt)]

>Should there be some kind of timeout, say a minute or so to catch the unlikely case where upstart
> is messed up an never starts listening again on dbus?
I can understand the concern, but I don't think we can realistically provide a timeout as we don't know what a reasonable re-exec time is in all circumstances: infinity's >1 minute re-exec time was clearly the result of a bug (bug 1338637), but the re-exec _did_ complete correctly - it just took a very long time.

Also, telinit is connecting to the private D-Bus socket and if that isn't available, Upstarts capabilities are then severly restricted.

> Also, you mention that a rejected reexec request is unlikely, but what happens if a non-root user
> does it, wouldn't that fail and then have the rest of the code return success anyway?
If a non-root user runs 'telinit u', they get rejected immediately as telinit ensures the user running the command is root. I think the only scenarios where the re-exec request would be rejected are:

1) Where telinit is attempting to talk to an old version of upstart that doesn't support re-exec.
   We can ignore this scenario as we don't support down-grade scenarios.

2) Where telinit is attempting to talk to a non-Upstart init daemon.

   I guess this could be an issue in Ubuntu, depending on exactly how we manage the init
   transition. However, we should be able to arrange for maintainer scripts to set
   UPSTART_TELINIT_U_NO_WAIT to avoid that issue.

[Steve Langasek (vorlon)]

> 1) Where telinit is attempting to talk to an old version of upstart that doesn't support
> re-exec. We can ignore this scenario as we don't support down-grade scenarios.

> 2) Where telinit is attempting to talk to a non-Upstart init daemon.

> I guess this could be an issue in Ubuntu, depending on exactly how we manage the init
> transition. However, we should be able to arrange for maintainer scripts to set
> UPSTART_TELINIT_U_NO_WAIT to avoid that issue.

I don't think any solution should require maintainer scripts to set an extra variable to get sane behavior out of telinit.

If telinit is trying to talk to a non-upstart init, shouldn't it know it? In that case, it surely should not wait for a reconnect on an upstart-specific private socket, but should automatically fall back gracefully to a non-blocking restart.

However, the current upstart 'telinit u' code simply exits non-zero when pid 1 is not upstart - as does the proposed new code. So this is something we need to deal with if/when adding support for telinit u on top of non-upstart pid 1, but not something that should block this change now.

Given this, what is the expected use case for "UPSTART_TELINIT_U_NO_WAIT"? If you don't have a concrete user for this, please omit. Unnecessary interfaces always carry an incremental cost, and I can't see any case where it would ever be correct to use this interface, so this looks like overengineering to me.

Otherwise, this looks fine to me for merging.

review: Needs Fixing

[lp:~jamesodhunt/upstart/bug-901038]

1662. By James Hunt on 2014-09-08

* util/telinit.c: Remove UPSTART_TELINIT_U_NO_WAIT check as it
  shouldn't realistically be needed.

[James Hunt (jamesodhunt)]

OK - removed the UPSTART_TELINIT_U_NO_WAIT logic.

[Steve Langasek (vorlon)]

Ok, merging.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2014-09-04 11:07:22 +0000
+++ ChangeLog	2014-09-08 10:12:37 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
 2014-09-04  James Hunt  <[e-mail]>
 
 	* NEWS: Release 1.13.2
@@ -23,6 +24,20 @@
 	* init/tests/test_job.c: test_job_last_process(): New test for
 	  job_last_process().
 
+=======
[phone]  James Hunt  <[e-mail]>
+
+	* util/telinit.c: Remove UPSTART_TELINIT_U_NO_WAIT check as it
+	  shouldn't realistically be needed.
+
[phone]  James Hunt  <[e-mail]>
+
+	* util/telinit.c: restart_upstart():
+	  - Revert to synchronous behaviour coupled with unavoidable
+	    poll to ensure telinit only returns once a re-exec has
+	    completed (LP: #901038).
+
+>>>>>>> MERGE-SOURCE
 2014-08-14  James Hunt  <[e-mail]>
 
 	* init/control.c: Disallow modifying system jobs via SetEnv,
 
=== modified file 'util/telinit.c'
--- util/telinit.c	2014-03-10 13:43:50 +0000
+++ util/telinit.c	2014-09-08 10:12:37 +0000
@@ -160,27 +160,73 @@
 int
 restart_upstart (void)
 {
-	nih_local NihDBusProxy *upstart = NULL;
-	DBusPendingCall        *ret;
+	nih_local NihDBusProxy  *upstart = NULL;
+	NihError                *err;
+	int                      ret;
 
 	upstart = upstart_open (NULL);
 	if (! upstart)
 		return -1;
 
-	/* Fire and forget:
-	 *
-	 * Ask Upstart to restart itself using the async interface to
-	 * avoid the client-side complaining if and when it detects that
-	 * Upstart has severed all connections to perform the re-exec.
-	 */
-	ret = upstart_restart (upstart, NULL, NULL, NULL, 0);
-	dbus_connection_flush(upstart->connection);
-
-	/* We don't care about the return code, but we have to keep
-	 * the compiler happy.
-	 */
-	if (ret != (DBusPendingCall *)TRUE)
-		return 0;
+	/* Ask Upstart to restart itself.
+	 *
+	 * Since it is not possible to serialise a D-Bus connection,
+	 * Upstart is forced to sever all D-Bus client connections,
+	 * including this one.
+	 *
+	 * Further, since the user expects telinit to block _until the
+	 * re-exec has finished and Upstart is accepting connections
+	 * once again_, the only solution is to wait for the forced
+	 * disconnect, then poll until it is possible to create a new
+	 * connection.
+	 *
+	 * Note that we don't (can't) care about the return code since
+	 * it's not reliable:
+	 *
+	 * - either the re-exec request completed and D-Bus returned zero
+	 *   before Upstart started the re-exec.
+	 *
+	 * - or the re-exec request completed but upstart started the
+	 *   re-exec (severing all D-Bus connections) before D-Bus got a
+	 *   chance to finish cleanly meaning we receive a return of -1.
+	 *
+	 * We cannot know exactly what happened so have to allow for
+	 * both scenarios. Note the implicit assumption that the re-exec
+	 * request itself was accepted. If this assumption is incorrect
+	 * (should not be possible), the worst case scenario is that
+	 * upstart does not re-exec and then we quickly drop out of the
+	 * reconnect block since it never went offline.
+	 */
+	ret = upstart_restart_sync (NULL, upstart);
+
+	if (ret < 0) {
+		err = nih_error_get ();
+		nih_free (err);
+	}
+
+	nih_free (upstart);
+
+	nih_debug ("Waiting for upstart to finish re-exec");
+
+	/* We believe Upstart is now in the process of
+	 * re-exec'ing so attempt forever to reconnect.
+	 *
+	 * This sounds dangerous but there is no other option,
+	 * and a connection must be possible unless the system
+	 * is completely broken.
+	 */
+	while (TRUE) {
+
+		upstart = upstart_open (NULL);
+		if (upstart)
+			break;
+
+		err = nih_error_get ();
+		nih_free (err);
+
+		/* Avoid DoS'ing the system whilst we wait */
+		usleep (100000);
+	}
 
 	return 0;
 }