Ignore 404s from handoffs when choosing response code

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

Author: alistairncoles

swift/proxy/controllers/base.py

@@ -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 @@ def transfer_headers(self, src_headers, dst_headers):
     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 @@ def _make_request(self, nodes, part, method, path, headers, query,
                     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 @@ def make_requests(self, req, ring, part, method, path, headers,
                           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 @@ def make_requests(self, req, ring, part, method, path, headers,
         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)

test/unit/proxy/controllers/test_obj.py

@@ -1464,8 +1464,7 @@ def test_POST_insufficient_primaries_others_fail_handoffs_404(self):
         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 @@ def test_POST_all_primaries_fail_insufficient_handoff_succeeds(self):
         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):