From 26cb225c70847b7ac3c285455ef7308499d79e72 Mon Sep 17 00:00:00 2001 From: Chris Patterson Date: Tue, 19 Apr 2022 12:12:04 -0400 Subject: [PATCH 1/2] sources/azure: refactor http_with_retries to remove **kwargs Remove unused functionality and general kwargs support in favor of defined parameters. As only headers and data kwargs are used, add those explicitly to http_with_retries(). There should be no behavioral changes. Signed-off-by: Chris Patterson --- cloudinit/sources/helpers/azure.py | 26 +++------ tests/unittests/sources/test_azure_helper.py | 56 ++++---------------- 2 files changed, 16 insertions(+), 66 deletions(-) diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py index 196ee0311f4..a4ddc7ed80d 100755 --- a/cloudinit/sources/helpers/azure.py +++ b/cloudinit/sources/helpers/azure.py @@ -330,34 +330,22 @@ def get_ip_from_lease_value(fallback_lease_value): @azure_ds_telemetry_reporter -def http_with_retries(url, **kwargs) -> url_helper.UrlResponse: +def http_with_retries( + url, *, headers: dict, data=None +) -> url_helper.UrlResponse: """Wrapper around url_helper.readurl() with custom telemetry logging that url_helper.readurl() does not provide. """ max_readurl_attempts = 240 - default_readurl_timeout = 5 + timeout = 5 sleep_duration_between_retries = 5 periodic_logging_attempts = 12 - if "timeout" not in kwargs: - kwargs["timeout"] = default_readurl_timeout - - # remove kwargs that cause url_helper.readurl to retry, - # since we are already implementing our own retry logic. - if kwargs.pop("retries", None): - LOG.warning( - "Ignoring retries kwarg passed in for " - "communication with Azure endpoint." - ) - if kwargs.pop("infinite", None): - LOG.warning( - "Ignoring infinite kwarg passed in for communication " - "with Azure endpoint." - ) - for attempt in range(1, max_readurl_attempts + 1): try: - ret = url_helper.readurl(url, **kwargs) + ret = url_helper.readurl( + url, headers=headers, data=data, timeout=timeout + ) report_diagnostic_event( "Successful HTTP request with Azure endpoint %s after " diff --git a/tests/unittests/sources/test_azure_helper.py b/tests/unittests/sources/test_azure_helper.py index 326b41cc413..4279dc4f643 100644 --- a/tests/unittests/sources/test_azure_helper.py +++ b/tests/unittests/sources/test_azure_helper.py @@ -1,6 +1,5 @@ # This file is part of cloud-init. See LICENSE file for license information. -import copy import os import re import unittest @@ -358,7 +357,7 @@ def setUp(self): def test_http_with_retries(self): self.m_readurl.return_value = "TestResp" self.assertEqual( - azure_helper.http_with_retries("testurl"), + azure_helper.http_with_retries("testurl", headers={}), self.m_readurl.return_value, ) self.assertEqual(self.m_readurl.call_count, 1) @@ -367,7 +366,10 @@ def test_http_with_retries_propagates_readurl_exc_and_logs_exc(self): self.m_readurl.side_effect = SentinelException self.assertRaises( - SentinelException, azure_helper.http_with_retries, "testurl" + SentinelException, + azure_helper.http_with_retries, + "testurl", + headers={}, ) self.assertEqual(self.m_readurl.call_count, self.max_readurl_attempts) @@ -394,7 +396,7 @@ def test_http_with_retries_delayed_success_due_to_temporary_readurl_exc( ] * self.periodic_logging_attempts + ["TestResp"] self.m_readurl.return_value = "TestResp" - response = azure_helper.http_with_retries("testurl") + response = azure_helper.http_with_retries("testurl", headers={}) self.assertEqual(response, self.m_readurl.return_value) self.assertEqual( self.m_readurl.call_count, self.periodic_logging_attempts + 1 @@ -412,7 +414,7 @@ def test_http_with_retries_long_delay_logs_periodic_failure_msg(self): ] * self.periodic_logging_attempts + ["TestResp"] self.m_readurl.return_value = "TestResp" - azure_helper.http_with_retries("testurl") + azure_helper.http_with_retries("testurl", headers={}) self.assertEqual( self.m_readurl.call_count, self.periodic_logging_attempts + 1 @@ -440,7 +442,7 @@ def test_http_with_retries_short_delay_does_not_log_periodic_failure_msg( ) + ["TestResp"] self.m_readurl.return_value = "TestResp" - azure_helper.http_with_retries("testurl") + azure_helper.http_with_retries("testurl", headers={}) self.assertEqual( self.m_readurl.call_count, self.periodic_logging_attempts ) @@ -465,49 +467,9 @@ def test_http_with_retries_calls_url_helper_readurl_with_args_kwargs(self): kwargs = { "headers": mock.MagicMock(), "data": mock.MagicMock(), - # timeout kwarg should not be modified or deleted if present - "timeout": mock.MagicMock(), - } - azure_helper.http_with_retries(testurl, **kwargs) - self.m_readurl.assert_called_once_with(testurl, **kwargs) - - def test_http_with_retries_adds_timeout_kwarg_if_not_present(self): - testurl = mock.MagicMock() - kwargs = {"headers": mock.MagicMock(), "data": mock.MagicMock()} - expected_kwargs = copy.deepcopy(kwargs) - expected_kwargs["timeout"] = self.default_readurl_timeout - - azure_helper.http_with_retries(testurl, **kwargs) - self.m_readurl.assert_called_once_with(testurl, **expected_kwargs) - - def test_http_with_retries_deletes_retries_kwargs_passed_in(self): - """http_with_retries already implements retry logic, - so url_helper.readurl should not have retries. - http_with_retries should delete kwargs that - cause url_helper.readurl to retry. - """ - testurl = mock.MagicMock() - kwargs = { - "headers": mock.MagicMock(), - "data": mock.MagicMock(), - "timeout": mock.MagicMock(), - "retries": mock.MagicMock(), - "infinite": mock.MagicMock(), } - expected_kwargs = copy.deepcopy(kwargs) - expected_kwargs.pop("retries", None) - expected_kwargs.pop("infinite", None) - azure_helper.http_with_retries(testurl, **kwargs) - self.m_readurl.assert_called_once_with(testurl, **expected_kwargs) - self.assertIn( - "retries kwarg passed in for communication with Azure endpoint.", - self.logs.getvalue(), - ) - self.assertIn( - "infinite kwarg passed in for communication with Azure endpoint.", - self.logs.getvalue(), - ) + self.m_readurl.assert_called_once_with(testurl, **kwargs, timeout=5) class TestOpenSSLManager(CiTestCase): From e5706fb1d7566ec48c33e0c93c3d5e36790192fe Mon Sep 17 00:00:00 2001 From: Chris Patterson Date: Wed, 20 Apr 2022 07:49:20 -0400 Subject: [PATCH 2/2] complete type annotations and update docstring Signed-off-by: Chris Patterson --- cloudinit/sources/helpers/azure.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py index a4ddc7ed80d..4bb8b8dbd9a 100755 --- a/cloudinit/sources/helpers/azure.py +++ b/cloudinit/sources/helpers/azure.py @@ -331,20 +331,22 @@ def get_ip_from_lease_value(fallback_lease_value): @azure_ds_telemetry_reporter def http_with_retries( - url, *, headers: dict, data=None + url: str, *, headers: dict, data: Optional[str] = None ) -> url_helper.UrlResponse: - """Wrapper around url_helper.readurl() with custom telemetry logging - that url_helper.readurl() does not provide. + """Readurl wrapper for querying wireserver. + + Retries up to 40 minutes: + 240 attempts * (5s timeout + 5s sleep) """ max_readurl_attempts = 240 - timeout = 5 + readurl_timeout = 5 sleep_duration_between_retries = 5 periodic_logging_attempts = 12 for attempt in range(1, max_readurl_attempts + 1): try: ret = url_helper.readurl( - url, headers=headers, data=data, timeout=timeout + url, headers=headers, data=data, timeout=readurl_timeout ) report_diagnostic_event(