Fix: transient S3 errors were not being reported properly

Also improve usefulness of some blockstore runtime logs for debugging

Context:

Sometimes when trying to load an XBlock's XML file from Amazon S3, AWS will return a 4xx or 5xx response along with error XML like:

    <Error><Code>NoSuchKey</Code><Message>The specified key does not exist.</Message><Key>foo/bar</Key>...</Error>

A bug in the get_bundle_file_data_with_cache method would cause this XML to be returned to the runtime anyways, as if it were the expected OLX. This would then (obviously) lead to strange parsing bugs, e.g. when trying to interpret <Code> as an <xblock-include>.

This fixes the bug and improves the logging, both to make this sort of issue easier to debug in the future and to return whatever detailed error code S3 provides (or Blockstore, if S3 is not being used).

openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py

@@ -180,7 +180,9 @@ class BlockstoreFieldData(FieldData):
             # Otherwise, this is an anomalous get() before the XML was fully loaded:
             # This could happen if an XBlock's parse_xml() method tried to read a field before setting it,
             # if an XBlock read field data in its constructor (forbidden), or if an XBlock was loaded via
-            # some means other than runtime.get_block()
+            # some means other than runtime.get_block(). One way this can happen is if you log/print an XBlock during
+            # XML parsing, because ScopedStorageMixin.__repr__ will try to print all field values, and any fields which
+            # aren't mentioned in the XML (which are left at their default) will be "not loaded yet."
             log.exception(
                 "XBlock %s tried to read from field data (%s) that wasn't loaded from Blockstore!",
                 block.scope_ids.usage_id, name,

openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py

@@ -13,7 +13,7 @@ from xblock.fields import ScopeIds
 
 from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl
 from openedx.core.djangoapps.xblock.runtime.runtime import XBlockRuntime
-from openedx.core.djangoapps.xblock.runtime.olx_parsing import parse_xblock_include
+from openedx.core.djangoapps.xblock.runtime.olx_parsing import parse_xblock_include, BundleFormatException
 from openedx.core.djangoapps.xblock.runtime.serializer import serialize_xblock
 from openedx.core.djangolib.blockstore_cache import (
     BundleCache,
@@ -85,7 +85,12 @@ class BlockstoreXBlockRuntime(XBlockRuntime):
         This runtime API should normally be used via
         runtime.get_block() -> block.parse_xml() -> runtime.add_node_as_child
         """
-        parsed_include = parse_xblock_include(node)
+        try:
+            parsed_include = parse_xblock_include(node)
+        except BundleFormatException:
+            # We need to log the XBlock ID or this will be hard to debug
+            log.error("BundleFormatException when parsing XBlock %s", block.scope_ids.usage_id)
+            raise  # Also log details and stack trace
         self.add_child_include(block, parsed_include)
 
     def add_child_include(self, block, parsed_include):

openedx/core/djangoapps/xblock/runtime/olx_parsing.py

@@ -27,6 +27,9 @@ def parse_xblock_include(include_node):
     # An XBlock include looks like:
     # <xblock-include source="link_id" definition="block_type/definition_id" usage="alias" />
     # Where "source" and "usage" are optional.
+    if include_node.tag != 'xblock-include':
+        # xss-lint: disable=python-wrap-html
+        raise BundleFormatException("Expected an <xblock-include /> XML node, but got <{}>".format(include_node.tag))
     try:
         definition_path = include_node.attrib['definition']
     except KeyError:

openedx/core/djangolib/blockstore_cache.py

@@ -202,8 +202,16 @@ def get_bundle_file_data_with_cache(bundle_uuid, path, bundle_version=None, draf
     cached list of files in each bundle if available.
     """
     file_info = get_bundle_file_metadata_with_cache(bundle_uuid, path, bundle_version, draft_name)
-    with requests.get(file_info.url, stream=True) as r:
-        return r.content
+    response = requests.get(file_info.url)
+    if response.status_code != 200:
+        try:
+            error_response = response.content.decode('utf-8')[:500]
+        except UnicodeDecodeError:
+            error_response = '(error details unavailable - response was not a [unicode] string)'
+        raise blockstore_api.BundleStorageError(
+            "Unexpected error trying to read {} from bundle {}: \n{}".format(path, bundle_uuid, error_response)
+        )
+    return response.content
 
 
 @lru_cache(maxsize=200)

openedx/core/lib/blockstore_api/__init__.py

@@ -51,4 +51,5 @@ from .exceptions import (
     BundleNotFound,
     DraftNotFound,
     BundleFileNotFound,
+    BundleStorageError,
 )

openedx/core/lib/blockstore_api/exceptions.py

@@ -25,3 +25,7 @@ class DraftNotFound(NotFound):
 
 class BundleFileNotFound(NotFound):
     pass
+
+
+class BundleStorageError(BlockstoreException):
+    pass