From 45a5ecc8a148a0f1c494d88b5d982cf236be489f Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 13 Nov 2020 10:59:58 -0800 Subject: [PATCH] Have expirer use IC's delete_object That way, it properly drains the response and we'll see logs/stats for 404s and 409s. Change-Id: I1d5c6e40e1833c53994f8e15b4850871789413ac --- swift/common/internal_client.py | 21 +++++++++------------ swift/obj/expirer.py | 6 +++--- test/unit/obj/test_expirer.py | 23 ++++++++++++----------- 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py index 58b06d1cb5..89a26175a8 100644 --- a/swift/common/internal_client.py +++ b/swift/common/internal_client.py @@ -31,7 +31,7 @@ from swift.common.http import (HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES, is_client_error, is_server_error) from swift.common.request_helpers import USE_REPLICATION_NETWORK_HEADER from swift.common.swob import Request, bytes_to_wsgi -from swift.common.utils import quote, closing_if_possible +from swift.common.utils import quote, close_if_possible, drain_and_close from swift.common.wsgi import loadapp, pipeline_property if six.PY3: @@ -215,13 +215,13 @@ class InternalClient(object): # sleep only between tries, not after each one if attempt < self.request_tries - 1: if resp: - # always close any resp.app_iter before we discard it - with closing_if_possible(resp.app_iter): - # for non 2XX requests it's safe and useful to drain - # the response body so we log the correct status code - if resp.status_int // 100 != 2: - for iter_body in resp.app_iter: - pass + # for non 2XX requests it's safe and useful to drain + # the response body so we log the correct status code + if resp.status_int // 100 != 2: + drain_and_close(resp) + else: + # Just close; the 499 is appropriate + close_if_possible(resp.app_iter) sleep(2 ** (attempt + 1)) if resp: msg = 'Unexpected response: %s' % resp.status @@ -657,12 +657,9 @@ class InternalClient(object): path = self.make_path(account, container, obj) resp = self.make_request('DELETE', path, (headers or {}), acceptable_statuses) - # Drain the response body to prevent unexpected disconnect # in proxy-server - with closing_if_possible(resp.app_iter): - for iter_body in resp.app_iter: - pass + drain_and_close(resp) def get_object_metadata( self, account, container, obj, metadata_prefix='', diff --git a/swift/obj/expirer.py b/swift/obj/expirer.py index 39c26e1784..188f3a66fb 100644 --- a/swift/obj/expirer.py +++ b/swift/obj/expirer.py @@ -32,7 +32,6 @@ from swift.common.utils import get_logger, dump_recon_cache, split_path, \ RateLimitedIterator, md5 from swift.common.http import HTTP_NOT_FOUND, HTTP_CONFLICT, \ HTTP_PRECONDITION_FAILED -from swift.common.swob import wsgi_quote, str_to_wsgi from swift.common.recon import RECON_OBJECT_FILE, DEFAULT_RECON_CACHE_PATH from swift.container.reconciler import direct_delete_container_entry @@ -487,7 +486,6 @@ class ObjectExpirer(Daemon): :raises UnexpectedResponse: if the delete was unsuccessful and should be retried later """ - path = '/v1/' + wsgi_quote(str_to_wsgi(actual_obj.lstrip('/'))) if is_async_delete: headers = {'X-Timestamp': timestamp.normal} acceptable_statuses = (2, HTTP_CONFLICT, HTTP_NOT_FOUND) @@ -496,4 +494,6 @@ class ObjectExpirer(Daemon): 'X-If-Delete-At': timestamp.normal, 'X-Backend-Clean-Expiring-Object-Queue': 'no'} acceptable_statuses = (2, HTTP_CONFLICT) - self.swift.make_request('DELETE', path, headers, acceptable_statuses) + self.swift.delete_object(*split_path('/' + actual_obj, 3, 3, True), + headers=headers, + acceptable_statuses=acceptable_statuses) diff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py index 19e4c2a39d..cb9e274d9b 100644 --- a/test/unit/obj/test_expirer.py +++ b/test/unit/obj/test_expirer.py @@ -90,7 +90,7 @@ class FakeInternalClient(object): resp.append(obj) return resp - def make_request(*a, **kw): + def delete_object(*a, **kw): pass @@ -855,7 +855,7 @@ class TestObjectExpirer(TestCase): x = expirer.ObjectExpirer({}) ts = Timestamp('1234') - x.delete_actual_object('/path/to/object', ts, False) + x.delete_actual_object('path/to/object', ts, False) self.assertEqual(got_env[0]['HTTP_X_IF_DELETE_AT'], ts) self.assertEqual(got_env[0]['HTTP_X_TIMESTAMP'], got_env[0]['HTTP_X_IF_DELETE_AT']) @@ -874,7 +874,7 @@ class TestObjectExpirer(TestCase): x = expirer.ObjectExpirer({}) ts = Timestamp('1234') - x.delete_actual_object('/path/to/object', ts, True) + x.delete_actual_object('path/to/object', ts, True) self.assertNotIn('HTTP_X_IF_DELETE_AT', got_env[0]) self.assertNotIn('HTTP_X_BACKEND_CLEAN_EXPIRING_OBJECT_QUEUE', got_env[0]) @@ -894,7 +894,7 @@ class TestObjectExpirer(TestCase): x = expirer.ObjectExpirer({}) ts = Timestamp('1234') - x.delete_actual_object('/path/to/object name', ts, False) + x.delete_actual_object('path/to/object name', ts, False) self.assertEqual(got_env[0]['HTTP_X_IF_DELETE_AT'], ts) self.assertEqual(got_env[0]['HTTP_X_TIMESTAMP'], got_env[0]['HTTP_X_IF_DELETE_AT']) @@ -916,9 +916,9 @@ class TestObjectExpirer(TestCase): ts = Timestamp('1234') if should_raise: with self.assertRaises(internal_client.UnexpectedResponse): - x.delete_actual_object('/path/to/object', ts, True) + x.delete_actual_object('path/to/object', ts, True) else: - x.delete_actual_object('/path/to/object', ts, True) + x.delete_actual_object('path/to/object', ts, True) self.assertEqual(calls[0], 1, calls) # object was deleted and tombstone reaped @@ -944,9 +944,9 @@ class TestObjectExpirer(TestCase): ts = Timestamp('1234') if should_raise: with self.assertRaises(internal_client.UnexpectedResponse): - x.delete_actual_object('/path/to/object', ts, False) + x.delete_actual_object('path/to/object', ts, False) else: - x.delete_actual_object('/path/to/object', ts, False) + x.delete_actual_object('path/to/object', ts, False) self.assertEqual(calls[0], 1) # object was deleted and tombstone reaped @@ -971,7 +971,7 @@ class TestObjectExpirer(TestCase): x = expirer.ObjectExpirer({}) exc = None try: - x.delete_actual_object('/path/to/object', Timestamp('1234'), False) + x.delete_actual_object('path/to/object', Timestamp('1234'), False) except Exception as err: exc = err finally: @@ -979,18 +979,19 @@ class TestObjectExpirer(TestCase): self.assertEqual(503, exc.resp.status_int) def test_delete_actual_object_quotes(self): - name = 'this name should get quoted' + name = 'this name/should get/quoted' timestamp = Timestamp('1366063156.863045') x = expirer.ObjectExpirer({}) x.swift.make_request = mock.Mock() x.swift.make_request.return_value.status_int = 204 + x.swift.make_request.return_value.app_iter = [] x.delete_actual_object(name, timestamp, False) self.assertEqual(x.swift.make_request.call_count, 1) self.assertEqual(x.swift.make_request.call_args[0][1], '/v1/' + urllib.parse.quote(name)) def test_delete_actual_object_queue_cleaning(self): - name = 'something' + name = 'acc/cont/something' timestamp = Timestamp('1515544858.80602') x = expirer.ObjectExpirer({}) x.swift.make_request = mock.MagicMock()