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

Author: gillesbiannic

swift/common/internal_client.py

@@ -28,7 +28,7 @@
 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 @@ def retry_request(self, method, **kwargs):
             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)
 

swift/container/sync.py

@@ -42,7 +42,7 @@
     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 @@ def container_sync_row(self, row, sync_to, user_key, broker, info,
                                   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

test/unit/common/test_internal_client.py

@@ -1600,6 +1600,36 @@ def test_request_object_with_retries_with_HTTPError(self,
             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'

test/unit/container/test_sync.py

@@ -909,6 +909,24 @@ def fake_delete_object(*args, **kwargs):
             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