Merge lp:~jml/txpkgme/long-check into lp:txpkgme

[Jonathan Lange]

Add --shutdown-delay option to delay shutting down the webserver.

We're getting error messages in our celery log and in our OOPS reports saying
"Connection Refused". Really, these errors are mostly due to a buggy client,
submit-for-packaging, that shuts down the receiving web server before fully
providing a response.

It would be nice to prevent this properly. However, until then, to silence
these warnings and to make debugging nagios failures easier, let's just add
a delay between the webserver sending a response and shutting itself down.

We'll need a txpkgme rollout and an update to the cronjob that feeds the
nagios check (adding '--shutdown-delay 5') before this kicks in.

[Michael Vogt (mvo)]

I give this a tentative +1 - tentative because its friday evening and also because my twisted foo is weak. A second opinion is probably good, but otoh its compact enough for low risk.

[Jonathan Lange (jml)]

I'm going to seize this as an actual approval. I trust our test suite & our staging server to prevent any real problems.

review: Approve

=== modified file 'txpkgme/submitfromdisk.py'
--- txpkgme/submitfromdisk.py	2012-07-30 15:14:08 +0000
+++ txpkgme/submitfromdisk.py	2012-08-17 16:14:24 +0000
@@ -20,6 +20,7 @@
 import time
 
 from twisted.internet import reactor as mod_reactor
+from twisted.internet.task import deferLater
 from twisted.web.resource import Resource
 from twisted.web.static import File
 
@@ -167,7 +168,8 @@
     return result, result_message
 
 
-def submit_local_files(service_root, file_path, metadata, **webserver_args):
+def submit_local_files(service_root, file_path, metadata, shutdown_delay,
+                       **webserver_args):
     """Submit 'path' to pkgme-service at 'service_root' with 'metadata'.
 
     :return: A Deferred that files with ('PUT', json_result).
@@ -184,12 +186,21 @@
     def web_server_up(metadata, webserver):
         return webserver.send_api_request(metadata, service_root)
     d.addCallback(web_server_up, harness)
+    d.addBoth(delay_callbacks, shutdown_delay, harness.reactor)
     d.addBoth(harness.shut_down_web_server)
     return d
 
 
+def delay_callbacks(value, delay, clock=None):
+    if clock is None:
+        clock = mod_reactor
+    if not delay:
+        return value
+    return deferLater(clock, delay, lambda: value)
+
+
 def fire_request(service_root, app_path, metadata_path, timeout,
-                 hostname, port, result_writer, clock):
+                 hostname, port, result_writer, clock, shutdown_delay):
     successful = [False]
 
     start_time = clock.seconds()
@@ -222,7 +233,7 @@
 
     def send_request(metadata):
         d = submit_local_files(
-            service_root, app_path, metadata,
+            service_root, app_path, metadata, shutdown_delay,
             reactor=mod_reactor, timeout=timeout,
             hostname=hostname, port=port, clock=clock)
         d.addCallback(got_response)
@@ -263,6 +274,9 @@
     parser.add_option("-f", "--file", dest="output_file",
                       help="write the result to FILE", type="str",
                       metavar="FILE", default=None)
+    parser.add_option("--shutdown-delay", dest="shutdown_delay", type="int",
+                      help=("Delay shutting down the web server by this "
+                            "many seconds."))
     options, args = parser.parse_args()
     metadata_path = METADATA_FILE
     service_url = None
@@ -283,9 +297,10 @@
     result_writer = ResultWriter()
     if options.output_file is not None:
         result_writer = OutputFileResultWriter(options.output_file, clock)
-    successful =  fire_request(
+    successful = fire_request(
         service_url, app_path, metadata_path, options.timeout,
-        options.hostname, options.port, result_writer, clock)
+        options.hostname, options.port, result_writer, clock,
+        options.shutdown_delay)
     if not successful:
         return 1
     return 0

=== modified file 'txpkgme/tests/test_submitfromdisk.py'
--- txpkgme/tests/test_submitfromdisk.py	2012-07-19 17:49:22 +0000
+++ txpkgme/tests/test_submitfromdisk.py	2012-08-17 16:14:24 +0000
@@ -20,17 +20,63 @@
     TempDir,
     )
 from testtools import TestCase
+from testtools._spinner import (
+    DeferredNotFired,
+    extract_result,
+    )
+from twisted.internet.defer import Deferred
 from twisted.internet.task import Clock
 from twisted.web.resource import Resource
 
 from txpkgme.submitfromdisk import (
     add_local_files,
     check_saved_output,
+    delay_callbacks,
     OutputFileResultWriter,
     parse_output,
 )
 
 
+class DelayCallbacksTest(TestCase):
+
+    def test_no_delay(self):
+        clock = Clock()
+        marker = object()
+        d = Deferred()
+        d.addBoth(delay_callbacks, 0, clock)
+        d.callback(marker)
+        self.assertIs(marker, extract_result(d))
+
+    def test_stops_firing(self):
+        clock = Clock()
+        marker = object()
+        d = Deferred()
+        d.addBoth(delay_callbacks, 5, clock)
+        d.callback(marker)
+        self.assertRaises(DeferredNotFired, extract_result, d)
+
+    def test_fires_after_time(self):
+        clock = Clock()
+        marker = object()
+        delay = 5
+        d = Deferred()
+        d.addBoth(delay_callbacks, 5, clock)
+        d.callback(marker)
+        clock.advance(delay + 1)
+        self.assertIs(marker, extract_result(d))
+
+    def test_failure(self):
+        clock = Clock()
+        delay = 5
+        d = Deferred()
+        d.addCallback(lambda x: 1/0)
+        d.addBoth(delay_callbacks, 5, clock)
+        d.callback(None)
+        clock.advance(delay + 1)
+        self.assertRaises(ZeroDivisionError, extract_result, d)
+
+
+
 class OutputFileResultWriterTests(TestCase):
 
     def write(self, success, duration, msg, clock=None):