From f367a64481e1c88a5b9cb239e599a44a7c6e3a90 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 31 Jul 2024 09:40:53 +0200 Subject: [PATCH] [IMP] *: trigger-ify merge cron The merge cron is the one in charge of checking staging state and either integrating the staging into the reference branch (if successful) or cancelling the staging (if failed). The most obvious trigger for the merge cron is a change in staging state from the states computation (transition from pending to either success or failure). Explicitly cancelling / failing a staging marks it as inactive so the merge cron isn't actually needed. However an other major trigger is *timeout*, which doesn't have a trivial signal. Instead, it needs to be hooked on the `timeout_limit`, and has to be re-triggered at every update to the `timeout_limit`, which in normal operations is mostly from "pending" statuses bumping the timeout limit. In that case, `_trigger` to the `timeout_limit` as that's where / when we expect a status change. Worst case scenario with this is we have parasitic wakeups of this cron, but having half a dozen wakeups unnecessary wakeups in an hour is still probably better than having a wakeup every minute. --- forwardport/tests/conftest.py | 1 - runbot_merge/data/merge_cron.xml | 4 +- runbot_merge/models/pull_requests.py | 21 ++- runbot_merge/tests/conftest.py | 2 - runbot_merge/tests/test_basic.py | 146 +++++++++++---------- runbot_merge/tests/test_multirepo.py | 2 +- runbot_merge/tests/test_project_toggles.py | 2 +- 7 files changed, 99 insertions(+), 79 deletions(-) diff --git a/forwardport/tests/conftest.py b/forwardport/tests/conftest.py index 5243d96e..c5951502 100644 --- a/forwardport/tests/conftest.py +++ b/forwardport/tests/conftest.py @@ -7,7 +7,6 @@ import requests @pytest.fixture def default_crons(): return [ - 'runbot_merge.merge_cron', 'runbot_merge.staging_cron', 'runbot_merge.check_linked_prs_status', ] diff --git a/runbot_merge/data/merge_cron.xml b/runbot_merge/data/merge_cron.xml index be06078f..f6a4f3da 100644 --- a/runbot_merge/data/merge_cron.xml +++ b/runbot_merge/data/merge_cron.xml @@ -4,8 +4,8 @@ code model._check_stagings(True) - 1 - minutes + 6 + hours -1 30 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index cd331642..0955c853 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -2062,16 +2062,25 @@ class Stagings(models.Model): for context, status in statuses.get(commit.sha, {}).items() ] + def write(self, vals): + if timeout := vals.get('timeout_limit'): + self.env.ref("runbot_merge.merge_cron")\ + ._trigger(fields.Datetime.to_datetime(timeout)) + + return super().write(vals) + # only depend on staged_at as it should not get modified, but we might # update the CI timeout after the staging have been created and we # *do not* want to update the staging timeouts in that case @api.depends('staged_at') def _compute_timeout_limit(self): + timeouts = set() for st in self: - st.timeout_limit = fields.Datetime.to_string( - fields.Datetime.from_string(st.staged_at) - + datetime.timedelta(minutes=st.target.project_id.ci_timeout) - ) + t = st.timeout_limit = st.staged_at + datetime.timedelta(minutes=st.target.project_id.ci_timeout) + timeouts.add(t) + if timeouts: + # we might have very different limits for each staging so need to schedule them all + self.env.ref("runbot_merge.merge_cron")._trigger_list(timeouts) @api.depends('batch_ids.prs') def _compute_prs(self): @@ -2147,9 +2156,11 @@ class Stagings(models.Model): s.state = st if s.state != 'pending': + self.env.ref("runbot_merge.merge_cron")._trigger() s.staging_end = fields.Datetime.now() if update_timeout_limit: - s.timeout_limit = fields.Datetime.to_string(datetime.datetime.now() + datetime.timedelta(minutes=s.target.project_id.ci_timeout)) + s.timeout_limit = datetime.datetime.now() + datetime.timedelta(minutes=s.target.project_id.ci_timeout) + self.env.ref("runbot_merge.merge_cron")._trigger(s.timeout_limit) _logger.debug("%s got pending status, bumping timeout to %s (%s)", self, s.timeout_limit, cmap) def action_cancel(self): diff --git a/runbot_merge/tests/conftest.py b/runbot_merge/tests/conftest.py index 34d70479..e200434a 100644 --- a/runbot_merge/tests/conftest.py +++ b/runbot_merge/tests/conftest.py @@ -7,8 +7,6 @@ def module(): @pytest.fixture def default_crons(): return [ - # env['runbot_merge.project']._check_stagings() - 'runbot_merge.merge_cron', # env['runbot_merge.project']._create_stagings() 'runbot_merge.staging_cron', # env['runbot_merge.pull_requests']._check_linked_prs_statuses() diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 3208e48f..9026aa02 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -3,6 +3,7 @@ import itertools import json import textwrap import time +from typing import Callable from unittest import mock import pytest @@ -13,8 +14,9 @@ import odoo from utils import _simple_init, seen, matches, get_partner, Commit, pr_page, to_pr, part_of, ensure_one @pytest.fixture(autouse=True) -def _configure_statuses(project, repo): - project.repo_ids.required_statuses = 'legal/cla,ci/runbot' +def _configure_statuses(request, project, repo): + if 'defaultstatuses' not in request.keywords: + project.repo_ids.required_statuses = 'legal/cla,ci/runbot' @pytest.fixture(autouse=True, params=["statuses", "rpc"]) def stagings(request, env, project, repo): @@ -562,19 +564,32 @@ def test_staging_conflict_second(env, repo, users, config): assert pr1_id.state == 'error', "now pr1 should be in error" -def test_staging_ci_timeout(env, repo, config, page): +@pytest.mark.defaultstatuses +@pytest.mark.parametrize('update_op', [ + pytest.param( + lambda _: {'timeout_limit': datetime.datetime.now().isoformat(" ", "seconds")}, + id="set-timeout-limit", + ), + pytest.param( + lambda timeout: {'staged_at': (datetime.datetime.now() - datetime.timedelta(minutes=2*timeout)).isoformat(" ", "seconds")}, + id="set-staged-at", + ), +]) +def test_staging_ci_timeout(env, repo, config, page, update_op: Callable[[int], dict]): """If a staging timeouts (~ delay since staged greater than configured)... requeue? """ with repo: - m = repo.make_commit(None, 'initial', None, tree={'f': 'm'}) + m, _, c2 = repo.make_commits( + None, + Commit('initial', tree={'f': 'm'}), + Commit('first', tree={'f': 'c1'}), + Commit('second', tree={'f': 'c2'}), + ) repo.make_ref('heads/master', m) - c1 = repo.make_commit(m, 'first', None, tree={'f': 'c1'}) - c2 = repo.make_commit(c1, 'second', None, tree={'f': 'c2'}) pr = repo.make_pr(title='title', body='body', target='master', head=c2) - repo.post_status(pr.head, 'success', 'ci/runbot') - repo.post_status(pr.head, 'success', 'legal/cla') + repo.post_status(pr.head, 'success') pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token']) env.run_crons() @@ -582,23 +597,26 @@ def test_staging_ci_timeout(env, repo, config, page): assert pr_id.staging_id timeout = env['runbot_merge.project'].search([]).ci_timeout - pr_id.staging_id.staged_at = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(minutes=2*timeout)) - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + pr_id.staging_id.write(update_op(timeout)) + env.run_crons('runbot_merge.staging_cron') assert pr_id.state == 'error', "timeout should fail the PR" dangerbox = pr_page(page, pr).cssselect('.alert-danger span') assert dangerbox assert dangerbox[0].text == 'timed out (>60 minutes)' +@pytest.mark.defaultstatuses def test_timeout_bump_on_pending(env, repo, config): with repo: - m = repo.make_commit(None, 'initial', None, tree={'f': '0'}) + [m, c] = repo.make_commits( + None, + Commit('initial', tree={'f': '0'}), + Commit('c', tree={'f': '1'}), + ) repo.make_ref('heads/master', m) - c = repo.make_commit(m, 'c', None, tree={'f': '1'}) prx = repo.make_pr(title='title', body='body', target='master', head=c) - repo.post_status(prx.head, 'success', 'ci/runbot') - repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success') prx.post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() @@ -606,7 +624,7 @@ def test_timeout_bump_on_pending(env, repo, config): old_timeout = odoo.fields.Datetime.to_string(datetime.datetime.now() - datetime.timedelta(days=15)) st.timeout_limit = old_timeout with repo: - repo.post_status('staging.master', 'pending', 'ci/runbot') + repo.post_status('staging.master', 'pending') env.run_crons(None) assert st.timeout_limit > old_timeout @@ -1092,63 +1110,63 @@ def test_reopen_merged_pr(env, repo, config, users): ] class TestNoRequiredStatus: + @pytest.mark.defaultstatuses def test_basic(self, env, repo, config): """ check that mergebot can work on a repo with no CI at all """ env['runbot_merge.repository'].search([('name', '=', repo.name)]).status_ids = False with repo: - m = repo.make_commit(None, 'initial', None, tree={'0': '0'}) + [m, c] = repo.make_commits( + None, + Commit('initial', tree={'0': '0'}), + Commit('first', tree={'0': '1'}), + ) repo.make_ref('heads/master', m) - c = repo.make_commit(m, 'first', None, tree={'0': '1'}) - prx = repo.make_pr(title='title', body='body', target='master', head=c) - prx.post_comment('hansen r+', config['role_reviewer']['token']) + pr = repo.make_pr(title='title', body='body', target='master', head=c) + pr.post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]) - assert pr.state == 'ready' - st = pr.staging_id - assert st - env.run_crons() + pr_id = to_pr(env, pr) + + st = env['runbot_merge.stagings'].search([], context={'active_test': False}) assert st.state == 'success' - assert pr.state == 'merged' + assert pr_id.state == 'merged' + @pytest.mark.defaultstatuses def test_updated(self, env, repo, config): env['runbot_merge.repository'].search([('name', '=', repo.name)]).status_ids = False with repo: - m = repo.make_commit(None, 'initial', None, tree={'0': '0'}) + [m, c] = repo.make_commits( + None, + Commit('initial', tree={'0': '0'}), + Commit('first', tree={'0': '1'}), + ) repo.make_ref('heads/master', m) - c = repo.make_commit(m, 'first', None, tree={'0': '1'}) - prx = repo.make_pr(title='title', body='body', target='master', head=c) + pr = repo.make_pr(title='title', body='body', target='master', head=c) env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]) - assert pr.state == 'validated' + pr_id = to_pr(env, pr) + assert pr_id.state == 'validated' # normal push with repo: - repo.make_commits(c, repo.Commit('second', tree={'0': '2'}), ref=prx.ref) + repo.make_commits(c, repo.Commit('second', tree={'0': '2'}), ref=pr.ref) env.run_crons() - assert pr.state == 'validated' + assert pr_id.state == 'validated' with repo: - prx.post_comment('hansen r+', config['role_reviewer']['token']) - assert pr.state == 'ready' + pr.post_comment('hansen r+', config['role_reviewer']['token']) + assert pr_id.state == 'ready' # force push with repo: - repo.make_commits(m, repo.Commit('xxx', tree={'0': 'm'}), ref=prx.ref) + repo.make_commits(m, repo.Commit('xxx', tree={'0': 'm'}), ref=pr.ref) env.run_crons() - assert pr.state == 'validated' + assert pr_id.state == 'validated' with repo: - prx.post_comment('hansen r+', config['role_reviewer']['token']) - assert pr.state == 'ready' + pr.post_comment('hansen r+', config['role_reviewer']['token']) + assert pr_id.state == 'ready' class TestRetry: @pytest.mark.xfail(reason="This may not be a good idea as it could lead to tons of rebuild spam") @@ -1230,7 +1248,7 @@ class TestRetry: with repo: pr.post_comment('hansen retry', config['role_' + retrier]['token']) assert pr_id.state == 'ready' - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + env.run_crons('runbot_merge.staging_cron') staging_head2 = repo.commit('heads/staging.master') assert staging_head2 != staging_head @@ -1263,7 +1281,7 @@ class TestRetry: with repo: pr.post_comment('hansen retry', config['role_reviewer']['token']) - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + env.run_crons('runbot_merge.staging_cron') with repo: repo.post_status('staging.master', 'success', 'legal/cla') @@ -1293,42 +1311,36 @@ class TestRetry: (users['user'], "@{reviewer} retry makes no sense when the PR is not in error.".format_map(users)), ] + @pytest.mark.defaultstatuses @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer']) def test_retry_disable(self, env, repo, disabler, users, config): with repo: prx = _simple_init(repo) - repo.post_status(prx.head, 'success', 'ci/runbot') - repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success') prx.post_comment('hansen r+ delegate=%s rebase-merge' % users['other'], config["role_reviewer"]['token']) env.run_crons() - assert env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]).staging_id + pr_id = to_pr(env, prx) + staging_id = pr_id.staging_id + assert staging_id - staging_head = repo.commit('heads/staging.master') with repo: - repo.post_status('staging.master', 'success', 'legal/cla') - repo.post_status('staging.master', 'failure', 'ci/runbot') + repo.post_status('staging.master', 'failure') env.run_crons() - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]) - assert pr.state == 'error' + assert staging_id.state == 'failure' + assert not staging_id.active + assert pr_id.state == 'error' with repo: prx.post_comment('hansen r-', config['role_' + disabler]['token']) - assert pr.state == 'validated' + assert pr_id.state == 'validated' with repo: repo.make_commit(prx.ref, 'third', None, tree={'m': 'c3'}) # just in case, apparently in some case the first post_status uses the old head... with repo: - repo.post_status(prx.head, 'success', 'ci/runbot') - repo.post_status(prx.head, 'success', 'legal/cla') + repo.post_status(prx.head, 'success') env.run_crons() - assert pr.state == 'validated' + assert pr_id.state == 'validated' class TestMergeMethod: """ @@ -1759,7 +1771,7 @@ commits, I need to know how to merge it: c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'}) prx = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c0) - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + env.run_crons('runbot_merge.staging_cron') with repo: repo.post_status(prx.head, 'success', 'legal/cla') @@ -1801,7 +1813,7 @@ commits, I need to know how to merge it: c0 = repo.make_commit(m, 'C0', None, tree={'a': 'b'}) prx = repo.make_pr(title="gibberish", body=None, target='master', head=c0) - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + env.run_crons('runbot_merge.staging_cron') with repo: repo.post_status(prx.head, 'success', 'legal/cla') @@ -3077,7 +3089,7 @@ class TestBatching(object): with repo: repo.post_status('staging.master', 'success', 'ci/runbot') repo.post_status('staging.master', 'success', 'legal/cla') - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + env.run_crons('runbot_merge.staging_cron') assert pr2.state == 'merged' class TestReviewing: diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 3ac35a8f..3c68bd9e 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -493,7 +493,7 @@ def test_ff_fail(env, project, repo_a, repo_b, config): with repo_a, repo_b: repo_a.post_status('heads/staging.master', 'success') repo_b.post_status('heads/staging.master', 'success') - env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron') + env.run_crons('runbot_merge.staging_cron') assert repo_b.commit('heads/master').id == cn,\ "B should still be at the conflicting commit" assert repo_a.commit('heads/master').id == root_a,\ diff --git a/runbot_merge/tests/test_project_toggles.py b/runbot_merge/tests/test_project_toggles.py index 30ce5f78..0dffdccc 100644 --- a/runbot_merge/tests/test_project_toggles.py +++ b/runbot_merge/tests/test_project_toggles.py @@ -114,7 +114,7 @@ def test_staging_priority(env, project, repo, config, mode, cutoff, second): env[model].browse([cron_id]).write({ 'nextcall': (datetime.datetime.utcnow() + datetime.timedelta(minutes=10)).isoformat(" ", "seconds") }) - env.run_crons('runbot_merge.merge_cron') + env.run_crons(None) assert not staging.active assert not env['runbot_merge.stagings'].search([]).active assert env['runbot_merge.split'].search_count([]) == 2