Use atomic inserts to claim build requests

This differs from the previous practice of trying to update rows to reflect a claim, which is not possible in an atomic fashion across all database engines. This brings a few non-compatible changes: The db.buildrequests.claimBuildRequests method can no longer re-claim already-claimed requests; use reclaimBuildRequests instead. The database no longer tracks master instances, so the unclaimOldIncarnationRequests method has been removed. Note that several of the methods in this module now perform fewer consistency checks, for efficiency. Requests are now claimed to a particular master, but not to a particular "incarnation" of that master. The identity of the master is drawn from the existing objects table. This commit upgrades the database to version 11. Fixes #1036.
buildbot · Jul 17, 2011 · 8b1cee8 · benallard · Nov 21, 2011 · djmitche
1 parent 5fa1f10
commit 8b1cee8
Show file tree

Hide file tree

Showing 17 changed files with 799 additions and 693 deletions.
diff --git a/master/NEWS b/master/NEWS
@@ -5,6 +5,14 @@ Major User visible changes in Buildbot.             -*- outline -*-
 
 * Next Version
 
+** Deprecations, Removals, and Non-Compatible Changes
+
+*** The db.buildrequests.claimBuildRequests method can no longer re-claim
+already-claimed requests; use reclaimBuildRequests instead.  The database no
+longer tracks master instances, so the unclaimOldIncarnationRequests method has
+been removed.  Note that several of the methods in this module now perform
+fewer consistency checks, for efficiency.
+
 ** Customizable validation regexps
 
 The global c['validation'] parameter can be used to adjust the regular

diff --git a/master/buildbot/db/buildrequests.py b/master/buildbot/db/buildrequests.py
diff --git a/master/buildbot/db/migrate/versions/011_add_buildrequest_claims.py b/master/buildbot/db/migrate/versions/011_add_buildrequest_claims.py
@@ -0,0 +1,115 @@
+# This file is part of Buildbot.  Buildbot is free software: you can
+# redistribute it and/or modify it under the terms of the GNU General Public
+# License as published by the Free Software Foundation, version 2.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+# FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
+# details.
+#
+# You should have received a copy of the GNU General Public License along with
+# this program; if not, write to the Free Software Foundation, Inc., 51
+# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# Copyright Buildbot Team Members
+
+import sqlalchemy as sa
+import migrate
+from buildbot.util import sautils
+
+def migrate_claims(migrate_engine, metadata, buildrequests, objects,
+                   buildrequest_claims):
+
+    # First, ensure there is an object row for each master
+    new_objects = sa.select([
+            sa.null().label('id'),
+            buildrequests.c.claimed_by_name.label("name"),
+            sa.literal_column("'BuildMaster'").label("class_name"),
+        ],
+        whereclause=buildrequests.c.claimed_by_name != None,
+        distinct=True)
+
+    # this doesn't seem to work without str() -- verified in sqla 0.6.0 - 0.7.1
+    migrate_engine.execute(
+            str(sautils.InsertFromSelect(objects, new_objects)))
+
+    # now make a buildrequest_claims row for each claimed build request
+    join = buildrequests.join(objects,
+            (buildrequests.c.claimed_by_name == objects.c.name)
+            # (have to use sa.text because str, below, doesn't work
+            # with placeholders)
+            & (objects.c.class_name == sa.text("'BuildMaster'")))
+    claims = sa.select([
+            buildrequests.c.id.label('brid'),
+            objects.c.id.label('objectid'),
+            buildrequests.c.claimed_at,
+        ], from_obj=[ join ],
+        whereclause=buildrequests.c.claimed_by_name != None)
+    migrate_engine.execute(
+            str(sautils.InsertFromSelect(buildrequest_claims, claims)))
+
+def drop_columns(metadata, buildrequests):
+    # sqlalchemy-migrate <0.7.0 has a bug with sqlalchemy >=0.7.0, where
+    # it tries to change an immutable column; this is the workaround, from
+    # http://code.google.com/p/sqlalchemy-migrate/issues/detail?id=112
+    if not sa.__version__.startswith('0.6.'):
+        if not hasattr(migrate, '__version__'): # that is, older than 0.7
+            buildrequests.columns = buildrequests._columns
+
+    buildrequests.c.claimed_at.drop()
+    buildrequests.c.claimed_by_name.drop()
+    buildrequests.c.claimed_by_incarnation.drop()
+
+def upgrade(migrate_engine):
+    metadata = sa.MetaData()
+    metadata.bind = migrate_engine
+
+    # a copy of the buildrequests table, but with the foreign keys stripped
+    buildrequests = sa.Table('buildrequests', metadata,
+        sa.Column('id', sa.Integer,  primary_key=True),
+        sa.Column('buildsetid', sa.Integer, nullable=False),
+        sa.Column('buildername', sa.String(length=256), nullable=False),
+        sa.Column('priority', sa.Integer, nullable=False,
+            server_default=sa.DefaultClause("0")),
+        sa.Column('claimed_at', sa.Integer,
+            server_default=sa.DefaultClause("0")),
+        sa.Column('claimed_by_name', sa.String(length=256)),
+        sa.Column('claimed_by_incarnation', sa.String(length=256)),
+        sa.Column('complete', sa.Integer,
+            server_default=sa.DefaultClause("0")),
+        sa.Column('results', sa.SmallInteger),
+        sa.Column('submitted_at', sa.Integer, nullable=False),
+        sa.Column('complete_at', sa.Integer),
+    )
+
+    # existing objects table, used as a foreign key
+    objects = sa.Table("objects", metadata,
+        # unique ID for this object
+        sa.Column("id", sa.Integer, primary_key=True),
+        # object's user-given name
+        sa.Column('name', sa.String(128), nullable=False),
+        # object's class name, basically representing a "type" for the state
+        sa.Column('class_name', sa.String(128), nullable=False),
+
+        # prohibit multiple id's for the same object
+        sa.UniqueConstraint('name', 'class_name', name='object_identity'),
+    )
+
+    # and a new buildrequest_claims table
+    buildrequest_claims = sa.Table('buildrequest_claims', metadata,
+        sa.Column('brid', sa.Integer, sa.ForeignKey('buildrequests.id'),
+            index=True, unique=True),
+        sa.Column('objectid', sa.Integer, sa.ForeignKey('objects.id'),
+            index=True, nullable=True),
+        sa.Column('claimed_at', sa.Integer, nullable=False),
+    )
+
+    # create the new table
+    buildrequest_claims.create()
+
+    # migrate the claims into that table
+    migrate_claims(migrate_engine, metadata, buildrequests,
+                   objects, buildrequest_claims)
+
+    # and drop the claim-related columns in buildrequests
+    drop_columns(metadata, buildrequests)
diff --git a/master/buildbot/db/model.py b/master/buildbot/db/model.py
@@ -63,27 +63,15 @@ class Model(base.DBConnectorComponent):
 
     buildrequests = sa.Table('buildrequests', metadata,
         sa.Column('id', sa.Integer,  primary_key=True),
-        sa.Column('buildsetid', sa.Integer, sa.ForeignKey("buildsets.id"), nullable=False),
+        sa.Column('buildsetid', sa.Integer, sa.ForeignKey("buildsets.id"),
+            nullable=False),
         sa.Column('buildername', sa.String(length=256), nullable=False),
-        sa.Column('priority', sa.Integer, nullable=False, server_default=sa.DefaultClause("0")), # TODO: used?
-
-        # claimed_at is the time at which a master most recently asserted that
-        # it is responsible for running the build: this will be updated
-        # periodically to maintain the claim.  Note that 0 and NULL mean the
-        # same thing here (and not 1969!)
-        sa.Column('claimed_at', sa.Integer, server_default=sa.DefaultClause("0")),
-
-        # claimed_by indicates which buildmaster has claimed this request. The
-        # 'name' contains hostname/basedir, and will be the same for subsequent
-        # runs of any given buildmaster. The 'incarnation' contains bootime/pid,
-        # and will be different for subsequent runs. This allows each buildmaster
-        # to distinguish their current claims, their old claims, and the claims
-        # of other buildmasters, to treat them each appropriately.
-        sa.Column('claimed_by_name', sa.String(length=256)),
-        sa.Column('claimed_by_incarnation', sa.String(length=256)),
+        sa.Column('priority', sa.Integer, nullable=False,
+            server_default=sa.DefaultClause("0")), # TODO: used?
 
         # if this is zero, then the build is still pending
-        sa.Column('complete', sa.Integer, server_default=sa.DefaultClause("0")), # TODO: boolean
+        sa.Column('complete', sa.Integer,
+            server_default=sa.DefaultClause("0")), # TODO: boolean
 
         # results is only valid when complete == 1; 0 = SUCCESS, 1 = WARNINGS,
         # etc - see master/buildbot/status/builder.py
@@ -99,6 +87,16 @@ class Model(base.DBConnectorComponent):
     Each BuildRequest is a part of a BuildSet.  BuildRequests are claimed by
     masters, to avoid multiple masters running the same build."""
 
+    buildrequest_claims = sa.Table('buildrequest_claims', metadata,
+        sa.Column('brid', sa.Integer, sa.ForeignKey('buildrequests.id'),
+            index=True, unique=True),
+        sa.Column('objectid', sa.Integer, sa.ForeignKey('objects.id'),
+            index=True, nullable=True),
+        sa.Column('claimed_at', sa.Integer, nullable=False),
+    )
+    """Each row in this table represents a claimed build request, where the
+    claim is made by the object referenced by objectid."""
+
     # builds
 
     builds = sa.Table('builds', metadata,
@@ -339,8 +337,6 @@ class Model(base.DBConnectorComponent):
     sa.Index('buildrequests_buildsetid', buildrequests.c.buildsetid)
     sa.Index('buildrequests_buildername', buildrequests.c.buildername)
     sa.Index('buildrequests_complete', buildrequests.c.complete)
-    sa.Index('buildrequests_claimed_at', buildrequests.c.claimed_at)
-    sa.Index('buildrequests_claimed_by_name', buildrequests.c.claimed_by_name)
     sa.Index('builds_number', builds.c.number)
     sa.Index('builds_brid', builds.c.brid)
     sa.Index('buildsets_complete', buildsets.c.complete)

diff --git a/master/buildbot/master.py b/master/buildbot/master.py
@@ -17,8 +17,8 @@
 import re
 import os
 import signal
-import time
 import textwrap
+import socket
 
 from zope.interface import implements
 from twisted.python import log, components
@@ -97,13 +97,6 @@ def __init__(self, basedir, configFileName="master.cfg"):
         self.change_svc = ChangeManager()
         self.change_svc.setServiceParent(self)
 
-        try:
-            hostname = os.uname()[1] # only on unix
-        except AttributeError:
-            hostname = "?"
-        self.master_name = "%s:%s" % (hostname, os.path.abspath(self.basedir))
-        self.master_incarnation = "pid%d-boot%d" % (os.getpid(), time.time())
-
         self.botmaster = BotMaster(self)
         self.botmaster.setName("botmaster")
         self.botmaster.setServiceParent(self)
@@ -148,6 +141,9 @@ def __init__(self, basedir, configFileName="master.cfg"):
         # points are initialized)
         self.status = Status(self)
 
+        # local cache for this master's object ID
+        self._object_id = None
+
     def startService(self):
         # first, apply all monkeypatches
         monkeypatches.patch_all()
@@ -818,6 +814,33 @@ def logCount(_):
         d.addBoth(logCount)
         return d
 
+    ## informational methods
+
+    def getObjectId(self):
+        """
+        Return the obejct id for this master, for associating state with the master.
+
+        @returns: ID, via Deferred
+        """
+        # try to get the cached value
+        if self._object_id is not None:
+            return defer.succeed(self._object_id)
+
+        # failing that, get it from the DB; multiple calls to this function
+        # at the same time will not hurt
+        try:
+            hostname = os.uname()[1] # only on unix
+        except AttributeError:
+            hostname = socket.getfqdn()
+        master_name = "%s:%s" % (hostname, os.path.abspath(self.basedir))
+
+        d = self.db.state.getObjectId(master_name, "BuildMaster")
+        def keep(id):
+            self._object_id = id
+        d.addCallback(keep)
+        return d
+
+
     ## triggering methods and subscriptions
 
     def addChange(self, who=None, files=None, comments=None, author=None,
@@ -1146,24 +1169,14 @@ def pollDatabaseChanges(self):
         timer.stop()
 
     _last_unclaimed_brids_set = None
-    _last_claim_cleanup = None
+    _last_claim_cleanup = 0
     @defer.deferredGenerator
     def pollDatabaseBuildRequests(self):
         # deal with cleaning up unclaimed requests, and (if necessary)
         # requests from a previous instance of this master
         timer = metrics.Timer("BuildMaster.pollDatabaseBuildRequests()")
         timer.start()
 
-        if self._last_claim_cleanup is None:
-            self._last_claim_cleanup = 0
-
-            # first time through - clean up any builds belonging to a previous
-            # instance
-            wfd = defer.waitForDeferred(
-                    self.db.buildrequests.unclaimOldIncarnationRequests())
-            yield wfd
-            wfd.getResult()
-
         # cleanup unclaimed builds
         since_last_cleanup = reactor.seconds() - self._last_claim_cleanup 
         if since_last_cleanup < self.RECLAIM_BUILD_INTERVAL:

diff --git a/master/buildbot/process/builder.py b/master/buildbot/process/builder.py
@@ -150,8 +150,6 @@ def setBotmaster(self, botmaster):
         self.botmaster = botmaster
         self.master = botmaster.master
         self.db = self.master.db
-        self.master_name = self.master.master_name
-        self.master_incarnation = self.master.master_incarnation
 
     def compareToSetup(self, setup):
         diffs = []
@@ -290,7 +288,7 @@ def reclaimAllBuilds(self):
         if not brids:
             return defer.succeed(None)
 
-        d = self.master.db.buildrequests.claimBuildRequests(brids)
+        d = self.master.db.buildrequests.reclaimBuildRequests(brids)
         d.addErrback(log.err, 'while re-claiming running BuildRequests')
         return d