Merge lp:~doanac/ubuntu-ci-services-itself/lander-sigterm into lp:ubuntu-ci-services-itself

[Andy Doan]

lander: ensure ticket is marked failed when cancelled

We have an issue where a user might cancel a jenkins job, but the
lander fails to mark the ticket as failed. This can cause the same
ticket to get run again by the lander.

This checks SIGTERM (what jenkins sends when a job is cancelled)
and ensures the ticket is marked failed

Try and help ensure the lander marks a ticket as failed if a job gets canceled.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:342
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/346/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/346/rebuild

review: Approve (continuous-integration)

[Vincent Ladeuil (vila)]

19 + def _on_sigterm(signame, frame):
20 + logger.error('job was terminated, failing ticket')
21 + api.fail_ticket()

Just curious: what's the reason you define that function *inside* the other one ?

In practice, it often make them harder to test (you can't override them anymore).

review: Approve

[Andy Doan (doanac)]

On 03/09/2014 04:47 AM, Vincent Ladeuil wrote:
> 19 + def _on_sigterm(signame, frame):
> 20 + logger.error('job was terminated, failing ticket')
> 21 + api.fail_ticket()
>
> Just curious: what's the reason you define that function*inside* the other one ?

You always keep me honest :)

The problem I was trying to address was the fact that the "api" object
needed to be accessible to the signal handler. As the module is
currently written, I would have needed to introduce a global variable
for this. It wouldn't have been terrible, I just thought this was less
terrible.

> In practice, it often make them harder to test (you can't override them anymore).

yep. i agree.

[Andy Doan (doanac)]

Francis - i'm wondering if this is sufficient. I could envision a loop-hole where the lander_service_wrapper crashes do to an unforeseen bug. SIGTERM wouldn't be hit in this scenario.

I suppose we could create a jenkins job that always runs at the very end. It checks for the ticket state to be COMPLETE|FAILED, and if not FAILS it.

Maybe I'm overthinking this?

[Vincent Ladeuil (vila)]

>>>>> Andy Doan <email address hidden> writes:

    > On 03/09/2014 04:47 AM, Vincent Ladeuil wrote:
    >> 19 + def _on_sigterm(signame, frame):
    >> 20 + logger.error('job was terminated, failing ticket')
    >> 21 + api.fail_ticket()
    >>
    >> Just curious: what's the reason you define that function*inside* the other one ?

    > You always keep me honest :)

    > The problem I was trying to address was the fact that the "api" object
    > needed to be accessible to the signal handler. As the module is
    > currently written, I would have needed to introduce a global variable
    > for this. It wouldn't have been terrible, I just thought this was less
    > terrible.

Oh, right trickier, good enough then.

[Andy Doan (doanac)]

I'm going to top approve. Even if we want to do more, this change would still be perfectly fine.

review: Approve

=== modified file 'lander/bin/lander_service_wrapper.py'
--- lander/bin/lander_service_wrapper.py	2014-03-07 07:42:41 +0000
+++ lander/bin/lander_service_wrapper.py	2014-03-09 05:22:40 +0000
@@ -18,6 +18,7 @@
 import json
 import logging
 import os
+import signal
 import sys
 import time
 import urllib2
@@ -115,7 +116,14 @@
 
 def _ticket_api(config):
     master = config['master']
-    return TicketApi(master['ts_url'], master['request_id'])
+    api = TicketApi(master['ts_url'], master['request_id'])
+
+    def _on_sigterm(signame, frame):
+        logger.error('job was terminated, failing ticket')
+        api.fail_ticket()
+
+    signal.signal(signal.SIGTERM, _on_sigterm)
+    return api
 
 
 def _get_ppa(ppa_assigner_url, ticket_id):

=== modified file 'lander/bin/ticket_api.py'
--- lander/bin/ticket_api.py	2014-03-06 23:35:46 +0000
+++ lander/bin/ticket_api.py	2014-03-09 05:22:40 +0000
@@ -46,6 +46,10 @@
         resp = resp.read()
         return json.loads(resp)
 
+    def fail_ticket(self):
+        url = self._url + '/updateticketstatus/' + self._ticket_id + '/'
+        self._patch(url, {'status': TicketWorkflowStep.FAILED.value})
+
     def _patch_pkg_status(self, status):
         step = TicketWorkflowStep.PKG_BUILDING.value
         status = status.value