From ac2a60dc0d2932583e70406b6d456e6ae094c3dd Mon Sep 17 00:00:00 2001
From: Yan Xiao <yanxiao@nvidia.com>
Date: Fri, 15 Dec 2023 09:19:35 -0500
Subject: [PATCH] Add transaction id to errors
Add transaction id to errors to help troubleshoot, including error
when downloading object with truncted/missing segment, stat'ing or
listing non-existent container.
Change response headers returned by client.py Connection APIs
such as get_object() from lowercase dict to CaseInsensitiveDict,
as some error paths rely on this to retrieve the transaction id,
while there are existing code paths using response.headers which
is CaseInsensitiveDict from requests lib.
Change-Id: Ice9cc9fe68684563f18ee527996e5a4292230a96
---
swiftclient/client.py | 10 ++-
swiftclient/multithreading.py | 4 +
swiftclient/service.py | 25 ++++--
swiftclient/shell.py | 22 ++---
test/unit/test_service.py | 27 +++++-
test/unit/test_shell.py | 165 +++++++++++++++++++++++++++++++++-
test/unit/test_swiftclient.py | 8 ++
7 files changed, 239 insertions(+), 22 deletions(-)
diff --git a/swiftclient/client.py b/swiftclient/client.py
index 06350901..074e3062 100644
--- a/swiftclient/client.py
+++ b/swiftclient/client.py
@@ -19,10 +19,13 @@ OpenStack Swift client library used internally
import socket
import re
import logging
+from urllib3.exceptions import HTTPError as urllib_http_error
+
import warnings
from requests.exceptions import RequestException, SSLError
import http.client as http_client
+from requests.structures import CaseInsensitiveDict
from urllib.parse import quote, unquote
from urllib.parse import urljoin, urlparse, urlunparse
from time import sleep, time
@@ -287,7 +290,7 @@ class _RetryBody(_ObjectBody):
try:
buf = self.resp.read(length)
self.bytes_read += len(buf)
- except (socket.error, RequestException):
+ except (socket.error, urllib_http_error, RequestException):
if self.conn.attempts > self.conn.retries:
raise
if (not buf and self.bytes_read < self.expected_length and
@@ -735,9 +738,9 @@ def get_auth(auth_url, user, key, **kwargs):
def resp_header_dict(resp):
- resp_headers = {}
+ resp_headers = CaseInsensitiveDict()
for header, value in resp.getheaders():
- header = parse_header_string(header).lower()
+ header = parse_header_string(header)
resp_headers[header] = parse_header_string(value)
return resp_headers
@@ -1926,6 +1929,7 @@ class Connection:
is_not_range_request and resp_chunk_size and
self.attempts <= self.retries and
rheaders.get('transfer-encoding') is None)
+
if retry_is_possible:
body = _RetryBody(body.resp, self, container, obj,
resp_chunk_size=resp_chunk_size,
diff --git a/swiftclient/multithreading.py b/swiftclient/multithreading.py
index eaccec68..9659997c 100644
--- a/swiftclient/multithreading.py
+++ b/swiftclient/multithreading.py
@@ -89,6 +89,10 @@ class OutputManager:
msg = msg % fmt_args
self.error_print_pool.submit(self._print_error, msg)
+ def error_with_txn_id(self, swift_err):
+ self.error("{}\nFailed Transaction ID: {}".format(
+ swift_err.value, swift_err.transaction_id or 'unknown'))
+
def get_error_count(self):
return self.error_count
diff --git a/swiftclient/service.py b/swiftclient/service.py
index 3d1b4764..d0e93075 100644
--- a/swiftclient/service.py
+++ b/swiftclient/service.py
@@ -32,6 +32,9 @@ from time import time
from threading import Thread
from queue import Queue
from queue import Empty as QueueEmpty
+from requests.exceptions import RequestException
+from socket import error as socket_error
+from urllib3.exceptions import HTTPError as urllib_http_error
from urllib.parse import quote
import json
@@ -68,12 +71,16 @@ class ResultsIterator:
class SwiftError(Exception):
def __init__(self, value, container=None, obj=None,
- segment=None, exc=None):
+ segment=None, exc=None, transaction_id=None):
self.value = value
self.container = container
self.obj = obj
self.segment = segment
self.exception = exc
+ if transaction_id is None:
+ self.transaction_id = getattr(exc, 'transaction_id', None)
+ else:
+ self.transaction_id = transaction_id
def __str__(self):
value = repr(self.value)
@@ -459,7 +466,9 @@ class _SwiftReader:
try:
self._content_length = int(headers.get('content-length'))
except ValueError:
- raise SwiftError('content-length header must be an integer')
+ raise SwiftError(
+ 'content-length header must be an integer',
+ transaction_id=self._txn_id)
def __iter__(self):
for chunk in self._body:
@@ -1306,9 +1315,15 @@ class SwiftService:
else:
pseudodir = True
- for chunk in obj_body:
- if fp is not None:
- fp.write(chunk)
+ try:
+ for chunk in obj_body:
+ if fp is not None:
+ fp.write(chunk)
+ except (socket_error,
+ urllib_http_error,
+ RequestException) as err:
+ raise ClientException(
+ str(err), http_response_headers=headers)
finish_time = time()
diff --git a/swiftclient/shell.py b/swiftclient/shell.py
index c5fb2190..4f6cf2de 100755
--- a/swiftclient/shell.py
+++ b/swiftclient/shell.py
@@ -227,7 +227,7 @@ def st_delete(parser, args, output_manager, return_parser=False):
output_manager.error('Error Deleting: {0}: {1}'
.format(p, r['error']))
except SwiftError as err:
- output_manager.error(err.value)
+ output_manager.error_with_txn_id(err)
st_download_options = '''[--all] [--marker <marker>] [--prefix <prefix>]
@@ -484,11 +484,13 @@ def st_download(parser, args, output_manager, return_parser=False):
"Object '%s/%s' not found", container, obj)
continue
output_manager.error(
- "Error downloading object '%s/%s': %s",
- container, obj, error)
+ "Error downloading object '%s/%s': %s\n"
+ "Failed Transaction ID: %s",
+ container, obj, error,
+ getattr(error, 'transaction_id', 'unknown'))
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
except Exception as e:
output_manager.error(e)
@@ -670,7 +672,7 @@ def st_list(parser, args, output_manager, return_parser=False):
prt_bytes(totals['bytes'], human))
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
st_stat_options = '''[--lh] [--header <header:value>]
@@ -761,7 +763,7 @@ def st_stat(parser, args, output_manager, return_parser=False):
st_stat_options, st_stat_help)
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
st_post_options = '''[--read-acl <acl>] [--write-acl <acl>] [--sync-to <sync-to>]
@@ -867,7 +869,7 @@ def st_post(parser, args, output_manager, return_parser=False):
raise(result["error"])
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
st_copy_options = '''[--destination </container/object>] [--fresh-metadata]
@@ -972,7 +974,7 @@ def st_copy(parser, args, output_manager, return_parser=False):
return
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
st_upload_options = '''[--changed] [--skip-identical] [--segment-size <size>]
@@ -1270,7 +1272,7 @@ def st_upload(parser, args, output_manager, return_parser=False):
"to chunk the object")
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
st_capabilities_options = '''[--json] [<proxy_url>]
@@ -1332,7 +1334,7 @@ def st_capabilities(parser, args, output_manager, return_parser=False):
del capabilities['swift']
_print_compo_cap('Additional middleware', capabilities)
except SwiftError as e:
- output_manager.error(e.value)
+ output_manager.error_with_txn_id(e)
st_info = st_capabilities
diff --git a/test/unit/test_service.py b/test/unit/test_service.py
index 99351255..1c6207d8 100644
--- a/test/unit/test_service.py
+++ b/test/unit/test_service.py
@@ -27,6 +27,7 @@ from unittest import mock
from concurrent.futures import Future
from hashlib import md5
from queue import Queue, Empty as QueueEmptyError
+from requests.structures import CaseInsensitiveDict
from time import sleep
import swiftclient
@@ -167,8 +168,10 @@ class TestSwiftReader(unittest.TestCase):
self.assertIsNone(sr._actual_md5)
# Check Contentlength raises error if it isn't an integer
- self.assertRaises(SwiftError, self.sr, 'path', 'body',
- {'content-length': 'notanint'})
+ with self.assertRaises(SwiftError) as cm:
+ self.sr('path', 'body', {'content-length': 'notanint'})
+ self.assertEqual("'content-length header must be an integer'",
+ str(cm.exception))
def test_iterator_usage(self):
def _consume(sr):
@@ -653,6 +656,7 @@ class TestSwiftError(unittest.TestCase):
self.assertIsNone(se.exception)
self.assertEqual(str(se), '5')
+ self.assertNotIn(str(se), 'Transaction ID')
def test_swifterror_creation(self):
test_exc = Exception('test exc')
@@ -665,6 +669,25 @@ class TestSwiftError(unittest.TestCase):
self.assertEqual(se.exception, test_exc)
self.assertEqual(str(se), '5 container:con object:obj segment:seg')
+ self.assertNotIn(str(se), 'Transaction ID')
+
+ def test_swifterror_clientexception_creation(self):
+ test_exc = ClientException(
+ Exception('test exc'),
+ http_response_headers=CaseInsensitiveDict({
+ 'x-trans-id': 'someTransId'})
+ )
+ se = SwiftError(5, 'con', 'obj', 'seg', test_exc)
+
+ self.assertEqual(se.value, 5)
+ self.assertEqual(se.container, 'con')
+ self.assertEqual(se.obj, 'obj')
+ self.assertEqual(se.segment, 'seg')
+ self.assertEqual(se.exception, test_exc)
+
+ self.assertEqual('someTransId', se.transaction_id)
+ self.assertNotIn('someTransId', str(se))
+ self.assertIn('5 container:con object:obj segment:seg', str(se))
class TestServiceUtils(unittest.TestCase):
diff --git a/test/unit/test_shell.py b/test/unit/test_shell.py
index eedbc4be..13940794 100644
--- a/test/unit/test_shell.py
+++ b/test/unit/test_shell.py
@@ -15,17 +15,21 @@
import io
import contextlib
+import socket
from genericpath import getmtime
import getpass
import hashlib
import json
import logging
import os
+from requests.structures import CaseInsensitiveDict
import tempfile
import unittest
from unittest import mock
import textwrap
from time import localtime, mktime, strftime, strptime
+from requests.exceptions import RequestException
+from urllib3.exceptions import HTTPError
import swiftclient
from swiftclient.service import SwiftError
@@ -207,6 +211,28 @@ class TestShell(unittest.TestCase):
' Sync To: other\n'
' Sync Key: secret\n')
+ @mock.patch('swiftclient.service.Connection')
+ def test_stat_container_not_found(self, connection):
+ connection.return_value.head_container.side_effect = \
+ swiftclient.ClientException(
+ 'test',
+ http_status=404,
+ http_response_headers=CaseInsensitiveDict({
+ 'x-trans-id': 'someTransId'})
+ )
+ argv = ["", "stat", "container"]
+
+ with CaptureOutput() as output:
+ with self.assertRaises(SystemExit):
+ swiftclient.shell.main(argv)
+ connection.return_value.head_container.assert_called_with(
+ 'container', headers={}, resp_chunk_size=65536,
+ response_dict={})
+
+ self.assertIn('Container \'container\' not found\n'
+ 'Failed Transaction ID: someTransId',
+ output.err)
+
@mock.patch('swiftclient.service.Connection')
def test_stat_container_with_headers(self, connection):
return_headers = {
@@ -285,6 +311,27 @@ class TestShell(unittest.TestCase):
' ETag: md5\n'
' Manifest: manifest\n')
+ @mock.patch('swiftclient.service.Connection')
+ def test_stat_object_not_found(self, connection):
+ connection.return_value.head_object.side_effect = \
+ swiftclient.ClientException(
+ 'test', http_status=404,
+ http_response_headers=CaseInsensitiveDict({
+ 'x-trans-id': 'someTransId'})
+ )
+ argv = ["", "stat", "container", "object"]
+
+ with CaptureOutput() as output:
+ with self.assertRaises(SystemExit):
+ swiftclient.shell.main(argv)
+ connection.return_value.head_object.assert_called_with(
+ 'container', 'object', headers={}, resp_chunk_size=65536,
+ response_dict={})
+
+ self.assertIn('test: 404\n'
+ 'Failed Transaction ID: someTransId',
+ output.err)
+
@mock.patch('swiftclient.service.Connection')
def test_stat_object_with_headers(self, connection):
return_headers = {
@@ -707,6 +754,117 @@ class TestShell(unittest.TestCase):
swiftclient.shell.main(argv)
self.assertEqual('objcontent', output.out)
+ def _do_test_download_clientexception(self, exc):
+ retry_calls = []
+
+ def _fake_retry(conn, *args, **kwargs):
+ retry_calls.append((args, kwargs))
+ conn.attempts += 1
+
+ body = mock.MagicMock()
+ body.resp.read.side_effect = RequestException('test_exc')
+ return (CaseInsensitiveDict({
+ 'content-type': 'text/plain',
+ 'etag': '2cbbfe139a744d6abbe695e17f3c1991',
+ 'x-trans-id': 'someTransId'}),
+ body)
+
+ argv = ["", "download", "container", "object", "--retries", "1"]
+ with CaptureOutput() as output:
+ with mock.patch(BUILTIN_OPEN) as mock_open:
+ with mock.patch("swiftclient.service.Connection._retry",
+ _fake_retry):
+ with self.assertRaises(SystemExit):
+ swiftclient.shell.main(argv)
+ mock_open.assert_called_with('object', 'wb', 65536)
+ self.assertEqual([
+ ((None, swiftclient.client.get_object, 'container', 'object'),
+ {'headers': {},
+ 'query_string': None,
+ 'resp_chunk_size': 65536,
+ 'response_dict': {}}),
+ ((None, swiftclient.client.get_object, 'container', 'object'),
+ {'attempts': 1,
+ 'headers': {'If-Match': mock.ANY, 'Range': 'bytes=0-'},
+ 'query_string': None,
+ 'resp_chunk_size': 65536,
+ 'response_dict': {}})],
+ retry_calls)
+ self.assertIn('Error downloading object \'container/object\': '
+ 'test_exc',
+ str(output.err))
+ self.assertIn('someTransId', str(output.err))
+
+ def test_download_request_exception(self):
+ self._do_test_download_clientexception(RequestException('text_exc'))
+
+ def test_download_socket_error(self):
+ self._do_test_download_clientexception(socket.error())
+
+ def test_download_http_error(self):
+ self._do_test_download_clientexception(HTTPError)
+
+ def test_download_request_exception_retries_0(self):
+ retry_calls = []
+
+ def _fake_retry(conn, *args, **kwargs):
+ retry_calls.append((args, kwargs))
+ conn.attempts += 1
+
+ body = mock.MagicMock()
+ body.__iter__.side_effect = RequestException('test_exc')
+ return (CaseInsensitiveDict({
+ 'content-type': 'text/plain',
+ 'etag': '2cbbfe139a744d6abbe695e17f3c1991',
+ 'x-trans-id': 'someTransId'}),
+ body)
+
+ argv = ["", "download", "container", "object", "--retries", "0"]
+ with CaptureOutput() as output:
+ with mock.patch(BUILTIN_OPEN) as mock_open:
+ with mock.patch("swiftclient.service.Connection._retry",
+ _fake_retry):
+ with self.assertRaises(SystemExit):
+ swiftclient.shell.main(argv)
+ mock_open.assert_called_with('object', 'wb', 65536)
+ self.assertEqual([
+ ((None, swiftclient.client.get_object, 'container', 'object'),
+ {'headers': {},
+ 'query_string': None,
+ 'resp_chunk_size': 65536,
+ 'response_dict': {}}), ],
+ retry_calls)
+ self.assertIn('Error downloading object \'container/object\': '
+ 'test_exc',
+ str(output.err))
+ self.assertIn('someTransId', str(output.err))
+
+ @mock.patch('swiftclient.service.Connection')
+ def test_download_bad_content_length(self, connection):
+ objcontent = io.BytesIO(b'objcontent')
+ connection.return_value.get_object.side_effect = [
+ (CaseInsensitiveDict({
+ 'content-type': 'text/plain',
+ 'content-length': 'BAD',
+ 'etag': '2cbbfe139a744d6abbe695e17f3c1991',
+ 'x-trans-id': 'someTransId'}),
+ objcontent)
+ ]
+ with CaptureOutput() as output:
+ with self.assertRaises(SystemExit):
+ with mock.patch(BUILTIN_OPEN) as mock_open:
+ argv = ["", "download", "container", "object"]
+ swiftclient.shell.main(argv)
+ connection.return_value.get_object.assert_called_with(
+ 'container', 'object', headers={}, resp_chunk_size=65536,
+ response_dict={})
+ mock_open.assert_called_with('object', 'wb', 65536)
+
+ self.assertIn("Error downloading object \'container/object\': "
+ "'content-length header must be an integer'"
+ "\nFailed Transaction ID: someTransId",
+ str(output.err))
+
@mock.patch('swiftclient.service.shuffle')
@mock.patch('swiftclient.service.Connection')
def test_download_shuffle(self, connection, mock_shuffle):
@@ -1901,7 +2059,9 @@ class TestShell(unittest.TestCase):
with self.assertRaises(SystemExit):
swiftclient.shell.main(argv)
- self.assertEqual(output.err, 'Account not found\n')
+ self.assertEqual(
+ output.err,
+ 'Account not found\nFailed Transaction ID: unknown\n')
@mock.patch('swiftclient.service.Connection')
def test_post_container(self, connection):
@@ -2093,7 +2253,8 @@ class TestShell(unittest.TestCase):
self.assertEqual(
output.err,
'Combination of multiple objects and destination '
- 'including object is invalid\n')
+ 'including object is invalid\n'
+ 'Failed Transaction ID: unknown\n')
@mock.patch('swiftclient.service.Connection')
def test_copy_object_bad_auth(self, connection):
diff --git a/test/unit/test_swiftclient.py b/test/unit/test_swiftclient.py
index be4f4a5e..a34e94f1 100644
--- a/test/unit/test_swiftclient.py
+++ b/test/unit/test_swiftclient.py
@@ -1118,6 +1118,14 @@ class TestGetObject(MockHttpTest):
self.assertEqual('%ff', headers.get('x-non-utf-8-header', ''))
self.assertEqual('%FF', headers.get('x-binary-header', ''))
+ self.assertEqual('t\xe9st', headers.get('X-Utf-8-Header', ''))
+ self.assertEqual('%ff', headers.get('X-Non-Utf-8-Header', ''))
+ self.assertEqual('%FF', headers.get('X-Binary-Header', ''))
+
+ self.assertEqual('t\xe9st', headers.get('X-UTF-8-HEADER', ''))
+ self.assertEqual('%ff', headers.get('X-NON-UTF-8-HEADER', ''))
+ self.assertEqual('%FF', headers.get('X-BINARY-HEADER', ''))
+
def test_chunk_size_read_method(self):
conn = c.Connection('http://auth.url/', 'some_user', 'some_key')
with mock.patch('swiftclient.client.get_auth_1_0') as mock_get_auth: