[re-land] sync_stages: fix _VerifyMasterId to respect milestone_version
This is a re-land of https://chromium-review.googlesource.com/344762
The original relied on cidb to return a build status is that it was not
returning. The issue was masked in unit tests, because fake_cidb *was*
returning this key. In this patch, the underlying issue is fixed, and
fake_cidb is brought into compliance with cidb in terms of exactly what
keys it returns (so that in the future, unit tests will not be fooled).
BUG=chromium:611929
TEST=cidb_integration_test; unit tests
Change-Id: Ib002348e957f75cc12fc626c2729e0c27e0bb2a7
Previous-Reviewed-on: https://chromium-review.googlesource.com/345430
(cherry picked from commit 3468867ee9e0a8469afb8fba5da0ca82ad1715e3)
Reviewed-on: https://chromium-review.googlesource.com/345699
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Aviv Keshet <akeshet@chromium.org>
Commit-Queue: Aviv Keshet <akeshet@chromium.org>
diff --git a/cbuildbot/stages/sync_stages.py b/cbuildbot/stages/sync_stages.py
index 254177f..20202e0 100644
--- a/cbuildbot/stages/sync_stages.py
+++ b/cbuildbot/stages/sync_stages.py
@@ -699,7 +699,9 @@
if db and master_id:
assert not self._run.options.force_version
master_build_status = db.GetBuildStatus(master_id)
- latest = db.GetBuildHistory(master_build_status['build_config'], 1)
+ latest = db.GetBuildHistory(
+ master_build_status['build_config'], 1,
+ milestone_version=master_build_status['milestone_version'])
if latest and latest[0]['id'] != master_id:
raise failures_lib.MasterSlaveVersionMismatchFailure(
'This slave\'s master (id=%s) has been supplanted by a newer '
diff --git a/cbuildbot/stages/sync_stages_unittest.py b/cbuildbot/stages/sync_stages_unittest.py
index dd38654..9c4d7ba 100644
--- a/cbuildbot/stages/sync_stages_unittest.py
+++ b/cbuildbot/stages/sync_stages_unittest.py
@@ -16,6 +16,7 @@
from chromite.cbuildbot import chromeos_config
from chromite.cbuildbot import constants
+from chromite.cbuildbot import failures_lib
from chromite.cbuildbot import lkgm_manager
from chromite.cbuildbot import manifest_version
from chromite.cbuildbot import manifest_version_unittest
@@ -185,8 +186,6 @@
self.PatchObject(sync_stages.ManifestVersionedSyncStage,
'_GetMasterVersion', return_value='foo',
autospec=True)
- self.PatchObject(sync_stages.ManifestVersionedSyncStage,
- '_VerifyMasterId', autospec=True)
self.PatchObject(manifest_version.BuildSpecsManager, 'BootstrapFromVersion',
autospec=True)
self.PatchObject(repository.RepoRepository, 'Sync', autospec=True)
@@ -450,14 +449,17 @@
class SlaveCQSyncTest(BaseCQTestCase):
"""Tests the CommitQueueSync stage for the paladin slaves."""
BOT_ID = 'x86-alex-paladin'
+ MILESTONE_VERSION = '10'
def setUp(self):
- self._run.options.master_build_id = 1234
+ self._run.options.master_build_id = self.fake_db.InsertBuild(
+ 'master builder name', 'waterfall', 1, 'master-paladin', 'bot hostname')
+ self.fake_db.UpdateMetadata(
+ self._run.options.master_build_id,
+ {'version': {'milestone': self.MILESTONE_VERSION}})
self.PatchObject(sync_stages.ManifestVersionedSyncStage,
'_GetMasterVersion', return_value='foo',
autospec=True)
- self.PatchObject(sync_stages.MasterSlaveLKGMSyncStage,
- '_VerifyMasterId', autospec=True)
self.PatchObject(lkgm_manager.LKGMManager, 'BootstrapFromVersion',
return_value=self.manifest_path, autospec=True)
self.PatchObject(repository.RepoRepository, 'Sync', autospec=True)
@@ -467,6 +469,29 @@
self.sync_stage.PerformStage()
self.ReloadPool()
+ def testSupplantedMaster(self):
+ """Test that stage fails if master has been supplanted."""
+ new_master_build_id = self.fake_db.InsertBuild(
+ 'master builder name', 'waterfall', 2, 'master-paladin', 'bot hostname')
+ self.fake_db.UpdateMetadata(
+ new_master_build_id,
+ {'version': {'milestone': self.MILESTONE_VERSION}})
+ with self.assertRaises(failures_lib.MasterSlaveVersionMismatchFailure):
+ self.sync_stage.PerformStage()
+
+ def testSupplantedMasterDifferentMilestone(self):
+ """Test that master supplanting logic respects milestone.
+
+ The master-was-supplanted logic should ignore masters for different
+ milestone version.
+ """
+ new_master_build_id = self.fake_db.InsertBuild(
+ 'master builder name', 'waterfall', 2, 'master-paladin', 'bot hostname')
+ self.fake_db.UpdateMetadata(
+ new_master_build_id,
+ {'version': {'milestone': 'foo'}})
+ self.sync_stage.PerformStage()
+
class MasterCQSyncTestCase(BaseCQTestCase):
"""Helper class for testing the CommitQueueSync stage masters."""
@@ -1169,8 +1194,8 @@
{'version': {'full': 'R43-7141.0.0-rc1'}})
self._run.attrs.metadata.UpdateWithDict(
{'version': {'full': 'R44-7142.0.0-rc1'}})
- self.fake_db.UpdateMetadata(id1 + 1, metadata_1)
- self.fake_db.UpdateMetadata(id2 + 1, metadata_2)
+ self.fake_db.UpdateMetadata(id1, metadata_1)
+ self.fake_db.UpdateMetadata(id2, metadata_2)
v = self.sync_stage.GetLastChromeOSVersion()
self.assertEqual(v.milestone, '43')
self.assertEqual(v.platform, '7141.0.0-rc1')
diff --git a/lib/cidb.py b/lib/cidb.py
index e581d2f..fec50c3 100644
--- a/lib/cidb.py
+++ b/lib/cidb.py
@@ -588,6 +588,11 @@
NUM_RESULTS_NO_LIMIT = -1
+ BUILD_STATUS_KEYS = (
+ 'id', 'build_config', 'start_time', 'finish_time', 'status', 'waterfall',
+ 'build_number', 'builder_name', 'platform_version', 'full_version',
+ 'milestone_version', 'important')
+
def __init__(self, db_credentials_dir, *args, **kwargs):
super(CIDBConnection, self).__init__('cidb', CIDB_MIGRATIONS_DIR,
db_credentials_dir, *args, **kwargs)
@@ -947,15 +952,13 @@
Returns:
A list of dictionary with keys (id, build_config, start_time,
finish_time, status, waterfall, build_number, builder_name,
- platform_version, full_version, important), or None if no build
- with this id was found.
+ platform_version, full_version, milestone_version, important)
+ (see BUILD_STATUS_KEYS) or None if no build with this id was found.
"""
return self._SelectWhere(
'buildTable',
'id IN (%s)' % ','.join(str(int(x)) for x in build_ids),
- ['id', 'build_config', 'start_time', 'finish_time', 'status',
- 'waterfall', 'build_number', 'builder_name', 'platform_version',
- 'full_version', 'important'])
+ self.BUILD_STATUS_KEYS)
@minimum_schema(30)
def GetBuildStages(self, build_id):
@@ -1001,13 +1004,11 @@
Returns:
A list containing, for each slave build (row) found, a dictionary
- with keys (id, build_config, start_time, finish_time, status, important).
+ with keys BUILD_STATUS_KEYS.
"""
return self._SelectWhere('buildTable',
'master_build_id = %d' % master_build_id,
- ['id', 'build_config', 'start_time',
- 'finish_time', 'status', 'important',
- 'waterfall'])
+ self.BUILD_STATUS_KEYS)
@minimum_schema(30)
def GetSlaveStages(self, master_build_id):
@@ -1093,7 +1094,7 @@
@minimum_schema(43)
def GetBuildHistory(self, build_config, num_results,
ignore_build_id=None, start_date=None, end_date=None,
- starting_build_number=None):
+ starting_build_number=None, milestone_version=None):
"""Returns basic information about most recent builds.
By default this function returns the most recent builds. Some arguments can
@@ -1113,6 +1114,8 @@
before this date.
starting_build_number: (Optional) The minimum build_number on the CQ
master for which data should be retrieved.
+ milestone_version: (Optional) Return only results for this
+ milestone_version.
Returns:
A sorted list of dicts containing up to |number| dictionaries for
@@ -1121,6 +1124,7 @@
start_time, finish_time, platform_version, full_version, status,
important].
"""
+ # TODO(akeshet): Unify this with BUILD_STATUS_KEYS
columns = ['id', 'build_config', 'buildbot_generation', 'waterfall',
'build_number', 'start_time', 'finish_time', 'platform_version',
'full_version', 'status', 'important']
@@ -1136,6 +1140,8 @@
where_clauses.append('build_number >= %d' % starting_build_number)
if ignore_build_id is not None:
where_clauses.append('id != %d' % ignore_build_id)
+ if milestone_version is not None:
+ where_clauses.append('milestone_version = "%s"' % milestone_version)
query = (
'SELECT %s'
' FROM buildTable'
diff --git a/lib/cidb_integration_test.py b/lib/cidb_integration_test.py
index 3a57316..682e70a 100644
--- a/lib/cidb_integration_test.py
+++ b/lib/cidb_integration_test.py
@@ -200,6 +200,14 @@
current_db_time = db.GetTime()
self.assertEqual(type(current_db_time), datetime.datetime)
+ def testGetBuildStatusKeys(self):
+ db = self._PrepareFreshDatabase()
+ build_id = db.InsertBuild('builder_name', 'waterfall', 1, 'build_config',
+ 'bot_hostname')
+ build_status = db.GetBuildStatus(build_id)
+ for k in db.BUILD_STATUS_KEYS:
+ self.assertIn(k, build_status)
+
def testBuildMessages(self):
db = self._PrepareFreshDatabase(45)
self.assertEqual([], db.GetBuildMessages(1))
@@ -356,6 +364,9 @@
self.assertEqual(len(last_status), 1)
last_status = readonly_db.GetBuildHistory('master-paladin', 5)
self.assertEqual(len(last_status), 5)
+ last_status = readonly_db.GetBuildHistory('master-paladin', 5,
+ milestone_version=52)
+ self.assertEqual(len(last_status), 0)
# Make sure keys are sorted correctly.
build_ids = []
for index, status in enumerate(last_status):
diff --git a/lib/fake_cidb.py b/lib/fake_cidb.py
index 5cabeb7..2908c49 100644
--- a/lib/fake_cidb.py
+++ b/lib/fake_cidb.py
@@ -10,6 +10,7 @@
import itertools
from chromite.cbuildbot import constants
+from chromite.lib import cidb
from chromite.lib import clactions
@@ -30,6 +31,11 @@
self.fake_time = None
self.fake_keyvals = fake_keyvals or {}
+ def _TrimStatus(self, status):
+ """Trims a build row to keys that should be returned by GetBuildStatus"""
+ return {k: v for (k, v) in status.items()
+ if k in cidb.CIDBConnection.BUILD_STATUS_KEYS}
+
def SetTime(self, fake_time):
"""Sets a fake time to be retrieved by GetTime.
@@ -74,10 +80,18 @@
return build_id
def UpdateMetadata(self, build_id, metadata):
- """See cidb.UpdateMetadata."""
- d = metadata.GetDict()
+ """See cidb.UpdateMetadata.
+
+ Args:
+ build_id: The build to update.
+ metadata: A cbuildbot metadata object. Or, a dictionary (note: using
+ a dictionary is not supported by the base cidb API, but
+ is provided for this fake class for ease of use in test
+ set-up code).
+ """
+ d = metadata if isinstance(metadata, dict) else metadata.GetDict()
versions = d.get('version') or {}
- self.buildTable[build_id - 1].update(
+ self.buildTable[build_id].update(
{'chrome_version': versions.get('chrome'),
'milestone_version': versions.get('milestone'),
'platform_version': versions.get('platform'),
@@ -192,48 +206,48 @@
def GetBuildStatus(self, build_id):
"""Gets the status of the build."""
- return self.buildTable[build_id - 1]
+ try:
+ return self._TrimStatus(self.buildTable[build_id])
+ except IndexError:
+ return None
def GetBuildStatuses(self, build_ids):
"""Gets the status of the builds."""
- return [self.buildTable[x -1] for x in build_ids]
+ return [self._TrimStatus(self.buildTable[x]) for x in build_ids]
def GetSlaveStatuses(self, master_build_id):
"""Gets the slaves of given build."""
- return [b for b in self.buildTable
+ return [self._TrimStatus(b) for b in self.buildTable
if b['master_build_id'] == master_build_id]
def GetBuildHistory(self, build_config, num_results,
ignore_build_id=None, start_date=None, end_date=None,
- starting_build_number=None):
+ starting_build_number=None, milestone_version=None):
"""Returns the build history for the given |build_config|."""
- def ReduceToBuildConfig(new_list, current_build):
- """Filters a build list to only those of a given config."""
- if current_build['build_config'] == build_config:
- new_list.append(current_build)
-
- return new_list
-
- build_configs = reduce(ReduceToBuildConfig, self.buildTable, [])
+ builds = [b for b in self.buildTable
+ if b['build_config'] == build_config]
# Reverse sort as that's what's expected.
- build_configs = sorted(build_configs[-num_results:], reverse=True)
+ builds = sorted(builds[-num_results:], reverse=True)
# Filter results.
if ignore_build_id is not None:
- build_configs = [b for b in build_configs if b['id'] != ignore_build_id]
+ builds = [b for b in builds if b['id'] != ignore_build_id]
if start_date is not None:
- build_configs = [b for b in build_configs
- if b['start_time'].date() >= start_date]
+ builds = [b for b in builds
+ if b['start_time'].date() >= start_date]
if end_date is not None:
- build_configs = [b for b in build_configs
- if 'finish_time' in b and
- b['finish_time'] and
- b['finish_time'].date() <= end_date]
+ builds = [b for b in builds
+ if 'finish_time' in b and
+ b['finish_time'] and
+ b['finish_time'].date() <= end_date]
if starting_build_number is not None:
- build_configs = [b for b in build_configs
- if b['build_number'] >= starting_build_number]
+ builds = [b for b in builds
+ if b['build_number'] >= starting_build_number]
+ if milestone_version is not None:
+ builds = [b for b in builds
+ if b['milestone_version'] == milestone_version]
- return build_configs
+ return builds
def GetTimeToDeadline(self, build_id):
"""Gets the time remaining until deadline."""
