From a5f4d7a40f18e0f415a0361ae932ff539babbe38 Mon Sep 17 00:00:00 2001 From: Kota Tsuyuzaki Date: Sun, 31 Jan 2016 22:36:04 -0800 Subject: [PATCH] Fix cleanup segment container This is a follow up patch for https://review.openstack.org/#/c/159130/ Current swift3 can delete bucket even if swift3 got a trouble while clenaup the segment bucket. This can cause that orphaned segment bucket remaining at inside of backend Swift. This patch achieves following things: - When failure occured during segment bucket cleanup, don't delete original bucket - If a client doesn't have the permission to delete original bucket, Nothing deleted (both original and segment bucket) Change-Id: Ied330336c717828f4f1f8abffc770b21406cb3b1 --- swift3/acl_handlers.py | 13 ++++- swift3/controllers/bucket.py | 15 +++++- swift3/test/functional/test_multi_upload.py | 54 ++++++++++++++++++++ swift3/test/unit/test_bucket.py | 56 ++++++++++++++++++--- 4 files changed, 128 insertions(+), 10 deletions(-) diff --git a/swift3/acl_handlers.py b/swift3/acl_handlers.py index 12ea08bd..2e1f2ee7 100644 --- a/swift3/acl_handlers.py +++ b/swift3/acl_handlers.py @@ -160,12 +160,23 @@ class BucketAclHandler(BaseAclHandler): def DELETE(self, app): if self.container.endswith(MULTIUPLOAD_SUFFIX): # anyways, delete multiupload container doesn't need acls + # because it depends on GET segment container result for + # cleanup pass else: return self._handle_acl(app, 'DELETE') + def HEAD(self, app): + if self.method == 'DELETE': + return self._handle_acl(app, 'DELETE') + else: + return self._handle_acl(app, 'HEAD') + def GET(self, app): - if self.method != 'DELETE': + if self.method == 'DELETE' and \ + self.container.endswith(MULTIUPLOAD_SUFFIX): + pass + else: return self._handle_acl(app, 'GET') def PUT(self, app): diff --git a/swift3/controllers/bucket.py b/swift3/controllers/bucket.py index 24d119a1..e4709124 100644 --- a/swift3/controllers/bucket.py +++ b/swift3/controllers/bucket.py @@ -41,6 +41,19 @@ class BucketController(Controller): container = req.container_name + MULTIUPLOAD_SUFFIX marker = '' seg = '' + + try: + resp = req.get_response(self.app, 'HEAD') + if int(resp.sw_headers['X-Container-Object-Count']) > 0: + raise BucketNotEmpty() + # FIXME: This extra HEAD saves unexpected segment deletion + # but if a complete multipart upload happen while cleanup + # segment container below, completed object may be missing its + # segments unfortunately. To be safer, it might be good + # to handle if the segments can be deleted for each object. + except NoSuchBucket: + pass + try: while True: # delete all segments @@ -187,9 +200,9 @@ class BucketController(Controller): """ Handle DELETE Bucket request """ - resp = req.get_response(self.app) if CONF.allow_multipart_uploads: self._delete_segments_bucket(req) + resp = req.get_response(self.app) return resp def POST(self, req): diff --git a/swift3/test/functional/test_multi_upload.py b/swift3/test/functional/test_multi_upload.py index ea2dc151..dfeb1e30 100644 --- a/swift3/test/functional/test_multi_upload.py +++ b/swift3/test/functional/test_multi_upload.py @@ -521,6 +521,60 @@ class TestSwift3MultiUpload(Swift3FunctionalTestCase): query=query) self.assertEquals(status, 200) + def test_delete_bucket_multi_upload_object_exisiting(self): + bucket = 'bucket' + keys = ['obj1'] + uploads = [] + + results_generator = self._initiate_multi_uploads_result_generator( + bucket, keys) + + # Initiate Multipart Upload + for expected_key, (status, _, body) in \ + izip(keys, results_generator): + self.assertEquals(status, 200) # sanity + elem = fromstring(body, 'InitiateMultipartUploadResult') + key = elem.find('Key').text + self.assertEquals(expected_key, key) # sanity + upload_id = elem.find('UploadId').text + self.assertTrue(upload_id is not None) # sanity + self.assertTrue((key, upload_id) not in uploads) + uploads.append((key, upload_id)) + + self.assertEquals(len(uploads), len(keys)) # sanity + + # Upload Part + key, upload_id = uploads[0] + content = 'a' * MIN_SEGMENT_SIZE + status, headers, body = \ + self._upload_part(bucket, key, upload_id, content) + self.assertEquals(status, 200) + + # Complete Multipart Upload + key, upload_id = uploads[0] + etags = [md5(content).hexdigest()] + xml = self._gen_comp_xml(etags) + status, headers, body = \ + self._complete_multi_upload(bucket, key, upload_id, xml) + self.assertEquals(status, 200) # sanity + + # GET multipart object + status, headers, body = \ + self.conn.make_request('GET', bucket, key) + self.assertEquals(status, 200) # sanity + self.assertEquals(content, body) # sanity + + # DELETE bucket while the object existing + status, headers, body = \ + self.conn.make_request('DELETE', bucket) + self.assertEquals(status, 409) # sanity + + # The object must still be there. + status, headers, body = \ + self.conn.make_request('GET', bucket, key) + self.assertEquals(status, 200) # sanity + self.assertEquals(content, body) # sanity + if __name__ == '__main__': unittest.main() diff --git a/swift3/test/unit/test_bucket.py b/swift3/test/unit/test_bucket.py index e9605b5f..f8db0e89 100644 --- a/swift3/test/unit/test_bucket.py +++ b/swift3/test/unit/test_bucket.py @@ -74,7 +74,6 @@ class TestSwift3Bucket(Swift3TestCase): def setUp(self): super(TestSwift3Bucket, self).setUp() - self.setup_objects() def test_bucket_HEAD(self): @@ -434,23 +433,34 @@ class TestSwift3Bucket(Swift3TestCase): status, headers, body = self.call_swift3(req) self.assertEquals(self._get_error_code(body), 'MalformedXML') + def _test_method_error_delete(self, path, sw_resp): + self.swift.register('HEAD', '/v1/AUTH_test' + path, sw_resp, {}, None) + return self._test_method_error('DELETE', path, sw_resp) + @s3acl def test_bucket_DELETE_error(self): - code = self._test_method_error('DELETE', '/bucket', - swob.HTTPUnauthorized) + code = self._test_method_error_delete('/bucket', swob.HTTPUnauthorized) self.assertEquals(code, 'SignatureDoesNotMatch') - code = self._test_method_error('DELETE', '/bucket', swob.HTTPForbidden) + code = self._test_method_error_delete('/bucket', swob.HTTPForbidden) self.assertEquals(code, 'AccessDenied') - code = self._test_method_error('DELETE', '/bucket', swob.HTTPNotFound) + code = self._test_method_error_delete('/bucket', swob.HTTPNotFound) self.assertEquals(code, 'NoSuchBucket') + code = self._test_method_error_delete('/bucket', swob.HTTPServerError) + self.assertEquals(code, 'InternalError') + + # bucket not empty is now validated at swift3 + self.swift.register('HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, + {'X-Container-Object-Count': '1'}, None) code = self._test_method_error('DELETE', '/bucket', swob.HTTPConflict) self.assertEquals(code, 'BucketNotEmpty') - code = self._test_method_error('DELETE', '/bucket', - swob.HTTPServerError) - self.assertEquals(code, 'InternalError') @s3acl def test_bucket_DELETE(self): + # overwrite default HEAD to return x-container-object-count + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, + {'X-Container-Object-Count': 0}, None) + req = Request.blank('/bucket', environ={'REQUEST_METHOD': 'DELETE'}, headers={'Authorization': 'AWS test:tester:hmac', @@ -458,6 +468,27 @@ class TestSwift3Bucket(Swift3TestCase): status, headers, body = self.call_swift3(req) self.assertEquals(status.split()[0], '204') + @s3acl + def test_bucket_DELETE_error_while_segment_bucket_delete(self): + # An error occured while deleting segment objects + self.swift.register('DELETE', '/v1/AUTH_test/bucket+segments/lily', + swob.HTTPServiceUnavailable, {}, json.dumps([])) + # overwrite default HEAD to return x-container-object-count + self.swift.register( + 'HEAD', '/v1/AUTH_test/bucket', swob.HTTPNoContent, + {'X-Container-Object-Count': 0}, None) + + req = Request.blank('/bucket', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header()}) + status, headers, body = self.call_swift3(req) + self.assertEquals(status.split()[0], '503') + called = [(method, path) for method, path, _ in + self.swift.calls_with_headers] + # Don't delete original bucket when error occured in segment container + self.assertNotIn(('DELETE', '/v1/AUTH_test/bucket'), called) + def _test_bucket_for_s3acl(self, method, account): req = Request.blank('/bucket', environ={'REQUEST_METHOD': method}, @@ -514,18 +545,27 @@ class TestSwift3Bucket(Swift3TestCase): status, headers, body = self._test_bucket_for_s3acl('DELETE', 'test:other') self.assertEquals(self._get_error_code(body), 'AccessDenied') + # Don't delete anything in backend Swift + called = [method for method, _, _ in self.swift.calls_with_headers] + self.assertNotIn('DELETE', called) @s3acl(s3acl_only=True) def test_bucket_DELETE_with_write_permission(self): status, headers, body = self._test_bucket_for_s3acl('DELETE', 'test:write') self.assertEquals(self._get_error_code(body), 'AccessDenied') + # Don't delete anything in backend Swift + called = [method for method, _, _ in self.swift.calls_with_headers] + self.assertNotIn('DELETE', called) @s3acl(s3acl_only=True) def test_bucket_DELETE_with_fullcontrol_permission(self): status, headers, body = \ self._test_bucket_for_s3acl('DELETE', 'test:full_control') self.assertEquals(self._get_error_code(body), 'AccessDenied') + # Don't delete anything in backend Swift + called = [method for method, _, _ in self.swift.calls_with_headers] + self.assertNotIn('DELETE', called) if __name__ == '__main__': unittest.main()