From f9f7ffd7156db28391c006d3ac7685fa6c5afbdd Mon Sep 17 00:00:00 2001 From: Rémy Léone Date: Thu, 1 Mar 2018 18:19:43 +0100 Subject: Simplify some comparisions. Just replace a couple things like: if b > a and b < c: with: if a < b < c: --- cloudinit/url_helper.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'cloudinit/url_helper.py') diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 0a5be0b3..4e814a5f 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -47,7 +47,7 @@ try: _REQ_VER = LooseVersion(_REQ.version) # pylint: disable=no-member if _REQ_VER >= LooseVersion('0.8.8'): SSL_ENABLED = True - if _REQ_VER >= LooseVersion('0.7.0') and _REQ_VER < LooseVersion('1.0.0'): + if LooseVersion('0.7.0') <= _REQ_VER < LooseVersion('1.0.0'): CONFIG_ENABLED = True except ImportError: pass @@ -121,7 +121,7 @@ class UrlResponse(object): upper = 300 if redirects_ok: upper = 400 - if self.code >= 200 and self.code < upper: + if 200 <= self.code < upper: return True else: return False -- cgit v1.2.3 From 097a2967abd6c78edfbdc035e7141f2d142f17ae Mon Sep 17 00:00:00 2001 From: Kurt Garloff Date: Fri, 23 Mar 2018 12:31:20 -0400 Subject: Revert the logic of exception_cb in read_url. In commit e9e8616, there was an inversion of the logic of the exception_cb return value meaning, breaking the (network) OpenStack DataSource, which implemented exception_cb as should_retry_cb, returning True when a retry should be done and False when the retry loop should be broken and the exception reraised again immediately. The OpenStack DS was the only user of this callback at the time and not touched by the commit (nor did the commit message mention an intended change), so this almost certainly happened by mistake. These days, we have a second user of the callback in DataSourceScaleway. It uses the new logic, so it needs change if we fix the meaning of the return value. This patch reverts the meaning of url_helper.read_url() execption_cb to the old semantics. It updates the comment and adjusts the Scaleway datasource. The patch has been tested on Open Telekom Cloud (which uses the OpenStack network Datasource) where previously a missing user_data and network_data.json would be retried 6 times each despite them not being present (they are optional!) and the server repsonding with a correct 404. After the patch, boot times are 10s faster, as we no longer pointlessly retry these files. LP: #1702160 LP: #1298921 --- cloudinit/sources/DataSourceScaleway.py | 6 +++--- cloudinit/url_helper.py | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) (limited to 'cloudinit/url_helper.py') diff --git a/cloudinit/sources/DataSourceScaleway.py b/cloudinit/sources/DataSourceScaleway.py index 90056249..e2502b02 100644 --- a/cloudinit/sources/DataSourceScaleway.py +++ b/cloudinit/sources/DataSourceScaleway.py @@ -113,9 +113,9 @@ def query_data_api_once(api_address, timeout, requests_session): retries=0, session=requests_session, # If the error is a HTTP/404 or a ConnectionError, go into raise - # block below. - exception_cb=lambda _, exc: exc.code == 404 or ( - isinstance(exc.cause, requests.exceptions.ConnectionError) + # block below and don't bother retrying. + exception_cb=lambda _, exc: exc.code != 404 and ( + not isinstance(exc.cause, requests.exceptions.ConnectionError) ) ) return util.decode_binary(resp.contents) diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 4e814a5f..36289af5 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -258,9 +258,10 @@ def readurl(url, data=None, timeout=None, retries=0, sec_between=1, # ssl exceptions are not going to get fixed by waiting a # few seconds break - if exception_cb and exception_cb(req_args.copy(), excps[-1]): - # if an exception callback was given it should return None - # a true-ish value means to break and re-raise the exception + if exception_cb and not exception_cb(req_args.copy(), excps[-1]): + # if an exception callback was given, it should return True + # to continue retrying and False to break and re-raise the + # exception break if i + 1 < manual_tries and sec_between > 0: LOG.debug("Please wait %s seconds while we wait to try again", -- cgit v1.2.3 From 2d618e27687470a8a3649f44598819bdee8cdb03 Mon Sep 17 00:00:00 2001 From: Douglas Jordan Date: Fri, 23 Mar 2018 12:04:18 -0600 Subject: Reduce AzurePreprovisioning HTTP timeouts. Reducing timeout to 1 second as IMDS responds within a handful of milliseconds. Also get rid of max_retries to prevent exiting out of polling loop early due to IMDS outage / upgrade. Reduce Azure PreProvisioning HTTP timeouts during polling to avoid waiting an extra minute. LP: #1752977 --- cloudinit/sources/DataSourceAzure.py | 31 ++++++++------------------- cloudinit/url_helper.py | 13 ++++++----- tests/unittests/test_datasource/test_azure.py | 22 ++++++------------- 3 files changed, 24 insertions(+), 42 deletions(-) (limited to 'cloudinit/url_helper.py') diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py index 0bb7fad9..0ee622e2 100644 --- a/cloudinit/sources/DataSourceAzure.py +++ b/cloudinit/sources/DataSourceAzure.py @@ -20,7 +20,7 @@ from cloudinit import net from cloudinit.net.dhcp import EphemeralDHCPv4 from cloudinit import sources from cloudinit.sources.helpers.azure import get_metadata_from_fabric -from cloudinit.url_helper import readurl, wait_for_url, UrlError +from cloudinit.url_helper import readurl, UrlError from cloudinit import util LOG = logging.getLogger(__name__) @@ -49,7 +49,6 @@ DEFAULT_FS = 'ext4' AZURE_CHASSIS_ASSET_TAG = '7783-7084-3265-9085-8269-3286-77' REPROVISION_MARKER_FILE = "/var/lib/cloud/data/poll_imds" IMDS_URL = "http://169.254.169.254/metadata/reprovisiondata" -IMDS_RETRIES = 5 def find_storvscid_from_sysctl_pnpinfo(sysctl_out, deviceid): @@ -451,36 +450,24 @@ class DataSourceAzure(sources.DataSource): headers = {"Metadata": "true"} LOG.debug("Start polling IMDS") - def sleep_cb(response, loop_n): - return 1 - - def exception_cb(msg, exception): + def exc_cb(msg, exception): if isinstance(exception, UrlError) and exception.code == 404: - return - LOG.warning("Exception during polling. Will try DHCP.", - exc_info=True) - + return True # If we get an exception while trying to call IMDS, we # call DHCP and setup the ephemeral network to acquire the new IP. - raise exception + return False need_report = report_ready - for i in range(IMDS_RETRIES): + while True: try: with EphemeralDHCPv4() as lease: if need_report: self._report_ready(lease=lease) need_report = False - wait_for_url([url], max_wait=None, timeout=60, - status_cb=LOG.info, - headers_cb=lambda url: headers, sleep_time=1, - exception_cb=exception_cb, - sleep_time_cb=sleep_cb) - return str(readurl(url, headers=headers)) - except Exception: - LOG.debug("Exception during polling-retrying dhcp" + - " %d more time(s).", (IMDS_RETRIES - i), - exc_info=True) + return readurl(url, timeout=1, headers=headers, + exception_cb=exc_cb, infinite=True).contents + except UrlError: + pass def _report_ready(self, lease): """Tells the fabric provisioning has completed diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py index 36289af5..03a573af 100644 --- a/cloudinit/url_helper.py +++ b/cloudinit/url_helper.py @@ -16,7 +16,7 @@ import time from email.utils import parsedate from functools import partial - +from itertools import count from requests import exceptions from six.moves.urllib.parse import ( @@ -172,7 +172,7 @@ def _get_ssl_args(url, ssl_details): def readurl(url, data=None, timeout=None, retries=0, sec_between=1, headers=None, headers_cb=None, ssl_details=None, check_status=True, allow_redirects=True, exception_cb=None, - session=None): + session=None, infinite=False): url = _cleanurl(url) req_args = { 'url': url, @@ -220,7 +220,8 @@ def readurl(url, data=None, timeout=None, retries=0, sec_between=1, excps = [] # Handle retrying ourselves since the built-in support # doesn't handle sleeping between tries... - for i in range(0, manual_tries): + # Infinitely retry if infinite is True + for i in count() if infinite else range(0, manual_tries): req_args['headers'] = headers_cb(url) filtered_req_args = {} for (k, v) in req_args.items(): @@ -229,7 +230,8 @@ def readurl(url, data=None, timeout=None, retries=0, sec_between=1, filtered_req_args[k] = v try: LOG.debug("[%s/%s] open '%s' with %s configuration", i, - manual_tries, url, filtered_req_args) + "infinite" if infinite else manual_tries, url, + filtered_req_args) if session is None: session = requests.Session() @@ -263,7 +265,8 @@ def readurl(url, data=None, timeout=None, retries=0, sec_between=1, # to continue retrying and False to break and re-raise the # exception break - if i + 1 < manual_tries and sec_between > 0: + if (infinite and sec_between > 0) or \ + (i + 1 < manual_tries and sec_between > 0): LOG.debug("Please wait %s seconds while we wait to try again", sec_between) time.sleep(sec_between) diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py index da7da0ca..3e8b7913 100644 --- a/tests/unittests/test_datasource/test_azure.py +++ b/tests/unittests/test_datasource/test_azure.py @@ -1177,7 +1177,8 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): url = 'http://{0}/metadata/reprovisiondata?api-version=2017-04-02' host = "169.254.169.254" full_url = url.format(host) - fake_resp.return_value = mock.MagicMock(status_code=200, text="ovf") + fake_resp.return_value = mock.MagicMock(status_code=200, text="ovf", + content="ovf") dsa = dsaz.DataSourceAzure({}, distro=None, paths=self.paths) self.assertTrue(len(dsa._poll_imds()) > 0) self.assertEqual(fake_resp.call_args_list, @@ -1185,13 +1186,8 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): headers={'Metadata': 'true', 'User-Agent': 'Cloud-Init/%s' % vs() - }, method='GET', timeout=60.0, - url=full_url), - mock.call(allow_redirects=True, - headers={'Metadata': 'true', - 'User-Agent': - 'Cloud-Init/%s' % vs() - }, method='GET', url=full_url)]) + }, method='GET', timeout=1, + url=full_url)]) self.assertEqual(m_dhcp.call_count, 1) m_net.assert_any_call( broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', @@ -1217,7 +1213,8 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): username = "myuser" odata = {'HostName': hostname, 'UserName': username} content = construct_valid_ovf_env(data=odata) - fake_resp.return_value = mock.MagicMock(status_code=200, text=content) + fake_resp.return_value = mock.MagicMock(status_code=200, text=content, + content=content) dsa = dsaz.DataSourceAzure({}, distro=None, paths=self.paths) md, ud, cfg, d = dsa._reprovision() self.assertEqual(md['local-hostname'], hostname) @@ -1227,12 +1224,7 @@ class TestAzureDataSourcePreprovisioning(CiTestCase): headers={'Metadata': 'true', 'User-Agent': 'Cloud-Init/%s' % vs()}, - method='GET', timeout=60.0, url=full_url), - mock.call(allow_redirects=True, - headers={'Metadata': 'true', - 'User-Agent': - 'Cloud-Init/%s' % vs()}, - method='GET', url=full_url)]) + method='GET', timeout=1, url=full_url)]) self.assertEqual(m_dhcp.call_count, 1) m_net.assert_any_call( broadcast='192.168.2.255', interface='eth9', ip='192.168.2.9', -- cgit v1.2.3