Handle missing diff_refs attribute

Recently, on a production Zuul acting on projects hosted on gitlab.com,
it has been discovered that a merge requested fetched from the
API (just after Zuul receives the merge request created event) could have
the "diff_refs" attribute set to None.

Related bug: https://gitlab.com/gitlab-org/gitlab/-/issues/386562

Leading to the following stacktrace in the logs:

2022-12-14 10:08:47,921 ERROR zuul.GitlabEventConnector: Exception handling Gitlab event:
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/zuul/driver/gitlab/gitlabconnection.py", line 102, in run
    self.event_queue.election.run(self._run)
  File "/usr/local/lib/python3.8/site-packages/zuul/zk/election.py", line 28, in run
    return super().run(func, *args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/kazoo/recipe/election.py", line 54, in run
    func(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/zuul/driver/gitlab/gitlabconnection.py", line 110, in _run
    self._handleEvent(event)
  File "/usr/local/lib/python3.8/site-packages/zuul/driver/gitlab/gitlabconnection.py", line 246, in _handleEvent
    self.connection._getChange(change_key, refresh=True,
  File "/usr/local/lib/python3.8/site-packages/zuul/driver/gitlab/gitlabconnection.py", line 621, in _getChange
    change = self._change_cache.updateChangeWithRetry(change_key, change,
  File "/usr/local/lib/python3.8/site-packages/zuul/zk/change_cache.py", line 432, in updateChangeWithRetry
    update_func(change)
  File "/usr/local/lib/python3.8/site-packages/zuul/driver/gitlab/gitlabconnection.py", line 619, in _update_change
    self._updateChange(c, event, mr)
  File "/usr/local/lib/python3.8/site-packages/zuul/driver/gitlab/gitlabconnection.py", line 665, in _updateChange
    change.commit_id = change.mr['diff_refs'].get('head_sha')
AttributeError: 'NoneType' object has no attribute 'get'

The attribute "diff_refs" becomes an object (with the expected keys) few
moments later.

In order to avoid this situation, this change adds a mechanism to retry
fetching a MR until it owns some expected fields. In our case only
"diff_refs".

https://docs.gitlab.com/ee/api/merge_requests.html#response

Tests are included with that change.

Change-Id: I6f279516728def655acb8933542a02a4dbb3ccb6

3 files changed, 93 insertions, 7 deletions

diff --git a/tests/base.py b/tests/base.py
index 239b243a5..731d6c3cc 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -2117,6 +2117,21 @@ class FakeGitlabConnection(gitlabconnection.GitlabConnection):
         yield
         self._test_web_server.options['community_edition'] = False
 
+    @contextmanager
+    def enable_delayed_complete_mr(self, complete_at):
+        self._test_web_server.options['delayed_complete_mr'] = complete_at
+        yield
+        self._test_web_server.options['delayed_complete_mr'] = 0
+
+    @contextmanager
+    def enable_uncomplete_mr(self):
+        self._test_web_server.options['uncomplete_mr'] = True
+        orig = self.gl_client.get_mr_wait_factor
+        self.gl_client.get_mr_wait_factor = 0.1
+        yield
+        self.gl_client.get_mr_wait_factor = orig
+        self._test_web_server.options['uncomplete_mr'] = False
+
 
 class GitlabChangeReference(git.Reference):
     _common_path_default = "refs/merge-requests"
diff --git a/tests/fakegitlab.py b/tests/fakegitlab.py
index c4706b3b1..59b85ca96 100644
--- a/tests/fakegitlab.py
+++ b/tests/fakegitlab.py
@@ -21,6 +21,7 @@ import re
 import socketserver
 import threading
 import urllib.parse
+import time
 
 from git.util import IterableList
 
@@ -32,12 +33,17 @@ class GitlabWebServer(object):
         self.merge_requests = merge_requests
         self.fake_repos = defaultdict(lambda: IterableList('name'))
         # A dictionary so we can mutate it
-        self.options = dict(community_edition=False)
+        self.options = dict(
+            community_edition=False,
+            delayed_complete_mr=0,
+            uncomplete_mr=False)
+        self.stats = {"get_mr": 0}
 
     def start(self):
         merge_requests = self.merge_requests
         fake_repos = self.fake_repos
         options = self.options
+        stats = self.stats
 
         class Server(http.server.SimpleHTTPRequestHandler):
             log = logging.getLogger("zuul.test.GitlabWebServer")
@@ -146,6 +152,7 @@ class GitlabWebServer(object):
                 self.wfile.write(data)
 
             def get_mr(self, project, mr):
+                stats["get_mr"] += 1
                 mr = self._get_mr(project, mr)
                 data = {
                     'target_branch': mr.branch,
@@ -162,13 +169,20 @@ class GitlabWebServer(object):
                     'labels': mr.labels,
                     'merged_at': mr.merged_at.strftime('%Y-%m-%dT%H:%M:%S.%fZ')
                     if mr.merged_at else mr.merged_at,
-                    'diff_refs': {
+                    'merge_status': mr.merge_status,
+                }
+                if options['delayed_complete_mr'] and \
+                        time.monotonic() < options['delayed_complete_mr']:
+                    diff_refs = None
+                elif options['uncomplete_mr']:
+                    diff_refs = None
+                else:
+                    diff_refs = {
                         'base_sha': mr.base_sha,
                         'head_sha': mr.sha,
                         'start_sha': 'c380d3acebd181f13629a25d2e2acca46ffe1e00'
-                    },
-                    'merge_status': mr.merge_status,
-                }
+                    }
+                data['diff_refs'] = diff_refs
                 self.send_data(data)
 
             def get_mr_approvals(self, project, mr):
diff --git a/tests/unit/test_gitlab_driver.py b/tests/unit/test_gitlab_driver.py
index 6c4d4eeb9..e04a3b5d6 100644
--- a/tests/unit/test_gitlab_driver.py
+++ b/tests/unit/test_gitlab_driver.py
@@ -126,6 +126,27 @@ class TestGitlabDriver(ZuulTestCase):
         self.assertTrue(A.approved)
 
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
+    def test_merge_request_opened_imcomplete(self):
+
+        now = time.monotonic()
+        complete_at = now + 3
+        with self.fake_gitlab.enable_delayed_complete_mr(complete_at):
+            description = "This is the\nMR description."
+            A = self.fake_gitlab.openFakeMergeRequest(
+                'org/project', 'master', 'A', description=description)
+            self.fake_gitlab.emitEvent(
+                A.getMergeRequestOpenedEvent(), project='org/project')
+            self.waitUntilSettled()
+
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test1').result)
+
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test2').result)
+
+        self.assertTrue(self.fake_gitlab._test_web_server.stats["get_mr"] > 2)
+
+    @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_merge_request_updated(self):
 
         A = self.fake_gitlab.openFakeMergeRequest('org/project', 'master', 'A')
@@ -407,7 +428,7 @@ class TestGitlabDriver(ZuulTestCase):
 
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_pull_request_with_dyn_reconf(self):
-
+        path = os.path.join(self.upstream_root, 'org/project')
         zuul_yaml = [
             {'job': {
                 'name': 'project-test3',
@@ -424,11 +445,13 @@ class TestGitlabDriver(ZuulTestCase):
         playbook = "- hosts: all\n  tasks: []"
 
         A = self.fake_gitlab.openFakeMergeRequest(
-            'org/project', 'master', 'A')
+            'org/project', 'master', 'A',
+            base_sha=git.Repo(path).head.object.hexsha)
         A.addCommit(
             {'.zuul.yaml': yaml.dump(zuul_yaml),
              'job.yaml': playbook}
         )
+        A.addCommit({"dummy.file": ""})
         self.fake_gitlab.emitEvent(A.getMergeRequestOpenedEvent())
         self.waitUntilSettled()
 
@@ -440,6 +463,40 @@ class TestGitlabDriver(ZuulTestCase):
                          self.getJobFromHistory('project-test3').result)
 
     @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
+    def test_pull_request_with_dyn_reconf_alt(self):
+        with self.fake_gitlab.enable_uncomplete_mr():
+            zuul_yaml = [
+                {'job': {
+                    'name': 'project-test3',
+                    'run': 'job.yaml'
+                }},
+                {'project': {
+                    'check': {
+                        'jobs': [
+                            'project-test3'
+                        ]
+                    }
+                }}
+            ]
+            playbook = "- hosts: all\n  tasks: []"
+            A = self.fake_gitlab.openFakeMergeRequest(
+                'org/project', 'master', 'A')
+            A.addCommit(
+                {'.zuul.yaml': yaml.dump(zuul_yaml),
+                 'job.yaml': playbook}
+            )
+            A.addCommit({"dummy.file": ""})
+            self.fake_gitlab.emitEvent(A.getMergeRequestOpenedEvent())
+            self.waitUntilSettled()
+
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test1').result)
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test2').result)
+            self.assertEqual('SUCCESS',
+                             self.getJobFromHistory('project-test3').result)
+
+    @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab')
     def test_ref_updated_and_tenant_reconfigure(self):
 
         self.waitUntilSettled()