From f4d3facdf4b6ec8ee0dcacc7be8999731c68a8ec Mon Sep 17 00:00:00 2001 From: Matthew Oliver Date: Thu, 14 Aug 2014 14:39:18 +1000 Subject: [PATCH] Treat 404s as 204 on object delete in proxy This change adds an optional overrides map to _make_request method in the base Controller class. def make_requests(self, req, ring, part, method, path, headers, query_string='', overrides=None) Which will be passed on the the best_response method. If set and no quorum it reached, the override map is used to attempt to find quorum. The overrides map is in the form: { : , .. } The ObjectController, in the DELETE method now passes an override map to make_requests method in the base Controller class in the form of: { 404: 204 } Statuses/responses that have been overridden are used in calculation of the quorum but never returned to the user. They are replaced by: (STATUS, '', '', '') And left out of the search for best response. Change-Id: Ibf969eac3a09d67668d5275e808ed626152dd7eb Closes-Bug: 1318375 --- swift/proxy/controllers/base.py | 77 +++++++++++++++++------- swift/proxy/controllers/obj.py | 4 +- test/unit/proxy/controllers/test_base.py | 26 ++++++++ test/unit/proxy/controllers/test_obj.py | 25 ++++++++ test/unit/proxy/test_server.py | 4 +- 5 files changed, 112 insertions(+), 24 deletions(-) diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py index 7033b5ee70..2a3698f9cf 100644 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -28,6 +28,7 @@ import os import time import functools import inspect +import operator from sys import exc_info from swift import gettext_ as _ from urllib import quote @@ -1039,7 +1040,7 @@ class Controller(object): {'method': method, 'path': path}) def make_requests(self, req, ring, part, method, path, headers, - query_string=''): + query_string='', overrides=None): """ Sends an HTTP request to multiple nodes and aggregates the results. It attempts the primary nodes concurrently, then iterates over the @@ -1054,6 +1055,8 @@ class Controller(object): :param headers: a list of dicts, where each dict represents one backend request that should be made. :param query_string: optional query string to send to the backend + :param overrides: optional return status override map used to override + the returned status of a request. :returns: a swob.Response object """ start_nodes = ring.get_part_nodes(part) @@ -1078,7 +1081,7 @@ class Controller(object): statuses, reasons, resp_headers, bodies = zip(*response) return self.best_response(req, statuses, reasons, bodies, '%s %s' % (self.server_type, req.method), - headers=resp_headers) + overrides=overrides, headers=resp_headers) def have_quorum(self, statuses, node_count): """ @@ -1098,7 +1101,7 @@ class Controller(object): return False def best_response(self, req, statuses, reasons, bodies, server_type, - etag=None, headers=None): + etag=None, headers=None, overrides=None): """ Given a list of responses from several servers, choose the best to return to the API. @@ -1112,26 +1115,58 @@ class Controller(object): :param headers: headers of each response :returns: swob.Response object with the correct status, body, etc. set """ - resp = Response(request=req) - if len(statuses): - for hundred in (HTTP_OK, HTTP_MULTIPLE_CHOICES, HTTP_BAD_REQUEST): - hstatuses = \ - [s for s in statuses if hundred <= s < hundred + 100] - if len(hstatuses) >= quorum_size(len(statuses)): - status = max(hstatuses) - status_index = statuses.index(status) - resp.status = '%s %s' % (status, reasons[status_index]) - resp.body = bodies[status_index] - if headers: - update_headers(resp, headers[status_index]) - if etag: - resp.headers['etag'] = etag.strip('"') - return resp - self.app.logger.error(_('%(type)s returning 503 for %(statuses)s'), - {'type': server_type, 'statuses': statuses}) - resp.status = '503 Internal Server Error' + resp = self._compute_quorum_response( + req, statuses, reasons, bodies, etag, headers) + if overrides and not resp: + faked_up_status_indices = set() + transformed = [] + for (i, (status, reason, hdrs, body)) in enumerate(zip( + statuses, reasons, headers, bodies)): + if status in overrides: + faked_up_status_indices.add(i) + transformed.append((overrides[status], '', '', '')) + else: + transformed.append((status, reason, hdrs, body)) + statuses, reasons, headers, bodies = zip(*transformed) + resp = self._compute_quorum_response( + req, statuses, reasons, bodies, etag, headers, + indices_to_avoid=faked_up_status_indices) + + if not resp: + resp = Response(request=req) + self.app.logger.error(_('%(type)s returning 503 for %(statuses)s'), + {'type': server_type, 'statuses': statuses}) + resp.status = '503 Internal Server Error' + return resp + def _compute_quorum_response(self, req, statuses, reasons, bodies, etag, + headers, indices_to_avoid=()): + if not statuses: + return None + for hundred in (HTTP_OK, HTTP_MULTIPLE_CHOICES, HTTP_BAD_REQUEST): + hstatuses = \ + [(i, s) for i, s in enumerate(statuses) + if hundred <= s < hundred + 100] + if len(hstatuses) >= quorum_size(len(statuses)): + resp = Response(request=req) + try: + status_index, status = max( + ((i, stat) for i, stat in hstatuses + if i not in indices_to_avoid), + key=operator.itemgetter(1)) + except ValueError: + # All statuses were indices to avoid + continue + resp.status = '%s %s' % (status, reasons[status_index]) + resp.body = bodies[status_index] + if headers: + update_headers(resp, headers[status_index]) + if etag: + resp.headers['etag'] = etag.strip('"') + return resp + return None + @public def GET(self, req): """ diff --git a/swift/proxy/controllers/obj.py b/swift/proxy/controllers/obj.py index 7b3eba440e..2bc7fe0b67 100644 --- a/swift/proxy/controllers/obj.py +++ b/swift/proxy/controllers/obj.py @@ -875,9 +875,11 @@ class ObjectController(Controller): headers = self._backend_requests( req, len(nodes), container_partition, containers) + # When deleting objects treat a 404 status as 204. + status_overrides = {404: 204} resp = self.make_requests(req, obj_ring, partition, 'DELETE', req.swift_entity_path, - headers) + headers, overrides=status_overrides) return resp @public diff --git a/test/unit/proxy/controllers/test_base.py b/test/unit/proxy/controllers/test_base.py index 51b104f151..ccfe149b98 100644 --- a/test/unit/proxy/controllers/test_base.py +++ b/test/unit/proxy/controllers/test_base.py @@ -550,6 +550,32 @@ class TestFuncs(unittest.TestCase): self.assertEqual(base.have_quorum([404, 404], 2), True) self.assertEqual(base.have_quorum([201, 404, 201, 201], 4), True) + def test_best_response_overrides(self): + base = Controller(self.app) + responses = [ + (302, 'Found', '', 'The resource has moved temporarily.'), + (100, 'Continue', '', ''), + (404, 'Not Found', '', 'Custom body'), + ] + server_type = "Base DELETE" + req = Request.blank('/v1/a/c/o', method='DELETE') + statuses, reasons, headers, bodies = zip(*responses) + + # First test that you can't make a quorum with only overridden + # responses + overrides = {302: 204, 100: 204} + resp = base.best_response(req, statuses, reasons, bodies, server_type, + headers=headers, overrides=overrides) + self.assertEqual(resp.status, '503 Internal Server Error') + + # next make a 404 quorum and make sure the last delete (real) 404 + # status is the one returned. + overrides = {100: 404} + resp = base.best_response(req, statuses, reasons, bodies, server_type, + headers=headers, overrides=overrides) + self.assertEqual(resp.status, '404 Not Found') + self.assertEqual(resp.body, 'Custom body') + def test_range_fast_forward(self): req = Request.blank('/') handler = GetOrHeadHandler(None, req, None, None, None, None, {}) diff --git a/test/unit/proxy/controllers/test_obj.py b/test/unit/proxy/controllers/test_obj.py index 645b2742d1..bf5e9b5e01 100755 --- a/test/unit/proxy/controllers/test_obj.py +++ b/test/unit/proxy/controllers/test_obj.py @@ -226,6 +226,31 @@ class TestObjController(unittest.TestCase): resp = req.get_response(self.app) self.assertEquals(resp.status_int, 204) + def test_DELETE_half_not_found_statuses(self): + self.obj_ring.set_replicas(4) + + req = swift.common.swob.Request.blank('/v1/a/c/o', method='DELETE') + with set_http_connect(404, 204, 404, 204): + resp = req.get_response(self.app) + self.assertEquals(resp.status_int, 204) + + def test_DELETE_half_not_found_headers_and_body(self): + # Transformed responses have bogus bodies and headers, so make sure we + # send the client headers and body from a real node's response. + self.obj_ring.set_replicas(4) + + status_codes = (404, 404, 204, 204) + bodies = ('not found', 'not found', '', '') + headers = [{}, {}, {'Pick-Me': 'yes'}, {'Pick-Me': 'yes'}] + + req = swift.common.swob.Request.blank('/v1/a/c/o', method='DELETE') + with set_http_connect(*status_codes, body_iter=bodies, + headers=headers): + resp = req.get_response(self.app) + self.assertEquals(resp.status_int, 204) + self.assertEquals(resp.headers.get('Pick-Me'), 'yes') + self.assertEquals(resp.body, '') + def test_DELETE_not_found(self): req = swift.common.swob.Request.blank('/v1/a/c/o', method='DELETE') with set_http_connect(404, 404, 204): diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index 65a476ff14..37b4b142a1 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -1859,9 +1859,9 @@ class TestObjectController(unittest.TestCase): test_status_map((200, 200, 204, 204, 204), 204) test_status_map((200, 200, 204, 204, 500), 204) test_status_map((200, 200, 204, 404, 404), 404) - test_status_map((200, 200, 204, 500, 404), 503) + test_status_map((200, 204, 500, 500, 404), 503) test_status_map((200, 200, 404, 404, 404), 404) - test_status_map((200, 200, 404, 404, 500), 404) + test_status_map((200, 200, 400, 400, 400), 400) def test_HEAD(self): with save_globals():