internal_client: Don't retry when we expect the same reponse
This boils down to 404, 412, or 416; or 409 when we provided an X-Timestamp. This means, among other things, that the expirer won't issue 3 DELETEs every cycle for every stale work item. Related-Change: Icd63c80c73f864d2561e745c3154fbfda02bd0cc Change-Id: Ie5f2d3824e040bbc76d511a54d1316c4c2503732
This commit is contained in:
Tim Burke
parent
840e2c67db
commit
cd2c73fd95
@ -26,9 +26,11 @@ from time import gmtime, strftime, time
|
||||
from zlib import compressobj
|
||||
|
||||
from swift.common.exceptions import ClientException
|
||||
from swift.common.http import HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES
|
||||
from swift.common.http import HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES, \
|
||||
HTTP_CONFLICT
|
||||
from swift.common.swob import Request
|
||||
from swift.common.utils import quote, closing_if_possible
|
||||
from swift.common.utils import quote, closing_if_possible, \
|
||||
server_handled_successfully
|
||||
from swift.common.wsgi import loadapp, pipeline_property
|
||||
|
||||
if six.PY3:
|
||||
@ -191,11 +193,20 @@ class InternalClient(object):
|
||||
req.params = params
|
||||
try:
|
||||
resp = req.get_response(self.app)
|
||||
except (Exception, Timeout):
|
||||
exc_type, exc_value, exc_traceback = exc_info()
|
||||
else:
|
||||
if resp.status_int in acceptable_statuses or \
|
||||
resp.status_int // 100 in acceptable_statuses:
|
||||
return resp
|
||||
except (Exception, Timeout):
|
||||
exc_type, exc_value, exc_traceback = exc_info()
|
||||
elif server_handled_successfully(resp.status_int):
|
||||
# No sense retrying when we expect the same result
|
||||
break
|
||||
elif resp.status_int == HTTP_CONFLICT and 'x-timestamp' in [
|
||||
header.lower() for header in headers]:
|
||||
# Since the caller provided the timestamp, retrying won't
|
||||
# change the result
|
||||
break
|
||||
# sleep only between tries, not after each one
|
||||
if attempt < self.request_tries - 1:
|
||||
if resp:
|
||||
|
||||
@ -462,48 +462,65 @@ class TestInternalClient(unittest.TestCase):
|
||||
mock.patch('swift.common.internal_client.sleep'):
|
||||
client.make_request('DELETE', '/container', {}, (200,))
|
||||
|
||||
for logline in client.logger.get_lines_for_level('info'):
|
||||
# add spaces before/after 409 because it can match with
|
||||
# something like timestamp
|
||||
self.assertIn(' 409 ', logline)
|
||||
# Since we didn't provide an X-Timestamp, retrying gives us a chance to
|
||||
# succeed (assuming the failure was due to clock skew between servers)
|
||||
expected = (' HTTP/1.0 409 ', ' HTTP/1.0 409 ', ' HTTP/1.0 409 ', )
|
||||
loglines = client.logger.get_lines_for_level('info')
|
||||
for expected, logline in izip_longest(expected, loglines):
|
||||
if not expected:
|
||||
self.fail('Unexpected extra log line: %r' % logline)
|
||||
self.assertIn(expected, logline)
|
||||
|
||||
def test_make_request_acceptable_status_not_2xx(self):
|
||||
closed_paths = []
|
||||
|
||||
def mark_closed(path):
|
||||
closed_paths.append(path)
|
||||
|
||||
class InternalClient(internal_client.InternalClient):
|
||||
def __init__(self):
|
||||
def __init__(self, resp_status):
|
||||
self.logger = DebugLogger()
|
||||
# wrap the fake app with ProxyLoggingMiddleware
|
||||
self.app = ProxyLoggingMiddleware(
|
||||
self.fake_app, {}, self.logger)
|
||||
self.user_agent = 'some_agent'
|
||||
self.resp_status = resp_status
|
||||
self.request_tries = 3
|
||||
self.closed_paths = []
|
||||
|
||||
def fake_app(self, env, start_response):
|
||||
body = 'fake error response'
|
||||
start_response('200 Ok', [('Content-Length', str(len(body)))])
|
||||
return LeakTrackingIter((c for c in body),
|
||||
mark_closed, env['PATH_INFO'])
|
||||
start_response(self.resp_status,
|
||||
[('Content-Length', str(len(body)))])
|
||||
return LeakTrackingIter(body, self.closed_paths.append,
|
||||
env['PATH_INFO'])
|
||||
|
||||
client = InternalClient()
|
||||
with self.assertRaises(internal_client.UnexpectedResponse) as ctx, \
|
||||
mock.patch('swift.common.internal_client.sleep'):
|
||||
# This is obvious strange tests to expect only 400 Bad Request
|
||||
# but this test intended to avoid extra body drain if it's
|
||||
# correct object body with 2xx.
|
||||
client.make_request('GET', '/cont/obj', {}, (400,))
|
||||
self.assertEqual(closed_paths, ['/cont/obj'] * 2)
|
||||
# swob's body resp property will call close
|
||||
self.assertEqual(ctx.exception.resp.body, 'fake error response')
|
||||
def do_test(resp_status):
|
||||
client = InternalClient(resp_status)
|
||||
with self.assertRaises(internal_client.UnexpectedResponse) as ctx, \
|
||||
mock.patch('swift.common.internal_client.sleep'):
|
||||
# This is obvious strange tests to expect only 400 Bad Request
|
||||
# but this test intended to avoid extra body drain if it's
|
||||
# correct object body with 2xx.
|
||||
client.make_request('GET', '/cont/obj', {}, (400,))
|
||||
loglines = client.logger.get_lines_for_level('info')
|
||||
return client.closed_paths, ctx.exception.resp, loglines
|
||||
|
||||
closed_paths, resp, loglines = do_test('200 OK')
|
||||
# Since the 200 is considered "properly handled", it won't be retried
|
||||
self.assertEqual(closed_paths, [])
|
||||
# ...and it'll be on us (the caller) to close (for example, by using
|
||||
# swob.Response's body property)
|
||||
self.assertEqual(resp.body, 'fake error response')
|
||||
self.assertEqual(closed_paths, ['/cont/obj'])
|
||||
|
||||
expected = (' HTTP/1.0 200 ', )
|
||||
for expected, logline in izip_longest(expected, loglines):
|
||||
if not expected:
|
||||
self.fail('Unexpected extra log line: %r' % logline)
|
||||
self.assertIn(expected, logline)
|
||||
|
||||
closed_paths, resp, loglines = do_test('503 Service Unavailable')
|
||||
# But since 5xx is neither "properly handled" not likely to include
|
||||
# a large body, it will be retried and responses will already be closed
|
||||
self.assertEqual(closed_paths, ['/cont/obj'] * 3)
|
||||
|
||||
# Retries like this will cause 499 because internal_client won't
|
||||
# automatically consume (potentially large) 2XX responses.
|
||||
expected = (' 499 ', ' 499 ', ' 200 ')
|
||||
loglines = client.logger.get_lines_for_level('info')
|
||||
expected = (' HTTP/1.0 503 ', ' HTTP/1.0 503 ', ' HTTP/1.0 503 ', )
|
||||
for expected, logline in izip_longest(expected, loglines):
|
||||
if not expected:
|
||||
self.fail('Unexpected extra log line: %r' % logline)
|
||||
@ -562,30 +579,33 @@ class TestInternalClient(unittest.TestCase):
|
||||
self.test.assertEqual(0, whence)
|
||||
|
||||
class InternalClient(internal_client.InternalClient):
|
||||
def __init__(self):
|
||||
def __init__(self, status):
|
||||
self.app = self.fake_app
|
||||
self.user_agent = 'some_agent'
|
||||
self.request_tries = 3
|
||||
self.status = status
|
||||
self.call_count = 0
|
||||
|
||||
def fake_app(self, env, start_response):
|
||||
start_response('404 Not Found', [('Content-Length', '0')])
|
||||
self.call_count += 1
|
||||
start_response(self.status, [('Content-Length', '0')])
|
||||
return []
|
||||
|
||||
fobj = FileObject(self)
|
||||
client = InternalClient()
|
||||
def do_test(status, expected_calls):
|
||||
fobj = FileObject(self)
|
||||
client = InternalClient(status)
|
||||
|
||||
try:
|
||||
old_sleep = internal_client.sleep
|
||||
internal_client.sleep = not_sleep
|
||||
try:
|
||||
client.make_request('PUT', '/', {}, (2,), fobj)
|
||||
except Exception as err:
|
||||
pass
|
||||
self.assertEqual(404, err.resp.status_int)
|
||||
finally:
|
||||
internal_client.sleep = old_sleep
|
||||
with mock.patch.object(internal_client, 'sleep', not_sleep):
|
||||
with self.assertRaises(Exception) as exc_mgr:
|
||||
client.make_request('PUT', '/', {}, (2,), fobj)
|
||||
self.assertEqual(int(status[:3]),
|
||||
exc_mgr.exception.resp.status_int)
|
||||
|
||||
self.assertEqual(client.request_tries, fobj.seek_called)
|
||||
self.assertEqual(client.call_count, fobj.seek_called)
|
||||
self.assertEqual(client.call_count, expected_calls)
|
||||
|
||||
do_test('404 Not Found', 1)
|
||||
do_test('503 Service Unavailable', 3)
|
||||
|
||||
def test_make_request_request_exception(self):
|
||||
class InternalClient(internal_client.InternalClient):
|
||||
@ -650,17 +670,15 @@ class TestInternalClient(unittest.TestCase):
|
||||
self.assertEqual(1, client.make_request_called)
|
||||
|
||||
def test_get_metadata_invalid_status(self):
|
||||
class FakeApp(object):
|
||||
|
||||
def __call__(self, environ, start_response):
|
||||
start_response('404 Not Found', [('x-foo', 'bar')])
|
||||
return ['nope']
|
||||
|
||||
class InternalClient(internal_client.InternalClient):
|
||||
def __init__(self):
|
||||
self.user_agent = 'test'
|
||||
self.request_tries = 1
|
||||
self.app = FakeApp()
|
||||
self.app = self.fake_app
|
||||
|
||||
def fake_app(self, environ, start_response):
|
||||
start_response('404 Not Found', [('x-foo', 'bar')])
|
||||
return ['nope']
|
||||
|
||||
client = InternalClient()
|
||||
self.assertRaises(internal_client.UnexpectedResponse,
|
||||
|
||||
@ -741,30 +741,30 @@ class TestObjectExpirer(TestCase):
|
||||
got_env[0]['HTTP_X_IF_DELETE_AT'])
|
||||
self.assertEqual(got_env[0]['PATH_INFO'], '/v1/path/to/object name')
|
||||
|
||||
def test_delete_actual_object_raises_404(self):
|
||||
def test_delete_actual_object_returns_expected_error(self):
|
||||
def do_test(test_status):
|
||||
calls = [0]
|
||||
|
||||
def fake_app(env, start_response):
|
||||
start_response('404 Not Found', [('Content-Length', '0')])
|
||||
return []
|
||||
def fake_app(env, start_response):
|
||||
calls[0] += 1
|
||||
start_response(test_status, [('Content-Length', '0')])
|
||||
return []
|
||||
|
||||
internal_client.loadapp = lambda *a, **kw: fake_app
|
||||
internal_client.loadapp = lambda *a, **kw: fake_app
|
||||
|
||||
x = expirer.ObjectExpirer({})
|
||||
self.assertRaises(internal_client.UnexpectedResponse,
|
||||
x.delete_actual_object, '/path/to/object', '1234')
|
||||
x = expirer.ObjectExpirer({})
|
||||
self.assertRaises(internal_client.UnexpectedResponse,
|
||||
x.delete_actual_object, '/path/to/object',
|
||||
'1234')
|
||||
self.assertEqual(calls[0], 1)
|
||||
|
||||
def test_delete_actual_object_raises_412(self):
|
||||
|
||||
def fake_app(env, start_response):
|
||||
start_response('412 Precondition Failed',
|
||||
[('Content-Length', '0')])
|
||||
return []
|
||||
|
||||
internal_client.loadapp = lambda *a, **kw: fake_app
|
||||
|
||||
x = expirer.ObjectExpirer({})
|
||||
self.assertRaises(internal_client.UnexpectedResponse,
|
||||
x.delete_actual_object, '/path/to/object', '1234')
|
||||
# object was deleted and tombstone reaped
|
||||
do_test('404 Not Found')
|
||||
# object was overwritten *after* the original expiration, or
|
||||
# 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')
|
||||
|
||||
def test_delete_actual_object_does_not_handle_odd_stuff(self):
|
||||
|
||||
@ -790,7 +790,8 @@ class TestObjectExpirer(TestCase):
|
||||
name = 'this name should get quoted'
|
||||
timestamp = '1366063156.863045'
|
||||
x = expirer.ObjectExpirer({})
|
||||
x.swift.make_request = mock.MagicMock()
|
||||
x.swift.make_request = mock.Mock()
|
||||
x.swift.make_request.return_value.status_int = 204
|
||||
x.delete_actual_object(name, timestamp)
|
||||
self.assertEqual(x.swift.make_request.call_count, 1)
|
||||
self.assertEqual(x.swift.make_request.call_args[0][1],
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user