Merge ~smoser/cloud-init:cleanup/consistent-exception_cb into cloud-init:master

[Scott Moser]

Make exception_cb consistent in readurl and wait_for_url.

This message needs improving.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:42a9b8e599167b2a8f004873a99b1ab00f346530
https://jenkins.ubuntu.com/server/job/cloud-init-ci/920/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/920/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

_raise_cause_if_not_found is broken as it expects 3 parameters. I think we need to also scrub the other callsites to clarify that param1 in exc_cb is "args" not msg". And we need some additional unit test coverage on readurl w/ exception_cb to make this less risky overall (and to show examples of the intended behavior of exception_cb).

Holding this up until after SRU is cut to avoid adding risk to SRU.

review: Needs Fixing

[Scott Moser (smoser)]

OK.
A bit of digging.

readurl and wait_for_url currently call exceptin_cb differently.
currently:
- readurl calls exception_cb(req_args, exception)
   req_args: a copy of kwargs that went to requests.request.
   exception: the exception that occurred
- wait_for_url calls exception_cb(status_msg, exception)
   status_msg: a string like "Calling <url> failed [0/120s]: <why>".
   exception: the exception that occurred.

I'll dig and see if the callers ever actually use the argument
that isnt 'exception'. And then see if we can't do something consistent
there.

so this needs some work still.

review: Approve

[Scott Moser (smoser)]

review: Abstain

[Scott Moser (smoser)]

due to last comment, marking wip

Unmerged commits

42a9b8e... by Scott Moser on 2018-03-23

Make exception_cb consistent in readurl and wait_for_url.

This message needs improving.

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