From c13128a2291eb479845cb259437142d1db4828c5 Mon Sep 17 00:00:00 2001 From: Christophe Monniez Date: Wed, 5 Jun 2019 17:11:10 +0200 Subject: [PATCH] [FIX] runbot: try to update github status multiple times When updating github statuses, it happens that we face a "Bad gateway" from github. In that case, the error is logged in the runbot logs and that's it. As a consequence, when the runbot_merge is waiting status for the staging branch and this kind of error occurs, the runbot_merge timeouts and the users vainly search the reason. With this commit, the runbot tries to update the status at least twice. If it fails, an INFO message is logged on the build itself. --- runbot/models/build.py | 11 +++++---- runbot/models/repo.py | 50 +++++++++++++++++++++++---------------- runbot/tests/test_repo.py | 27 ++++++++++++++++++++- 3 files changed, 62 insertions(+), 26 deletions(-) diff --git a/runbot/models/build.py b/runbot/models/build.py index 07f56f84..aa2dd566 100644 --- a/runbot/models/build.py +++ b/runbot/models/build.py @@ -858,10 +858,13 @@ class runbot_build(models.Model): """Notify each repo with a status""" self.ensure_one() if self.config_id.update_github_state: - commits = {(b.repo_id, b.name) for b in self.search([('name', '=', self.name)])} - for repo, commit_hash in commits: - _logger.debug("github updating %s status %s to %s in repo %s", status['context'], commit_hash, status['state'], repo.name) - repo._github('/repos/:owner/:repo/statuses/%s' % commit_hash, status, ignore_errors=True) + repos = {b.repo_id for b in self.search([('name', '=', self.name)])} + for repo in repos: + _logger.debug("github updating %s status %s to %s in repo %s", status['context'], self.name, status['state'], repo.name) + try: + repo._github('/repos/:owner/:repo/statuses/%s' % self.name, status, ignore_errors=True) + except Exception: + self._log('_github_status_notify_all', 'Status notification failed for "%s" in repo "%s"' % (self.name, repo.name)) def _github_status(self): """Notify github of failed/successful builds""" diff --git a/runbot/models/repo.py b/runbot/models/repo.py index a6b238d5..f090004a 100644 --- a/runbot/models/repo.py +++ b/runbot/models/repo.py @@ -123,31 +123,39 @@ class runbot_repo(models.Model): return False return True - def _github(self, url, payload=None, ignore_errors=False): + def _github(self, url, payload=None, ignore_errors=False, nb_tries=2): """Return a http request to be sent to github""" for repo in self: if not repo.token: return - try: - match_object = re.search('([^/]+)/([^/]+)/([^/.]+(.git)?)', repo.base) - if match_object: - url = url.replace(':owner', match_object.group(2)) - url = url.replace(':repo', match_object.group(3)) - url = 'https://api.%s%s' % (match_object.group(1), url) - session = requests.Session() - session.auth = (repo.token, 'x-oauth-basic') - session.headers.update({'Accept': 'application/vnd.github.she-hulk-preview+json'}) - if payload: - response = session.post(url, data=json.dumps(payload)) - else: - response = session.get(url) - response.raise_for_status() - return response.json() - except Exception: - if ignore_errors: - _logger.exception('Ignored github error %s %r', url, payload) - else: - raise + match_object = re.search('([^/]+)/([^/]+)/([^/.]+(.git)?)', repo.base) + if match_object: + url = url.replace(':owner', match_object.group(2)) + url = url.replace(':repo', match_object.group(3)) + url = 'https://api.%s%s' % (match_object.group(1), url) + session = requests.Session() + session.auth = (repo.token, 'x-oauth-basic') + session.headers.update({'Accept': 'application/vnd.github.she-hulk-preview+json'}) + try_count = 0 + while try_count < nb_tries: + try: + if payload: + response = session.post(url, data=json.dumps(payload)) + else: + response = session.get(url) + response.raise_for_status() + if try_count > 0: + _logger.info('Success after %s tries' % (try_count + 1)) + return response.json() + except Exception as e: + try_count += 1 + if try_count < nb_tries: + time.sleep(2) + else: + if ignore_errors: + _logger.exception('Ignored github error %s %r (try %s/%s)' % (url, payload, try_count + 1, nb_tries)) + else: + raise def _get_fetch_head_time(self): self.ensure_one() diff --git a/runbot/tests/test_repo.py b/runbot/tests/test_repo.py index daffef20..9a395492 100644 --- a/runbot/tests/test_repo.py +++ b/runbot/tests/test_repo.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- +import datetime from unittest import skip -from unittest.mock import patch +from unittest.mock import patch, Mock from odoo.tests import common import logging import odoo @@ -152,6 +153,30 @@ class Test_Repo(common.TransactionCase): _logger.info('Create pending builds took: %ssec', (time.time() - inserted_time)) + def test_github(self): + """ Test different github responses or failures""" + repo = self.Repo.create({'name': 'bla@example.com:foo/foo'}) + self.assertEqual(repo._github('/repos/:owner/:repo/statuses/abcdef', dict(), ignore_errors=True), None, 'A repo without token should return None') + repo.token = 'abc' + with patch('odoo.addons.runbot.models.repo.requests.Session') as mock_session: + with self.assertRaises(Exception, msg='should raise an exception with ignore_errors=False'): + mock_session.return_value.post.side_effect = Exception('301: Bad gateway') + repo._github('/repos/:owner/:repo/statuses/abcdef', {'foo': 'bar'}, ignore_errors=False) + + mock_session.return_value.post.reset_mock() + with self.assertLogs(logger='odoo.addons.runbot.models.repo') as assert_log: + repo._github('/repos/:owner/:repo/statuses/abcdef', {'foo': 'bar'}, ignore_errors=True) + self.assertIn('Ignored github error', assert_log.output[0]) + + self.assertEqual(2, mock_session.return_value.post.call_count, "_github method should try two times by default") + + mock_session.return_value.post.reset_mock() + mock_session.return_value.post.side_effect = [Exception('301: Bad gateway'), Mock()] + with self.assertLogs(logger='odoo.addons.runbot.models.repo') as assert_log: + repo._github('/repos/:owner/:repo/statuses/abcdef', {'foo': 'bar'}, ignore_errors=True) + self.assertIn('Success after 2 tries', assert_log.output[0]) + + self.assertEqual(2, mock_session.return_value.post.call_count, "_github method should try two times by default") class Test_Repo_Scheduler(common.TransactionCase):