Skip to content

Commit

Permalink
Fix handling of DELETE obj reqs with old timestamp
Browse files Browse the repository at this point in the history 
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.

We also prevent PUT and POST operations using old timestamps as well.

Change-Id: I631957029d17c6578bca5779367df5144ba01fc9
Signed-off-by: Peter Portante <peter.portante@redhat.com>
  • Loading branch information
@portante @ttx
portante authored and ttx committed Aug 7, 2013
1 parent 6659382 commit 1f4ec23
Show file tree
Hide file tree
Showing 2 changed files with 225 additions and 27 deletions.
58 changes: 34 additions & 24 deletions swift/obj/server.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
HTTPInternalServerError, HTTPNoContent, HTTPNotFound, HTTPNotModified, \
HTTPPreconditionFailed, HTTPRequestTimeout, HTTPUnprocessableEntity, \
HTTPClientDisconnect, HTTPMethodNotAllowed, Request, Response, UTC, \
HTTPInsufficientStorage, multi_range_iterator
HTTPInsufficientStorage, multi_range_iterator, HTTPConflict


DATADIR = 'objects'
Expand Down Expand Up @@ -121,7 +121,6 @@ def __init__(self, path, device, partition, account, container, obj,
self.tmppath = None
self.logger = logger
self.metadata = {}
self.meta_file = None
self.data_file = None
self.fp = None
self.iter_etag = None
Expand All @@ -133,24 +132,27 @@ def __init__(self, path, device, partition, account, container, obj,
if not os.path.exists(self.datadir):
return
files = sorted(os.listdir(self.datadir), reverse=True)
for file in files:
if file.endswith('.ts'):
self.data_file = self.meta_file = None
self.metadata = {'deleted': True}
return
if file.endswith('.meta') and not self.meta_file:
self.meta_file = os.path.join(self.datadir, file)
if file.endswith('.data') and not self.data_file:
self.data_file = os.path.join(self.datadir, file)
meta_file = None
for afile in files:
if afile.endswith('.ts'):
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
if not self.data_file:
return
self.fp = open(self.data_file, 'rb')
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]
Expand Down Expand Up @@ -594,6 +596,9 @@ def POST(self, request):
except (DiskFileError, DiskFileNotExist):
file.quarantine()
return HTTPNotFound(request=request)
orig_timestamp = file.metadata.get('X-Timestamp', '0')
if orig_timestamp >= request.headers['x-timestamp']:
return HTTPConflict(request=request)
metadata = {'X-Timestamp': request.headers['x-timestamp']}
metadata.update(val for val in request.headers.iteritems()
if val[0].lower().startswith('x-object-meta-'))
Expand Down Expand Up @@ -639,6 +644,8 @@ def PUT(self, request):
file = DiskFile(self.devices, device, partition, account, container,
obj, self.logger, disk_chunk_size=self.disk_chunk_size)
orig_timestamp = file.metadata.get('X-Timestamp')
if orig_timestamp and orig_timestamp >= request.headers['x-timestamp']:
return HTTPConflict(request=request)
upload_expiration = time.time() + self.max_upload_time
etag = md5()
upload_size = 0
Expand Down Expand Up @@ -863,23 +870,26 @@ def DELETE(self, request):
return HTTPPreconditionFailed(
request=request,
body='X-If-Delete-At and X-Delete-At do not match')
orig_timestamp = file.metadata.get('X-Timestamp')
if file.is_deleted() or file.is_expired():
response_class = HTTPNotFound
metadata = {
'X-Timestamp': request.headers['X-Timestamp'], 'deleted': True,
}
old_delete_at = int(file.metadata.get('X-Delete-At') or 0)
if old_delete_at:
self.delete_at_update('DELETE', old_delete_at, account,
container, obj, request.headers, device)
file.put_metadata(metadata, tombstone=True)
file.unlinkold(metadata['X-Timestamp'])
if not orig_timestamp or \
orig_timestamp < request.headers['x-timestamp']:
orig_timestamp = file.metadata.get('X-Timestamp', 0)
req_timestamp = request.headers['X-Timestamp']
if file.is_deleted() or file.is_expired():
response_class = HTTPNotFound
else:
if orig_timestamp < req_timestamp:
response_class = HTTPNoContent
else:
response_class = HTTPConflict
if orig_timestamp < req_timestamp:
file.put_metadata({'X-Timestamp': req_timestamp},
tombstone=True)
file.unlinkold(req_timestamp)
self.container_update(
'DELETE', account, container, obj, request.headers,
{'x-timestamp': metadata['X-Timestamp'],
{'x-timestamp': req_timestamp,
'x-trans-id': request.headers.get('x-trans-id', '-')},
device)
resp = response_class(request=request)
Expand Down
194 changes: 191 additions & 3 deletions test/unit/obj/test_server.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,41 @@ def test_POST_update_meta(self):
"X-Object-Meta-3" in resp.headers)
self.assertEquals(resp.headers['Content-Type'], 'application/x-test')

def test_POST_old_timestamp(self):
ts = time()
timestamp = normalize_timestamp(ts)
req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
headers={'X-Timestamp': timestamp,
'Content-Type': 'application/x-test',
'X-Object-Meta-1': 'One',
'X-Object-Meta-Two': 'Two'})
req.body = 'VERIFY'
resp = self.object_controller.PUT(req)
self.assertEquals(resp.status_int, 201)

# Same timestamp should result in 409
req = Request.blank('/sda1/p/a/c/o',
environ={'REQUEST_METHOD': 'POST'},
headers={'X-Timestamp': timestamp,
'X-Object-Meta-3': 'Three',
'X-Object-Meta-4': 'Four',
'Content-Encoding': 'gzip',
'Content-Type': 'application/x-test'})
resp = self.object_controller.POST(req)
self.assertEquals(resp.status_int, 409)

# Earlier timestamp should result in 409
timestamp = normalize_timestamp(ts - 1)
req = Request.blank('/sda1/p/a/c/o',
environ={'REQUEST_METHOD': 'POST'},
headers={'X-Timestamp': timestamp,
'X-Object-Meta-5': 'Five',
'X-Object-Meta-6': 'Six',
'Content-Encoding': 'gzip',
'Content-Type': 'application/x-test'})
resp = self.object_controller.POST(req)
self.assertEquals(resp.status_int, 409)

def test_POST_not_exist(self):
timestamp = normalize_timestamp(time())
req = Request.blank('/sda1/p/a/c/fail',
Expand Down Expand Up @@ -555,11 +590,15 @@ def read(self, amt=None):

old_http_connect = object_server.http_connect
try:
timestamp = normalize_timestamp(time())
ts = time()
timestamp = normalize_timestamp(ts)
req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD':
'POST'}, headers={'X-Timestamp': timestamp, 'Content-Type':
'text/plain', 'Content-Length': '0'})
resp = self.object_controller.PUT(req)
self.assertEquals(resp.status_int, 201)

timestamp = normalize_timestamp(ts + 1)
req = Request.blank('/sda1/p/a/c/o',
environ={'REQUEST_METHOD': 'POST'},
headers={'X-Timestamp': timestamp,
Expand All @@ -571,6 +610,8 @@ def read(self, amt=None):
object_server.http_connect = mock_http_connect(202)
resp = self.object_controller.POST(req)
self.assertEquals(resp.status_int, 202)

timestamp = normalize_timestamp(ts + 2)
req = Request.blank('/sda1/p/a/c/o',
environ={'REQUEST_METHOD': 'POST'},
headers={'X-Timestamp': timestamp,
Expand All @@ -582,6 +623,8 @@ def read(self, amt=None):
object_server.http_connect = mock_http_connect(202, with_exc=True)
resp = self.object_controller.POST(req)
self.assertEquals(resp.status_int, 202)

timestamp = normalize_timestamp(ts + 3)
req = Request.blank('/sda1/p/a/c/o',
environ={'REQUEST_METHOD': 'POST'},
headers={'X-Timestamp': timestamp,
Expand Down Expand Up @@ -718,6 +761,32 @@ def test_PUT_overwrite(self):
'name': '/a/c/o',
'Content-Encoding': 'gzip'})

def test_PUT_old_timestamp(self):
ts = time()
req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
headers={'X-Timestamp': normalize_timestamp(ts),
'Content-Length': '6',
'Content-Type': 'application/octet-stream'})
req.body = 'VERIFY'
resp = self.object_controller.PUT(req)
self.assertEquals(resp.status_int, 201)

req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
headers={'X-Timestamp': normalize_timestamp(ts),
'Content-Type': 'text/plain',
'Content-Encoding': 'gzip'})
req.body = 'VERIFY TWO'
resp = self.object_controller.PUT(req)
self.assertEquals(resp.status_int, 409)

req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
headers={'X-Timestamp': normalize_timestamp(ts - 1),
'Content-Type': 'text/plain',
'Content-Encoding': 'gzip'})
req.body = 'VERIFY THREE'
resp = self.object_controller.PUT(req)
self.assertEquals(resp.status_int, 409)

def test_PUT_no_etag(self):
req = Request.blank('/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
headers={'X-Timestamp': normalize_timestamp(time()),
Expand Down Expand Up @@ -1306,12 +1375,32 @@ def test_DELETE(self):
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.exists(objfile))
self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1)

sleep(.00001)
timestamp = normalize_timestamp(time())
Expand All @@ -1325,17 +1414,19 @@ def test_DELETE(self):
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.exists(objfile))
self.assertEquals(len(os.listdir(os.path.dirname(objfile))), 1)

sleep(.00001)
timestamp = normalize_timestamp(time())
Expand All @@ -1350,6 +1441,103 @@ def test_DELETE(self):
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()
Expand Down

0 comments on commit 1f4ec23

Please sign in to comment.