Repository: osbuild/osbuild
Pull request #1198 (2022-12-09 08:08:50)

[v2] util/fscache: introduce versioning
(duplicate PR to work around github-actions-CI weirdness)

Add a new field to the cache-information called `version`, which is a simple integer that is incremented on any backward-incompatible change.

The cache-implementation is modified to avoid any access to the cache except for `<cache>/staging/`. This means, changes to the staging area must be backwards compatible at all cost. Furthermore, it means we can always successfully run osbuild even on possibly incompatible caches, because we can always just ignore the cache and fully rely on the staging area being accessible.

The `load()` method will always return cache-misses. The `store()` method simply discards the entry instead of storing it. Note that `store()` needs to provide a context to the caller, hence this implementation simply creates another staging-context to provide to the caller and then discard. This is non-optimal, but keeps the API simple and avoids raising an exception to the caller (but this can be changed if it turns out to be problematic or unwanted).

Lastly, the `cache.info` field behaves as usual, since this is also the field used to read the cache-version. However, this file is never written to improve resiliency and allow blacklisting buggy versions from the past.

gicmo: > Note that store() needs to provide a context to the caller, hence this implementation simply creates another staging-context to provide to the caller and then discard. This is non-optimal, but keeps the API simple and avoids raising an exception to the caller (but this can be changed if it turns out to be problematic or unwanted).

One more reason to have `store` just take a path. 
gicmo: Looks great. I guess you planned a way to "upgrade" the store, maybe by throwing everything out?
dvdhrm: > Looks great. I guess you planned a way to "upgrade" the store, maybe by throwing everything out?

I think this would really depend on what kind of incompatible change we do. With this I just want to allow for future updates to do such cache-logic-upgrades. Any old osbuild binary will simply not look into the object-store, and thus does not care what kind of changes happened. And new binaries would ignore the cache as well, or support online-upgrades, depending on what we decide then.

So this is really just a safety-net to ensure no code ever accesses incompatible caches, but instead just computes everything manually and ignore the cache. If possible, we would never need to bump the major version. But who knows what will happen...

Patch:
diff --git a/osbuild/util/fscache.py b/osbuild/util/fscache.py
index d08da631..f2f1c4fc 100644
--- a/osbuild/util/fscache.py
+++ b/osbuild/util/fscache.py
@@ -35,10 +35,12 @@ class FsCacheInfo(NamedTuple):
 
     creation_boot_id - Hashed linux boot-id at the time of cache-creation
     maximum_size - Maximum cache size in bytes, or "unlimited"
+    version - version of the cache data structures
     """
 
     creation_boot_id: Optional[str] = None
     maximum_size: MaximumSizeType = None
+    version: Optional[int] = None
 
     @classmethod
     def from_json(cls, data: Any) -> "FsCacheInfo":
@@ -54,6 +56,7 @@ class FsCacheInfo(NamedTuple):
 
         creation_boot_id = None
         maximum_size: MaximumSizeType = None
+        version = None
 
         # parse "creation-boot-id"
         _creation_boot_id = data.get("creation-boot-id", None)
@@ -67,10 +70,16 @@ class FsCacheInfo(NamedTuple):
         elif isinstance(_maximum_size, str) and _maximum_size == "unlimited":
             maximum_size = "unlimited"
 
+        # parse "version"
+        _version = data.get("version", None)
+        if isinstance(_version, int):
+            version = _version
+
         # create immutable tuple
         return cls(
             creation_boot_id,
             maximum_size,
+            version,
         )
 
     def to_json(self) -> Dict[str, Any]:
@@ -86,6 +95,8 @@ class FsCacheInfo(NamedTuple):
             data["creation-boot-id"] = self.creation_boot_id
         if self.maximum_size is not None:
             data["maximum-size"] = self.maximum_size
+        if self.version is not None:
+            data["version"] = self.version
         return data
 
 
@@ -195,6 +206,8 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
     _filename_cache_size = "cache.size"
     _filename_object_info = "object.info"
     _filename_object_lock = "object.lock"
+    _version_current = 1
+    _version_minimum = 1
 
     # constant properties
     _appid: str
@@ -551,7 +564,7 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
         # Create the file-scaffolding of the cache. We fill in the default
         # information and ignore racing operations.
         with self._atomic_file(self._filename_cache_info, self._dirname_objects, ignore_exist=True) as f:
-            f.write("{}")
+            json.dump({"version": self._version_current}, f)
         with self._atomic_file(self._filename_cache_lock, self._dirname_objects, ignore_exist=True) as f:
             pass
         with self._atomic_file(self._filename_cache_size, self._dirname_objects, ignore_exist=True) as f:
@@ -604,6 +617,11 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
         # Internal helper to verify we are in an active context-manager.
         return self._active
 
+    def _is_compatible(self):
+        # Internal helper to verify the cache-version is supported.
+        return self._info.version is not None and \
+            self._version_minimum <= self._info.version <= self._version_current
+
     def __enter__(self):
         assert not self._active
 
@@ -662,6 +680,7 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
         """
 
         assert self._is_active()
+        assert self._is_compatible()
 
         # Open the cache-size and lock it for writing. But instead of writing
         # directly to it, we replace it with a new file. This guarantees that
@@ -754,6 +773,10 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
         Hence, any cache management routine will discard it.
         """
 
+        # We check for an active context, but we never check for
+        # version-compatibility, because there is no way we can run without
+        # a staging area. Hence, the staging-area has to be backwards
+        # compatible at all times.
         assert self._is_active()
 
         uuidname = None
@@ -805,6 +828,15 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
         if not name:
             raise ValueError()
 
+        # If the cache-version is incompatible to this implementation, we short
+        # this call into the staging-area (which is always compatible). This
+        # avoids raising an exception (at the cost of dealing with this in the
+        # caller), and instead just creates a temporary copy which we discard.
+        if not self._is_compatible():
+            with self.stage() as p:
+                yield p
+            return
+
         uuidname = None
         lockfd = None
 
@@ -912,6 +944,8 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
 
         if not name:
             raise ValueError()
+        if not self._is_compatible():
+            raise self.MissError()
 
         with contextlib.ExitStack() as es:
             # Use an ExitStack so we can catch exceptions raised by the
@@ -1005,7 +1039,8 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
                 info = FsCacheInfo.from_json(info_raw)
 
                 # Replace the file with the new values. This releases the lock.
-                with self._atomic_file(self._filename_cache_info, self._dirname_objects, replace=True) as f:
-                    json.dump(info_raw, f)
+                if self._is_compatible():
+                    with self._atomic_file(self._filename_cache_info, self._dirname_objects, replace=True) as f:
+                        json.dump(info_raw, f)
 
         self._load_cache_info(info)


Test patch:
diff --git a/test/mod/test_objectstore.py b/test/mod/test_objectstore.py
index c68e7ce0..0cbedd2f 100644
--- a/test/mod/test_objectstore.py
+++ b/test/mod/test_objectstore.py
@@ -4,11 +4,11 @@
 
 import contextlib
 import os
-import shutil
 import tempfile
-import unittest
 from pathlib import Path
 
+import pytest
+
 from osbuild import objectstore
 
 from .. import test
@@ -21,245 +21,255 @@ def store_path(store: objectstore.ObjectStore, ref: str, path: str) -> bool:
     return os.path.exists(os.path.join(obj, path))
 
 
-@unittest.skipUnless(test.TestBase.can_bind_mount(), "root-only")
-class TestObjectStore(unittest.TestCase):
+@pytest.fixture(name="tmpdir")
+def tmpdir_fixture():
+    with tempfile.TemporaryDirectory() as tmp:
+        yield tmp
 
-    def setUp(self):
-        self.store = tempfile.mkdtemp(prefix="osbuild-test-", dir="/var/tmp")
 
-    def tearDown(self):
-        shutil.rmtree(self.store)
+@pytest.fixture(name="object_store")
+def store_fixture():
+    with tempfile.TemporaryDirectory(
+        prefix="osbuild-test-",
+        dir="/var/tmp",
+    ) as tmp:
+        with objectstore.ObjectStore(tmp) as store:
+            yield store
 
-    def test_basic(self):
-        # always use a temporary store so item counting works
-        with objectstore.ObjectStore(self.store) as object_store:
-            object_store.maximum_size = 1024*1024*1024
 
-            # No objects or references should be in the store
-            assert len(os.listdir(object_store.objects)) == 0
+def test_basic(object_store):
+    object_store.maximum_size = 1024*1024*1024
 
-            tree = object_store.new("a")
+    # No objects or references should be in the store
+    assert len(os.listdir(object_store.objects)) == 0
 
-            # new object should be in write mode
-            assert tree.mode == objectstore.Object.Mode.WRITE
+    tree = object_store.new("a")
 
-            p = Path(tree, "A")
-            p.touch()
+    # new object should be in write mode
+    assert tree.mode == objectstore.Object.Mode.WRITE
 
-            tree.finalize()  # put the object into READ mode
-            assert tree.mode == objectstore.Object.Mode.READ
+    p = Path(tree, "A")
+    p.touch()
 
-            # commit makes a copy, if space
-            object_store.commit(tree, "a")
-            assert store_path(object_store, "a", "A")
+    tree.finalize()  # put the object into READ mode
+    assert tree.mode == objectstore.Object.Mode.READ
 
-            # second object, based on the first one
-            obj2 = object_store.new("b")
-            obj2.init(tree)
+    # commit makes a copy, if space
+    object_store.commit(tree, "a")
+    assert store_path(object_store, "a", "A")
 
-            p = Path(obj2, "B")
-            p.touch()
+    # second object, based on the first one
+    obj2 = object_store.new("b")
+    obj2.init(tree)
 
-            obj2.finalize()  # put the object into READ mode
-            assert obj2.mode == objectstore.Object.Mode.READ
+    p = Path(obj2, "B")
+    p.touch()
 
-            # commit always makes a copy, if space
-            object_store.commit(tree, "b")
+    obj2.finalize()  # put the object into READ mode
+    assert obj2.mode == objectstore.Object.Mode.READ
 
-            assert object_store.contains("b")
-            assert store_path(object_store, "b", "A")
-            assert store_path(object_store, "b", "B")
+    # commit always makes a copy, if space
+    object_store.commit(tree, "b")
 
-            assert len(os.listdir(object_store.objects)) == 2
+    assert object_store.contains("b")
+    assert store_path(object_store, "b", "A")
+    assert store_path(object_store, "b", "B")
 
-            # object should exist and should be in read mode
-            tree = object_store.get("b")
-            assert tree is not None
-            assert tree.mode == objectstore.Object.Mode.READ
+    assert len(os.listdir(object_store.objects)) == 2
 
-    def test_cleanup(self):
-        # always use a temporary store so item counting works
-        with objectstore.ObjectStore(self.store) as object_store:
-            object_store.maximum_size = 1024*1024*1024
+    # object should exist and should be in read mode
+    tree = object_store.get("b")
+    assert tree is not None
+    assert tree.mode == objectstore.Object.Mode.READ
 
-            stage = os.path.join(object_store, "stage")
-            tree = object_store.new("a")
-            self.assertEqual(len(os.listdir(stage)), 1)
-            p = Path(tree, "A")
-            p.touch()
 
-        # there should be no temporary Objects dirs anymore
-        with objectstore.ObjectStore(self.store) as object_store:
-            assert object_store.get("A") is None
+def test_cleanup(tmpdir):
+    with objectstore.ObjectStore(tmpdir) as object_store:
+        object_store.maximum_size = 1024*1024*1024
+
+        stage = os.path.join(object_store, "stage")
+        tree = object_store.new("a")
+        assert len(os.listdir(stage)) == 1
+        p = Path(tree, "A")
+        p.touch()
+
+    # there should be no temporary Objects dirs anymore
+    with objectstore.ObjectStore(tmpdir) as object_store:
+        assert object_store.get("A") is None
 
-    def test_metadata(self):
 
-        # test metadata object directly first
-        with tempfile.TemporaryDirectory() as tmp:
-            md = objectstore.Object.Metadata(tmp)
+def test_metadata(tmpdir):
 
-            assert os.fspath(md) == tmp
+    # test metadata object directly first
+    with tempfile.TemporaryDirectory() as tmp:
+        md = objectstore.Object.Metadata(tmp)
 
-            with self.assertRaises(KeyError):
-                with md.read("a"):
-                    pass
+        assert os.fspath(md) == tmp
 
-            # do not write anything to the file, it should not get stored
-            with md.write("a"):
+        with pytest.raises(KeyError):
+            with md.read("a"):
                 pass
 
-            assert len(list(os.scandir(tmp))) == 0
+        # do not write anything to the file, it should not get stored
+        with md.write("a"):
+            pass
 
-            # also we should not write anything if an exception was raised
-            with self.assertRaises(AssertionError):
-                with md.write("a") as f:
-                    f.write("{}")
-                    raise AssertionError
+        assert len(list(os.scandir(tmp))) == 0
 
+        # also we should not write anything if an exception was raised
+        with pytest.raises(AssertionError):
             with md.write("a") as f:
                 f.write("{}")
+                raise AssertionError
 
-            assert len(list(os.scandir(tmp))) == 1
+        with md.write("a") as f:
+            f.write("{}")
 
-            with md.read("a") as f:
-                assert f.read() == "{}"
+        assert len(list(os.scandir(tmp))) == 1
 
-        data = {
-            "boolean": True,
-            "number": 42,
-            "string": "yes, please"
-        }
+        with md.read("a") as f:
+            assert f.read() == "{}"
 
-        extra = {
-            "extra": "data"
-        }
+    data = {
+        "boolean": True,
+        "number": 42,
+        "string": "yes, please"
+    }
 
-        with tempfile.TemporaryDirectory() as tmp:
-            md = objectstore.Object.Metadata(tmp)
+    extra = {
+        "extra": "data"
+    }
 
-            d = md.get("a")
-            assert d is None
+    with tempfile.TemporaryDirectory() as tmp:
+        md = objectstore.Object.Metadata(tmp)
 
-            md.set("a", None)
-            with self.assertRaises(KeyError):
-                with md.read("a"):
-                    pass
+        d = md.get("a")
+        assert d is None
 
-            md.set("a", data)
-            assert md.get("a") == data
+        md.set("a", None)
+        with pytest.raises(KeyError):
+            with md.read("a"):
+                pass
 
-        with objectstore.ObjectStore(self.store) as store:
-            store.maximum_size = 1024*1024*1024
-            obj = store.new("a")
-            p = Path(obj, "A")
-            p.touch()
+        md.set("a", data)
+        assert md.get("a") == data
+
+    # use tmpdir fixture from here on
+    with objectstore.ObjectStore(tmpdir) as store:
+        store.maximum_size = 1024*1024*1024
+        obj = store.new("a")
+        p = Path(obj, "A")
+        p.touch()
 
-            obj.meta.set("md", data)
-            assert obj.meta.get("md") == data
+        obj.meta.set("md", data)
+        assert obj.meta.get("md") == data
 
-            store.commit(obj, "x")
-            obj.meta.set("extra", extra)
-            assert obj.meta.get("extra") == extra
+        store.commit(obj, "x")
+        obj.meta.set("extra", extra)
+        assert obj.meta.get("extra") == extra
 
-            store.commit(obj, "a")
+        store.commit(obj, "a")
 
-        with objectstore.ObjectStore(self.store) as store:
-            obj = store.get("a")
+    with objectstore.ObjectStore(tmpdir) as store:
+        obj = store.get("a")
 
-            assert obj.meta.get("md") == data
-            assert obj.meta.get("extra") == extra
+        assert obj.meta.get("md") == data
+        assert obj.meta.get("extra") == extra
 
-            ext = store.get("x")
+        ext = store.get("x")
 
-            assert ext.meta.get("md") == data
-            assert ext.meta.get("extra") is None
+        assert ext.meta.get("md") == data
+        assert ext.meta.get("extra") is None
 
-    def test_host_tree(self):
-        with objectstore.ObjectStore(self.store) as store:
-            host = store.host_tree
 
-            assert host.tree
-            assert os.fspath(host)
+@pytest.mark.skipif(not test.TestBase.can_bind_mount(), reason="Need root for bind mount")
+def test_host_tree(tmpdir):
+    with objectstore.ObjectStore(tmpdir) as store:
+        host = store.host_tree
 
-            # check we actually cannot write to the path
-            p = Path(host.tree, "osbuild-test-file")
-            with self.assertRaises(OSError):
-                p.touch()
-                print("FOO")
+        assert host.tree
+        assert os.fspath(host)
+
+        # check we actually cannot write to the path
+        p = Path(host.tree, "osbuild-test-file")
+        with pytest.raises(OSError):
+            p.touch()
+            print("FOO")
 
-        # We cannot access the tree property after cleanup
-        with self.assertRaises(AssertionError):
-            _ = host.tree
+    # We cannot access the tree property after cleanup
+    with pytest.raises(AssertionError):
+        _ = host.tree
 
-    # pylint: disable=too-many-statements
-    def test_store_server(self):
 
-        with contextlib.ExitStack() as stack:
+# pylint: disable=too-many-statements
+@pytest.mark.skipif(not test.TestBase.can_bind_mount(), reason="Need root for bind mount")
+def test_store_server(tmpdir):
+    with contextlib.ExitStack() as stack:
 
-            store = objectstore.ObjectStore(self.store)
-            stack.enter_context(store)
+        store = objectstore.ObjectStore(tmpdir)
+        stack.enter_context(store)
 
-            tmpdir = tempfile.TemporaryDirectory()
-            tmpdir = stack.enter_context(tmpdir)
+        tmp = tempfile.TemporaryDirectory()
+        tmp = stack.enter_context(tmp)
 
-            server = objectstore.StoreServer(store)
-            stack.enter_context(server)
+        server = objectstore.StoreServer(store)
+        stack.enter_context(server)
 
-            client = objectstore.StoreClient(server.socket_address)
+        client = objectstore.StoreClient(server.socket_address)
 
-            have = client.source("org.osbuild.files")
-            want = os.path.join(self.store, "sources")
-            assert have.startswith(want)
+        have = client.source("org.osbuild.files")
+        want = os.path.join(tmpdir, "sources")
+        assert have.startswith(want)
 
-            tmp = client.mkdtemp(suffix="suffix", prefix="prefix")
-            assert tmp.startswith(store.tmp)
-            name = os.path.basename(tmp)
-            assert name.startswith("prefix")
-            assert name.endswith("suffix")
+        tmp = client.mkdtemp(suffix="suffix", prefix="prefix")
+        assert tmp.startswith(store.tmp)
+        name = os.path.basename(tmp)
+        assert name.startswith("prefix")
+        assert name.endswith("suffix")
 
-            obj = store.new("42")
-            p = Path(obj, "file.txt")
-            p.write_text("osbuild")
+        obj = store.new("42")
+        p = Path(obj, "file.txt")
+        p.write_text("osbuild")
 
-            p = Path(obj, "directory")
-            p.mkdir()
-            obj.finalize()
+        p = Path(obj, "directory")
+        p.mkdir()
+        obj.finalize()
 
-            mountpoint = Path(tmpdir, "mountpoint")
-            mountpoint.mkdir()
+        mountpoint = Path(tmpdir, "mountpoint")
+        mountpoint.mkdir()
 
-            assert store.contains("42")
-            path = client.read_tree_at("42", mountpoint)
-            assert Path(path) == mountpoint
-            filepath = Path(mountpoint, "file.txt")
-            assert filepath.exists()
-            txt = filepath.read_text(encoding="utf8")
-            assert txt == "osbuild"
+        assert store.contains("42")
+        path = client.read_tree_at("42", mountpoint)
+        assert Path(path) == mountpoint
+        filepath = Path(mountpoint, "file.txt")
+        assert filepath.exists()
+        txt = filepath.read_text(encoding="utf8")
+        assert txt == "osbuild"
 
-            # check we can mount subtrees via `read_tree_at`
+        # check we can mount subtrees via `read_tree_at`
 
-            filemount = Path(tmpdir, "file")
-            filemount.touch()
+        filemount = Path(tmpdir, "file")
+        filemount.touch()
 
-            path = client.read_tree_at("42", filemount, "/file.txt")
-            filepath = Path(path)
-            assert filepath.is_file()
-            txt = filepath.read_text(encoding="utf8")
-            assert txt == "osbuild"
+        path = client.read_tree_at("42", filemount, "/file.txt")
+        filepath = Path(path)
+        assert filepath.is_file()
+        txt = filepath.read_text(encoding="utf8")
+        assert txt == "osbuild"
 
-            dirmount = Path(tmpdir, "dir")
-            dirmount.mkdir()
+        dirmount = Path(tmpdir, "dir")
+        dirmount.mkdir()
 
-            path = client.read_tree_at("42", dirmount, "/directory")
-            dirpath = Path(path)
-            assert dirpath.is_dir()
+        path = client.read_tree_at("42", dirmount, "/directory")
+        dirpath = Path(path)
+        assert dirpath.is_dir()
 
-            # check proper exceptions are raised for non existent
-            # mount points and sub-trees
+        # check proper exceptions are raised for non existent
+        # mount points and sub-trees
 
-            with self.assertRaises(RuntimeError):
-                nonexistent = os.path.join(tmpdir, "nonexistent")
-                _ = client.read_tree_at("42", nonexistent)
+        with pytest.raises(RuntimeError):
+            nonexistent = os.path.join(tmpdir, "nonexistent")
+            _ = client.read_tree_at("42", nonexistent)
 
-            with self.assertRaises(RuntimeError):
-                _ = client.read_tree_at("42", tmpdir, "/nonexistent")
+        with pytest.raises(RuntimeError):
+            _ = client.read_tree_at("42", tmpdir, "/nonexistent")
diff --git a/test/mod/test_util_fscache.py b/test/mod/test_util_fscache.py
index 1125abd9..334f4736 100644
--- a/test/mod/test_util_fscache.py
+++ b/test/mod/test_util_fscache.py
@@ -4,6 +4,7 @@
 
 # pylint: disable=protected-access
 
+import json
 import os
 import tempfile
 
@@ -194,7 +195,7 @@ def test_scaffolding(tmpdir):
     assert len(list(os.scandir(os.path.join(tmpdir, cache._dirname_stage)))) == 0
 
     with open(os.path.join(tmpdir, cache._filename_cache_info), "r", encoding="utf8") as f:
-        assert f.read() == "{}"
+        assert json.load(f) == {"version": 1}
     with open(os.path.join(tmpdir, cache._filename_cache_lock), "r", encoding="utf8") as f:
         assert f.read() == ""
     with open(os.path.join(tmpdir, cache._filename_cache_size), "r", encoding="utf8") as f:
@@ -210,7 +211,7 @@ def test_cache_info(tmpdir):
     cache = fscache.FsCache("osbuild-test-appid", tmpdir)
 
     with cache:
-        assert cache._info == fscache.FsCacheInfo()
+        assert cache._info == fscache.FsCacheInfo(version=1)
         assert cache.info == cache._info
 
         assert cache.info.maximum_size is None