openstack/swift
Code Issues Proposed changes

Ignore 404s from handoffs when choosing response code

Browse Source 
Previously, when handling an object POST, if the proxy got a 202 and
two connection timeouts from the 3 primary backend object servers, it
then went to two handoffs which return 404. The proxy would consider
this to be a quorum of 404s and return the client a 404 response. This
is alarming for the client.

With this patch, the proxy will only treat a 404 response from a
handoff as authoritative if it has an x-backend-timestamp header
(i.e. there's a tombstone on the handoff).  POST responses never have
an x-backend-timestamp header so in the scenario described above the
proxy will return a 503.

The Related-Change previously made a similar fix such that 404s from
handoffs are already non-authoritative for object GETs and HEADs
unless they have an x-backend-timestamp header.

Related-Change: Ia832e9bab13167948f01bc50aa8a61974ce189fb
Cloeses-Bug: #2077743
Change-Id: I96f28ab0b2b5f9374c399e8905ee240e7b093f8b
This commit is contained in:
Alistair Coles 2024-08-08 16:29:19 +01:00
parent 92eebe24c6
commit d0f6a0ce56
2 changed files with 31 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
swift/proxy/controllers/base.py
View File

@ -1107,6 +1107,18 @@ def is_good_source(status, server_type):
return is_success(status) or is_redirection(status)
def is_useful_response(resp, node):
if not resp:
return False
if ('handoff_index' in node
and resp.status == 404
and resp.headers.get('x-backend-timestamp') is None):
# a 404 from a handoff are not considered authoritative unless they
# have an x-backend-timestamp that indicates that there is a tombstone
return False
return True
class ByteCountEnforcer(object):
"""
Enforces that successive calls to file_like.read() give at least
@ -1956,7 +1968,7 @@ class Controller(object):
def generate_request_headers(self, orig_req=None, additional=None,
transfer=False):
"""
Create a list of headers to be used in backend requests
Create a dict of headers to be used in backend requests
:param orig_req: the original request sent by the client to the proxy
:param additional: additional headers to send to the backend
@ -2088,14 +2100,14 @@ class Controller(object):
if (self.app.check_response(node, self.server_type, resp,
method, path)
and not is_informational(resp.status)):
return resp.status, resp.reason, resp.getheaders(), \
resp.read()
return resp, resp.read(), node
except (Exception, Timeout):
self.app.exception_occurred(
node, self.server_type,
'Trying to %(method)s %(path)s' %
{'method': method, 'path': path})
return None, None, None
def make_requests(self, req, ring, part, method, path, headers,
query_string='', overrides=None, node_count=None,
@ -2118,6 +2130,8 @@ class Controller(object):
the returned status of a request.
:param node_count: optional number of nodes to send request to.
:param node_iterator: optional node iterator.
:param body: byte string to use as the request body.
Try to keep it small.
:returns: a swob.Response object
"""
nodes = GreenthreadSafeIterator(node_iterator or NodeIter(
@ -2128,25 +2142,25 @@ class Controller(object):
for head in headers:
pile.spawn(self._make_request, nodes, part, method, path,
head, query_string, body, self.logger.thread_locals)
response = []
results = []
statuses = []
for resp in pile:
if not resp:
for resp, body, node in pile:
if not is_useful_response(resp, node):
continue
response.append(resp)
statuses.append(resp[0])
results.append((resp.status, resp.reason, resp.getheaders(), body))
statuses.append(resp.status)
if self.have_quorum(statuses, node_number):
break
# give any pending requests *some* chance to finish
finished_quickly = pile.waitall(self.app.post_quorum_timeout)
for resp in finished_quickly:
if not resp:
for resp, body, node in finished_quickly:
if not is_useful_response(resp, node):
continue
response.append(resp)
statuses.append(resp[0])
while len(response) < node_number:
response.append((HTTP_SERVICE_UNAVAILABLE, '', '', b''))
statuses, reasons, resp_headers, bodies = zip(*response)
results.append((resp.status, resp.reason, resp.getheaders(), body))
statuses.append(resp.status)
while len(results) < node_number:
results.append((HTTP_SERVICE_UNAVAILABLE, '', '', b''))
statuses, reasons, resp_headers, bodies = zip(*results)
return self.best_response(req, statuses, reasons, bodies,
'%s %s' % (self.server_type, req.method),
overrides=overrides, headers=resp_headers)

6
test/unit/proxy/controllers/test_obj.py
View File

@ -1464,8 +1464,7 @@ class CommonObjectControllerMixin(BaseObjectControllerMixin):
handoff_codes = [404] * primary_failure
with mocked_http_conn(*(primary_codes + handoff_codes)):
resp = req.get_response(self.app)
# TODO: this should really be a 503
self.assertEqual(404, resp.status_int,
self.assertEqual(503, resp.status_int,
'replicas = %s' % self.replicas())
def test_POST_insufficient_primaries_others_fail_handoffs_fail(self):
@ -1487,8 +1486,7 @@ class CommonObjectControllerMixin(BaseObjectControllerMixin):
handoff_codes = [202] * handoff_success + [404] * handoff_not_found
with mocked_http_conn(*(primary_codes + handoff_codes)):
resp = req.get_response(self.app)
# TODO: this should really be a 503
self.assertEqual(404, resp.status_int,
self.assertEqual(503, resp.status_int,
'replicas = %s' % self.replicas())
def test_POST_all_primaries_fail_sufficient_handoff_succeeds(self):