Merge pull request #2823 from jakedt/buildhilarity

Release database connections after updating build statuses.

buildman/jobutil/buildstatus.py

@@ -76,7 +76,7 @@ class StatusHandler(object):
     yield From(self._append_log_message(phase, self._build_logs.PHASE, extra_data))
 
     # Update the repository build with the new phase
-    raise Return(self._build_model.update_phase(self._uuid, phase))
+    raise Return(self._build_model.update_phase_then_close(self._uuid, phase))
 
   def __enter__(self):
     return self._status

data/model/build.py

@@ -6,9 +6,9 @@ from peewee import JOIN_LEFT_OUTER
 
 import features
 from data.database import (BuildTriggerService, RepositoryBuildTrigger, Repository, Namespace, User,
-                           RepositoryBuild, BUILD_PHASE, db_for_update, db_random_func)
+                           RepositoryBuild, BUILD_PHASE, db_random_func, UseThenDisconnect)
 from data.model import (InvalidBuildTriggerException, InvalidRepositoryBuildException,
-                        db_transaction, user as user_model)
+                        db_transaction, user as user_model, config)
 
 PRESUMED_DEAD_BUILD_AGE = timedelta(days=15)
 PHASES_NOT_ALLOWED_TO_CANCEL_FROM = (BUILD_PHASE.PUSHING, BUILD_PHASE.COMPLETE,
@@ -153,36 +153,40 @@ def get_pull_robot_name(trigger):
   return trigger.pull_robot.username
 
 
-def _get_build_row_for_update(build_uuid):
-  return db_for_update(RepositoryBuild.select().where(RepositoryBuild.uuid == build_uuid)).get()
+def _get_build_row(build_uuid):
+  return RepositoryBuild.select().where(RepositoryBuild.uuid == build_uuid).get()
 
 
-def update_phase(build_uuid, phase):
+def update_phase_then_close(build_uuid, phase):
   """ A function to change the phase of a build """
-  try:
-    build = _get_build_row_for_update(build_uuid)
-  except RepositoryBuild.DoesNotExist:
+  with UseThenDisconnect(config.app_config):
+    try:
+      build = _get_build_row(build_uuid)
+    except RepositoryBuild.DoesNotExist:
       return False
 
-  # Can't update a cancelled build
-  if build.phase == BUILD_PHASE.CANCELLED:
-    return False
+    # Can't update a cancelled build
+    if build.phase == BUILD_PHASE.CANCELLED:
+      return False
 
-  build.phase = phase
-  build.save()
-  return True
+    updated = (RepositoryBuild
+               .update(phase=phase)
+               .where(RepositoryBuild.id == build.id, RepositoryBuild.phase == build.phase)
+               .execute())
+    
+    return updated > 0
 
 
-def create_cancel_build_in_queue(build, build_queue):
+def create_cancel_build_in_queue(build_phase, build_queue_id, build_queue):
   """ A function to cancel a build before it leaves the queue """
 
   def cancel_build():
     cancelled = False
 
-    if build.queue_id is not None:
-      cancelled = build_queue.cancel(build.queue_id)
+    if build_queue_id is not None:
+      cancelled = build_queue.cancel(build_queue_id)
 
-    if build.phase != BUILD_PHASE.WAITING:
+    if build_phase != BUILD_PHASE.WAITING:
       return False
 
     return cancelled
@@ -190,42 +194,35 @@ def create_cancel_build_in_queue(build, build_queue):
   return cancel_build
 
 
-def create_cancel_build_in_manager(build, build_canceller):
+def create_cancel_build_in_manager(build_phase, build_uuid, build_canceller):
   """ A function to cancel the build before it starts to push """
 
   def cancel_build():
-    if build.phase in PHASES_NOT_ALLOWED_TO_CANCEL_FROM:
+    if build_phase in PHASES_NOT_ALLOWED_TO_CANCEL_FROM:
       return False
 
-    return build_canceller.try_cancel_build(build.uuid)
+    return build_canceller.try_cancel_build(build_uuid)
 
   return cancel_build
 
 
 def cancel_repository_build(build, build_queue):
-  """ This tries to cancel the build returns true if request is successful false if it can't be cancelled """
-  with db_transaction():
-    from app import build_canceller
-    from buildman.jobutil.buildjob import BuildJobNotifier
-    # Reload the build for update.
-    # We are loading the build for update so checks should be as quick as possible.
-    try:
-      build = _get_build_row_for_update(build.uuid)
-    except RepositoryBuild.DoesNotExist:
-      return False
+  """ This tries to cancel the build returns true if request is successful false
+      if it can't be cancelled """
+  from app import build_canceller
+  from buildman.jobutil.buildjob import BuildJobNotifier
 
-    cancel_builds = [create_cancel_build_in_queue(build, build_queue),
-                     create_cancel_build_in_manager(build, build_canceller), ]
-    original_phase = build.phase
-    for cancelled in cancel_builds:
-      if cancelled():
-        build.phase = BUILD_PHASE.CANCELLED
+  cancel_builds = [create_cancel_build_in_queue(build.phase, build.queue_id, build_queue),
+                   create_cancel_build_in_manager(build.phase, build.uuid, build_canceller), ]
+  for cancelled in cancel_builds:
+    if cancelled():
+      updated = update_phase_then_close(build.uuid, BUILD_PHASE.CANCELLED)
+      if updated:
         BuildJobNotifier(build.uuid).send_notification("build_cancelled")
-        build.save()
-        return True
-    build.phase = original_phase
-    build.save()
-    return False
+
+      return updated
+
+  return False
 
 
 def get_archivable_build():

test/test_queries.py

@@ -2,7 +2,7 @@ import unittest
 from unittest import TestCase
 
 from app import app
-from data.model.build import get_repository_build, update_phase, create_repository_build
+from data.model.build import get_repository_build, update_phase_then_close, create_repository_build
 from initdb import setup_database_for_testing, finished_database_for_testing
 from data import model
 from data.database import RepositoryBuild, Image, ImageStorage, BUILD_PHASE
@@ -83,7 +83,7 @@ class TestUpdatePhase(unittest.TestCase):
     repo_build = get_repository_build(build.uuid)
 
     self.assertEqual(repo_build.phase, BUILD_PHASE.WAITING)
-    self.assertTrue(update_phase(build.uuid, BUILD_PHASE.COMPLETE))
+    self.assertTrue(update_phase_then_close(build.uuid, BUILD_PHASE.COMPLETE))
 
     repo_build = get_repository_build(build.uuid)
 
@@ -91,7 +91,7 @@ class TestUpdatePhase(unittest.TestCase):
 
     repo_build.delete_instance()
 
-    self.assertFalse(update_phase(repo_build.uuid, BUILD_PHASE.PULLING))
+    self.assertFalse(update_phase_then_close(repo_build.uuid, BUILD_PHASE.PULLING))
 
   @staticmethod
   def create_build(repository):