From f4cff9fbba76932a4b67f1130d630098497bb4bc Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 17 Jan 2018 22:07:26 +0000 Subject: [PATCH] object-server can 409 in response to x-if-delete-at Previously, if the expirer had a stale work item (because the object was overwritten or deleted, or some other process handled the delete), then it would keep retrying for reclaim_age, but every time it'd get back a 412. Now, have the object-server be smart enough to say, "I have more recent information than you" and let the expirer accept that as success. Change-Id: I0a94482ed16cb30ce79074e053e6177fe97bcaa9 --- swift/obj/expirer.py | 7 +++++-- swift/obj/server.py | 3 +++ test/unit/obj/test_expirer.py | 15 +++++++++------ test/unit/obj/test_server.py | 22 ++++++++++++++++++++-- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/swift/obj/expirer.py b/swift/obj/expirer.py index 67b8e93d79..2e9ae09e18 100644 --- a/swift/obj/expirer.py +++ b/swift/obj/expirer.py @@ -306,10 +306,13 @@ class ObjectExpirer(Daemon): '//' :param timestamp: The timestamp the X-Delete-At value must match to perform the actual delete. + :raises UnexpectedResponse: if the delete was unsuccessful and + should be retried later """ path = '/v1/' + urllib.parse.quote(actual_obj.lstrip('/')) self.swift.make_request( 'DELETE', path, - {'X-If-Delete-At': str(timestamp), 'X-Timestamp': str(timestamp), + {'X-If-Delete-At': str(timestamp), + 'X-Timestamp': str(timestamp), 'X-Backend-Clean-Expiring-Object-Queue': 'no'}, - (2,)) + (2, HTTP_CONFLICT)) diff --git a/swift/obj/server.py b/swift/obj/server.py index 30c147e6c3..788480d7b1 100644 --- a/swift/obj/server.py +++ b/swift/obj/server.py @@ -1076,6 +1076,9 @@ class ObjectController(BaseStorageServer): if not orig_timestamp: # no object found at all return HTTPNotFound() + if orig_timestamp >= req_timestamp: + # Found a newer object -- return 409 as work item is stale + return HTTPConflict() if orig_delete_at != req_if_delete_at: return HTTPPreconditionFailed( request=request, diff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py index 7bf6e29bcd..5b1bb913ae 100644 --- a/test/unit/obj/test_expirer.py +++ b/test/unit/obj/test_expirer.py @@ -742,7 +742,7 @@ class TestObjectExpirer(TestCase): self.assertEqual(got_env[0]['PATH_INFO'], '/v1/path/to/object name') def test_delete_actual_object_returns_expected_error(self): - def do_test(test_status): + def do_test(test_status, should_raise): calls = [0] def fake_app(env, start_response): @@ -753,18 +753,21 @@ class TestObjectExpirer(TestCase): internal_client.loadapp = lambda *a, **kw: fake_app x = expirer.ObjectExpirer({}) - self.assertRaises(internal_client.UnexpectedResponse, - x.delete_actual_object, '/path/to/object', - '1234') + if should_raise: + with self.assertRaises(internal_client.UnexpectedResponse): + x.delete_actual_object('/path/to/object', '1234') + else: + x.delete_actual_object('/path/to/object', '1234') self.assertEqual(calls[0], 1) # object was deleted and tombstone reaped - do_test('404 Not Found') + do_test('404 Not Found', True) # object was overwritten *after* the original expiration, or + do_test('409 Conflict', False) # object was deleted but tombstone still exists, or # object was overwritten ahead of the original expiration, or # object was POSTed to with a new (or no) expiration, or ... - do_test('412 Precondition Failed') + do_test('412 Precondition Failed', True) def test_delete_actual_object_does_not_handle_odd_stuff(self): diff --git a/test/unit/obj/test_server.py b/test/unit/obj/test_server.py index ef5c63beea..5c308f1796 100644 --- a/test/unit/obj/test_server.py +++ b/test/unit/obj/test_server.py @@ -6146,14 +6146,32 @@ class TestObjectController(unittest.TestCase): self.assertFalse(os.path.isfile(objfile)) # make the x-if-delete-at with all the right bits (again) + req = Request.blank( + '/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, + headers={'X-Timestamp': delete_at_timestamp, + 'X-If-Delete-At': delete_at_timestamp}) + resp = req.get_response(self.object_controller) + self.assertEqual(resp.status_int, 409) + self.assertFalse(os.path.isfile(objfile)) + + # overwrite with new content + req = Request.blank( + '/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'PUT'}, + headers={ + 'X-Timestamp': str(test_time + 100), + 'Content-Length': '0', + 'Content-Type': 'application/octet-stream'}) + resp = req.get_response(self.object_controller) + self.assertEqual(resp.status_int, 201, resp.body) + + # simulate processing a stale expirer queue entry req = Request.blank( '/sda1/p/a/c/o', environ={'REQUEST_METHOD': 'DELETE'}, headers={'X-Timestamp': delete_at_timestamp, 'X-If-Delete-At': delete_at_timestamp}) resp = req.get_response(self.object_controller) - self.assertEqual(resp.status_int, 412) - self.assertFalse(os.path.isfile(objfile)) + self.assertEqual(resp.status_int, 409) # make the x-if-delete-at for some not found req = Request.blank(