Merge lp:~chad.smith/landscape-client/avoid-metadata-pycurl-traceback into lp:~landscape/landscape-client/trunk

[Chad Smith]

Avoid pycurl tracebacks from missing EC2 API meta-data calls. Metadata queries allow 3 retries before giving up and logging warnings.

This branch reduces the noise of a metadata logs in a couple of ways:
  1. catch pycurl "host can't be contacted" error so that it doesn't read like a traceback.
  2. Remove the discrete "Queueing metadata at url ..../(instance-id|ami-id|instance-type)" and replace with single generic "Updating cloud meta-data." and "Updated cloud meta-data" upon success
  3. keep internal bool flag _check_cloud to prevent repetitive calls to _fetch_cloud_meta_data each message exchange when either the instance:
     -- is not a cloud instance (e.g. LXC or physical server)
     -- ls-client has already obtained the EC2 metadata (because instance-id, ami-id and instance-type don't change except across restarts)

[Free Ekanayaka (free.ekanayaka)]

Marking as needs fixing because of [1], thanks.

[0]

landscape/monitor/computerinfo.py:145:17: E126 continuation line over-indented for hanging indent
landscape/monitor/computerinfo.py:148:17: E126 continuation line over-indented for hanging indent

[1]

+ if error.check(PyCurlError):

I don't think there's a truly reliable way to discern if a fetch error is due to the client not being run in a cloud instance or to some other transient network error.

I propose that the code retries up to 3 times no matter what, then gives up.

[2]

         self._cloud_meta_data = None
+ self._check_cloud = True
+ self._cloud_retries = 0

Assuming that [1] gets fixed, we can drop self._check_cloud

    def __init__(self, ...)
        ...
        self._cloud_retries = 0
        ...

    def send_computer_message(self, urgent=False):
        if self._cloud_meta_data is None and self._cloud_retries < 3:
            self._cloud_retries += 1
            self._cloud_meta_data = yield self._fetch_cloud_meta_data()

fetch_cloud_meta_data should just return None in case any failure occurs.

[3]

Not related to this branch, but I re-raise a point I had made when this code was first put up for review. It'd be good to factor out ComputerInfo._fetch_cloud_meta_data into a standalone fetch_cloud_meta_data function (perhaps living in landscape.lib.cloud). This has the following advantages:

- it makes it possible to test fetch_cloud_meta_data in isolation

- it makes ComputerInfo info simpler, and it avoids the temptation of spreading stateful information around, like this branch is doing by updating self._check_cloud and self._cloud_retries inside ComputerInfo._fetch_cloud_meta_data

As a general note, keeping state generally makes code more complicated, so prefer passing data around, which is more explicit (you know exactly what the context is), and if keeping state is absolutely necessary (like with the retries count), try to keep all uses of such state close to each others (like checking and updating in the same spot).

review: Needs Fixing

[Chad Smith (chad.smith)]

Free, I'm still working the refactor of some of the fetch_cloud_meta_data callbacks into a separate lib that encapsulates the retries as well. May have questions for you tomorrow.

[Free Ekanayaka (free.ekanayaka)]

I'm not totally sure encapsulating retries is a good idea, since you'll want some delay between subsequent attempts and that would be a bottle neck for calling code (the computer info plugin). I believe the current retry logic in the computer info plugin is good enough (we don't have any other use case and don't foresee any).

[Chad Smith (chad.smith)]

Hi Free,

   I think I have addressed your review comments. I have now separated the fetch_ec2_meta_data functions out into their own very simple cloud.py library with local unit tests.

I have left a number of computerinfo meta-data tests intact as they represented integration tests using the results of fetch_ec2_meta_data. I have also boiled down a bit of the existing computerinfo meta-data unit tests, dropping some duplicated add_query_results which are handled by the test class' setUp method.

[Free Ekanayaka (free.ekanayaka)]

Nice work Chad, a few other minor points, but looks good. +1!

[4]

+def fetch_ec2_item(path, accumulate, fetch=None):

Since this is an internal helper, not relevant to cloud.py consumers would you please make it private? And move it to the bottom of the file (you can still keep the unit tests and just mark them as wb, e.g. test_wb_fetch_ec2_item_error_returns_failure).

See also:

https://wiki.canonical.com/Landscape/SpecRegistry/0009#Public_and_private

[5]

+def fetch_ec2_meta_data(fetch=None):

Please document the fetch parameter and mention that it's there for testing purposes.

[6]

+ def _unicode_none(value):
+ if value is None:
+ return None
+ else:
+ return value.decode("utf-8")

This could be simply:

        def _unicode_none(value):
            if value is not None:
                return value.decode("utf-8")

I'd also rename it to unicode_or_none (no need to make it private since it's local scope).

[7]

+ logging.info("Acquired cloud meta-data.")

It's generally better to keep logging functionality outside of library functions, this grants a bit more flexibility to the calling code, that can decide what, when and how to log (and indeed computerinfo.py is where the rest of the logging is done). This is minor, so feel free to address it or not.

[8]

+METADATA_RETRY_MAX = 3 # Number of retries to get EC2 meta-data

This should probably be moved to computerinfo.py, since it's where it's used (while cloud.py ignores it).

[9]

+ else:
+ logging.warning(
+ "Temporary failure accessing cloud meta-data, retrying.")

There's no need to log retries I think, especially because non-cloud clients will always get this message.

[10]

+ log_failure(
+ error, msg=(
+ "Max retries reached querying meta-data. %s" %
+ error.getErrorMessage()))

I don't think it should be logged as an error, as this just fine for non-cloud clients. I'd simply use logging.info with a message like "No cloud meta-data available.".

review: Approve

[lp:~chad.smith/landscape-client/avoid-metadata-pycurl-traceback]

726. By Chad Smith on 2013-09-05

address free's remaining comments:
 - fetch_ec2_item made private
 - METADATA_RETRY_MAX moved from lib/cloud into computerinfo.py
 - drop retry logging message and handle all logging on computerinfo
 - metadata query failure is not a real failure, just info message for non-cloud computers

[Chris Glass (tribaal)]

Looks good! +1 with a couple of refactoring/renaming comments:

[1]
I feel the term "cloud" is a little overused everywhere, making things less obvious that they could be. In the future I suppose we will add meta data for more than just EC2, so how about:
- Renaming _fetch_cloud_meta_data to _fetch_and_log_ec2_data (or similar) as this is really an EC2 specific method.
- Changing the logs to say "EC2" instead of "Cloud" in the aforementioned method

[2]
I would like to keep the message content gathering and creation logic in _create_computer_info_message. This would simply mean moving

+ if (self._cloud_meta_data is None and
+ self._cloud_retries < METADATA_RETRY_MAX):
+ self._cloud_meta_data = yield self._fetch_cloud_meta_data()

to _create_computer_info_message, and marking _create_computer_info_message as @inlineCallback instead of send_computer_message

If this is done, I'm not certain it's useful to keep self._cloud_meta_data (maybe a local variable is sufficient inside _create_computer_info_message)

Then in the future we can then easily extend the functionality by adding _fetch_and_log_<whatever>_data() methods to the class and hooking it into _create_computer_info_message.

review: Approve

[Chad Smith (chad.smith)]

thanks Chris
 [1] addressed
  agree with point [2] addressed, though we still need the persistent self._cloud_meta_data across message exchanges to prevent us from trying to pull cloud information during subsequent exchanges if we already have it.

[lp:~chad.smith/landscape-client/avoid-metadata-pycurl-traceback]

727. By Chad Smith on 2013-09-06

fetch_cloud_meta_data -> fetch_ec2_meta_data

728. By Chad Smith on 2013-09-07

shuffle METADATA_RETRY_MAX logic and fetch_ec2_metadata from send_computer_message and into _create_computer_message

=== added file 'landscape/lib/cloud.py'
--- landscape/lib/cloud.py	1970-01-01 00:00:00 +0000
+++ landscape/lib/cloud.py	2013-09-08 12:39:26 +0000
@@ -0,0 +1,45 @@
+from landscape.lib.fetch import fetch_async
+
+EC2_HOST = "169.254.169.254"
+EC2_API = "http://%s/latest" % (EC2_HOST,)
+
+
+def fetch_ec2_meta_data(fetch=None):
+    """Fetch EC2 information about the cloud instance.
+
+    The C{fetch} parameter provided above is non-mocker testing purposes.
+    """
+    cloud_data = []
+    # We're not using a DeferredList here because we want to keep the
+    # number of connections to the backend minimal. See lp:567515.
+    deferred = _fetch_ec2_item("instance-id", cloud_data, fetch)
+    deferred.addCallback(
+        lambda ignore: _fetch_ec2_item("instance-type", cloud_data, fetch))
+    deferred.addCallback(
+        lambda ignore: _fetch_ec2_item("ami-id", cloud_data, fetch))
+
+    def return_result(ignore):
+        """Record the instance data returned by the EC2 API."""
+
+        def _unicode_or_none(value):
+            if value is not None:
+                return value.decode("utf-8")
+
+        (instance_id, instance_type, ami_id) = cloud_data
+        return {
+            "instance-id": _unicode_or_none(instance_id),
+            "ami-id": _unicode_or_none(ami_id),
+            "instance-type": _unicode_or_none(instance_type)}
+    deferred.addCallback(return_result)
+    return deferred
+
+
+def _fetch_ec2_item(path, accumulate, fetch=None):
+    """
+     Get data at C{path} on the EC2 API endpoint, and add the result to the
+    C{accumulate} list. The C{fetch} parameter is provided for testing only.
+    """
+    url = EC2_API + "/meta-data/" + path
+    if fetch is None:
+        fetch = fetch_async
+    return fetch(url).addCallback(accumulate.append)
 
=== added file 'landscape/lib/tests/test_cloud.py'
--- landscape/lib/tests/test_cloud.py	1970-01-01 00:00:00 +0000
+++ landscape/lib/tests/test_cloud.py	2013-09-08 12:39:26 +0000
@@ -0,0 +1,107 @@
+from landscape.lib.cloud import (EC2_API, _fetch_ec2_item, fetch_ec2_meta_data)
+from landscape.lib.fetch import HTTPCodeError, PyCurlError
+from landscape.tests.helpers import LandscapeTest
+from twisted.internet.defer import succeed, fail
+
+
+class CloudTest(LandscapeTest):
+
+    def setUp(self):
+        LandscapeTest.setUp(self)
+        self.query_results = {}
+
+        def fetch_stub(url):
+            value = self.query_results[url]
+            if isinstance(value, Exception):
+                return fail(value)
+            else:
+                return succeed(value)
+
+        self.fetch_func = fetch_stub
+        self.add_query_result("instance-id", "i00001")
+        self.add_query_result("ami-id", "ami-00002")
+        self.add_query_result("instance-type", "hs1.8xlarge")
+
+    def add_query_result(self, name, value):
+        """
+        Add a url to self.query_results that is then available through
+        self.fetch_func.
+        """
+        url = "%s/meta-data/%s" % (EC2_API, name)
+        self.query_results[url] = value
+
+    def test_fetch_ec2_meta_data_error_on_any_item_error(self):
+        """
+        L{_fetch_ec2_meta_data} returns a deferred C{Failure} containing the
+        error message when an error occurs on any of the queried meta-data
+        items C{instance-id}, C{ami-id} or C{instance-type}.
+        """
+        self.log_helper.ignore_errors(HTTPCodeError)
+        error = HTTPCodeError(404, "notfound")
+        metadata_items = ["instance-id", "ami-id", "instance-type"]
+        for item in metadata_items:
+            # reset all item data adding the error to only 1 item per iteration
+            for setup_item in metadata_items:
+                if setup_item == item:
+                    self.add_query_result(item, error)
+                else:
+                    self.add_query_result(setup_item, "value%s" % setup_item)
+
+            deferred = fetch_ec2_meta_data(fetch=self.fetch_func)
+            failure = self.failureResultOf(deferred)
+            self.assertEqual(
+                "Server returned HTTP code 404",
+                failure.getErrorMessage())
+
+    def test_fetch_ec2_meta_data(self):
+        """
+        L{_fetch_ec2_meta_data} returns a C{dict} containing meta-data for
+        C{instance-id}, C{ami-id} and C{instance-type}.
+        """
+        deferred = fetch_ec2_meta_data(fetch=self.fetch_func)
+        result = self.successResultOf(deferred)
+        self.assertEqual(
+            {"ami-id": u"ami-00002",
+             "instance-id": u"i00001",
+             "instance-type": u"hs1.8xlarge"},
+            result)
+
+    def test_fetch_ec2_meta_data_utf8(self):
+        """
+        L{_fetch_ec2_meta_data} decodes utf-8 strings returned from the
+        external service.
+        """
+        self.add_query_result("ami-id", "asdf\xe1\x88\xb4")
+        deferred = fetch_ec2_meta_data(fetch=self.fetch_func)
+        result = self.successResultOf(deferred)
+        self.assertEqual({"instance-id": u"i00001",
+                          "ami-id": u"asdf\u1234",
+                          "instance-type": u"hs1.8xlarge"},
+                         result)
+
+    def test_wb_fetch_ec2_item_multiple_items_appends_accumulate_list(self):
+        """
+        L{_fetch_ec2_item} retrieves individual meta-data items from the
+        EC2 api and appends them to the C{list} provided by the C{accumulate}
+        parameter.
+        """
+        accumulate = []
+        self.successResultOf(
+            _fetch_ec2_item("instance-id", accumulate, fetch=self.fetch_func))
+        self.successResultOf(
+            _fetch_ec2_item(
+                "instance-type", accumulate, fetch=self.fetch_func))
+        self.assertEqual(["i00001", "hs1.8xlarge"], accumulate)
+
+    def test_wb_fetch_ec2_item_error_returns_failure(self):
+        """
+        L{_fetch_ec2_item} returns a deferred C{Failure} containing the error
+        message when faced with no EC2 cloud API service.
+        """
+        self.log_helper.ignore_errors(PyCurlError)
+        self.add_query_result("other-id", PyCurlError(60, "pycurl error"))
+        accumulate = []
+        deferred = _fetch_ec2_item(
+            "other-id", accumulate, fetch=self.fetch_func)
+        failure = self.failureResultOf(deferred)
+        self.assertEqual("Error 60: pycurl error", failure.getErrorMessage())
 
=== modified file 'landscape/monitor/computerinfo.py'
--- landscape/monitor/computerinfo.py	2013-08-23 21:06:26 +0000
+++ landscape/monitor/computerinfo.py	2013-09-08 12:39:26 +0000
@@ -1,16 +1,15 @@
 import os
 import logging
-from twisted.internet.defer import inlineCallbacks
+from twisted.internet.defer import inlineCallbacks, returnValue
 
 from landscape.lib.fetch import fetch_async
 from landscape.lib.fs import read_file
-from landscape.lib.log import log_failure
 from landscape.lib.lsb_release import LSB_RELEASE_FILENAME, parse_lsb_release
+from landscape.lib.cloud import fetch_ec2_meta_data
 from landscape.lib.network import get_fqdn
 from landscape.monitor.plugin import MonitorPlugin
 
-EC2_HOST = "169.254.169.254"
-EC2_API = "http://%s/latest" % (EC2_HOST,)
+METADATA_RETRY_MAX = 3  # Number of retries to get EC2 meta-data
 
 
 class DistributionInfoError(Exception):
@@ -32,6 +31,7 @@
         self._lsb_release_filename = lsb_release_filename
         self._root_path = root_path
         self._cloud_meta_data = None
+        self._cloud_retries = 0
         self._fetch_async = fetch_async
 
     def register(self, registry):
@@ -44,10 +44,7 @@
 
     @inlineCallbacks
     def send_computer_message(self, urgent=False):
-        if self._cloud_meta_data is None:
-            self._cloud_meta_data = yield self._fetch_cloud_meta_data()
-
-        message = self._create_computer_info_message()
+        message = yield self._create_computer_info_message()
         if message:
             message["type"] = "computer-info"
             logging.info("Queueing message with updated computer info.")
@@ -69,6 +66,7 @@
         broker.call_if_accepted("distribution-info",
                                 self.send_distribution_message, urgent)
 
+    @inlineCallbacks
     def _create_computer_info_message(self):
         message = {}
         self._add_if_new(message, "hostname",
@@ -83,12 +81,16 @@
                 meta_data[key] = read_file(
                     os.path.join(self._meta_data_path, key))
 
+        if (self._cloud_meta_data is None and
+            self._cloud_retries < METADATA_RETRY_MAX):
+            self._cloud_meta_data = yield self._fetch_ec2_meta_data()
+
         if self._cloud_meta_data:
             meta_data = dict(
                 meta_data.items() + self._cloud_meta_data.items())
         if meta_data:
             self._add_if_new(message, "meta-data", meta_data)
-        return message
+        returnValue(message)
 
     def _add_if_new(self, message, key, value):
         if value != self._persist.get(key):
@@ -122,47 +124,22 @@
         message.update(parse_lsb_release(self._lsb_release_filename))
         return message
 
-    def _fetch_data(self, path, accumulate):
-        """
-        Get data at C{path} on the EC2 API endpoint, and add the result to the
-        C{accumulate} list.
-        """
-        url = EC2_API + "/meta-data/" + path
-        logging.info("Queueing url fetch %s." % url)
-        return self._fetch_async(url).addCallback(accumulate.append)
-
-    def _fetch_cloud_meta_data(self):
+    def _fetch_ec2_meta_data(self):
         """Fetch information about the cloud instance."""
-        cloud_data = []
-        # We're not using a DeferredList here because we want to keep the
-        # number of connections to the backend minimal. See lp:567515.
-        deferred = self._fetch_data("instance-id", cloud_data)
-        deferred.addCallback(
-            lambda ignore:
-                self._fetch_data("instance-type", cloud_data))
-        deferred.addCallback(
-            lambda ignore:
-                self._fetch_data("ami-id", cloud_data))
-
-        def store_data(ignore):
-            """Record the instance data returned by the EC2 API."""
-
-            def _unicode_none(value):
-                if value is None:
-                    return None
-                else:
-                    return value.decode("utf-8")
-
-            (instance_id, instance_type, ami_id) = cloud_data
-            return {
-                "instance-id": _unicode_none(instance_id),
-                "instance-type": _unicode_none(instance_type),
-                "ami-id": _unicode_none(ami_id)}
-
-        def log_error(error):
-            log_failure(error, msg="Got error while fetching meta-data: %r"
-                        % (error.value,))
-
-        deferred.addCallback(store_data)
-        deferred.addErrback(log_error)
+        if self._cloud_retries == 0:
+            logging.info("Querying cloud meta-data.")
+        deferred = fetch_ec2_meta_data(self._fetch_async)
+
+        def log_no_meta_data_found(error):
+            self._cloud_retries += 1
+            if self._cloud_retries >= METADATA_RETRY_MAX:
+                logging.info("No cloud meta-data available. %s" %
+                        error.getErrorMessage())
+
+        def log_success(result):
+            logging.info("Acquired cloud meta-data.")
+            return result
+
+        deferred.addCallback(log_success)
+        deferred.addErrback(log_no_meta_data_found)
         return deferred
 
=== modified file 'landscape/monitor/tests/test_computerinfo.py'
--- landscape/monitor/tests/test_computerinfo.py	2013-08-23 21:06:26 +0000
+++ landscape/monitor/tests/test_computerinfo.py	2013-09-08 12:39:26 +0000
@@ -3,9 +3,9 @@
 
 from twisted.internet.defer import succeed, fail, inlineCallbacks
 
-from landscape.lib.fetch import HTTPCodeError
+from landscape.lib.fetch import HTTPCodeError, PyCurlError
 from landscape.lib.fs import create_file
-from landscape.monitor.computerinfo import ComputerInfo
+from landscape.monitor.computerinfo import ComputerInfo, METADATA_RETRY_MAX
 from landscape.tests.helpers import LandscapeTest, MonitorHelper
 from landscape.tests.mocker import ANY
 
@@ -405,9 +405,6 @@
     def test_with_cloud_info(self):
         """Fetch cloud information"""
         self.config.cloud = True
-        self.add_query_result("instance-id", "i00001")
-        self.add_query_result("ami-id", "ami-00002")
-        self.add_query_result("instance-type", "hs1.8xlarge")
         self.mstore.set_accepted_types(["computer-info"])
 
         plugin = ComputerInfo(fetch_async=self.fetch_func)
@@ -421,47 +418,87 @@
                           "instance-type": u"hs1.8xlarge"},
                          messages[0]["meta-data"])
 
+    def test_no_fetch_ec2_meta_data_when_cloud_retries_is_max(self):
+        """
+        Do not fetch EC2 info when C{_cloud_retries} is C{METADATA_RETRY_MAX}
+        """
+        self.config.cloud = True
+        self.mstore.set_accepted_types(["computer-info"])
+
+        plugin = ComputerInfo(fetch_async=self.fetch_func)
+        plugin._cloud_retries = METADATA_RETRY_MAX
+        self.monitor.add(plugin)
+        plugin.exchange()
+        messages = self.mstore.get_pending_messages()
+        self.assertEqual(1, len(messages))
+        self.assertNotIn("meta-data", messages[0])
+
     @inlineCallbacks
-    def test_fetch_cloud_meta_data(self):
+    def test_fetch_ec2_meta_data(self):
         """
-        L{_fetch_cloud_meta_data} retrieves instance information from the
+        L{_fetch_ec2_meta_data} retrieves instance information from the
         EC2 api.
         """
-        self.add_query_result("instance-id", "i00001")
-        self.add_query_result("ami-id", "ami-00002")
-        self.add_query_result("instance-type", "hs1.8xlarge")
-
         plugin = ComputerInfo(fetch_async=self.fetch_func)
-        result = yield plugin._fetch_cloud_meta_data()
+        result = yield plugin._fetch_ec2_meta_data()
         self.assertEqual({"instance-id": u"i00001", "ami-id": u"ami-00002",
                           "instance-type": u"hs1.8xlarge"}, result)
-
-    @inlineCallbacks
-    def test_fetch_cloud_meta_data_bad_result(self):
-        """
-        L{_fetch_cloud_meta_data} returns C{None} when faced with errors from
-        the EC2 api.
-        """
-        self.log_helper.ignore_errors(HTTPCodeError)
-        self.add_query_result("instance-id", "i7337")
-        self.add_query_result("ami-id", HTTPCodeError(404, "notfound"))
-        self.add_query_result("instance-type", "hs1.8xlarge")
-        plugin = ComputerInfo(fetch_async=self.fetch_func)
-        result = yield plugin._fetch_cloud_meta_data()
-        self.assertEqual(None, result)
-
-    @inlineCallbacks
-    def test_fetch_cloud_meta_data_utf8(self):
-        """
-        L{_fetch_cloud_meta_data} decodes utf-8 strings returned from the
-        external service.
-        """
-        self.add_query_result("instance-id", "i00001")
-        self.add_query_result("ami-id", "asdf\xe1\x88\xb4")
-        self.add_query_result("instance-type", "m1.large")
-        plugin = ComputerInfo(fetch_async=self.fetch_func)
-        result = yield plugin._fetch_cloud_meta_data()
-        self.assertEqual({"instance-id": u"i00001",
-                          "ami-id": u"asdf\u1234",
-                          "instance-type": u"m1.large"},
+        self.assertEqual(
+            "    INFO: Querying cloud meta-data.\n"
+            "    INFO: Acquired cloud meta-data.\n",
+            self.logfile.getvalue())
+
+    @inlineCallbacks
+    def test_fetch_ec2_meta_data_no_cloud_api_max_retry(self):
+        """
+        L{_fetch_ec2_meta_data} returns C{None} when faced with no EC2 cloud
+        API service and reports the specific C{PyCurlError} upon message
+        exchange when L{_cloud_retries} equals C{METADATA_RETRY_MAX}.
+        """
+        self.log_helper.ignore_errors(PyCurlError)
+        self.add_query_result("instance-id", PyCurlError(60, "pycurl error"))
+        plugin = ComputerInfo(fetch_async=self.fetch_func)
+        plugin._cloud_retries = METADATA_RETRY_MAX
+        result = yield plugin._fetch_ec2_meta_data()
+        self.assertIn(
+            "INFO: No cloud meta-data available. "
+            "Error 60: pycurl error\n", self.logfile.getvalue())
+        self.assertEqual(None, result)
+
+    @inlineCallbacks
+    def test_fetch_ec2_meta_data_bad_result_max_retry(self):
+        """
+        L{_fetch_ec2_meta_data} returns C{None} and logs an error when
+        crossing the retry threshold C{METADATA_RETRY_MAX}.
+        """
+        self.log_helper.ignore_errors(HTTPCodeError)
+        self.add_query_result("ami-id", HTTPCodeError(404, "notfound"))
+        plugin = ComputerInfo(fetch_async=self.fetch_func)
+        plugin._cloud_retries = METADATA_RETRY_MAX
+        result = yield plugin._fetch_ec2_meta_data()
+        self.assertIn(
+            "INFO: No cloud meta-data available. Server returned "
+            "HTTP code 404",
+            self.logfile.getvalue())
+        self.assertEqual(None, result)
+
+    @inlineCallbacks
+    def test_fetch_ec2_meta_data_bad_result_retry(self):
+        """
+        L{_fetch_ec2_meta_data} returns C{None} when faced with spurious
+        errors from the EC2 api. The method increments L{_cloud_retries}
+        counter which allows L{_fetch_ec2_meta_data} to run again next
+        message exchange.
+        """
+        self.log_helper.ignore_errors(HTTPCodeError)
+        self.add_query_result("ami-id", HTTPCodeError(404, "notfound"))
+        plugin = ComputerInfo(fetch_async=self.fetch_func)
+        result = yield plugin._fetch_ec2_meta_data()
+        self.assertEqual(1, plugin._cloud_retries)
+        self.assertEqual(None, result)
+        # Fix the error condition for the retry.
+        self.add_query_result("ami-id", "ami-00002")
+        result = yield plugin._fetch_ec2_meta_data()
+        self.assertEqual({"instance-id": u"i00001", "ami-id": u"ami-00002",
+                          "instance-type": u"hs1.8xlarge"},
                          result)