launchpad-reviewers team mailing list archive

[Colin Watson <mp+412023@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:tolerate-pymemcache-server-errors into launchpad:master.

Commit message:
Tolerate some errors from pymemcache

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/412023

python-memcached mostly ignored memcached being unavailable, returning None or similar depending on the method; pymemcache normally raises an exception instead.  In our environment, memcached being unavailable should never be treated as an error, since we only ever use it as an opportunistic cache, so the python-memcached behaviour was more appropriate.

Adjust the `get`, `set`, and `delete` methods (the only ones we use) to handle pymemcache exceptions other than those that indicate client errors; we now log them where possible so that they're more discoverable, but otherwise restore the old behaviour of being resilient against server unavailability.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:tolerate-pymemcache-server-errors into launchpad:master.

diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index 12cfdf4..6a1e70e 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -850,7 +850,8 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             if enable_memcache:
                 # Cache the file list in case there's a request for another
                 # file in the same directory.
-                memcache_client.set_json(memcache_key, file_list)
+                memcache_client.set_json(
+                    memcache_key, file_list, logger=logger)
         file_id = (file_list or {}).get(os.path.basename(filename))
         if file_id is None:
             raise BranchFileNotFound(
diff --git a/lib/lp/code/model/gitref.py b/lib/lp/code/model/gitref.py
index 8571c4a..7358336 100644
--- a/lib/lp/code/model/gitref.py
+++ b/lib/lp/code/model/gitref.py
@@ -362,7 +362,7 @@ class GitRefMixin:
                 log = removeSecurityProxy(hosting_client.getLog(
                     path, start, limit=limit, stop=stop, logger=logger))
                 if enable_memcache:
-                    memcache_client.set_json(memcache_key, log)
+                    memcache_client.set_json(memcache_key, log, logger=logger)
             else:
                 # Fall back to synthesising something reasonable based on
                 # information in our own database.
diff --git a/lib/lp/services/memcache/client.py b/lib/lp/services/memcache/client.py
index 55b9d03..2eb2b9a 100644
--- a/lib/lp/services/memcache/client.py
+++ b/lib/lp/services/memcache/client.py
@@ -12,6 +12,10 @@ import json
 import re
 
 from pymemcache.client.hash import HashClient
+from pymemcache.exceptions import (
+    MemcacheClientError,
+    MemcacheError,
+    )
 from pymemcache.serde import (
     python_memcache_deserializer,
     python_memcache_serializer,
@@ -23,6 +27,40 @@ from lp.services.config import config
 class MemcacheClient(HashClient):
     """memcached client with added JSON handling"""
 
+    def get(self, key, default=None, logger=None):
+        """Get a key from memcached, disregarding server failures."""
+        try:
+            return super().get(key, default=None)
+        except MemcacheClientError:
+            raise
+        except MemcacheError as e:
+            if logger is not None:
+                logger.exception("Cannot get %s from memcached: %s" % (key, e))
+            return default
+
+    def set(self, key, value, expire=0, logger=None):
+        """Set a key in memcached, disregarding server failures."""
+        try:
+            return super().set(key, value, expire=expire)
+        except MemcacheClientError:
+            raise
+        except MemcacheError as e:
+            if logger is not None:
+                logger.exception("Cannot set %s in memcached: %s" % (key, e))
+            return False
+
+    def delete(self, key, logger=None):
+        """Set a key in memcached, disregarding server failures."""
+        try:
+            return super().delete(key)
+        except MemcacheClientError:
+            raise
+        except MemcacheError as e:
+            if logger is not None:
+                logger.exception(
+                    "Cannot delete %s from memcached: %s" % (key, e))
+            return False
+
     def get_json(self, key, logger, description, default=None):
         """Returns decoded JSON data from a memcache instance for a given key
 
@@ -34,7 +72,7 @@ class MemcacheClient(HashClient):
 
         :returns: dict or the default value
         """
-        data = self.get(key)
+        data = self.get(key, logger=logger)
         if data is not None:
             try:
                 rv = json.loads(data)
@@ -45,19 +83,19 @@ class MemcacheClient(HashClient):
                     logger.exception(
                         "Cannot load cached %s; deleting" % description
                     )
-                self.delete(key)
+                self.delete(key, logger=logger)
                 rv = default
         else:
             rv = default
         return rv
 
-    def set_json(self, key, value, expire=0):
+    def set_json(self, key, value, expire=0, logger=None):
         """Saves the given key/value pair, after converting the value.
 
         `expire` (optional int) is the number of seconds until the item
             expires from the cache; zero means no expiry.
         """
-        self.set(key, json.dumps(value), expire)
+        self.set(key, json.dumps(value), expire, logger=logger)
 
 
 def memcache_client_factory(timeline=True):
diff --git a/lib/lp/services/memcache/testing.py b/lib/lp/services/memcache/testing.py
index 23ef232..14b6ca5 100644
--- a/lib/lp/services/memcache/testing.py
+++ b/lib/lp/services/memcache/testing.py
@@ -23,7 +23,7 @@ class MemcacheFixture(fixtures.Fixture, MemcacheClient):
     def _setUp(self):
         self.useFixture(ZopeUtilityFixture(self, IMemcacheClient))
 
-    def get(self, key):
+    def get(self, key, default=None, logger=None):
         value, expiry_time = self._cache.get(key, (None, None))
         if expiry_time and _time.time() >= expiry_time:
             self.delete(key)
@@ -31,7 +31,7 @@ class MemcacheFixture(fixtures.Fixture, MemcacheClient):
         else:
             return value
 
-    def set(self, key, val, expire=0):
+    def set(self, key, val, expire=0, logger=None):
         # memcached accepts either delta-seconds from the current time or
         # absolute epoch-seconds, and tells them apart using a magic
         # threshold.  See memcached/memcached.c:realtime.
@@ -41,7 +41,7 @@ class MemcacheFixture(fixtures.Fixture, MemcacheClient):
         self._cache[key] = (val, expire)
         return 1
 
-    def delete(self, key):
+    def delete(self, key, logger=None):
         self._cache.pop(key, None)
         return 1
 
diff --git a/lib/lp/services/memcache/tests/test_memcache_client.py b/lib/lp/services/memcache/tests/test_memcache_client.py
index ee49844..72400fe 100644
--- a/lib/lp/services/memcache/tests/test_memcache_client.py
+++ b/lib/lp/services/memcache/tests/test_memcache_client.py
@@ -3,8 +3,13 @@
 
 """Tests for the IMemcacheClient utility."""
 
+from unittest.mock import patch
+
 from lazr.restful.utils import get_current_browser_request
-from pymemcache.exceptions import MemcacheIllegalInputError
+from pymemcache.exceptions import (
+    MemcacheError,
+    MemcacheIllegalInputError,
+    )
 from zope.component import getUtility
 
 from lp.services.log.logger import BufferLogger
@@ -51,6 +56,36 @@ class MemcacheClientTestCase(TestCase):
         self.assertEqual('memcache-get', action.category)
         self.assertEqual('foo', action.detail)
 
+    def test_get_failure(self):
+        logger = BufferLogger()
+        with patch.object(self.client, "_get_client") as mock_get_client:
+            mock_get_client.side_effect = MemcacheError("All servers down")
+            self.assertIsNone(self.client.get("foo"))
+            self.assertIsNone(self.client.get("foo", logger=logger))
+            self.assertEqual(
+                "ERROR Cannot get foo from memcached: All servers down\n",
+                logger.content.as_text())
+
+    def test_set_failure(self):
+        logger = BufferLogger()
+        with patch.object(self.client, "_get_client") as mock_get_client:
+            mock_get_client.side_effect = MemcacheError("All servers down")
+            self.assertFalse(self.client.set("foo", "bar"))
+            self.assertFalse(self.client.set("foo", "bar", logger=logger))
+            self.assertEqual(
+                "ERROR Cannot set foo in memcached: All servers down\n",
+                logger.content.as_text())
+
+    def test_delete_failure(self):
+        logger = BufferLogger()
+        with patch.object(self.client, "_get_client") as mock_get_client:
+            mock_get_client.side_effect = MemcacheError("All servers down")
+            self.assertFalse(self.client.delete("foo"))
+            self.assertFalse(self.client.delete("foo", logger=logger))
+            self.assertEqual(
+                "ERROR Cannot delete foo from memcached: All servers down\n",
+                logger.content.as_text())
+
 
 class MemcacheClientFactoryTestCase(TestCase):
 
diff --git a/lib/lp/services/memcache/timeline.py b/lib/lp/services/memcache/timeline.py
index 3427e73..52841e6 100644
--- a/lib/lp/services/memcache/timeline.py
+++ b/lib/lp/services/memcache/timeline.py
@@ -31,21 +31,21 @@ class TimelineRecordingClient(MemcacheClient):
         else:
             return configured_value
 
-    def get(self, key):
+    def get(self, key, default=None, logger=None):
         if not self._enabled:
             return None
         action = self.__get_timeline_action("get", key)
         try:
-            return super().get(key)
+            return super().get(key, default=default, logger=logger)
         finally:
             action.finish()
 
-    def set(self, key, value, expire=0):
+    def set(self, key, value, expire=0, logger=None):
         if not self._enabled:
             return None
         action = self.__get_timeline_action("set", key)
         try:
-            success = super().set(key, value, expire=expire)
+            success = super().set(key, value, expire=expire, logger=logger)
             if success:
                 logging.debug("Memcache set succeeded for %s", key)
             else:
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index 9532567..a288a35 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -418,7 +418,7 @@ class SnapStoreClient:
             # pymemcache's `expire` expects an int
             expire_time = int(time.time()) + DAY_IN_SECONDS
             memcache_client.set_json(
-                memcache_key, channels, expire_time)
+                memcache_key, channels, expire_time, logger=log)
         if channels is None:
             channels = _default_store_channels
         return channels