Recipe: Add timeout for fetching gerrit commit message in tryserver API

In crbug.com/1196159, a build got stuck at "gerrit changes" step for 3 hours. It would be good to add a timeout so that it fails early. Also adjust the timeout for get_changes to 60s, from 600s Bug: 1196159 Change-Id: Id2327ab40dd2fd8d95aaad6bb724f7ed06e6c244 Recipe-Nontrivial-Roll: infra Recipe-Nontrivial-Roll: chromiumos Recipe-Nontrivial-Roll: build_limited_scripts_slave Recipe-Nontrivial-Roll: build Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2811899 Commit-Queue: Haiyang Pan <hypan@google.com> Reviewed-by: Andrii Shyshkalov <tandrii@google.com>
4 years ago · 057831ef1f
parent b2c07b2300
commit 057831ef1f
19 changed files with 49 additions and 32 deletions
--- a/recipes/README.recipes.md
+++ b/recipes/README.recipes.md
@ -361,7 +361,7 @@ Module for interact with Gerrit endpoints

 Wrapper for easy calling of gerrit_utils steps.

-&mdash; **def [abandon\_change](/recipes/recipe_modules/gerrit/api.py#162)(self, host, change, message=None, name=None, step_test_data=None):**
+&mdash; **def [abandon\_change](/recipes/recipe_modules/gerrit/api.py#173)(self, host, change, message=None, name=None, step_test_data=None):**

 &mdash; **def [create\_gerrit\_branch](/recipes/recipe_modules/gerrit/api.py#32)(self, host, project, branch, commit, \*\*kwargs):**

@ -370,7 +370,7 @@ Creates a new branch from given project and commit
 Returns:
  The ref of the branch created

-&mdash; **def [get\_change\_description](/recipes/recipe_modules/gerrit/api.py#71)(self, host, change, patchset, step_test_data=None):**
+&mdash; **def [get\_change\_description](/recipes/recipe_modules/gerrit/api.py#71)(self, host, change, patchset, timeout=None, step_test_data=None):**

 Gets the description for a given CL and patchset.

@ -382,7 +382,7 @@ Args:
 Returns:
  The description corresponding to given CL and patchset.

-&mdash; **def [get\_changes](/recipes/recipe_modules/gerrit/api.py#120)(self, host, query_params, start=None, limit=None, o_params=None, step_test_data=None, \*\*kwargs):**
+&mdash; **def [get\_changes](/recipes/recipe_modules/gerrit/api.py#131)(self, host, query_params, start=None, limit=None, o_params=None, step_test_data=None, \*\*kwargs):**

 Queries changes for the given host.

@ -408,7 +408,7 @@ Gets a branch from given project and commit
 Returns:
  The revision of the branch

-&mdash; **def [get\_revision\_info](/recipes/recipe_modules/gerrit/api.py#85)(self, host, change, patchset, step_test_data=None):**
+&mdash; **def [get\_revision\_info](/recipes/recipe_modules/gerrit/api.py#90)(self, host, change, patchset, timeout=None, step_test_data=None):**

 Returns the info for a given patchset of a given change.

@ -421,7 +421,7 @@ Returns:
  A dict for the target revision as documented here:
      https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-changes

-&mdash; **def [move\_changes](/recipes/recipe_modules/gerrit/api.py#182)(self, host, project, from_branch, to_branch, step_test_data=None):**
+&mdash; **def [move\_changes](/recipes/recipe_modules/gerrit/api.py#193)(self, host, project, from_branch, to_branch, step_test_data=None):**
 ### *recipe_modules* / [git](/recipes/recipe_modules/git)

 [DEPS](/recipes/recipe_modules/git/__init__.py#1): [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [recipe\_engine/context][recipe_engine/recipe_modules/context], [recipe\_engine/path][recipe_engine/recipe_modules/path], [recipe\_engine/platform][recipe_engine/recipe_modules/platform], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/python][recipe_engine/recipe_modules/python], [recipe\_engine/raw\_io][recipe_engine/recipe_modules/raw_io], [recipe\_engine/runtime][recipe_engine/recipe_modules/runtime], [recipe\_engine/step][recipe_engine/recipe_modules/step]
@ -856,7 +856,7 @@ Args:

 Returned paths will be relative to to patch_root.

-&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#330)(self, tag, patch_text=None):**
+&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#332)(self, tag, patch_text=None):**

 Gets a specific tag from a CL description

@ -879,9 +879,9 @@ Returns true iff the properties exist to match a Gerrit issue.

 Returns true iff we have a change to check out.

-&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#338)(self, footer):**
+&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#340)(self, footer):**

-&mdash; **def [set\_change](/recipes/recipe_modules/tryserver/api.py#341)(self, change):**
+&mdash; **def [set\_change](/recipes/recipe_modules/tryserver/api.py#343)(self, change):**

 Set the gerrit change for this module.

--- a/recipes/recipe_modules/bot_update/examples/full.expected/deprecated_got_revision_mapping.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/deprecated_got_revision_mapping.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/no_apply_patch_on_gclient.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/no_apply_patch_on_gclient.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch_download.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch_download.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_angle.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_angle.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_branch_heads.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_branch_heads.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_feature_branch.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_feature_branch.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_v8_feature_branch.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_v8_feature_branch.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_webrtc.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_gerrit_webrtc.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_v8.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_v8.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_v8_head_by_default.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_v8_head_by_default.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/gerrit/api.py
+++ b/recipes/recipe_modules/gerrit/api.py
@ -68,7 +68,12 @@ class GerritApi(recipe_api.RecipeApi):
    revision = step_result.json.output.get('revision')
    return revision

-  def get_change_description(self, host, change, patchset, step_test_data=None):
+  def get_change_description(self,
+                             host,
+                             change,
+                             patchset,
+                             timeout=None,
+                             step_test_data=None):
    """Gets the description for a given CL and patchset.

    Args:
@ -79,10 +84,15 @@ class GerritApi(recipe_api.RecipeApi):
    Returns:
      The description corresponding to given CL and patchset.
    """
-    ri = self.get_revision_info(host, change, patchset, step_test_data)
+    ri = self.get_revision_info(host, change, patchset, timeout, step_test_data)
    return ri['commit']['message']

-  def get_revision_info(self, host, change, patchset, step_test_data=None):
+  def get_revision_info(self,
+                        host,
+                        change,
+                        patchset,
+                        timeout=None,
+                        step_test_data=None):
    """
    Returns the info for a given patchset of a given change.

@ -106,6 +116,7 @@ class GerritApi(recipe_api.RecipeApi):
                           query_params=[('change', str(change))],
                           o_params=['ALL_REVISIONS', 'ALL_COMMITS'],
                           limit=1,
+                           timeout=timeout,
                           step_test_data=step_test_data)
    cl = cls[0] if len(cls) == 1 else {'revisions': {}}
    for ri in cl['revisions'].values():
--- a/recipes/recipe_modules/tryserver/api.py
+++ b/recipes/recipe_modules/tryserver/api.py
@ -112,7 +112,7 @@ class TryserverApi(recipe_api.RecipeApi):
        o_params=['ALL_REVISIONS', 'DOWNLOAD_COMMANDS'],
        limit=1,
        name='fetch current CL info',
-        timeout=600,
+        timeout=60,
        step_test_data=lambda: self.m.json.test_api.output(mock_res))[0]

    self._gerrit_change_target_ref = res['branch']
@ -306,8 +306,10 @@ class TryserverApi(recipe_api.RecipeApi):
    """Fetch full commit message for Gerrit change."""
    self._ensure_gerrit_change_info()
    self._gerrit_commit_message = self.m.gerrit.get_change_description(
-        'https://%s' % self.gerrit_change.host, self.gerrit_change_number,
-        self.gerrit_patchset_number)
+        'https://%s' % self.gerrit_change.host,
+        self.gerrit_change_number,
+        self.gerrit_patchset_number,
+        timeout=60)

  def _get_footers(self, patch_text=None):
    if patch_text is not None:
--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
@ -66,6 +66,7 @@
    },
    "infra_step": true,
    "name": "gerrit changes",
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
@ -130,6 +131,7 @@
    },
    "infra_step": true,
    "name": "gerrit changes (2)",
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
@ -273,7 +275,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info (2)",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json
@ -23,7 +23,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
@ -66,6 +66,7 @@
    },
    "infra_step": true,
    "name": "gerrit changes",
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
@ -130,6 +131,7 @@
    },
    "infra_step": true,
    "name": "gerrit changes (2)",
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
@ -273,7 +275,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info (2)",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json
@ -40,7 +40,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",
--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json
@ -41,7 +41,7 @@
    },
    "infra_step": true,
    "name": "gerrit fetch current CL info",
-    "timeout": 600,
+    "timeout": 60,
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@json.output@[@@@",
      "@@@STEP_LOG_LINE@json.output@  {@@@",