From 76c4d24bf5be58053e5081cebea72d0b6c512c38 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 18 Jun 2018 15:23:23 +0200 Subject: [PATCH] [IMP] runbot_merge: don't create unstaged stagings Previously when splitting staging we'd create two never-staged stagings. In a system where the stagings get deleted once done with (succeeeded or failed) that's not really important, but now that we want to keep stagings around inactive things get problematic as this method gunks up the stagings table, plus the post-split stagings would "steal" the original's batches, losing information (relation between stagings and batches). Replace these empty stagings with dedicated *split* objects. A batch can belong to both a staging and a split, the split is deleted once a new staging has been created from it. Eventually we may want to make batches shared between stagings (so we can track the entire history of a batch) but currently that's only PR-level. --- runbot_merge/models/pull_requests.py | 50 ++++++++++++++++------------ runbot_merge/tests/test_basic.py | 17 ++++++---- runbot_merge/tests/test_multirepo.py | 26 +++++++-------- 3 files changed, 52 insertions(+), 41 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 30ed9e29..67325ffb 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -145,14 +145,11 @@ class Project(models.Model): PRs.browse(pr_ids) for _, pr_ids in takewhile(lambda r: r[0] == priority, rows) ] - elif branch.staging_ids: - # Splits can generate inactive stagings, restage these first - # FIXME: these are really weird because they're stagings which never actually get staged, they're just used as splits from failed stagings => maybe add splits for this? - staging = branch.staging_ids[0] - logger.info("Found inactive staging of PRs %s, re-staging", staging.mapped('batch_ids.prs')) - batched_prs = [batch.prs for batch in staging.batch_ids] - staging.batch_ids.write({'active': False}) - staging.active = False + elif branch.split_ids: + split_ids = branch.split_ids[0] + logger.info("Found split of PRs %s, re-staging", split_ids.mapped('batch_ids.prs')) + batched_prs = [batch.prs for batch in split_ids.batch_ids] + split_ids.unlink() elif rows: # p=1 or p=2 batched_prs = [PRs.browse(pr_ids) for _, pr_ids in takewhile(lambda r: r[0] == priority, rows)] @@ -338,12 +335,12 @@ class Branch(models.Model): name = fields.Char(required=True) project_id = fields.Many2one('runbot_merge.project', required=True) - active_staging_id = fields.One2many( - 'runbot_merge.stagings', 'target', - domain=[("heads", "!=", False), ('active', '=', True)], - help="Currently running staging for the branch, there should be only one" + active_staging_id = fields.Many2one( + 'runbot_merge.stagings', compute='_compute_active_staging', store=True, + help="Currently running staging for the branch." ) staging_ids = fields.One2many('runbot_merge.stagings', 'target') + split_ids = fields.One2many('runbot_merge.split', 'target') prs = fields.One2many('runbot_merge.pull_requests', 'target', domain=[ ('state', '!=', 'closed'), @@ -357,6 +354,11 @@ class Branch(models.Model): self._table, ['name', 'project_id']) return res + @api.depends('staging_ids.active') + def _compute_active_staging(self): + for b in self: + b.active_staging_id = b.staging_ids + class PullRequests(models.Model): _name = 'runbot_merge.pull_requests' _order = 'number desc' @@ -720,10 +722,8 @@ class Stagings(models.Model): staged_at = fields.Datetime(default=fields.Datetime.now) restaged = fields.Integer(default=0) - # seems simpler than adding yet another indirection through a model and - # makes checking for actually staged stagings way easier: just see if - # heads is set - heads = fields.Char(help="JSON-encoded map of heads, one per repo in the project") + # seems simpler than adding yet another indirection through a model + heads = fields.Char(required=True, help="JSON-encoded map of heads, one per repo in the project") def _validate(self): Commits = self.env['runbot_merge.commit'] @@ -755,7 +755,7 @@ class Stagings(models.Model): _logger.info(reason, *args) self.batch_ids.write({'active': False}) - self.write({'active': False}) + self.active = False def fail(self, message, prs=None): _logger.error("Staging %s failed: %s", self, message) @@ -766,24 +766,25 @@ class Stagings(models.Model): pr.number, "Staging failed: %s" % message) self.batch_ids.write({'active': False}) - self.write({'active': False}) + self.active = False def try_splitting(self): batches = len(self.batch_ids) if batches > 1: midpoint = batches // 2 h, t = self.batch_ids[:midpoint], self.batch_ids[midpoint:] - sh = self.env['runbot_merge.stagings'].create({ + # NB: batches remain attached to their original staging + sh = self.env['runbot_merge.split'].create({ 'target': self.target.id, 'batch_ids': [(4, batch.id, 0) for batch in h], }) - st = self.env['runbot_merge.stagings'].create({ + st = self.env['runbot_merge.split'].create({ 'target': self.target.id, 'batch_ids': [(4, batch.id, 0) for batch in t], }) _logger.info("Split %s to %s (%s) and %s (%s)", self, h, sh, t, st) - # apoptosis + self.batch_ids.write({'active': False}) self.active = False return True @@ -819,6 +820,12 @@ class Stagings(models.Model): return False +class Split(models.Model): + _name = 'runbot_merge.split' + + target = fields.Many2one('runbot_merge.branch', required=True) + batch_ids = fields.One2many('runbot_merge.batch', 'split_id', context={'active_test': False}) + class Batch(models.Model): """ A batch is a "horizontal" grouping of *codependent* PRs: PRs with the same label & target but for different repositories. These are @@ -830,6 +837,7 @@ class Batch(models.Model): target = fields.Many2one('runbot_merge.branch', required=True) staging_id = fields.Many2one('runbot_merge.stagings') + split_id = fields.Many2one('runbot_merge.split') prs = fields.Many2many('runbot_merge.pull_requests') diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index b90f9a48..0b132e0d 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1050,12 +1050,14 @@ class TestBatching(object): pr2 = env['runbot_merge.pull_requests'].search([('number', '=', pr2.number)]) env['runbot_merge.project']._check_progress() - # should have split the existing batch into two - assert len(env['runbot_merge.stagings'].search([])) == 2 - assert pr1.staging_id and pr2.staging_id - assert pr1.staging_id != pr2.staging_id - assert pr1.staging_id.heads - assert not pr2.staging_id.heads + # should have split the existing batch into two, with one of the + # splits having been immediately restaged + st = env['runbot_merge.stagings'].search([]) + assert len(st) == 1 + assert pr1.staging_id and pr1.staging_id == st + + sp = env['runbot_merge.split'].search([]) + assert len(sp) == 1 # This is the failing PR! h = repo.commit('heads/staging.master').id @@ -1063,7 +1065,8 @@ class TestBatching(object): repo.post_status(h, 'success', 'legal/cla') env['runbot_merge.project']._check_progress() assert pr1.state == 'error' - assert pr2.staging_id.heads + + assert pr2.staging_id h = repo.commit('heads/staging.master').id repo.post_status(h, 'success', 'ci/runbot') diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 1c37ed4e..5566f330 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -302,7 +302,7 @@ def test_batching_split(env, repo_a, repo_b): assert len(st0.batch_ids) == 5 assert len(st0.mapped('batch_ids.prs')) == 8 - # mark b.staging as failed -> should create two new stagings with (0, 1) + # mark b.staging as failed -> should create two splits with (0, 1) # and (2, 3, 4) and stage the first one repo_b.post_status('heads/staging.master', 'success', 'legal/cla') repo_b.post_status('heads/staging.master', 'failure', 'ci/runbot') @@ -310,21 +310,21 @@ def test_batching_split(env, repo_a, repo_b): env['runbot_merge.project']._check_progress() assert not st0.active - sts = env['runbot_merge.stagings'].search([]) - assert len(sts) == 2 - st1, st2 = sts - # a bit oddly st1 is probably the (2,3,4) one: the split staging for - # (0, 1) has been "exploded" and a new staging was created for it - assert not st1.heads - assert len(st1.batch_ids) == 3 - assert st1.mapped('batch_ids.prs') == \ - prs[2][0] | prs[2][1] | prs[3][0] | prs[3][1] | prs[4][0] - assert st2.heads - assert len(st2.batch_ids) == 2 - assert st2.mapped('batch_ids.prs') == \ + # at this point we have a re-staged split and an unstaged split + st = env['runbot_merge.stagings'].search([]) + sp = env['runbot_merge.split'].search([]) + assert st + assert sp + + assert len(st.batch_ids) == 2 + assert st.mapped('batch_ids.prs') == \ prs[0][0] | prs[0][1] | prs[1][1] + assert len(sp.batch_ids) == 3 + assert sp.mapped('batch_ids.prs') == \ + prs[2][0] | prs[2][1] | prs[3][0] | prs[3][1] | prs[4][0] + def test_urgent(env, repo_a, repo_b): """ Either PR of a co-dependent pair being p=0 leads to the entire pair being prioritized