From f1858d89e0e1889664ced654755c508f47a0c1f3 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Tue, 11 Jan 2022 16:05:39 -0800 Subject: [PATCH 01/84] Add option to skip container PUT during upload Currently, a user with read/write access to a container (but without access to creat new containers) recieves a warning every time they upload. Now, allow them to avoid the extra request and warning by specifying --skip-container-put on the command line. This is also useful when testing: developers can HEAD a container to ensure it's in memcache, shut down all container servers, then upload and creaate a bunch of async pendings. Previously, the 503 on container PUT would prevent the object upload from even being attempted. Closes-Bug: 1317956 Related-Bug: 1204558 Change-Id: I3d9129a0b6b65c6c6187ae6af003b221afceef47 Related-Change: If1f8a02ee7459ea2158ffa6e958f67d299ec529e --- swiftclient/service.py | 95 ++++++++++++++++++++++------------------- swiftclient/shell.py | 13 ++++-- test/unit/test_shell.py | 42 ++++++++++++++++++ 3 files changed, 102 insertions(+), 48 deletions(-) diff --git a/swiftclient/service.py b/swiftclient/service.py index 8e2c7b00..685b7482 100644 --- a/swiftclient/service.py +++ b/swiftclient/service.py @@ -202,6 +202,7 @@ def _build_default_global_options(): 'leave_segments': False, 'changed': None, 'skip_identical': False, + 'skip_container_put': False, 'version_id': None, 'yes_all': False, 'read_acl': None, @@ -1462,6 +1463,7 @@ def upload(self, container, objects, options=None): 'leave_segments': False, 'changed': None, 'skip_identical': False, + 'skip_container_put': False, 'fail_fast': False, 'dir_marker': False # Only for None sources } @@ -1487,54 +1489,57 @@ def upload(self, container, objects, options=None): # the object name. (same as passing --object-name). container, _sep, pseudo_folder = container.partition('/') - # Try to create the container, just in case it doesn't exist. If this - # fails, it might just be because the user doesn't have container PUT - # permissions, so we'll ignore any error. If there's really a problem, - # it'll surface on the first object PUT. - policy_header = {} - _header = split_headers(options["header"]) - if POLICY in _header: - policy_header[POLICY] = \ - _header[POLICY] - create_containers = [ - self.thread_manager.container_pool.submit( - self._create_container_job, container, headers=policy_header) - ] + if not options['skip_container_put']: + # Try to create the container, just in case it doesn't exist. If + # this fails, it might just be because the user doesn't have + # container PUT permissions, so we'll ignore any error. If there's + # really a problem, it'll surface on the first object PUT. + policy_header = {} + _header = split_headers(options["header"]) + if POLICY in _header: + policy_header[POLICY] = \ + _header[POLICY] + create_containers = [ + self.thread_manager.container_pool.submit( + self._create_container_job, container, + headers=policy_header) + ] - # wait for first container job to complete before possibly attempting - # segment container job because segment container job may attempt - # to HEAD the first container - for r in interruptable_as_completed(create_containers): - res = r.result() - yield res + # wait for first container job to complete before possibly + # attempting segment container job because segment container job + # may attempt to HEAD the first container + for r in interruptable_as_completed(create_containers): + res = r.result() + yield res - if segment_size: - seg_container = container + '_segments' - if options['segment_container']: - seg_container = options['segment_container'] - if seg_container != container: - if not policy_header: - # Since no storage policy was specified on the command - # line, rather than just letting swift pick the default - # storage policy, we'll try to create the segments - # container with the same policy as the upload container - create_containers = [ - self.thread_manager.container_pool.submit( - self._create_container_job, seg_container, - policy_source=container - ) - ] - else: - create_containers = [ - self.thread_manager.container_pool.submit( - self._create_container_job, seg_container, - headers=policy_header - ) - ] + if segment_size: + seg_container = container + '_segments' + if options['segment_container']: + seg_container = options['segment_container'] + if seg_container != container: + if not policy_header: + # Since no storage policy was specified on the command + # line, rather than just letting swift pick the default + # storage policy, we'll try to create the segments + # container with the same policy as the upload + # container + create_containers = [ + self.thread_manager.container_pool.submit( + self._create_container_job, seg_container, + policy_source=container + ) + ] + else: + create_containers = [ + self.thread_manager.container_pool.submit( + self._create_container_job, seg_container, + headers=policy_header + ) + ] - for r in interruptable_as_completed(create_containers): - res = r.result() - yield res + for r in interruptable_as_completed(create_containers): + res = r.result() + yield res # We maintain a results queue here and a separate thread to monitor # the futures because we want to get results back from potential diff --git a/swiftclient/shell.py b/swiftclient/shell.py index 6da9d667..76473fd6 100755 --- a/swiftclient/shell.py +++ b/swiftclient/shell.py @@ -985,8 +985,9 @@ def st_copy(parser, args, output_manager, return_parser=False): st_upload_options = '''[--changed] [--skip-identical] [--segment-size ] [--segment-container ] [--leave-segments] [--object-threads ] [--segment-threads ] - [--meta ] [--header
] [--use-slo] - [--ignore-checksum] [--object-name ] + [--meta ] [--header
] + [--use-slo] [--ignore-checksum] [--skip-container-put] + [--object-name ] [] [...] ''' @@ -1032,11 +1033,13 @@ def st_copy(parser, args, output_manager, return_parser=False): --use-slo When used in conjunction with --segment-size it will create a Static Large Object instead of the default Dynamic Large Object. + --ignore-checksum Turn off checksum validation for uploads. + --skip-container-put Assume all necessary containers already exist; don't + automatically try to create them. --object-name Upload file and name object to or upload dir and use as object prefix instead of folder name. - --ignore-checksum Turn off checksum validation for uploads. '''.strip('\n') @@ -1051,6 +1054,10 @@ def st_upload(parser, args, output_manager, return_parser=False): '--skip-identical', action='store_true', dest='skip_identical', default=False, help='Skip uploading files that are identical on ' 'both sides.') + parser.add_argument( + '--skip-container-put', action='store_true', dest='skip_container_put', + default=False, help='Assume all necessary containers already exist; ' + "don't automatically try to create them.") parser.add_argument( '-S', '--segment-size', dest='segment_size', help='Upload files ' 'in segments no larger than (in Bytes) and then create a ' diff --git a/test/unit/test_shell.py b/test/unit/test_shell.py index 295c918a..2331eaa0 100644 --- a/test/unit/test_shell.py +++ b/test/unit/test_shell.py @@ -912,6 +912,48 @@ def test_upload(self, connection, walk): query_string='multipart-manifest=put', response_dict=mock.ANY) + @mock.patch('swiftclient.shell.walk') + @mock.patch('swiftclient.service.Connection') + def test_upload_skip_container_put(self, connection, walk): + connection.return_value.head_object.return_value = { + 'content-length': '0'} + connection.return_value.put_object.return_value = EMPTY_ETAG + connection.return_value.attempts = 0 + argv = ["", "upload", "container", "--skip-container-put", + self.tmpfile, "-H", "X-Storage-Policy:one", + "--meta", "Color:Blue"] + swiftclient.shell.main(argv) + connection.return_value.put_container.assert_not_called() + + connection.return_value.put_object.assert_called_with( + 'container', + self.tmpfile.lstrip('/'), + mock.ANY, + content_length=0, + headers={'x-object-meta-mtime': mock.ANY, + 'X-Storage-Policy': 'one', + 'X-Object-Meta-Color': 'Blue'}, + response_dict={}) + + # Upload in segments + connection.return_value.head_container.return_value = { + 'x-storage-policy': 'one'} + argv = ["", "upload", "container", "--skip-container-put", + self.tmpfile, "-S", "10"] + with open(self.tmpfile, "wb") as fh: + fh.write(b'12345678901234567890') + swiftclient.shell.main(argv) + # Both base and segments container are assumed to exist already + connection.return_value.put_container.assert_not_called() + connection.return_value.put_object.assert_called_with( + 'container', + self.tmpfile.lstrip('/'), + '', + content_length=0, + headers={'x-object-manifest': mock.ANY, + 'x-object-meta-mtime': mock.ANY}, + response_dict={}) + @mock.patch('swiftclient.service.SwiftService.upload') def test_upload_object_with_account_readonly(self, upload): argv = ["", "upload", "container", self.tmpfile] From 5d451fb92031989ea9982fc29140175014448ea2 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Tue, 22 Mar 2022 10:14:19 -0700 Subject: [PATCH 02/84] More cleanup following py2 removal * Drop py2-only hacking pin from test-requirements. * Remove quote() helper; urllib.parse.quote() works fine. * Remove some useless code. Change-Id: I9ffc923f58f1d11538f83ff26f7beb53cdf134c3 --- swiftclient/client.py | 10 +--------- swiftclient/service.py | 5 ----- swiftclient/shell.py | 2 -- test-requirements.txt | 7 +++---- test/unit/test_multithreading.py | 3 +-- test/unit/test_shell.py | 4 ++-- 6 files changed, 7 insertions(+), 24 deletions(-) diff --git a/swiftclient/client.py b/swiftclient/client.py index a9130ab5..168bfeda 100644 --- a/swiftclient/client.py +++ b/swiftclient/client.py @@ -23,7 +23,7 @@ from requests.exceptions import RequestException, SSLError import http.client as http_client -from urllib.parse import quote as _quote, unquote +from urllib.parse import quote, unquote from urllib.parse import urljoin, urlparse, urlunparse from time import sleep, time @@ -181,14 +181,6 @@ def parse_header_string(data): return unquoted -def quote(value, safe='/'): - """ - Patched version of urllib.quote that encodes utf8 strings before quoting. - On Python 3, call directly urllib.parse.quote(). - """ - return _quote(value, safe=safe) - - def encode_utf8(value): if type(value) in (int, float, bool): # As of requests 2.11.0, headers must be byte- or unicode-strings. diff --git a/swiftclient/service.py b/swiftclient/service.py index 9d9fc594..ed0f40a7 100644 --- a/swiftclient/service.py +++ b/swiftclient/service.py @@ -2034,11 +2034,6 @@ def _upload_slo_manifest(conn, segment_results, container, obj, headers): if headers is None: headers = {} segment_results.sort(key=lambda di: di['segment_index']) - for seg in segment_results: - seg_loc = seg['segment_location'].lstrip('/') - if isinstance(seg_loc, str): - seg_loc = seg_loc.encode('utf-8') - manifest_data = json.dumps([ { 'path': d['segment_location'], diff --git a/swiftclient/shell.py b/swiftclient/shell.py index df7c5115..445d4cb8 100755 --- a/swiftclient/shell.py +++ b/swiftclient/shell.py @@ -1929,8 +1929,6 @@ def add_default_args(parser): def main(arguments=None): argv = sys_argv if arguments is None else arguments - argv = [a if isinstance(a, str) else a.decode('utf-8') for a in argv] - parser = argparse.ArgumentParser( add_help=False, formatter_class=HelpFormatter, usage=''' %(prog)s [--version] [--help] [--os-help] [--snet] [--verbose] diff --git a/test-requirements.txt b/test-requirements.txt index b0633eb9..a4b64ee7 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,7 +1,6 @@ -hacking>=1.1.0,<1.2.0;python_version<'3.0' # Apache-2.0 -hacking>=3.2.0,<3.3.0;python_version>='3.0' # Apache-2.0 +hacking>=3.2.0,<3.3.0 # Apache-2.0 -coverage!=4.4,>=4.0 # Apache-2.0 +coverage!=4.4,>=4.0 # Apache-2.0 keystoneauth1>=3.4.0 # Apache-2.0 stestr>=2.0.0,!=3.0.0 # Apache-2.0 -openstacksdk>=0.11.0 # Apache-2.0 +openstacksdk>=0.11.0 # Apache-2.0 diff --git a/test/unit/test_multithreading.py b/test/unit/test_multithreading.py index d51bfb7c..8237d829 100644 --- a/test/unit/test_multithreading.py +++ b/test/unit/test_multithreading.py @@ -216,12 +216,11 @@ def test_printers(self): # The threads should have been cleaned up self.assertEqual(starting_thread_count, threading.active_count()) - over_the = "over the '\u062a\u062a'\n" self.assertEqual(''.join([ 'one-argument\n', 'one fish, 88 fish\n', 'some\n', 'where\n', - over_the, + "over the '\u062a\u062a'\n", 'some raw bytes: \u062a\u062a', ' key: value\n', ' object: O\u0308bject\n' diff --git a/test/unit/test_shell.py b/test/unit/test_shell.py index 94168b5d..105dddb1 100644 --- a/test/unit/test_shell.py +++ b/test/unit/test_shell.py @@ -1622,7 +1622,7 @@ def test_delete_verbose_output_utf8(self): with mock.patch('swiftclient.shell.SwiftService.delete') as mock_func: with CaptureOutput() as out: mock_func.return_value = [res] - swiftclient.shell.main(base_argv + [container.encode('utf-8')]) + swiftclient.shell.main(base_argv + [container]) mock_func.assert_called_once_with(container=container) self.assertTrue(out.out.find( @@ -1635,7 +1635,7 @@ def test_delete_verbose_output_utf8(self): with mock.patch('swiftclient.shell.SwiftService.delete') as mock_func: with CaptureOutput() as out: mock_func.return_value = [res] - swiftclient.shell.main(base_argv + [container.encode('utf-8')]) + swiftclient.shell.main(base_argv + [container]) mock_func.assert_called_once_with(container=container) self.assertTrue(out.out.find( From 0bd2ab5cb0020bc15f36a2614aecdc28feecbea0 Mon Sep 17 00:00:00 2001 From: Takashi Natsume Date: Thu, 18 Aug 2022 22:24:00 +0900 Subject: [PATCH 03/84] Fix misuse of assertTrue Replace assertTrue with assertEqual. Change-Id: Ia3524bc5b3b01c0039bede6bb172535eb85bac08 Closes-Bug: 1986948 Signed-off-by: Takashi Natsume --- test/functional/test_swiftclient.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/functional/test_swiftclient.py b/test/functional/test_swiftclient.py index 91e31af0..a5c1211c 100644 --- a/test/functional/test_swiftclient.py +++ b/test/functional/test_swiftclient.py @@ -332,7 +332,7 @@ def test_download_object_retry_chunked(self): resp_chunk_size=resp_chunk_size) data = next(body) self.assertEqual(self.test_data[:resp_chunk_size], data) - self.assertTrue(1, self.conn.attempts) + self.assertEqual(1, self.conn.attempts) for chunk in body.resp: # Flush remaining data from underlying response # (simulate a dropped connection) @@ -369,13 +369,13 @@ def test_download_object_non_chunked(self): hdrs, body = self.conn.get_object(self.containername, self.objectname) data = body self.assertEqual(self.test_data, data) - self.assertTrue(1, self.conn.attempts) + self.assertEqual(1, self.conn.attempts) hdrs, body = self.conn.get_object(self.containername, self.objectname, resp_chunk_size=0) data = body self.assertEqual(self.test_data, data) - self.assertTrue(1, self.conn.attempts) + self.assertEqual(1, self.conn.attempts) def test_post_account(self): self.conn.post_account({'x-account-meta-data': 'Something'}) From a1d2f31131d79d7c551dbac4fc1e9c4d177d2df5 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 17 Aug 2022 16:58:36 -0700 Subject: [PATCH 04/84] Enable retry_on_ratelimit by default UpgradeImpact ============= The Connection class now enables retry_on_ratelimit by default. If you need to return to the old behavior, explicitly pass retry_on_ratelimit=False as a keyword arg. The SwiftService class will now enables the retry_on_ratelimit option by default. If you need to return to the old behavior, explicitly set it to false in your options dict. Change-Id: I3221fda84f0b8031c50128aa600e2c19deb5b102 --- swiftclient/client.py | 8 ++++---- swiftclient/service.py | 3 +++ test/unit/test_swiftclient.py | 3 ++- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/swiftclient/client.py b/swiftclient/client.py index a9130ab5..8415db22 100644 --- a/swiftclient/client.py +++ b/swiftclient/client.py @@ -1654,7 +1654,7 @@ def __init__(self, authurl=None, user=None, key=None, retries=5, starting_backoff=1, max_backoff=64, tenant_name=None, os_options=None, auth_version="1", cacert=None, insecure=False, cert=None, cert_key=None, - ssl_compression=True, retry_on_ratelimit=False, + ssl_compression=True, retry_on_ratelimit=True, timeout=None, session=None, force_auth_retry=False): """ :param authurl: authentication URL @@ -1686,9 +1686,9 @@ def __init__(self, authurl=None, user=None, key=None, retries=5, will be made. This may provide a performance increase for https upload/download operations. :param retry_on_ratelimit: by default, a ratelimited connection will - raise an exception to the caller. Setting - this parameter to True will cause a retry - after a backoff. + retry after a backoff. Setting this + parameter to False will cause an exception + to be raised to the caller. :param timeout: The connect timeout for the HTTP connection. :param session: A keystoneauth session object. :param force_auth_retry: reset auth info even if client got unexpected diff --git a/swiftclient/service.py b/swiftclient/service.py index 9d9fc594..545ea47f 100644 --- a/swiftclient/service.py +++ b/swiftclient/service.py @@ -155,6 +155,7 @@ def _build_default_global_options(): "user": environ.get('ST_USER'), "key": environ.get('ST_KEY'), "retries": 5, + "retry_on_ratelimit": True, "force_auth_retry": False, "os_username": environ.get('OS_USERNAME'), "os_user_id": environ.get('OS_USER_ID'), @@ -270,10 +271,12 @@ def get_conn(options): """ Return a connection building it from the options. """ + options = dict(_default_global_options, **options) return Connection(options['auth'], options['user'], options['key'], timeout=options.get('timeout'), + retry_on_ratelimit=options['retry_on_ratelimit'], retries=options['retries'], auth_version=options['auth_version'], os_options=options['os_options'], diff --git a/test/unit/test_swiftclient.py b/test/unit/test_swiftclient.py index ad2af50c..436245d8 100644 --- a/test/unit/test_swiftclient.py +++ b/test/unit/test_swiftclient.py @@ -2149,7 +2149,8 @@ def quick_sleep(*args): c.http_connection = self.fake_http_connection( 200, 498, headers=auth_resp_headers) - conn = c.Connection('http://www.test.com/auth/v1.0', 'asdf', 'asdf') + conn = c.Connection('http://www.test.com/auth/v1.0', 'asdf', 'asdf', + retry_on_ratelimit=False) with self.assertRaises(c.ClientException) as exc_context: conn.head_account() self.assertIn('Account HEAD failed', str(exc_context.exception)) From 653cbcb686fc34cc28f9c7889e8746e77b95371a Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Wed, 10 Aug 2022 12:38:54 -0500 Subject: [PATCH 05/84] Expand retry handling on ratelimit response We have seen middlewares that return ratelimit responses as 498 or 429, so tolerate either. Closes-Bug: #1879572 Change-Id: I027222157f6c2ad7882a0508302c9de097baae4c --- swiftclient/client.py | 2 +- test/unit/test_swiftclient.py | 54 +++++++++++++++++++---------------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/swiftclient/client.py b/swiftclient/client.py index 8415db22..16bbba82 100644 --- a/swiftclient/client.py +++ b/swiftclient/client.py @@ -1833,7 +1833,7 @@ def _retry(self, reset_func, func, *args, **kwargs): self.http_conn = None elif 500 <= err.http_status <= 599: pass - elif self.retry_on_ratelimit and err.http_status == 498: + elif self.retry_on_ratelimit and err.http_status in (498, 429): pass else: raise diff --git a/test/unit/test_swiftclient.py b/test/unit/test_swiftclient.py index 436245d8..ae3e76fd 100644 --- a/test/unit/test_swiftclient.py +++ b/test/unit/test_swiftclient.py @@ -2130,31 +2130,37 @@ def quick_sleep(*args): pass c.sleep = quick_sleep - # test retries - conn = c.Connection('http://www.test.com/auth/v1.0', 'asdf', 'asdf', - retry_on_ratelimit=True) - code_iter = [200] + [498] * (conn.retries + 1) - auth_resp_headers = { - 'x-auth-token': 'asdf', - 'x-storage-url': 'http://storage/v1/test', - } - c.http_connection = self.fake_http_connection( - *code_iter, headers=auth_resp_headers) - with self.assertRaises(c.ClientException) as exc_context: - conn.head_account() - self.assertIn('Account HEAD failed', str(exc_context.exception)) - self.assertEqual(conn.attempts, conn.retries + 1) + def test_status_code(code): + # test retries + conn = c.Connection('http://www.test.com/auth/v1.0', + 'asdf', 'asdf', retry_on_ratelimit=True) + code_iter = [200] + [code] * (conn.retries + 1) + auth_resp_headers = { + 'x-auth-token': 'asdf', + 'x-storage-url': 'http://storage/v1/test', + } + c.http_connection = self.fake_http_connection( + *code_iter, headers=auth_resp_headers) + with self.assertRaises(c.ClientException) as exc_context: + conn.head_account() + self.assertIn('Account HEAD failed', str(exc_context.exception)) + self.assertEqual(code, exc_context.exception.http_status) + self.assertEqual(conn.attempts, conn.retries + 1) - # test default no-retry - c.http_connection = self.fake_http_connection( - 200, 498, - headers=auth_resp_headers) - conn = c.Connection('http://www.test.com/auth/v1.0', 'asdf', 'asdf', - retry_on_ratelimit=False) - with self.assertRaises(c.ClientException) as exc_context: - conn.head_account() - self.assertIn('Account HEAD failed', str(exc_context.exception)) - self.assertEqual(conn.attempts, 1) + # test default no-retry + c.http_connection = self.fake_http_connection( + 200, code, + headers=auth_resp_headers) + conn = c.Connection('http://www.test.com/auth/v1.0', + 'asdf', 'asdf', retry_on_ratelimit=False) + with self.assertRaises(c.ClientException) as exc_context: + conn.head_account() + self.assertIn('Account HEAD failed', str(exc_context.exception)) + self.assertEqual(code, exc_context.exception.http_status) + self.assertEqual(conn.attempts, 1) + + test_status_code(498) + test_status_code(429) def test_retry_with_socket_error(self): def quick_sleep(*args): From defbb4a8f390c7de73ac6a90fc1ab5009e8105ee Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Thu, 15 Oct 2020 14:05:28 -0700 Subject: [PATCH 06/84] Allow tempurl times to have units Specifically, let users add a suffix for seconds, minutes, hours, or days. Change-Id: Ibbe7e5aa8aa8e54935da76109c2ea13fb83bc7ab --- swiftclient/shell.py | 18 +++++---- swiftclient/utils.py | 89 +++++++++++++++++++++++++---------------- test/unit/test_shell.py | 8 ++++ test/unit/test_utils.py | 39 ++++++++++++++++++ 4 files changed, 112 insertions(+), 42 deletions(-) diff --git a/swiftclient/shell.py b/swiftclient/shell.py index 5bcff7fc..ed39b819 100755 --- a/swiftclient/shell.py +++ b/swiftclient/shell.py @@ -1381,14 +1381,16 @@ def st_auth(parser, args, thread_manager, return_parser=False): An HTTP method to allow for this temporary URL. Usually 'GET' or 'PUT'.