From 4ddb02eff835689e5ff563edfe779b7d7257f12d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 25 Feb 2020 13:27:35 -0800 Subject: [PATCH] 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: NoSuchKeyThe specified key does not exist.foo/bar... 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 as an . 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). --- .../xblock/runtime/blockstore_field_data.py | 4 +++- .../djangoapps/xblock/runtime/blockstore_runtime.py | 9 +++++++-- .../core/djangoapps/xblock/runtime/olx_parsing.py | 3 +++ openedx/core/djangolib/blockstore_cache.py | 12 ++++++++++-- openedx/core/lib/blockstore_api/__init__.py | 1 + openedx/core/lib/blockstore_api/exceptions.py | 4 ++++ 6 files changed, 28 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py b/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py index d849bfcfb9..517e93b3e2 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py +++ b/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, diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index 8b3558bced..2f474ebdd2 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/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): diff --git a/openedx/core/djangoapps/xblock/runtime/olx_parsing.py b/openedx/core/djangoapps/xblock/runtime/olx_parsing.py index e4a27e996b..9318cf415a 100644 --- a/openedx/core/djangoapps/xblock/runtime/olx_parsing.py +++ b/openedx/core/djangoapps/xblock/runtime/olx_parsing.py @@ -27,6 +27,9 @@ def parse_xblock_include(include_node): # An XBlock include looks like: # # Where "source" and "usage" are optional. + if include_node.tag != 'xblock-include': + # xss-lint: disable=python-wrap-html + raise BundleFormatException("Expected an XML node, but got <{}>".format(include_node.tag)) try: definition_path = include_node.attrib['definition'] except KeyError: diff --git a/openedx/core/djangolib/blockstore_cache.py b/openedx/core/djangolib/blockstore_cache.py index c1d12d99c3..040bc6a127 100644 --- a/openedx/core/djangolib/blockstore_cache.py +++ b/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) diff --git a/openedx/core/lib/blockstore_api/__init__.py b/openedx/core/lib/blockstore_api/__init__.py index 288aeada6c..8b6de205c6 100644 --- a/openedx/core/lib/blockstore_api/__init__.py +++ b/openedx/core/lib/blockstore_api/__init__.py @@ -51,4 +51,5 @@ from .exceptions import ( BundleNotFound, DraftNotFound, BundleFileNotFound, + BundleStorageError, ) diff --git a/openedx/core/lib/blockstore_api/exceptions.py b/openedx/core/lib/blockstore_api/exceptions.py index 06e6de93c6..b58251d31f 100644 --- a/openedx/core/lib/blockstore_api/exceptions.py +++ b/openedx/core/lib/blockstore_api/exceptions.py @@ -25,3 +25,7 @@ class DraftNotFound(NotFound): class BundleFileNotFound(NotFound): pass + + +class BundleStorageError(BlockstoreException): + pass