From 7be7cc966b5a98a35cbbccfc426a4efb65583c12 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 12 Jun 2020 08:35:06 -0700 Subject: [PATCH] proxy: Stop killing memcache entries on 5xx responses When you've got a container server that's struggling to respond to HEAD requests, it's really not helpful to keep evicting the cache when some of the hundreds of concurrent requests trying to repopulate the cache return 503. Change-Id: I49174a21a854a4e8e564a7bbf997e1841f9dda71 Closes-Bug: #1883211 --- swift/proxy/controllers/base.py | 39 +++++++++--------- test/probe/test_container_failures.py | 29 +++++++++++++ test/unit/proxy/controllers/test_account.py | 19 +++++++-- test/unit/proxy/controllers/test_base.py | 41 +++++++++++++++++-- test/unit/proxy/controllers/test_container.py | 23 ++++++++--- 5 files changed, 118 insertions(+), 33 deletions(-) diff --git a/swift/proxy/controllers/base.py b/swift/proxy/controllers/base.py index 6bb5e4257d..bf2f3d1ea8 100644 --- a/swift/proxy/controllers/base.py +++ b/swift/proxy/controllers/base.py @@ -539,33 +539,32 @@ def set_info_cache(app, env, account, container, resp): :returns: the info that was placed into the cache, or None if the request status was not in (404, 410, 2xx). """ - infocache = env.setdefault('swift.infocache', {}) - - cache_time = None - if container and resp: - cache_time = int(resp.headers.get( - 'X-Backend-Recheck-Container-Existence', - DEFAULT_RECHECK_CONTAINER_EXISTENCE)) - elif resp: - cache_time = int(resp.headers.get( - 'X-Backend-Recheck-Account-Existence', - DEFAULT_RECHECK_ACCOUNT_EXISTENCE)) cache_key = get_cache_key(account, container) - - if resp: - if resp.status_int in (HTTP_NOT_FOUND, HTTP_GONE): - cache_time *= 0.1 - elif not is_success(resp.status_int): - cache_time = None - - # Next actually set both memcache and the env cache + infocache = env.setdefault('swift.infocache', {}) memcache = getattr(app, 'memcache', None) or env.get('swift.cache') - if cache_time is None: + + if resp is None: infocache.pop(cache_key, None) if memcache: memcache.delete(cache_key) return + if container: + cache_time = int(resp.headers.get( + 'X-Backend-Recheck-Container-Existence', + DEFAULT_RECHECK_CONTAINER_EXISTENCE)) + else: + cache_time = int(resp.headers.get( + 'X-Backend-Recheck-Account-Existence', + DEFAULT_RECHECK_ACCOUNT_EXISTENCE)) + + if resp.status_int in (HTTP_NOT_FOUND, HTTP_GONE): + cache_time *= 0.1 + elif not is_success(resp.status_int): + # If we got a response, it was unsuccessful, and it wasn't an + # "authoritative" failure, bail without touching caches. + return + if container: info = headers_to_container_info(resp.headers, resp.status_int) else: diff --git a/test/probe/test_container_failures.py b/test/probe/test_container_failures.py index 3f87f9b57f..cb611dc36f 100644 --- a/test/probe/test_container_failures.py +++ b/test/probe/test_container_failures.py @@ -125,6 +125,35 @@ class TestContainerFailures(ReplProbeTest): self.assertEqual(headers['x-account-object-count'], '0') self.assertEqual(headers['x-account-bytes-used'], '0') + def test_all_nodes_fail(self): + # Create container1 + container1 = 'container-%s' % uuid4() + cpart, cnodes = self.container_ring.get_nodes(self.account, container1) + client.put_container(self.url, self.token, container1) + client.put_object(self.url, self.token, container1, 'obj1', 'data1') + + # All primaries go down + for cnode in cnodes: + kill_server((cnode['ip'], cnode['port']), self.ipport2server) + + # Can't GET the container + with self.assertRaises(client.ClientException) as caught: + client.get_container(self.url, self.token, container1) + self.assertEqual(caught.exception.http_status, 503) + + # But we can still write objects! The old info is still in memcache + client.put_object(self.url, self.token, container1, 'obj2', 'data2') + + # Can't POST the container, either + with self.assertRaises(client.ClientException) as caught: + client.post_container(self.url, self.token, container1, {}) + self.assertEqual(caught.exception.http_status, 503) + + # Though it *does* evict the cache + with self.assertRaises(client.ClientException) as caught: + client.put_object(self.url, self.token, container1, 'obj3', 'x') + self.assertEqual(caught.exception.http_status, 503) + def _get_container_db_files(self, container): opart, onodes = self.container_ring.get_nodes(self.account, container) onode = onodes[0] diff --git a/test/unit/proxy/controllers/test_account.py b/test/unit/proxy/controllers/test_account.py index a46dcc90f1..663f0d61a8 100644 --- a/test/unit/proxy/controllers/test_account.py +++ b/test/unit/proxy/controllers/test_account.py @@ -69,10 +69,21 @@ class TestAccountController(unittest.TestCase): req = Request.blank('/v1/AUTH_bob', {'PATH_INFO': '/v1/AUTH_bob'}) resp = controller.HEAD(req) self.assertEqual(2, resp.status_int // 100) - self.assertIn('account/AUTH_bob', resp.environ['swift.infocache']) - self.assertEqual( - headers_to_account_info(resp.headers), - resp.environ['swift.infocache']['account/AUTH_bob']) + info_cache = resp.environ['swift.infocache'] + self.assertIn('account/AUTH_bob', info_cache) + header_info = headers_to_account_info(resp.headers) + self.assertEqual(header_info, info_cache['account/AUTH_bob']) + + with mock.patch('swift.proxy.controllers.base.http_connect', + fake_http_connect(500, body='')): + req = Request.blank('/v1/AUTH_bob', { + 'PATH_INFO': '/v1/AUTH_bob', 'swift.infocache': info_cache}) + resp = controller.HEAD(req) + self.assertEqual(5, resp.status_int // 100) + self.assertIs(info_cache, resp.environ['swift.infocache']) + # The *old* header info is all still there + self.assertIn('account/AUTH_bob', info_cache) + self.assertEqual(header_info, info_cache['account/AUTH_bob']) def test_swift_owner(self): owner_headers = { diff --git a/test/unit/proxy/controllers/test_base.py b/test/unit/proxy/controllers/test_base.py index dc7e6245a7..cbff2d23ad 100644 --- a/test/unit/proxy/controllers/test_base.py +++ b/test/unit/proxy/controllers/test_base.py @@ -24,7 +24,8 @@ import six from swift.proxy.controllers.base import headers_to_container_info, \ headers_to_account_info, headers_to_object_info, get_container_info, \ get_cache_key, get_account_info, get_info, get_object_info, \ - Controller, GetOrHeadHandler, bytes_to_skip + Controller, GetOrHeadHandler, bytes_to_skip, clear_info_cache, \ + set_info_cache from swift.common.swob import Request, HTTPException, RESPONSE_REASONS, \ bytes_to_wsgi from swift.common import exceptions @@ -470,6 +471,36 @@ class TestFuncs(unittest.TestCase): resp = get_container_info(req.environ, 'xxx') self.assertEqual(resp['bytes'], 3867) + def test_info_clearing(self): + def check_in_cache(req, cache_key): + self.assertIn(cache_key, req.environ['swift.infocache']) + self.assertIn(cache_key, req.environ['swift.cache'].store) + + def check_not_in_cache(req, cache_key): + self.assertNotIn(cache_key, req.environ['swift.infocache']) + self.assertNotIn(cache_key, req.environ['swift.cache'].store) + + app = FakeApp(statuses=[200, 200]) + acct_cache_key = get_cache_key("account") + cont_cache_key = get_cache_key("account", "cont") + req = Request.blank( + "/v1/account/cont", environ={"swift.cache": FakeCache()}) + # populate caches + info = get_container_info(req.environ, app) + self.assertEqual(info['status'], 200) + + check_in_cache(req, acct_cache_key) + check_in_cache(req, cont_cache_key) + + clear_info_cache('app-is-unused', req.environ, 'account', 'cont') + check_in_cache(req, acct_cache_key) + check_not_in_cache(req, cont_cache_key) + + # Can also use set_info_cache interface + set_info_cache('app-is-unused', req.environ, 'account', None, None) + check_not_in_cache(req, acct_cache_key) + check_not_in_cache(req, cont_cache_key) + def test_get_account_info_swift_source(self): app = FakeApp() req = Request.blank("/v1/a", environ={'swift.cache': FakeCache()}) @@ -684,9 +715,11 @@ class TestFuncs(unittest.TestCase): base.account_name = 'a' base.container_name = 'c' - container_info = \ - base.container_info(base.account_name, - base.container_name) + with mock.patch('swift.proxy.controllers.base.' + 'http_connect', fake_http_connect(200)): + container_info = \ + base.container_info(base.account_name, + base.container_name) self.assertEqual(container_info['status'], 0) def test_headers_to_account_info_missing(self): diff --git a/test/unit/proxy/controllers/test_container.py b/test/unit/proxy/controllers/test_container.py index ba0e4fff1a..9e9fc4e696 100644 --- a/test/unit/proxy/controllers/test_container.py +++ b/test/unit/proxy/controllers/test_container.py @@ -113,12 +113,25 @@ class TestContainerController(TestRingBase): resp = controller.HEAD(req) self.assertEqual(2, resp.status_int // 100) # Make sure it's in both swift.infocache and memcache + header_info = headers_to_container_info(resp.headers) + info_cache = resp.environ['swift.infocache'] self.assertIn("container/a/c", resp.environ['swift.infocache']) - self.assertEqual( - headers_to_container_info(resp.headers), - resp.environ['swift.infocache']['container/a/c']) - from_memcache = self.app.memcache.get('container/a/c') - self.assertTrue(from_memcache) + self.assertEqual(header_info, info_cache['container/a/c']) + self.assertEqual(header_info, self.app.memcache.get('container/a/c')) + + controller = proxy_server.ContainerController(self.app, 'a', 'c') + with mock.patch('swift.proxy.controllers.base.http_connect', + fake_http_connect(500, body='')): + req = Request.blank('/v1/a/c', { + 'PATH_INFO': '/v1/a/c', 'swift.infocache': info_cache}) + resp = controller.HEAD(req) + self.assertEqual(5, resp.status_int // 100) + # The failure doesn't lead to cache eviction + self.assertIs(info_cache, resp.environ['swift.infocache']) + self.assertIn("container/a/c", resp.environ['swift.infocache']) + # NB: this is the *old* header_info, from the good req + self.assertEqual(header_info, info_cache['container/a/c']) + self.assertEqual(header_info, self.app.memcache.get('container/a/c')) @mock.patch('swift.proxy.controllers.container.clear_info_cache') @mock.patch.object(Controller, 'make_requests')