From f68e22d43a1bfbbdf182b1cc6819bdc83d166d5d Mon Sep 17 00:00:00 2001
From: gillesbiannic <gilles.biannic@corp.ovh.com>
Date: Fri, 25 Oct 2019 20:11:30 +0000
Subject: [PATCH] Stop retrying every deletes in container-sync
Internal clients used to retry operations even on client errors, where
we had no expectation that we would get a different response. Among
other things, this would cause container-sync to needlessly retry
deletes that would 404, even though we had code specifically in place to
say that 404s are OK.
While we're at it, also flag 409s as being OK (since the remote has
newer data than us).
Change-Id: I1833a8ff2ac2f4126b6274d6bba47e2b58a41745
Closes-Bug: #1849841
---
swift/common/internal_client.py | 15 +++++++-----
swift/container/sync.py | 5 ++--
test/unit/common/test_internal_client.py | 30 ++++++++++++++++++++++++
test/unit/container/test_sync.py | 18 ++++++++++++++
4 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py
index 7908424939..e1f2f17941 100644
--- a/swift/common/internal_client.py
+++ b/swift/common/internal_client.py
@@ -28,7 +28,7 @@ from zlib import compressobj
from swift.common.constraints import AUTO_CREATE_ACCOUNT_PREFIX
from swift.common.exceptions import ClientException
from swift.common.http import (HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES,
- is_server_error)
+ is_client_error, is_server_error)
from swift.common.swob import Request, bytes_to_wsgi
from swift.common.utils import quote, closing_if_possible
from swift.common.wsgi import loadapp, pipeline_property
@@ -930,14 +930,17 @@ class SimpleClient(object):
self.attempts += 1
try:
return self.base_request(method, **kwargs)
+ except urllib2.HTTPError as err:
+ if is_client_error(err.getcode() or 500):
+ raise ClientException('Client error',
+ http_status=err.getcode())
+ elif self.attempts > retries:
+ raise ClientException('Raise too many retries',
+ http_status=err.getcode())
except (socket.error, httplib.HTTPException, urllib2.URLError) \
as err:
if self.attempts > retries:
- if isinstance(err, urllib2.HTTPError):
- raise ClientException('Raise too many retries',
- http_status=err.getcode())
- else:
- raise
+ raise
sleep(backoff)
backoff = min(backoff * 2, self.max_backoff)
diff --git a/swift/container/sync.py b/swift/container/sync.py
index 92c3d740c0..b793f8a4dc 100644
--- a/swift/container/sync.py
+++ b/swift/container/sync.py
@@ -42,7 +42,7 @@ from swift.common.utils import (
FileLikeIter, get_logger, hash_path, quote, validate_sync_to,
whataremyips, Timestamp, decode_timestamps)
from swift.common.daemon import Daemon
-from swift.common.http import HTTP_UNAUTHORIZED, HTTP_NOT_FOUND
+from swift.common.http import HTTP_UNAUTHORIZED, HTTP_NOT_FOUND, HTTP_CONFLICT
from swift.common.wsgi import ConfigString
@@ -561,7 +561,8 @@ class ContainerSync(Daemon):
logger=self.logger,
timeout=self.conn_timeout)
except ClientException as err:
- if err.http_status != HTTP_NOT_FOUND:
+ if err.http_status not in (
+ HTTP_NOT_FOUND, HTTP_CONFLICT):
raise
self.container_deletes += 1
self.container_stats['deletes'] += 1
diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py
index 597e30e3d9..e3e72a7fb5 100644
--- a/test/unit/common/test_internal_client.py
+++ b/test/unit/common/test_internal_client.py
@@ -1600,6 +1600,36 @@ class TestSimpleClient(unittest.TestCase):
self.assertEqual(mock_sleep.call_count, 1)
self.assertEqual(mock_urlopen.call_count, 2)
+ @mock.patch.object(urllib2, 'urlopen')
+ def test_delete_object_with_404_no_retry(self, mock_urlopen):
+ mock_response = mock.MagicMock()
+ mock_response.read.return_value = b''
+ err_args = [None, 404, None, None, None]
+ mock_urlopen.side_effect = urllib2.HTTPError(*err_args)
+
+ with mock.patch('swift.common.internal_client.sleep') as mock_sleep, \
+ self.assertRaises(exceptions.ClientException) as caught:
+ internal_client.delete_object('http://127.0.0.1',
+ container='con', name='obj')
+ self.assertEqual(caught.exception.http_status, 404)
+ self.assertEqual(mock_sleep.call_count, 0)
+ self.assertEqual(mock_urlopen.call_count, 1)
+
+ @mock.patch.object(urllib2, 'urlopen')
+ def test_delete_object_with_409_no_retry(self, mock_urlopen):
+ mock_response = mock.MagicMock()
+ mock_response.read.return_value = b''
+ err_args = [None, 409, None, None, None]
+ mock_urlopen.side_effect = urllib2.HTTPError(*err_args)
+
+ with mock.patch('swift.common.internal_client.sleep') as mock_sleep, \
+ self.assertRaises(exceptions.ClientException) as caught:
+ internal_client.delete_object('http://127.0.0.1',
+ container='con', name='obj')
+ self.assertEqual(caught.exception.http_status, 409)
+ self.assertEqual(mock_sleep.call_count, 0)
+ self.assertEqual(mock_urlopen.call_count, 1)
+
def test_proxy(self):
# check that proxy arg is passed through to the urllib Request
scheme = 'http'
diff --git a/test/unit/container/test_sync.py b/test/unit/container/test_sync.py
index 9446ea727a..b61fdd0170 100644
--- a/test/unit/container/test_sync.py
+++ b/test/unit/container/test_sync.py
@@ -909,6 +909,24 @@ class TestContainerSync(unittest.TestCase):
self.assertEqual(cs.container_deletes, 2)
self.assertEqual(len(exc), 3)
self.assertEqual(str(exc[-1]), 'test client exception: 404')
+
+ def fake_delete_object(*args, **kwargs):
+ exc.append(ClientException('test client exception',
+ http_status=409))
+ raise exc[-1]
+
+ sync.delete_object = fake_delete_object
+ # Success because our tombstone is out of date
+ self.assertTrue(cs.container_sync_row(
+ {'deleted': True,
+ 'name': 'object',
+ 'created_at': '1.2'}, 'http://sync/to/path',
+ 'key', FakeContainerBroker('broker'),
+ {'account': 'a', 'container': 'c', 'storage_policy_index': 0},
+ realm, realm_key))
+ self.assertEqual(cs.container_deletes, 3)
+ self.assertEqual(len(exc), 4)
+ self.assertEqual(str(exc[-1]), 'test client exception: 409')
finally:
sync.uuid = orig_uuid
sync.delete_object = orig_delete_object