Merge lp:~rvb/maas/bug-lock-atexit into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Remove the lock if the process gets killed in start_up().

Testing shows this fixes bug 1221059: what we suspect happens here is that, when the call to the avahi bindings errors, apache kills (with SIGTERM) the WSGI process. This causes the lock to be left on disk, which in turn prevents any other WSGI to start because start_up() blocks, waiting for the lock to be removed.

Note that this will only make MAAS cope with transient avahi errors, if an error is permanent (i.e. if the call to the avahi bindings always errors) then this won't mask the problem.

Testing show that this fix works: I've build a package with this fix and tested it.
http://10.189.74.2:8080/view/MAAS/job/saucy-adt-maas-manual/buildTimeTrend
Runs [27-30] are done using the package with this fix.
Runs [31-34] are done with the daily package (i.e. without the fix)
(The tests used here are a simplified version of the integration tests, which only run up to the point where one can log into the API)

[Gavin Panella (allenap)]

Looks good. I'm still baffled that atexit works but finally doesn't,
but data is king and I'm happy that you've figured out a way to
address this issue.

[1]

+        atexit_recorder = self.patch(start_up, 'register_atexit')

You could patch the atexit module directly. That would remove the need
for the import-as. Or you could mock the atexit name in the start_up
module.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Glad to hear that this seems to work. A few notes:

 * Why rename register() on import? I'd just "import atexit" and call "atexit.register" — and patch(atexit, 'register') in the test. Simpler all round, I think.

 * Do we know that lock.break_lock won't blow up even when the lock is not held? It could be hard to detect if it did, but might prevent further cleanup.

 * The noun and the command are called "shutdown." The verb is "shut down."

review: Approve

[Raphaël Badin (rvb)]

> Glad to hear that this seems to work. A few notes:
>
> * Why rename register() on import? I'd just "import atexit" and call
> "atexit.register" — and patch(atexit, 'register') in the test. Simpler all
> round, I think.

All right, same remark from Gavin. I thought it was a bit clearer with the rename but it looks I'm the minority here.

>
> * Do we know that lock.break_lock won't blow up even when the lock is not
> held? It could be hard to detect if it did, but might prevent further
> cleanup.

Yes, I tested it.

> * The noun and the command are called "shutdown." The verb is "shut down."

Okay :).

[Raphaël Badin (rvb)]

> Looks good. I'm still baffled that atexit works but finally doesn't,
> but data is king and I'm happy that you've figured out a way to
> address this issue.

To be fair, Jeroen suggested this fix.

> [1]
>
> +        atexit_recorder = self.patch(start_up, 'register_atexit')
>
> You could patch the atexit module directly. That would remove the need
> for the import-as. Or you could mock the atexit name in the start_up
> module.

All right, Jeroen feels the same so I'm outnumbered :)

=== modified file 'src/maasserver/start_up.py'
--- src/maasserver/start_up.py	2013-08-23 06:59:22 +0000
+++ src/maasserver/start_up.py	2013-09-13 08:46:53 +0000
@@ -15,6 +15,7 @@
     ]
 
 
+import atexit
 from textwrap import dedent
 
 from django.db import connection
@@ -56,6 +57,9 @@
     internally are not ran concurrently.
     """
     lock = FileLock(LOCK_FILE_NAME)
+    # In case this process gets shut down, clean up the lock.
+    atexit.register(lock.break_lock)
+
     lock.acquire(timeout=LOCK_TIMEOUT)
     try:
         inner_start_up()
 
=== modified file 'src/maasserver/tests/test_start_up.py'
--- src/maasserver/tests/test_start_up.py	2013-09-11 08:13:20 +0000
+++ src/maasserver/tests/test_start_up.py	2013-09-13 08:46:53 +0000
@@ -30,7 +30,11 @@
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.celery import CeleryFixture
 from maastesting.fakemethod import FakeMethod
-from mock import Mock
+from mock import (
+    call,
+    MagicMock,
+    Mock,
+    )
 from provisioningserver import tasks
 from testresources import FixtureResource
 
@@ -153,3 +157,17 @@
         self.assertNotIn(
             COMPONENT.IMPORT_PXE_FILES,
             [args[0][0] for args in recorder.call_args_list])
+
+    def test_start_up_registers_atexit_lock_cleanup(self):
+        filelock_mock = MagicMock()
+        self.patch(start_up, 'FileLock', Mock(side_effect=filelock_mock))
+        # Patch atexit.register to assert it's called with the right
+        # argument.
+        atexit_mock = self.patch(start_up, 'atexit')
+
+        start_up.start_up()
+
+        self.assertEqual(
+            [call.register(
+                filelock_mock(start_up.LOCK_FILE_NAME).break_lock)],
+            atexit_mock.mock_calls)