cloud-init-dev team mailing list archive

[Scott Moser <ssmoser2+ubuntu@xxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:cleanup/consistent-exception_cb into cloud-init:master.

Commit message:
Make exception_cb consistent in readurl and wait_for_url.

This message needs improving.

Requested reviews:
  cloud-init commiters (cloud-init-dev)

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/342007
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:cleanup/consistent-exception_cb into cloud-init:master.

diff --git a/cloudinit/ec2_utils.py b/cloudinit/ec2_utils.py
index dc3f0fc..18553f0 100644
--- a/cloudinit/ec2_utils.py
+++ b/cloudinit/ec2_utils.py
@@ -134,9 +134,9 @@ class MetadataMaterializer(object):
         return joined
 
 
-def _skip_retry_on_codes(status_codes, _request_args, cause):
-    """Returns False if cause.code is in status_codes."""
-    return cause.code not in status_codes
+def _raise_cause_if_not_found(status_codes, _request_args, cause):
+    if cause.code not in SKIP_USERDATA_CODES:
+        raise cause
 
 
 def get_instance_userdata(api_version='latest',
@@ -148,13 +148,9 @@ def get_instance_userdata(api_version='latest',
     try:
         # It is ok for userdata to not exist (thats why we are stopping if
         # NOT_FOUND occurs) and just in that case returning an empty string.
-        exception_cb = functools.partial(_skip_retry_on_codes,
-                                         SKIP_USERDATA_CODES)
-        response = util.read_file_or_url(ud_url,
-                                         ssl_details=ssl_details,
-                                         timeout=timeout,
-                                         retries=retries,
-                                         exception_cb=exception_cb)
+        response = util.read_file_or_url(
+            ud_url, ssl_details=ssl_details, timeout=timeout, retries=retries,
+            exception_cb=_raise_cause_if_not_found)
         user_data = response.contents
     except url_helper.UrlError as e:
         if e.code not in SKIP_USERDATA_CODES:
diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
index 0ee622e..b2244f5 100644
--- a/cloudinit/sources/DataSourceAzure.py
+++ b/cloudinit/sources/DataSourceAzure.py
@@ -452,10 +452,10 @@ class DataSourceAzure(sources.DataSource):
 
         def exc_cb(msg, exception):
             if isinstance(exception, UrlError) and exception.code == 404:
-                return True
+                raise exception
             # If we get an exception while trying to call IMDS, we
             # call DHCP and setup the ephemeral network to acquire the new IP.
-            return False
+            return
 
         need_report = report_ready
         while True:
diff --git a/cloudinit/sources/DataSourceScaleway.py b/cloudinit/sources/DataSourceScaleway.py
index e2502b0..a89e125 100644
--- a/cloudinit/sources/DataSourceScaleway.py
+++ b/cloudinit/sources/DataSourceScaleway.py
@@ -103,21 +103,25 @@ def query_data_api_once(api_address, timeout, requests_session):
     ports below 1024). If requests raises ConnectionError (EADDRINUSE), the
     caller should retry to call this function on an other port.
     """
+    def _exc_cb(msg, exception):
+        """raise the provided exception if it is not 404."""
+        if (exception.code == 404 or
+                isinstance(exception.cause,
+                           requests.exceptions.ConnectionError)):
+            raise exception
+
     try:
         resp = url_helper.readurl(
             api_address,
             data=None,
             timeout=timeout,
-            # It's the caller's responsability to recall this function in case
+            # It's the caller's responsibility to recall this function in case
             # of exception. Don't let url_helper.readurl() retry by itself.
             retries=0,
             session=requests_session,
             # If the error is a HTTP/404 or a ConnectionError, go into raise
             # block below and don't bother retrying.
-            exception_cb=lambda _, exc: exc.code != 404 and (
-                not isinstance(exc.cause, requests.exceptions.ConnectionError)
-            )
-        )
+            exception_cb=_exc_cb)
         return util.decode_binary(resp.contents)
     except url_helper.UrlError as exc:
         # Empty user data.
diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py
index 26f3168..b29a00e 100644
--- a/cloudinit/sources/helpers/openstack.py
+++ b/cloudinit/sources/helpers/openstack.py
@@ -456,17 +456,15 @@ class MetadataReader(BaseReader):
             try:
                 code = int(cause.code)
                 if code >= 400:
-                    return False
+                    return
             except (TypeError, ValueError):
                 # Older versions of requests didn't have a code.
                 pass
-            return True
+            raise cause
 
-        response = url_helper.readurl(path,
-                                      retries=self.retries,
-                                      ssl_details=self.ssl_details,
-                                      timeout=self.timeout,
-                                      exception_cb=should_retry_cb)
+        response = url_helper.readurl(
+            path, retries=self.retries, ssl_details=self.ssl_details,
+            timeout=self.timeout, exception_cb=should_retry_cb)
         if decode:
             return response.contents.decode()
         else:
diff --git a/cloudinit/url_helper.py b/cloudinit/url_helper.py
index 03a573a..26b42ba 100644
--- a/cloudinit/url_helper.py
+++ b/cloudinit/url_helper.py
@@ -260,11 +260,11 @@ 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 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 exception_cb:
+                # Any exceptions that an exception_cb raises (including
+                # re-raising this one) are not caught here.
+                exception_cb(req_args.copy(), excps[-1])
+
             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",
@@ -290,6 +290,8 @@ def wait_for_url(urls, max_wait=None, timeout=None,
                 for request.
     exception_cb: call method with 2 arguments 'msg' (per status_cb) and
                   'exception', the exception that occurred.
+                  if this method raises exception, then that will not be
+                  caught.
     sleep_time_cb: call method with 2 arguments (response, loop_n) that
                    generates the next sleep time.
 
@@ -375,9 +377,6 @@ def wait_for_url(urls, max_wait=None, timeout=None,
                                                               reason)
             status_cb(status_msg)
             if exception_cb:
-                # This can be used to alter the headers that will be sent
-                # in the future, for example this is what the MAAS datasource
-                # does.
                 exception_cb(msg=status_msg, exception=url_exc)
 
         if timeup(max_wait, start_time):
@@ -487,7 +486,7 @@ class OauthUrlHelper(object):
         ret = None
         try:
             if extra_exception_cb:
-                ret = extra_exception_cb(msg, exception)
+                extra_exception_cb(msg, exception)
         finally:
                 self.exception_cb(msg, exception)
         return ret