Cache API tests & unifying behavior (saltstack#61229)

* Add tests for cache backends

* Tests run against everything!

* Get consul passing cache tests

* now mysql tests also pass

* comments

* add mysql cache tests

* Ensure consul cache is flushing timestamp keys

* Ensure etcd cache is flushing timestamp keys

* Update redis cache api to conform to localfs

There was a bug in some of the lookups where it was looking at sub banks
instead of the banks containing the leaf/key nodes. That was fixed. Also
restored the original behaviors, only adding the updated functionality,
namely adding timestamp key/values to redis.

* Fix etcd path inconsistency

Previously on X-Files. Wait nope. Previously on etcd cache files...
:joy: if a path were 1/2/3 then etcd would break with IndexError, but
when it was 1/2/3/4/5 then instead of returning the "file" (key?) 5 it
would return `4/5` as the name. Which is probably not correct, and
definitely inconsistent with localfs (and every other cache module).

Now it will always only return the final segment of the cache (e.g. 7
for 1/2/3/4/5/6/7 and 5 for 1/2/5)

* Remove comments and cleanup test setup

Comments were outdated, now they're gone or fixed. Also shifted around
some of the test setup to make it a bit cleaner.

* Add localfs backed memcache to the tests

Fixed an inconsistency when flushing banks with None key. Also added
to the test run. We may want to add other backing caches via alternate
fixtures?

* Bad serial

* No this is wrong

* Probably the right lazy loader fix

* Restore tested mysql cache functionality

Added a force_reconnect function because I couldn't figure out how to
remove the client from the context any other way. There may be a better
way to do that, but there will probably be some loader changes anyway.

* remove silly files

Saltfile never should have been added in the first place and the
integration test wasn't really necessary, functional was better/lighter.

* Fix redis_cache cluster mode import

Fixes saltstack#60272 - the import statement was totally incorrect. Now it will
actually attempt to connect to a redis cluster.

* payload should string-ify all the data

* Fix unit tests for parametrization

From what I can tell, previously the query was being built in Python
instead of using mysql to add the parameters, and this probably horked
up the handling of binary strings.

Unless some other version of the DB will be problematic...

* Fix missing docstring complaints

* incorporate PR feedback

* Skip docker if not found and add the rest of mysql

I really really hate the way this is configured but I can't make pytest
use fixtures as parameters in fixtures. I may have to struggle to come
up with another way.

* Tests all passing, woo!

* run black

* Disable pylint here

* Skip when mysql fails during CI

This has been tested locally and it runs all of them. I'm sure with the
CI environment it's possible that it fails for no particularly good
reason. And it's definitely not worth blocking a PR for (though arguably
it could be if *all* of the images fail, but...)

changelog/60272.fixed

@@ -0,0 +1 @@
+Corrected import statement for redis_cache in cluster mode.

changelog/61081.changed

@@ -0,0 +1 @@
+Updated mysql cache module to also store updated timestamp, making it consistent with default cache module. Users of mysql cache should ensure database size before updating, as ALTER TABLE will add the timestamp column.

salt/cache/__init__.py

@@ -39,7 +39,7 @@ class Cache:
         The name of the cache driver to use. This is the name of the python
         module of the `salt.cache` package. Default is `localfs`.
 
-    Terminology.
+    Terminology:
 
     Salt cache subsystem is organized as a tree with nodes and leafs like a
     filesystem. Cache consists of banks. Each bank can contain a number of
@@ -345,5 +345,10 @@ def store(self, bank, key, data):
         self.storage[(bank, key)] = [time.time(), data]
 
     def flush(self, bank, key=None):
-        self.storage.pop((bank, key), None)
+        if key is None:
+            for bank_, key_ in tuple(self.storage):
+                if bank == bank_:
+                    self.storage.pop((bank_, key_))
+        else:
+            self.storage.pop((bank, key), None)
         super().flush(bank, key)

salt/cache/consul.py

@@ -3,6 +3,10 @@
 
 .. versionadded:: 2016.11.2
 
+.. versionchanged:: 3005.0
+
+    Timestamp/cache updated support added.
+
 :depends: python-consul >= 0.2.0
 
 It is up to the system administrator to set up and configure the Consul
@@ -30,6 +34,12 @@
     consul.consistency: default
     consul.dc: dc1
     consul.verify: True
+    consul.timestamp_suffix: .tstamp  # Added in 3005.0
+
+In order to bring the cache APIs into conformity, in 3005.0 timestamp
+information gets stored as a separate ``{key}.tstamp`` key/value. If your
+existing functionality depends on being able to store normal keys with the
+``.tstamp`` suffix, override the ``consul.timestamp_suffix`` default config.
 
 Related docs could be found in the `python-consul documentation`_.
 
@@ -47,6 +57,7 @@
 """
 
 import logging
+import time
 
 import salt.payload
 from salt.exceptions import SaltCacheError
@@ -61,6 +72,7 @@
 
 log = logging.getLogger(__name__)
 api = None
+_tstamp_suffix = ".tstamp"
 
 
 # Define the module's virtual name
@@ -90,7 +102,8 @@ def __virtual__():
     }
 
     try:
-        global api
+        global api, _tstamp_suffix
+        _tstamp_suffix = __opts__.get("consul.timestamp_suffix", _tstamp_suffix)
         api = consul.Consul(**consul_kwargs)
     except AttributeError:
         return (
@@ -107,9 +120,12 @@ def store(bank, key, data):
     Store a key value.
     """
     c_key = "{}/{}".format(bank, key)
+    tstamp_key = "{}/{}{}".format(bank, key, _tstamp_suffix)
+
     try:
         c_data = salt.payload.dumps(data)
         api.kv.put(c_key, c_data)
+        api.kv.put(tstamp_key, salt.payload.dumps(int(time.time())))
     except Exception as exc:  # pylint: disable=broad-except
         raise SaltCacheError(
             "There was an error writing the key, {}: {}".format(c_key, exc)
@@ -138,9 +154,13 @@ def flush(bank, key=None):
     """
     if key is None:
         c_key = bank
+        tstamp_key = None
     else:
         c_key = "{}/{}".format(bank, key)
+        tstamp_key = "{}/{}{}".format(bank, key, _tstamp_suffix)
     try:
+        if tstamp_key:
+            api.kv.delete(tstamp_key)
         return api.kv.delete(c_key, recurse=key is None)
     except Exception as exc:  # pylint: disable=broad-except
         raise SaltCacheError(
@@ -166,22 +186,36 @@ def list_(bank):
         out = set()
         for key in keys:
             out.add(key[len(bank) + 1 :].rstrip("/"))
-        keys = list(out)
+        keys = [o for o in out if not o.endswith(_tstamp_suffix)]
     return keys
 
 
 def contains(bank, key):
     """
     Checks if the specified bank contains the specified key.
     """
-    if key is None:
-        return True  # any key could be a branch and a leaf at the same time in Consul
-    else:
-        try:
-            c_key = "{}/{}".format(bank, key)
-            _, value = api.kv.get(c_key)
-        except Exception as exc:  # pylint: disable=broad-except
-            raise SaltCacheError(
-                "There was an error getting the key, {}: {}".format(c_key, exc)
-            )
-        return value is not None
+    try:
+        c_key = "{}/{}".format(bank, key or "")
+        _, value = api.kv.get(c_key, keys=True)
+    except Exception as exc:  # pylint: disable=broad-except
+        raise SaltCacheError(
+            "There was an error getting the key, {}: {}".format(c_key, exc)
+        )
+    return value is not None
+
+
+def updated(bank, key):
+    """
+    Return the Unix Epoch timestamp of when the key was last updated. Return
+    None if key is not found.
+    """
+    c_key = "{}/{}{}".format(bank, key, _tstamp_suffix)
+    try:
+        _, value = api.kv.get(c_key)
+        if value is None:
+            return None
+        return salt.payload.loads(value["Value"])
+    except Exception as exc:  # pylint: disable=broad-except
+        raise SaltCacheError(
+            "There was an error reading the key, {}: {}".format(c_key, exc)
+        )

salt/cache/etcd_cache.py

@@ -2,6 +2,7 @@
 Minion data cache plugin for Etcd key/value data store.
 
 .. versionadded:: 2018.3.0
+.. versionchanged:: 3005
 
 It is up to the system administrator to set up and configure the Etcd
 infrastructure. All is needed for this plugin is a working Etcd agent
@@ -42,6 +43,9 @@
 
     cache: etcd
 
+In Phosphorus, ls/list was changed to always return the final name in the path.
+This should only make a difference if you were directly using ``ls`` on paths
+that were more or less nested than, for example: ``1/2/3/4``.
 
 .. _`Etcd documentation`: https://github.com/coreos/etcd
 .. _`python-etcd documentation`: http://python-etcd.readthedocs.io/en/latest/
@@ -50,6 +54,7 @@
 
 import base64
 import logging
+import time
 
 import salt.payload
 from salt.exceptions import SaltCacheError
@@ -72,6 +77,7 @@
 log = logging.getLogger(__name__)
 client = None
 path_prefix = None
+_tstamp_suffix = ".tstamp"
 
 # Module properties
 
@@ -94,7 +100,7 @@ def __virtual__():
 
 def _init_client():
     """Setup client and init datastore."""
-    global client, path_prefix
+    global client, path_prefix, _tstamp_suffix
     if client is not None:
         return
 
@@ -111,6 +117,7 @@ def _init_client():
         "cert": __opts__.get("etcd.cert", None),
         "ca_cert": __opts__.get("etcd.ca_cert", None),
     }
+    _tstamp_suffix = __opts__.get("etcd.timestamp_suffix", _tstamp_suffix)
     path_prefix = __opts__.get("etcd.path_prefix", _DEFAULT_PATH_PREFIX)
     if path_prefix != "":
         path_prefix = "/{}".format(path_prefix.strip("/"))
@@ -129,9 +136,11 @@ def store(bank, key, data):
     """
     _init_client()
     etcd_key = "{}/{}/{}".format(path_prefix, bank, key)
+    etcd_tstamp_key = "{}/{}/{}".format(path_prefix, bank, key + _tstamp_suffix)
     try:
         value = salt.payload.dumps(data)
         client.write(etcd_key, base64.b64encode(value))
+        client.write(etcd_tstamp_key, int(time.time()))
     except Exception as exc:  # pylint: disable=broad-except
         raise SaltCacheError(
             "There was an error writing the key, {}: {}".format(etcd_key, exc)
@@ -162,13 +171,17 @@ def flush(bank, key=None):
     _init_client()
     if key is None:
         etcd_key = "{}/{}".format(path_prefix, bank)
+        tstamp_key = None
     else:
         etcd_key = "{}/{}/{}".format(path_prefix, bank, key)
+        tstamp_key = "{}/{}/{}".format(path_prefix, bank, key + _tstamp_suffix)
     try:
         client.read(etcd_key)
     except etcd.EtcdKeyNotFound:
         return  # nothing to flush
     try:
+        if tstamp_key:
+            client.delete(tstamp_key)
         client.delete(etcd_key, recursive=True)
     except Exception as exc:  # pylint: disable=broad-except
         raise SaltCacheError(
@@ -182,7 +195,10 @@ def _walk(r):
     r: etcd.EtcdResult
     """
     if not r.dir:
-        return [r.key.split("/", 3)[3]]
+        if r.key.endswith(_tstamp_suffix):
+            return []
+        else:
+            return [r.key.rsplit("/", 1)[-1]]
 
     keys = []
     for c in client.read(r.key).children:
@@ -199,25 +215,44 @@ def ls(bank):
     path = "{}/{}".format(path_prefix, bank)
     try:
         return _walk(client.read(path))
+    except etcd.EtcdKeyNotFound:
+        return []
     except Exception as exc:  # pylint: disable=broad-except
         raise SaltCacheError(
             'There was an error getting the key "{}": {}'.format(bank, exc)
-        )
+        ) from exc
 
 
 def contains(bank, key):
     """
     Checks if the specified bank contains the specified key.
     """
     _init_client()
-    etcd_key = "{}/{}/{}".format(path_prefix, bank, key)
+    etcd_key = "{}/{}/{}".format(path_prefix, bank, key or "")
     try:
         r = client.read(etcd_key)
-        # return True for keys, not dirs
-        return r.dir is False
+        # return True for keys, not dirs, unless key is None
+        return r.dir if key is None else r.dir is False
     except etcd.EtcdKeyNotFound:
         return False
     except Exception as exc:  # pylint: disable=broad-except
         raise SaltCacheError(
             "There was an error getting the key, {}: {}".format(etcd_key, exc)
         )
+
+
+def updated(bank, key):
+    """
+    Return Unix Epoch based timestamp of when the bank/key was updated.
+    """
+    _init_client()
+    tstamp_key = "{}/{}/{}".format(path_prefix, bank, key + _tstamp_suffix)
+    try:
+        value = client.read(tstamp_key).value
+        return int(value)
+    except etcd.EtcdKeyNotFound:
+        return None
+    except Exception as exc:  # pylint: disable=broad-except
+        raise SaltCacheError(
+            "There was an error reading the key, {}: {}".format(tstamp_key, exc)
+        )