From 14981b47c5d5904b8355a5b1bf9899dd0460b3c5 Mon Sep 17 00:00:00 2001 From: Naoto Nishizono Date: Thu, 12 Feb 2015 15:22:37 +0900 Subject: [PATCH] Fix DELETE Object to delete segments when it is multipart object When deleting an object created via Multipart Upload, delete both the manifest file and the segments by adding "multipart-manifest=delete" to the query string. This requires an additional HEAD before each DELETE, which adds a good bit of overhead. There is a new config option, "allow_multipart_uploads" which operators may turn off to avoid this overhead if they know their use-case does not require Multipart Uploads. Co-Authored-By: Tim Burke Change-Id: Ie1889750b0e6fbe48af0da40596f09ed504b9099 Closes-Bug: #1420144 --- etc/proxy-server.conf-sample | 6 ++++ swift3/acl_handlers.py | 5 ++++ swift3/cfg.py | 1 + swift3/controllers/obj.py | 10 +++++-- swift3/middleware.py | 9 +++--- swift3/request.py | 8 +++++ swift3/response.py | 4 +++ swift3/test/unit/test_obj.py | 53 ++++++++++++++++++++++++++++++++- swift3/test/unit/test_s3_acl.py | 4 ++- 9 files changed, 92 insertions(+), 8 deletions(-) diff --git a/etc/proxy-server.conf-sample b/etc/proxy-server.conf-sample index f57b8bbc..9cd2f66d 100644 --- a/etc/proxy-server.conf-sample +++ b/etc/proxy-server.conf-sample @@ -87,6 +87,12 @@ use = egg:swift3#swift3 # middlewares in order to use other 3rd party (or your proprietary) authenticate middleware. # auth_pipeline_check = True # +# Enable multi-part uploads. (default: true) +# This is required to store files larger than Swift's max_file_size (by default, 5GiB). +# Note that has performance implications when deleting objects, as we now have to +# check for whether there are also segments to delete. +# allow_multipart_uploads = True +# # Set the maximum number of parts for Upload Part operation.(default: 1000) # When setting it to be larger than the default value in order to match the # specification of S3, set to be larger max_manifest_segments for slo diff --git a/swift3/acl_handlers.py b/swift3/acl_handlers.py index 835d4dac..afbad9ab 100644 --- a/swift3/acl_handlers.py +++ b/swift3/acl_handlers.py @@ -178,6 +178,11 @@ class ObjectAclHandler(BaseAclHandler): """ ObjectAclHandler: Handler for ObjectController """ + def HEAD(self, app): + # No check object permission needed at DELETE Object + if self.method != 'DELETE': + return self._handle_acl(app, 'HEAD') + def PUT(self, app): b_resp = self._handle_acl(app, 'HEAD', obj='') req_acl = ACL.from_headers(self.req.headers, diff --git a/swift3/cfg.py b/swift3/cfg.py index 809007c1..c474e19f 100644 --- a/swift3/cfg.py +++ b/swift3/cfg.py @@ -63,4 +63,5 @@ CONF = Config({ 'max_upload_part_num': 1000, 'check_bucket_owner': False, 'force_swift_request_proxy_log': False, + 'allow_multipart_uploads': True, }) diff --git a/swift3/controllers/obj.py b/swift3/controllers/obj.py index 7aa5f949..2e0ec9c4 100644 --- a/swift3/controllers/obj.py +++ b/swift3/controllers/obj.py @@ -15,7 +15,7 @@ import sys -from swift.common.http import HTTP_OK, HTTP_PARTIAL_CONTENT +from swift.common.http import HTTP_OK, HTTP_PARTIAL_CONTENT, HTTP_NO_CONTENT from swift.common.swob import Range, content_range_header_value from swift3.controllers.base import Controller @@ -113,7 +113,13 @@ class ObjectController(Controller): Handle DELETE Object request """ try: - resp = req.get_response(self.app) + query = req.gen_multipart_manifest_delete_query(self.app) + resp = req.get_response(self.app, query=query) + if query and resp.status_int == HTTP_OK: + for chunk in resp.app_iter: + pass # drain the bulk-deleter response + resp.status = HTTP_NO_CONTENT + resp.body = '' except NoSuchKey: # expect to raise NoSuchBucket when the bucket doesn't exist exc_type, exc_value, exc_traceback = sys.exc_info() diff --git a/swift3/middleware.py b/swift3/middleware.py index 3f940589..34db0687 100644 --- a/swift3/middleware.py +++ b/swift3/middleware.py @@ -69,7 +69,7 @@ class Swift3Middleware(object): """Swift3 S3 compatibility midleware""" def __init__(self, app, conf, *args, **kwargs): self.app = app - self.slo_enabled = True + self.slo_enabled = conf['allow_multipart_uploads'] self.check_pipeline(conf) def __call__(self, env, start_response): @@ -126,9 +126,9 @@ class Swift3Middleware(object): pipeline.index('proxy-server')] # Check SLO middleware - if 'slo' not in auth_pipeline: + if self.slo_enabled and 'slo' not in auth_pipeline: self.slo_enabled = False - LOGGER.warning('swift3 middleware is required SLO middleware ' + LOGGER.warning('swift3 middleware requires SLO middleware ' 'to support multi-part upload, please add it ' 'in pipline') @@ -181,7 +181,8 @@ def filter_factory(global_conf, **local_conf): max_bucket_listing=CONF['max_bucket_listing'], max_parts_listing=CONF['max_parts_listing'], max_upload_part_num=CONF['max_upload_part_num'], - max_multi_delete_objects=CONF['max_multi_delete_objects'] + max_multi_delete_objects=CONF['max_multi_delete_objects'], + allow_multipart_uploads=CONF['allow_multipart_uploads'], ) def swift3_filter(app): diff --git a/swift3/request.py b/swift3/request.py index 5dbb65fd..d99c3a89 100644 --- a/swift3/request.py +++ b/swift3/request.py @@ -539,6 +539,7 @@ class Request(swob.Request): HTTP_ACCEPTED, ], 'DELETE': [ + HTTP_OK, HTTP_NO_CONTENT, ], } @@ -737,6 +738,13 @@ class Request(swob.Request): return headers_to_container_info( resp.sw_headers, resp.status_int) # pylint: disable-msg=E1101 + def gen_multipart_manifest_delete_query(self, app): + if not CONF.allow_multipart_uploads: + return None + query = {'multipart-manifest': 'delete'} + resp = self.get_response(app, 'HEAD') + return query if resp.is_slo else None + class S3AclRequest(Request): """ diff --git a/swift3/response.py b/swift3/response.py index cd530ed5..ee44fd13 100644 --- a/swift3/response.py +++ b/swift3/response.py @@ -84,6 +84,7 @@ class Response(ResponseBase, swob.Response): sw_sysmeta_headers = swob.HeaderKeyDict() sw_headers = swob.HeaderKeyDict() headers = HeaderKeyDict() + self.is_slo = False for key, val in self.headers.iteritems(): _key = key.lower() @@ -103,6 +104,9 @@ class Response(ResponseBase, swob.Response): 'content-range', 'content-encoding', 'etag', 'last-modified'): headers[key] = val + elif _key == 'x-static-large-object': + # for delete slo + self.is_slo = val self.headers = headers # Used for pure swift header handling at the request layer diff --git a/swift3/test/unit/test_obj.py b/swift3/test/unit/test_obj.py index 04f4c1b6..ca6342d9 100644 --- a/swift3/test/unit/test_obj.py +++ b/swift3/test/unit/test_obj.py @@ -602,13 +602,64 @@ class TestSwift3Obj(Swift3TestCase): self.assertEquals(code, 'NoSuchBucket') @s3acl - def test_object_DELETE(self): + @patch('swift3.cfg.CONF.allow_multipart_uploads', False) + def test_object_DELETE_no_multipart(self): req = Request.blank('/bucket/object', environ={'REQUEST_METHOD': 'DELETE'}, headers={'Authorization': 'AWS test:tester:hmac'}) status, headers, body = self.call_swift3(req) self.assertEquals(status.split()[0], '204') + self.assertNotIn(('HEAD', '/v1/AUTH_test/bucket/object'), + self.swift.calls) + self.assertIn(('DELETE', '/v1/AUTH_test/bucket/object'), + self.swift.calls) + _, path = self.swift.calls[-1] + self.assertEquals(path.count('?'), 0) + + @s3acl + def test_object_DELETE_multipart(self): + req = Request.blank('/bucket/object', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'Authorization': 'AWS test:tester:hmac'}) + status, headers, body = self.call_swift3(req) + self.assertEquals(status.split()[0], '204') + + self.assertIn(('HEAD', '/v1/AUTH_test/bucket/object'), + self.swift.calls) + self.assertIn(('DELETE', '/v1/AUTH_test/bucket/object'), + self.swift.calls) + _, path = self.swift.calls[-1] + self.assertEquals(path.count('?'), 0) + + @s3acl + def test_slo_object_DELETE(self): + self.swift.register('HEAD', '/v1/AUTH_test/bucket/object', + swob.HTTPOk, + {'x-static-large-object': 'True'}, + None) + self.swift.register('DELETE', '/v1/AUTH_test/bucket/object', + swob.HTTPOk, {}, '') + req = Request.blank('/bucket/object', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'Authorization': 'AWS test:tester:hmac'}) + status, headers, body = self.call_swift3(req) + self.assertEqual(status.split()[0], '204') + self.assertEqual(body, '') + + self.assertIn(('HEAD', '/v1/AUTH_test/bucket/object'), + self.swift.calls) + self.assertIn(('DELETE', '/v1/AUTH_test/bucket/object' + '?multipart-manifest=delete'), + self.swift.calls) + _, path = self.swift.calls[-1] + path, query_string = path.split('?', 1) + query = {} + for q in query_string.split('&'): + key, arg = q.split('=') + query[key] = arg + self.assertEquals(query['multipart-manifest'], 'delete') + def _test_object_for_s3acl(self, method, account): req = Request.blank('/bucket/object', environ={'REQUEST_METHOD': method}, diff --git a/swift3/test/unit/test_s3_acl.py b/swift3/test/unit/test_s3_acl.py index 4d9fc417..e9bae89a 100644 --- a/swift3/test/unit/test_s3_acl.py +++ b/swift3/test/unit/test_s3_acl.py @@ -70,12 +70,14 @@ def s3acl(func=None, s3acl_only=False): message += failing_point raise exc_type(message) + instance = args[0] + if not s3acl_only: call_func() + instance.swift._calls = [] with patch('swift3.cfg.CONF.s3_acl', True): owner = Owner('test:tester', 'test:tester') - instance = args[0] generate_s3acl_environ('test', instance.swift, owner) call_func(' (fail at s3_acl)')