From 8a543c36fb837b19b70d1c9a34a2fc42abc61325 Mon Sep 17 00:00:00 2001 From: Peter Portante Date: Tue, 2 Jul 2013 11:48:19 -0400 Subject: [PATCH 2/2] Fix handling of DELETE obj reqs with old timestamp The DELETE object REST API was creating tombstone files with old timestamps, potentially filling up the disk, as well as sending container updates. Here we now make DELETEs with a request timestamp return a 409 (HTTP Conflict) if a data file exists with a newer timestamp, only creating tombstones if they have a newer timestamp. The key fix is to actually read the timestamp metadata from an existing tombstone file (thanks to Pete Zaitcev for catching this), and then only create tombstone files with newer timestamps. Change-Id: I631957029d17c6578bca5779367df5144ba01fc9 Signed-off-by: Peter Portante --- swift/obj/server.py | 46 ++++++++-------- test/unit/obj/test_server.py | 125 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 147 insertions(+), 24 deletions(-) diff --git a/swift/obj/server.py b/swift/obj/server.py index 7dac34d..6d7384c 100644 --- a/swift/obj/server.py +++ b/swift/obj/server.py @@ -49,7 +49,7 @@ from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPCreated, \ HTTPPreconditionFailed, HTTPRequestTimeout, HTTPUnprocessableEntity, \ HTTPClientDisconnect, HTTPMethodNotAllowed, Request, Response, UTC, \ HTTPInsufficientStorage, HTTPForbidden, multi_range_iterator, \ - HeaderKeyDict + HeaderKeyDict, HTTPConflict DATADIR = 'objects' @@ -201,7 +201,6 @@ class DiskFile(object): self.tmpdir = os.path.join(path, device, 'tmp') self.logger = logger self.metadata = {} - self.meta_file = None self.data_file = None self.fp = None self.iter_etag = None @@ -214,13 +213,16 @@ class DiskFile(object): if not os.path.exists(self.datadir): return files = sorted(os.listdir(self.datadir), reverse=True) + meta_file = None for afile in files: if afile.endswith('.ts'): - self.data_file = self.meta_file = None - self.metadata = {'deleted': True} - return - if afile.endswith('.meta') and not self.meta_file: - self.meta_file = os.path.join(self.datadir, afile) + self.data_file = None + with open(os.path.join(self.datadir, afile)) as mfp: + self.metadata = read_metadata(mfp) + self.metadata['deleted'] = True + break + if afile.endswith('.meta') and not meta_file: + meta_file = os.path.join(self.datadir, afile) if afile.endswith('.data') and not self.data_file: self.data_file = os.path.join(self.datadir, afile) break @@ -230,8 +232,8 @@ class DiskFile(object): self.metadata = read_metadata(self.fp) if not keep_data_fp: self.close(verify_file=False) - if self.meta_file: - with open(self.meta_file) as mfp: + if meta_file: + with open(meta_file) as mfp: for key in self.metadata.keys(): if key.lower() not in DISALLOWED_HEADERS: del self.metadata[key] @@ -976,7 +978,6 @@ class ObjectController(object): content_type='text/plain') if self.mount_check and not check_mount(self.devices, device): return HTTPInsufficientStorage(drive=device, request=request) - response_class = HTTPNoContent disk_file = self._diskfile(device, partition, account, container, obj) if 'x-if-delete-at' in request.headers and \ int(request.headers['x-if-delete-at']) != \ @@ -984,23 +985,26 @@ class ObjectController(object): return HTTPPreconditionFailed( request=request, body='X-If-Delete-At and X-Delete-At do not match') - orig_timestamp = disk_file.metadata.get('X-Timestamp') - if disk_file.is_deleted() or disk_file.is_expired(): - response_class = HTTPNotFound - metadata = { - 'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True, - } old_delete_at = int(disk_file.metadata.get('X-Delete-At') or 0) if old_delete_at: self.delete_at_update('DELETE', old_delete_at, account, container, obj, request, device) - disk_file.put_metadata(metadata, tombstone=True) - disk_file.unlinkold(metadata['X-Timestamp']) - if not orig_timestamp or \ - orig_timestamp < request.headers['x-timestamp']: + orig_timestamp = disk_file.metadata.get('X-Timestamp', 0) + req_timestamp = request.headers['X-Timestamp'] + if disk_file.is_deleted() or disk_file.is_expired(): + response_class = HTTPNotFound + else: + if orig_timestamp < req_timestamp: + response_class = HTTPNoContent + else: + response_class = HTTPConflict + if orig_timestamp < req_timestamp: + disk_file.put_metadata({'X-Timestamp': req_timestamp}, + tombstone=True) + disk_file.unlinkold(req_timestamp) self.container_update( 'DELETE', account, container, obj, request, - HeaderKeyDict({'x-timestamp': metadata['X-Timestamp']}), + HeaderKeyDict({'x-timestamp': req_timestamp}), device) resp = response_class(request=request) return resp diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index e3f24d6..6706f5e 100755 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -1354,7 +1354,7 @@ class TestObjectController(unittest.TestCase): self.assertEquals(resp.status_int, 404) def test_DELETE(self): - """ Test swift.object_server.ObjectController.DELETE """ + # Test swift.object_server.ObjectController.DELETE req = Request.blank('/sda1/p/a/c', environ={'REQUEST_METHOD': 'DELETE'}) resp = self.object_controller.DELETE(req) @@ -1366,12 +1366,32 @@ class TestObjectController(unittest.TestCase): self.assertEquals(resp.status_int, 400) # self.assertRaises(KeyError, self.object_controller.DELETE, req) + # The following should have created a tombstone file timestamp = normalize_timestamp(time()) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, headers={'X-Timestamp': timestamp}) resp = self.object_controller.DELETE(req) self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assert_(os.path.isfile(objfile)) + + # The following should *not* have created a tombstone file. + timestamp = normalize_timestamp(float(timestamp) - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) sleep(.00001) timestamp = normalize_timestamp(time()) @@ -1385,17 +1405,19 @@ class TestObjectController(unittest.TestCase): resp = self.object_controller.PUT(req) self.assertEquals(resp.status_int, 201) + # The following should *not* have created a tombstone file. timestamp = normalize_timestamp(float(timestamp) - 1) req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, headers={'X-Timestamp': timestamp}) resp = self.object_controller.DELETE(req) - self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.status_int, 409) objfile = os.path.join(self.testdir, 'sda1', storage_directory(object_server.DATADIR, 'p', hash_path('a', 'c', 'o')), timestamp + '.ts') - self.assert_(os.path.isfile(objfile)) + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) sleep(.00001) timestamp = normalize_timestamp(time()) @@ -1410,6 +1432,103 @@ class TestObjectController(unittest.TestCase): timestamp + '.ts') self.assert_(os.path.isfile(objfile)) + def test_DELETE_container_updates(self): + # Test swift.object_server.ObjectController.DELETE and container + # updates, making sure container update is called in the correct + # state. + timestamp = normalize_timestamp(time()) + req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={ + 'X-Timestamp': timestamp, + 'Content-Type': 'application/octet-stream', + 'Content-Length': '4', + }) + req.body = 'test' + resp = self.object_controller.PUT(req) + self.assertEquals(resp.status_int, 201) + + calls_made = [0] + + def our_container_update(*args, **kwargs): + calls_made[0] += 1 + + orig_cu = self.object_controller.container_update + self.object_controller.container_update = our_container_update + try: + # The following request should return 409 (HTTP Conflict). A + # tombstone file should not have been created with this timestamp. + timestamp = normalize_timestamp(float(timestamp) - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 409) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + self.assertEquals(0, calls_made[0]) + + # The following request should return 204, and the object should + # be truly deleted (container update is performed) because this + # timestamp is newer. A tombstone file should have been created + # with this timestamp. + sleep(.00001) + timestamp = normalize_timestamp(time()) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 204) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assert_(os.path.isfile(objfile)) + self.assertEquals(1, calls_made[0]) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + + # The following request should return a 404, as the object should + # already have been deleted, but it should have also performed a + # container update because the timestamp is newer, and a tombstone + # file should also exist with this timestamp. + sleep(.00001) + timestamp = normalize_timestamp(time()) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assert_(os.path.isfile(objfile)) + self.assertEquals(2, calls_made[0]) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + + # The following request should return a 404, as the object should + # already have been deleted, and it should not have performed a + # container update because the timestamp is older, or created a + # tombstone file with this timestamp. + timestamp = normalize_timestamp(float(timestamp) - 1) + req = Request.blank('/sda1/p/a/c/o', + environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': timestamp}) + resp = self.object_controller.DELETE(req) + self.assertEquals(resp.status_int, 404) + objfile = os.path.join(self.testdir, 'sda1', + storage_directory(object_server.DATADIR, 'p', + hash_path('a', 'c', 'o')), + timestamp + '.ts') + self.assertFalse(os.path.isfile(objfile)) + self.assertEquals(2, calls_made[0]) + self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1) + finally: + self.object_controller.container_update = orig_cu + def test_call(self): """ Test swift.object_server.ObjectController.__call__ """ inbuf = StringIO() -- 1.8.1.4