Remove UnableToGetLock alias, test that jobstep_allocate returns a 50…

…3 when unable to get lock

Summary:
Inability to allocate is expected in some circumstances, which is why the code intended to return
a 503 in those cases. The intended exception wasn't caught, though it looks like it should've
been. This change removes the indirection, making it do what it looks to be doing, and verifies
that a 503 is returned in that case.
This should resolve a bunch of errors from uncaught exceptions we see at deploy time.

Test Plan: Unit

Reviewers: jukka

Reviewed By: jukka

Subscribers: changesbot, wwu

Differential Revision: https://tails.corp.dropbox.com/D94591

changes/api/jobstep_allocate.py

@@ -9,6 +9,7 @@
 from changes.api.base import APIView, error
 from changes.constants import Status, Result
 from changes.config import db, redis
+from changes.ext.redis import UnableToGetLock
 from changes.models import Build, Job, JobPlan, JobStep
 
 
@@ -119,7 +120,7 @@ def post(self):
                     return self.respond([])
 
                 db.session.flush()
-        except redis.UnableToGetLock:
+        except UnableToGetLock:
             return error('Another allocation is in progress', http_code=503)
 
         context = []

changes/ext/redis.py

@@ -15,7 +15,6 @@ class UnableToGetLock(Exception):
 
 
 class _Redis(object):
-    UnableToGetLock = UnableToGetLock
 
     def __init__(self, app, options):
         self.app = app
@@ -50,7 +49,7 @@ def lock(self, lock_key, timeout=3, expire=None, nowait=False):
         self.logger.info('Acquiring lock on %s', lock_key)
 
         if not got_lock:
-            raise self.UnableToGetLock('Unable to fetch lock on %s' % (lock_key,))
+            raise UnableToGetLock('Unable to fetch lock on %s' % (lock_key,))
 
         try:
             yield

tests/changes/api/test_jobstep_allocate.py

@@ -6,6 +6,7 @@
 
 from changes.testutils import APITestCase
 from changes.constants import Status
+from changes.ext.redis import UnableToGetLock
 
 
 class JobStepAllocateTest(APITestCase):
@@ -33,6 +34,19 @@ def test_none_queued(self):
         assert resp.status_code == 200, resp
         assert resp.data == '[]', resp.data
 
+    @patch('changes.config.redis.lock',)
+    def test_cant_allocate(self, mock_allocate):
+        project = self.create_project()
+        build = self.create_build(project)
+        job = self.create_job(build)
+        jobphase = self.create_jobphase(job)
+
+        self.create_jobstep(jobphase, status=Status.unknown)
+
+        mock_allocate.side_effect = UnableToGetLock("Can't get lock")
+        resp = self.post_simple()
+        assert resp.status_code == 503
+
     @patch('changes.buildsteps.base.BuildStep.get_allocation_command',)
     def test_several_queued(self, mock_get_allocation_command):
         project = self.create_project()