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

@@ -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:

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]

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 = {

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):

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')