[ARVADOS] created: d8148b70c8bbf814a53e5d5b9d92710d44b11107
git at public.curoverse.com
git at public.curoverse.com
Tue Jan 13 10:30:23 EST 2015
at d8148b70c8bbf814a53e5d5b9d92710d44b11107 (commit)
commit d8148b70c8bbf814a53e5d5b9d92710d44b11107
Author: Brett Smith <brett at curoverse.com>
Date: Fri Jan 9 13:15:07 2015 -0500
3835: Improve error reporting in PySDK Keep client.
* Create a KeepRequestError base exception to store information about
errors encountered when talking to Keep services, and include those
in the default formatting.
* Include Keep service error information in exceptions raised by the
Keep client.
diff --git a/sdk/python/arvados/errors.py b/sdk/python/arvados/errors.py
index 4740a2d..f70fa17 100644
--- a/sdk/python/arvados/errors.py
+++ b/sdk/python/arvados/errors.py
@@ -1,7 +1,10 @@
# errors.py - Arvados-specific exceptions.
import json
+import requests
+
from apiclient import errors as apiclient_errors
+from collections import OrderedDict
class ApiError(apiclient_errors.HttpError):
def _get_reason(self):
@@ -11,6 +14,46 @@ class ApiError(apiclient_errors.HttpError):
return super(ApiError, self)._get_reason()
+class KeepRequestError(Exception):
+ """Base class for errors accessing Keep services."""
+ def __init__(self, message='', service_errors=()):
+ """KeepRequestError(message='', service_errors=())
+
+ Arguments:
+ * message: A human-readable message describing what Keep operation
+ failed.
+ * service_errors: An iterable that yields 2-tuples of Keep
+ service URLs to the error encountered when talking to
+ it--either an exception, or an HTTP response object. These
+ will be packed into an OrderedDict, available through the
+ service_errors() method.
+ """
+ self._service_errors = OrderedDict(service_errors)
+ if self._service_errors:
+ exc_reports = [self._format_error(*err_pair)
+ for err_pair in self._service_errors.iteritems()]
+ base_msg = "{}: {}".format(message, "; ".join(exc_reports))
+ else:
+ base_msg = message
+ super(KeepRequestError, self).__init__(base_msg)
+ self.message = message
+
+ def _format_error(self, service_root, error):
+ if isinstance(error, requests.Response):
+ err_fmt = "{} responded with {e.status_code} {e.reason}"
+ else:
+ err_fmt = "{} raised {e.__class__.__name__} ({e})"
+ return err_fmt.format(service_root, e=error)
+
+ def service_errors(self):
+ """service_errors() -> OrderedDict
+
+ The keys of the dictionary are Keep service URLs.
+ The corresponding value is the exception raised when sending the
+ request to it."""
+ return self._service_errors
+
+
class ArgumentError(Exception):
pass
class SyntaxError(Exception):
@@ -19,9 +62,9 @@ class AssertionError(Exception):
pass
class CommandFailedError(Exception):
pass
-class KeepReadError(Exception):
+class KeepReadError(KeepRequestError):
pass
-class KeepWriteError(Exception):
+class KeepWriteError(KeepRequestError):
pass
class NotFoundError(KeepReadError):
pass
diff --git a/sdk/python/arvados/keep.py b/sdk/python/arvados/keep.py
index f4c8596..471544c 100644
--- a/sdk/python/arvados/keep.py
+++ b/sdk/python/arvados/keep.py
@@ -650,10 +650,23 @@ class KeepClient(object):
# passing, but maybe they should expect KeepReadError instead?
not_founds = sum(1 for ks in roots_map.values()
if ks.last_status() in set([403, 404, 410]))
- if roots_map and ((float(not_founds) / len(roots_map)) >= .75):
- raise arvados.errors.NotFoundError(loc_s)
+ try:
+ all_roots = local_roots + hint_roots
+ except NameError:
+ # We never successfully fetched local_roots.
+ all_roots = hint_roots
+ service_errors = ((key, roots_map[key].last_result)
+ for key in all_roots)
+ if not roots_map:
+ raise arvados.errors.KeepReadError(
+ "failed to read {}: no Keep services available ({})".format(
+ loc_s, loop.last_result()))
+ elif ((float(not_founds) / len(roots_map)) >= .75):
+ raise arvados.errors.NotFoundError(
+ "{} not found".format(loc_s), service_errors)
else:
- raise arvados.errors.KeepReadError(loc_s)
+ raise arvados.errors.KeepReadError(
+ "failed to read {}".format(loc_s), service_errors)
@retry.retry_method
def put(self, data, copies=2, num_retries=None):
@@ -714,9 +727,17 @@ class KeepClient(object):
if loop.success():
return thread_limiter.response()
- raise arvados.errors.KeepWriteError(
- "Write fail for %s: wanted %d but wrote %d" %
- (data_hash, copies, thread_limiter.done()))
+ if not roots_map:
+ raise arvados.errors.KeepWriteError(
+ "failed to write {}: no Keep services available ({})".format(
+ data_hash, loop.last_result()))
+ else:
+ service_errors = ((key, roots_map[key].last_result)
+ for key in local_roots
+ if not roots_map[key].success_flag)
+ raise arvados.errors.KeepWriteError(
+ "failed to write {} (wanted {} copies but wrote {})".format(
+ data_hash, copies, thread_limiter.done()), service_errors)
# Local storage methods need no-op num_retries arguments to keep
# integration tests happy. With better isolation they could
diff --git a/sdk/python/tests/test_errors.py b/sdk/python/tests/test_errors.py
new file mode 100644
index 0000000..948a40b
--- /dev/null
+++ b/sdk/python/tests/test_errors.py
@@ -0,0 +1,69 @@
+#!/usr/bin/env python
+
+import collections
+import traceback
+import unittest
+
+import arvados.errors as arv_error
+import arvados_testutil as tutil
+
+class KeepRequestErrorTestCase(unittest.TestCase):
+ SERVICE_ERRORS = [
+ ('http://keep1.zzzzz.example.org/', IOError("test IOError")),
+ ('http://keep3.zzzzz.example.org/', MemoryError("test MemoryError")),
+ ('http://keep5.zzzzz.example.org/', tutil.fake_requests_response(
+ 500, "test 500")),
+ ('http://keep7.zzzzz.example.org/', IOError("second test IOError")),
+ ]
+
+ def check_get_message(self, *args):
+ test_exc = arv_error.KeepRequestError("test message", *args)
+ self.assertEqual("test message", test_exc.message)
+
+ def test_get_message_with_service_errors(self):
+ self.check_get_message(self.SERVICE_ERRORS[:])
+
+ def test_get_message_without_service_errors(self):
+ self.check_get_message()
+
+ def check_get_service_errors(self, *args):
+ expected = dict(args[0]) if args else {}
+ test_exc = arv_error.KeepRequestError("test service exceptions", *args)
+ self.assertEqual(expected, test_exc.service_errors())
+
+ def test_get_service_errors(self):
+ self.check_get_service_errors(self.SERVICE_ERRORS[:])
+
+ def test_get_service_errors_none(self):
+ self.check_get_service_errors({})
+
+ def test_empty_exception(self):
+ test_exc = arv_error.KeepRequestError()
+ self.assertFalse(test_exc.message)
+ self.assertEqual({}, test_exc.service_errors())
+
+ def traceback_str(self, exc):
+ return traceback.format_exception_only(type(exc), exc)[-1]
+
+ def test_traceback_str_without_service_errors(self):
+ message = "test plain traceback string"
+ test_exc = arv_error.KeepRequestError(message)
+ exc_report = self.traceback_str(test_exc)
+ self.assertTrue(exc_report.startswith("KeepRequestError: "))
+ self.assertIn(message, exc_report)
+
+ def test_traceback_str_with_service_errors(self):
+ message = "test traceback shows Keep services"
+ test_exc = arv_error.KeepRequestError(message, self.SERVICE_ERRORS[:])
+ exc_report = self.traceback_str(test_exc)
+ self.assertTrue(exc_report.startswith("KeepRequestError: "))
+ for expect_substr in [message, "raised IOError", "raised MemoryError",
+ "test MemoryError", "second test IOError",
+ "responded with 500 Internal Server Error"]:
+ self.assertIn(expect_substr, exc_report)
+ # Assert the report maintains order of listed services.
+ last_index = -1
+ for service_key, _ in self.SERVICE_ERRORS:
+ service_index = exc_report.find(service_key)
+ self.assertGreater(service_index, last_index)
+ last_index = service_index
diff --git a/sdk/python/tests/test_keep_client.py b/sdk/python/tests/test_keep_client.py
index 982e4b4..37f274a 100644
--- a/sdk/python/tests/test_keep_client.py
+++ b/sdk/python/tests/test_keep_client.py
@@ -394,6 +394,71 @@ class KeepClientServiceTestCase(unittest.TestCase):
min_penalty,
max_penalty))
+ def check_64_zeros_error_order(self, verb, exc_class):
+ data = '0' * 64
+ if verb == 'get':
+ data = hashlib.md5(data).hexdigest() + '+1234'
+ api_client = self.mock_n_keep_disks(16)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ with mock.patch('requests.' + verb,
+ side_effect=socket.timeout) as req_mock, \
+ self.assertRaises(exc_class) as err_check:
+ getattr(keep_client, verb)(data)
+ urls = [urlparse.urlparse(url)
+ for url in err_check.exception.service_errors()]
+ self.assertEqual([('keep0x' + c, 80) for c in '3eab2d5fc9681074'],
+ [(url.hostname, url.port) for url in urls])
+
+ def test_get_error_shows_probe_order(self):
+ self.check_64_zeros_error_order('get', arvados.errors.KeepReadError)
+
+ def test_put_error_shows_probe_order(self):
+ self.check_64_zeros_error_order('put', arvados.errors.KeepWriteError)
+
+ def check_no_services_error(self, verb, exc_class):
+ api_client = mock.MagicMock(name='api_client')
+ api_client.keep_services().accessible().execute.side_effect = (
+ arvados.errors.ApiError)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ with self.assertRaises(exc_class) as err_check:
+ getattr(keep_client, verb)('d41d8cd98f00b204e9800998ecf8427e+0')
+ self.assertEqual(0, len(err_check.exception.service_errors()))
+
+ def test_get_error_with_no_services(self):
+ self.check_no_services_error('get', arvados.errors.KeepReadError)
+
+ def test_put_error_with_no_services(self):
+ self.check_no_services_error('put', arvados.errors.KeepWriteError)
+
+ def check_errors_from_last_retry(self, verb, exc_class):
+ api_client = self.mock_n_keep_disks(2)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ req_mock = getattr(tutil, 'mock_{}_responses'.format(verb))(
+ "retry error reporting test", 500, 500, 403, 403)
+ with req_mock, tutil.skip_sleep, \
+ self.assertRaises(exc_class) as err_check:
+ getattr(keep_client, verb)('d41d8cd98f00b204e9800998ecf8427e+0',
+ num_retries=3)
+ self.assertEqual([403, 403], [
+ getattr(error, 'status_code', None)
+ for error in err_check.exception.service_errors().itervalues()])
+
+ def test_get_error_reflects_last_retry(self):
+ self.check_errors_from_last_retry('get', arvados.errors.KeepReadError)
+
+ def test_put_error_reflects_last_retry(self):
+ self.check_errors_from_last_retry('put', arvados.errors.KeepWriteError)
+
+ def test_put_error_does_not_include_successful_puts(self):
+ data = 'partial failure test'
+ data_loc = '{}+{}'.format(hashlib.md5(data).hexdigest(), len(data))
+ api_client = self.mock_n_keep_disks(3)
+ keep_client = arvados.KeepClient(api_client=api_client)
+ with tutil.mock_put_responses(data_loc, 200, 500, 500) as req_mock, \
+ self.assertRaises(arvados.errors.KeepWriteError) as exc_check:
+ keep_client.put(data)
+ self.assertEqual(2, len(exc_check.exception.service_errors()))
+
class KeepClientRetryTestMixin(object):
# Testing with a local Keep store won't exercise the retry behavior.
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list