Repository: Azure/WALinuxAgent
Pull request #1967 (2020-07-25 01:48:17)

Fixed delays for HTTP retries rather than exponential delays
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description

This PR addresses the issue where we can see upto 5mins of provisioning delay due to exponential delays in the restutil logic. Now, we will only have sequential delays. Changed the defaults to be at par with the Windows Guest Agent.

Issue # <!-- if any -->
<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [ ] The title of the PR is clear and informative.
- [ ] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [ ] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [ ] If applicable, the PR references the bug/issue that it fixes in the description.
- [ ] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [ ] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).

codecov[bot]: # [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=h1) Report
> Merging [#1967](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=desc) into [develop](https://codecov.io/gh/Azure/WALinuxAgent/commit/7bdc95efc21d1e09ff8ac7b9aa7f9d30b9d1bf5f&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967/graphs/tree.svg?width=650&height=150&src=pr&token=ae90E2KYCw)](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           develop    #1967      +/-   ##
===========================================
- Coverage    69.59%   69.58%   -0.02%     
===========================================
  Files           85       85              
  Lines        11998    11993       -5     
  Branches      1677     1676       -1     
===========================================
- Hits          8350     8345       -5     
  Misses        3276     3276              
  Partials       372      372              
```


| [Impacted Files](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [azurelinuxagent/common/utils/restutil.py](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967/diff?src=pr&el=tree#diff-YXp1cmVsaW51eGFnZW50L2NvbW1vbi91dGlscy9yZXN0dXRpbC5weQ==) | `90.58% <100.00%> (-0.19%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=footer). Last update [7bdc95e...3e3ad08](https://codecov.io/gh/Azure/WALinuxAgent/pull/1967?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

larohra: DCR Run - https://tuxgold.corp.microsoft.com/job/AzLinux/job/DungeonCrawler/job/DungeonCrawler.larohra/298/

Patch:
diff --git a/azurelinuxagent/common/utils/restutil.py b/azurelinuxagent/common/utils/restutil.py
index 4f9ca317..29b5b5fb 100644
--- a/azurelinuxagent/common/utils/restutil.py
+++ b/azurelinuxagent/common/utils/restutil.py
@@ -35,8 +35,8 @@ from azurelinuxagent.common.version import PY_VERSION_MAJOR, AGENT_NAME, GOAL_ST
 
 SECURE_WARNING_EMITTED = False
 
-DEFAULT_RETRIES = 6
-DELAY_IN_SECONDS = 1
+DEFAULT_RETRIES = 3
+DELAY_IN_SECONDS = 5
 
 THROTTLE_RETRIES = 25
 THROTTLE_DELAY_IN_SECONDS = 1
@@ -135,13 +135,6 @@ class IOErrorCounter(object):
         IOErrorCounter._protocol_endpoint = endpoint
 
 
-def _compute_delay(retry_attempt=1, delay=DELAY_IN_SECONDS):
-    fib = (1, 1)
-    for n in range(retry_attempt):
-        fib = (fib[1], fib[0]+fib[1])
-    return delay*fib[1]
-
-
 def _is_retry_status(status, retry_codes=RETRY_CODES):
     return status in retry_codes
 
@@ -385,12 +378,8 @@ def http_request(method,
             # Compute the request delay
             # -- Use a fixed delay if the server ever rate-throttles the request
             #    (with a safe, minimum number of retry attempts)
-            # -- Otherwise, compute a delay that is the product of the next
-            #    item in the Fibonacci series and the initial delay value
-            delay = THROTTLE_DELAY_IN_SECONDS \
-                        if was_throttled \
-                        else _compute_delay(retry_attempt=attempt,
-                                            delay=retry_delay)
+            # -- Otherwise, use the retry_delay (fixed) with maximum of max_retry retries for the rest of the requests.
+            delay = THROTTLE_DELAY_IN_SECONDS if was_throttled else retry_delay
 
             logger.verbose("[HTTP Retry] "
                         "Attempt {0} of {1} will delay {2} seconds: {3}",


Test patch:
diff --git a/tests/utils/test_rest_util.py b/tests/utils/test_rest_util.py
index 97d1903b..a200a96f 100644
--- a/tests/utils/test_rest_util.py
+++ b/tests/utils/test_rest_util.py
@@ -17,11 +17,14 @@
 
 import os
 import unittest
+from datetime import datetime, timedelta
+from random import randint
 
 from azurelinuxagent.common.exception import HttpError, ResourceGoneError, InvalidContainerError
 import azurelinuxagent.common.utils.restutil as restutil
 from azurelinuxagent.common.utils.restutil import HTTP_USER_AGENT
 from azurelinuxagent.common.future import httpclient, ustr
+from tests.protocol.mocks import MockHttpResponse
 from tests.tools import AgentTestCase, call, Mock, MagicMock, patch
 
 
@@ -578,27 +581,28 @@ class TestHttpOperations(AgentTestCase):
         self.assertEqual(2, _http_request.call_count)
         self.assertEqual(1, _sleep.call_count)
 
-    @patch("time.sleep")
-    @patch("azurelinuxagent.common.utils.restutil._http_request")
-    def test_http_request_retries_with_fibonacci_delay(self, _http_request, _sleep):
-        # Ensure the code is not a throttle code
-        self.assertFalse(httpclient.BAD_GATEWAY in restutil.THROTTLE_CODES)
-
-        _http_request.side_effect = [
-                Mock(status=httpclient.BAD_GATEWAY)
-                    for i in range(restutil.DEFAULT_RETRIES)
-            ] + [Mock(status=httpclient.OK)]
-
-        restutil.http_get("https://foo.bar",
-                            max_retry=restutil.DEFAULT_RETRIES+1)
-
-        self.assertEqual(restutil.DEFAULT_RETRIES+1, _http_request.call_count)
-        self.assertEqual(restutil.DEFAULT_RETRIES, _sleep.call_count)
-        self.assertEqual(
-            [
-                call(restutil._compute_delay(i+1, restutil.DELAY_IN_SECONDS))
-                    for i in range(restutil.DEFAULT_RETRIES)],
-            _sleep.call_args_list)
+    def test_it_should_have_http_request_retries_with_linear_delay(self):
+
+        self.assertTrue(httpclient.BAD_GATEWAY in restutil.RETRY_CODES, "Ensure that the test params are correct")
+        retry_delay_in_sec = 0.05
+
+        for _ in range(3):
+            mock_resp = Mock(return_value=MockHttpResponse(status=httpclient.BAD_GATEWAY))
+            mock_conn = MagicMock(getresponse=mock_resp)
+            max_retry = randint(5, 10)
+            duration = None
+            with patch("azurelinuxagent.common.future.httpclient.HTTPConnection", return_value=mock_conn):
+                with self.assertRaises(HttpError):
+                    start_time = datetime.utcnow()
+                    restutil.http_get("http://foo.bar", retry_delay=retry_delay_in_sec, max_retry=max_retry)
+                duration = datetime.utcnow() - start_time
+
+            self.assertEqual(max_retry, mock_resp.call_count, "Did not Retry the required amount of times")
+            upper_bound = timedelta(seconds=retry_delay_in_sec * (max_retry + 2))
+            lower_bound = timedelta(seconds=retry_delay_in_sec * (max_retry - 2))
+            self.assertTrue(upper_bound >= duration >= lower_bound,
+                            "The total duration for request not in acceptable range. UB: {0}; LB: {1}; Actual: {2}".format(
+                                upper_bound, lower_bound, duration))
 
     @patch("time.sleep")
     @patch("azurelinuxagent.common.utils.restutil._http_request")