From 6a8675e897e634abd26d993181ac4a45b9cf16f7 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Mon, 14 Nov 2022 13:17:47 -0800 Subject: Use SLO by default for segmented uploads if the cluster supports it We've had SLO for nearly ten years, and it is preferable for most use-cases. It's time to default to SLO rather than DLO. Add a new --use-dlo option to give users a way to return to old behavior; ensure there is still a --use-slo option so we don't break existing scripts that may use it. UpgradeImpact: ============== The default segmented-upload behavior has changed; Static Large Objects are now used by default rather than Dynamic Large Objects. To revert to the old behavior: * CLI users may use the new `--use-dlo` option * Service API users may explicitly set `use_slo` to False in their options dicts Change-Id: Iebcd64ced8eab6efebb9f02a0c4be6bf6338cb3b --- swiftclient/service.py | 16 ++++++++-- swiftclient/shell.py | 20 +++++++----- test/unit/test_service.py | 80 ++++++++++++++++++++++++++++++++++++++++++++++- test/unit/test_shell.py | 65 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 168 insertions(+), 13 deletions(-) diff --git a/swiftclient/service.py b/swiftclient/service.py index e905ea6..dd71409 100644 --- a/swiftclient/service.py +++ b/swiftclient/service.py @@ -196,7 +196,7 @@ _default_global_options = _build_default_global_options() _default_local_options = { 'sync_to': None, 'sync_key': None, - 'use_slo': False, + 'use_slo': None, 'segment_size': None, 'segment_container': None, 'leave_segments': False, @@ -1467,7 +1467,7 @@ class SwiftService: 'meta': [], 'header': [], 'segment_size': None, - 'use_slo': False, + 'use_slo': True, 'segment_container': None, 'leave_segments': False, 'changed': None, @@ -1493,6 +1493,18 @@ class SwiftService: except ValueError: raise SwiftError('Segment size should be an integer value') + if segment_size and options['use_slo'] is None: + try: + cap_result = self.capabilities() + except ClientException: + # pre-info swift, maybe? assume no slo middleware + options['use_slo'] = False + else: + if not cap_result['success']: + options['use_slo'] = False + else: + options['use_slo'] = 'slo' in cap_result['capabilities'] + # Incase we have a psudeo-folder path for arg, derive # the container name from the top path and prepend the rest to # the object name. (same as passing --object-name). diff --git a/swiftclient/shell.py b/swiftclient/shell.py index 882a1c0..319141d 100755 --- a/swiftclient/shell.py +++ b/swiftclient/shell.py @@ -978,8 +978,8 @@ 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] [--skip-container-put] + [--meta ] [--header
] [--use-slo] + [--use-dlo] [--ignore-checksum] [--skip-container-put] [--object-name ] [] [...] ''' @@ -1024,8 +1024,11 @@ Optional arguments: repeated. Example: -H "content-type:text/plain" -H "Content-Length: 4000". --use-slo When used in conjunction with --segment-size it will - create a Static Large Object instead of the default - Dynamic Large Object. + create a Static Large Object. Deprecated; this is now + the default behavior when the cluster supports it. + --use-dlo When used in conjunction with --segment-size it will + create a Dynamic Large Object. May be useful with old + swift clusters. --ignore-checksum Turn off checksum validation for uploads. --skip-container-put Assume all necessary containers already exist; don't automatically try to create them. @@ -1087,10 +1090,13 @@ def st_upload(parser, args, output_manager, return_parser=False): ' This option may be repeated. Example: -H "content-type:text/plain" ' '-H "Content-Length: 4000"') parser.add_argument( - '--use-slo', action='store_true', default=False, + '--use-slo', action='store_true', default=None, help='When used in conjunction with --segment-size, it will ' - 'create a Static Large Object instead of the default ' - 'Dynamic Large Object.') + 'create a Static Large Object.') + parser.add_argument( + '--use-dlo', action='store_false', dest="use_slo", default=None, + help='When used in conjunction with --segment-size, it will ' + 'create a Dynamic Large Object.') parser.add_argument( '--object-name', dest='object_name', help='Upload file and name object to or upload dir and ' diff --git a/test/unit/test_service.py b/test/unit/test_service.py index cdab61f..9935125 100644 --- a/test/unit/test_service.py +++ b/test/unit/test_service.py @@ -1410,7 +1410,7 @@ class TestServiceUpload(_TestServiceBase): self.assertTrue(fp.closed, 'Failed to close open(%s)' % formatted_args) - def test_upload_object_job_file_with_unicode_path(self): + def test_upload_dlo_job_file_with_unicode_path(self): # Uploading a file results in the file object being wrapped in a # LengthWrapper. This test sets the options in such a way that much # of _upload_object_job is skipped bringing the critical path down @@ -1471,6 +1471,84 @@ class TestServiceUpload(_TestServiceBase): headers={}, response_dict={}) + def test_upload_slo_job_file_with_unicode_path(self): + # Uploading a file results in the file object being wrapped in a + # LengthWrapper. This test sets the options in such a way that much + # of _upload_object_job is skipped bringing the critical path down + # to around 60 lines to ease testing. + with tempfile.NamedTemporaryFile() as f: + f.write(b'a' * 30) + f.flush() + expected_r = { + 'action': 'upload_object', + 'attempts': 2, + 'container': 'test_c', + 'headers': {}, + 'large_object': True, + 'object': 'テスト/dummy.dat', + 'manifest_response_dict': {}, + 'segment_results': [ + {'action': 'upload_segment', + 'success': True, + 'segment_index': i, + 'segment_location': 'test_c+segments/slo/%d' % i, + 'segment_etag': 'part-etag', + 'segment_size': 1000 + i} + for i in range(3)], + 'status': 'uploaded', + 'success': True, + } + expected_mtime = '%f' % os.path.getmtime(f.name) + + mock_conn = mock.Mock() + mock_conn.put_object.return_value = '' + type(mock_conn).attempts = mock.PropertyMock(return_value=2) + + s = SwiftService() + with mock.patch.object(s, '_upload_segment_job') as mock_job: + mock_job.side_effect = [ + {'action': 'upload_segment', + 'success': True, + 'segment_index': i, + 'segment_location': 'test_c+segments/slo/%d' % i, + 'segment_etag': 'part-etag', + 'segment_size': 1000 + i} + for i in range(3)] + + r = s._upload_object_job(conn=mock_conn, + container='test_c', + source=f.name, + obj='テスト/dummy.dat', + options=dict(s._options, + segment_size=10, + leave_segments=True, + use_slo=True)) + + mtime = r['headers']['x-object-meta-mtime'] + self.assertEqual(expected_mtime, mtime) + del r['headers']['x-object-meta-mtime'] + + self.assertEqual(r['path'], f.name) + del r['path'] + + self.assertEqual(r, expected_r) + self.assertEqual(mock_conn.put_object.call_count, 1) + mock_conn.put_object.assert_called_with( + 'test_c', + 'テスト/dummy.dat', + ' '.join([ + '[{"path": "test_c+segments/slo/0",', + '"etag": "part-etag", "size_bytes": 1000},', + '{"path": "test_c+segments/slo/1",', + '"etag": "part-etag", "size_bytes": 1001},', + '{"path": "test_c+segments/slo/2",', + '"etag": "part-etag", "size_bytes": 1002}]', + ]), + query_string='multipart-manifest=put', + headers={}, + response_dict={}, + ) + def test_upload_segment_job(self): with tempfile.NamedTemporaryFile() as f: f.write(b'a' * 10) diff --git a/test/unit/test_shell.py b/test/unit/test_shell.py index 98d73e9..d494167 100644 --- a/test/unit/test_shell.py +++ b/test/unit/test_shell.py @@ -861,7 +861,8 @@ class TestShell(unittest.TestCase): # Upload in segments connection.return_value.head_container.return_value = { 'x-storage-policy': 'one'} - argv = ["", "upload", "container", self.tmpfile, "-S", "10"] + argv = ["", "upload", "container", self.tmpfile, + "-S", "10", "--use-dlo"] with open(self.tmpfile, "wb") as fh: fh.write(b'12345678901234567890') swiftclient.shell.main(argv) @@ -907,6 +908,62 @@ class TestShell(unittest.TestCase): query_string='multipart-manifest=put', response_dict=mock.ANY) + # detect no SLO + connection.reset_mock() + connection.return_value.head_container.return_value = { + 'x-storage-policy': 'one'} + argv = ["", "upload", "container/pseudo-folder/nested", + self.tmpfile, "-S", "10"] + with open(self.tmpfile, "wb") as fh: + fh.write(b'12345678901234567890') + connection.return_value.get_capabilities.return_value = {} + swiftclient.shell.main(argv) + expected_calls = [mock.call('container', + {}, + response_dict={}), + mock.call('container_segments', + {'X-Storage-Policy': 'one'}, + response_dict={})] + connection.return_value.put_container.assert_has_calls(expected_calls) + connection.return_value.put_object.assert_called_with( + 'container', + 'pseudo-folder/nested' + self.tmpfile, + '', + content_length=0, + headers={'x-object-manifest': mock.ANY, + 'x-object-meta-mtime': mock.ANY}, + response_dict={}) + + # detect SLO + connection.reset_mock() + connection.return_value.head_container.return_value = { + 'x-storage-policy': 'one'} + argv = ["", "upload", "container/pseudo-folder/nested", + self.tmpfile, "-S", "10"] + with open(self.tmpfile, "wb") as fh: + fh.write(b'12345678901234567890') + connection.return_value.get_capabilities.return_value = { + 'slo': {}, + } + swiftclient.shell.main(argv) + expected_calls = [mock.call('container', + {}, + response_dict={}), + mock.call('container_segments', + {'X-Storage-Policy': 'one'}, + response_dict={})] + print(connection.return_value.mock_calls) + connection.return_value.put_container.assert_has_calls(expected_calls) + connection.return_value.put_object.assert_called_with( + 'container', + 'pseudo-folder/nested' + self.tmpfile, + mock.ANY, + headers={ + 'x-object-meta-mtime': mock.ANY, + }, + 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): @@ -934,7 +991,7 @@ class TestShell(unittest.TestCase): connection.return_value.head_container.return_value = { 'x-storage-policy': 'one'} argv = ["", "upload", "container", "--skip-container-put", - self.tmpfile, "-S", "10"] + self.tmpfile, "-S", "10", "--use-dlo"] with open(self.tmpfile, "wb") as fh: fh.write(b'12345678901234567890') swiftclient.shell.main(argv) @@ -1187,7 +1244,7 @@ class TestShell(unittest.TestCase): connection.return_value.attempts = 0 connection.return_value.put_object.return_value = EMPTY_ETAG argv = ["", "upload", "container", self.tmpfile, "-S", "10", - "-C", "container"] + "-C", "container", "--use-dlo"] with open(self.tmpfile, "wb") as fh: fh.write(b'12345678901234567890') swiftclient.shell.main(argv) @@ -3560,6 +3617,7 @@ class TestCrossAccountObjectAccess(TestBase, MockHttpTest): args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj, + '--use-dlo', '--leave-segments', '--segment-size=10', '--segment-container=%s' @@ -3600,6 +3658,7 @@ class TestCrossAccountObjectAccess(TestBase, MockHttpTest): args, env = self._make_cmd('upload', cmd_args=[self.cont, self.obj, + '--use-dlo', '--leave-segments', '--segment-size=10']) with mock.patch('swiftclient.client.ksclient_v3', self.fake_ks): -- cgit v1.2.1