Repository: osbuild/osbuild
Pull request #1453 (2023-11-15 21:50:26)

osbuild/util/fscache: calculate actual size of files
In OSBuild we'll often be operating on sparse files. Let's make the tabulation of the size of files on disk used when determining cache size for pruning consider the actual size of the file usage on disk rather than the size the file reports to be.

This means using os.lstat().st_blocks * 512 versus os.lstat().st_size.

See https://stackoverflow.com/a/55203604

bcl: I'd be tempted to rename the function since it is no longer size, but space used. Should also update the docstring to make it clear it is only counting actually used blocks.
mvo5: Thanks for this PR! I like the suggestion to handle sparse files better. I also like the idea of @bcl a lot to rename the helper to `calculate_space` (just to make a bit clearer what it's about). If we do this we should also change the semantic of the helper IMHO to include dirs. It's minor of course but feels more correct to me. 

The tests also need updates and we need to be a bit careful here because of course the block size of the fs may (in theory at least) vary. And maybe info["size"] should also become info["space"] just to be consistent but maybe I'm overdoing it here :) 

I attached a small draft of what I have in mind. 
[0001-util-tweak-_calculate_size-to-_calculate_space.patch.txt](https://github.com/osbuild/osbuild/files/13412798/0001-util-tweak-_calculate_size-to-_calculate_space.patch.txt)

dustymabe: I added your commit on top. 
mvo5: > I added your commit on top.

Thank you! I overlooked one small issue in `test_basic` and pushed a fix to your branch (amended my original commit). Hope this is okay :)
dustymabe: > > I added your commit on top.
> 
> Thank you! I overlooked one small issue in `test_basic` and pushed a fix to your branch (amended my original commit). Hope this is okay :)

Perfectly fine. Thanks!
dustymabe: "All checks have passed!"

Patch:
diff --git a/osbuild.spec b/osbuild.spec
index d2d48e43..41e14609 100644
--- a/osbuild.spec
+++ b/osbuild.spec
@@ -1,7 +1,7 @@
 %global         forgeurl https://github.com/osbuild/osbuild
 %global         selinuxtype targeted
 
-Version:        100
+Version:        101
 
 %forgemeta
 
diff --git a/osbuild/__init__.py b/osbuild/__init__.py
index 19942f67..250ac9c1 100644
--- a/osbuild/__init__.py
+++ b/osbuild/__init__.py
@@ -10,7 +10,7 @@ independent of osbuild but used across the osbuild codebase.
 
 from .pipeline import Manifest, Pipeline, Stage
 
-__version__ = "100"
+__version__ = "101"
 
 __all__ = [
     "Manifest",
diff --git a/osbuild/meta.py b/osbuild/meta.py
index 12cecb02..36ec9068 100644
--- a/osbuild/meta.py
+++ b/osbuild/meta.py
@@ -429,6 +429,8 @@ class ModuleInfo:
         except FileNotFoundError:
             return None
 
+        # using AST here and not importlib because we can read/parse
+        # even if some python imports that the module may need are missing
         tree = ast.parse(data, name)
 
         docstring = ast.get_docstring(tree)
diff --git a/osbuild/util/fscache.py b/osbuild/util/fscache.py
index 29b3ce90..669735b5 100644
--- a/osbuild/util/fscache.py
+++ b/osbuild/util/fscache.py
@@ -272,25 +272,28 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
             self._tracers[trace]()
 
     @staticmethod
-    def _calculate_size(path_target: str) -> int:
-        """Calculate total size of a directory tree
+    def _calculate_space(path_target: str) -> int:
+        """Calculate total space of a directory tree
 
         Calculate the total amount of storage required for a directory tree in
         bytes. This does not account for metadata, but only for stored file
         content.
 
+        Note that this may differ from the sum of the file sizes as it
+        takes sparse files into account.
+
         Parameters:
         -----------
         path_target
             File-system path to the directory to operate on.
         """
 
-        return sum(
+        return os.lstat(path_target).st_blocks * 512 + sum(
             os.lstat(
                 os.path.join(path, f)
-            ).st_size for path, dirs, files in os.walk(
+            ).st_blocks * 512 for path, dirs, files in os.walk(
                 path_target
-            ) for f in files
+            ) for f in files + dirs
         )
 
     def __fspath__(self) -> Any:
@@ -925,7 +928,7 @@ class FsCache(contextlib.AbstractContextManager, os.PathLike):
             # Collect metadata about the new entry.
             info: Dict[str, Any] = {}
             info["creation-boot-id"] = self._bootid
-            info["size"] = self._calculate_size(path_data)
+            info["size"] = self._calculate_space(path_data)
 
             # Update the total cache-size. If it exceeds the limits, bail out
             # but do not trigger an error. It behaves as if the entry was
diff --git a/setup.py b/setup.py
index 82be9be8..2e9bfc9b 100644
--- a/setup.py
+++ b/setup.py
@@ -2,7 +2,7 @@ import setuptools
 
 setuptools.setup(
     name="osbuild",
-    version="100",
+    version="101",
     description="A build system for OS images",
     packages=["osbuild", "osbuild.formats", "osbuild.util"],
     license='Apache-2.0',


Test patch:
diff --git a/test/mod/test_util_fscache.py b/test/mod/test_util_fscache.py
index b73e3f2c..1741b52c 100644
--- a/test/mod/test_util_fscache.py
+++ b/test/mod/test_util_fscache.py
@@ -6,6 +6,7 @@
 
 import json
 import os
+import subprocess
 import tempfile
 
 import pytest
@@ -19,25 +20,36 @@ def tmpdir_fixture():
         yield tmp
 
 
-def test_calculate_size(tmpdir):
+def test_calculate_space(tmpdir):
     #
-    # Test the `_calculate_size()` helper and verify it only includes file
+    # Test the `_calculate_space()` helper and verify it only includes file
     # content in its calculation.
     #
+    def du(path_target):
+        env = os.environ.copy()
+        env["POSIXLY_CORRECT"] = "1"
+        output = subprocess.check_output(["du", "-s", path_target], env=env, encoding="utf8")
+        return int(output.split()[0].strip())*512
 
-    os.mkdir(os.path.join(tmpdir, "dir"))
-
-    assert fscache.FsCache._calculate_size(os.path.join(tmpdir, "dir")) == 0
+    test_dir = os.path.join(tmpdir, "dir")
+    os.mkdir(test_dir)
+    assert fscache.FsCache._calculate_space(test_dir) == du(test_dir)
 
     with open(os.path.join(tmpdir, "dir", "file"), "x", encoding="utf8") as f:
         pass
-
-    assert fscache.FsCache._calculate_size(os.path.join(tmpdir, "dir")) == 0
+    assert fscache.FsCache._calculate_space(test_dir) == du(test_dir)
 
     with open(os.path.join(tmpdir, "dir", "file"), "w", encoding="utf8") as f:
         f.write("foobar")
+    assert fscache.FsCache._calculate_space(test_dir) == du(test_dir)
+
+    os.makedirs(os.path.join(test_dir, "dir"))
+    assert fscache.FsCache._calculate_space(test_dir) == du(test_dir)
 
-    assert fscache.FsCache._calculate_size(os.path.join(tmpdir, "dir")) == 6
+    with open(os.path.join(test_dir, "sparse-file"), "w") as f:
+        f.truncate(10*1024*1024)
+        f.write("I'm not an empty file")
+    assert fscache.FsCache._calculate_space(test_dir) == du(test_dir)
 
 
 def test_pathlike(tmpdir):
@@ -351,7 +363,7 @@ def test_basic(tmpdir):
 
     cache = fscache.FsCache("osbuild-test-appid", tmpdir)
     with cache:
-        cache.info = cache.info._replace(maximum_size=1024)
+        cache.info = cache.info._replace(maximum_size=1024*1024)
 
         with cache.stage() as rpath:
             with open(os.path.join(tmpdir, rpath, "bar"), "x", encoding="utf8") as f: