Repository: osbuild/osbuild
Pull request #1909 (2024-10-22 16:27:41)

stages/org.osbuild.tar: implement disk-full test (RHEL-4653)
As a followup of https://issues.redhat.com/browse/RHEL-4653 it seems that the stages don't return proper
error messages back if the disk is full.

Probably this is known and there is an issue for this, but I didn't find it.

This should be an example environment for more stages to test if they return a proper error in a "disk full scenario"

This PR is ment for a proof of concept for a test environment and basis of discussion
as the error of "disk full" is not properly handled not transformed to a proper error code and not handed over to the stack above in this code.

I think at least stages that have high risk of "disk full", like `tar`, `rpm`, `cp` … should handle "disk full" more gracefully and return a proper error message.


supakeen: What is in the original issue linked here? It is not public.
achilleas-k: In it's current state this PR isn't very useful.  Capturing the output actually makes it more difficult to troubleshoot failed builds, and doesn't seem to change anything about how the error is handled so I don't see how it relates to the Jira ticket.

The ticket itself should probably be closed since the issue appears on quite an old version of osbuild-composer that doesn't have the issue.  We could bisect some history to figure out which change affected the behaviour (my guess is something in osbuild-composer's handling of failed builds), and maybe fork off another ticket about detecting out-of-space errors and other common stage failures and reporting the error up the stack (instead of crashing).

Is this PR the first step towards this kind of error reporting?  If that were the case I'd expect to see some error string detection in the stage itself, not just in a test.  Right now, as far as production is concerned, the PR is hiding the stdout and stderr of the tar call and not changing any of the behaviour.  Why not the opposite?  Changing the call to `check=False` (or wrapping it in an exception handler), checking for an exit code, and exiting gracefully (instead of raising an exception) would solve a lot more problems, especially if the captured stdout and stderr were printed by the stage since those messages would end up in the osbuild-composer log.
schuellerf: > Is this PR the first step towards this kind of error reporting?

@achilleas-k yes, thanks! That's exactly the discussion I was looking for. If that's a feasible way forward I'd be happy to enrich this PR to really care about the output in the stage and make it non-draft then :-D 

schuellerf: @supakeen 
> I won't review the code until I understand what we are trying to solve here.

I tried to summarize in the first message. The error of "disk full" is not handled and just exiting is bad to debug,
as the messages of the underlying tools like tar are strange and it takes a while to see that actually the disk is full. I think some stages would still somehow continue, leading to very odd behavior.
…and yes, in the service this should never happen anyway, but osbuild should also generate useful error messages for the "users" or for us in the other use cases

So the Idea is to start with a setup to be able to reproduce "disk full" for all stages and implement the behavior we want to have for each separately.

Patch:
diff --git a/README.md b/README.md
index 8c7f1307..156caccf 100644
--- a/README.md
+++ b/README.md
@@ -68,6 +68,13 @@ To verify changes made to the code use included makefile rules:
  * `make test-all` to run base set of tests
  * `sudo make test-run` to run extended set of tests (takes long time)
 
+Also keep in mind that some tests require those prerequisites,
+otherwise they are skipped
+
+```
+sudo dnf install -y systemd-boot-unsigned erofs-utils pykickstart podman xfsprogs
+```
+
 ## Installation
 
 Installing `osbuild` requires to not only install the `osbuild` module, but also
diff --git a/stages/org.osbuild.mkdir b/stages/org.osbuild.mkdir
index f04549f6..d2d11a7a 100755
--- a/stages/org.osbuild.mkdir
+++ b/stages/org.osbuild.mkdir
@@ -3,23 +3,26 @@ import os
 import sys
 
 import osbuild.api
-from osbuild.util.path import in_tree
+from osbuild.util import parsing
 
 
-def main(tree, options):
+def main(args):
+    options = args["options"]
+
     for item in options["paths"]:
         path = item["path"]
         mode = item.get("mode", 0o777)
         parents = item.get("parents", False)
         exist_ok = item.get("exist_ok", False)
 
-        if not path.startswith("/"):
-            print("WARNING: relative path used, this is discouraged!")
-
-        target = os.path.join(tree, path.lstrip("/"))
-        if not in_tree(target, tree):
-            raise ValueError(f"path {path} not in tree")
+        if "://" not in path:
+            if not path.startswith("/"):
+                print("WARNING: relative path used, this is discouraged!")
+                path = f"tree:///{path}"
+            else:
+                path = f"tree://{path}"
 
+        target = parsing.parse_location(path, args)
         if parents:
             os.makedirs(target, mode=mode, exist_ok=exist_ok)
         else:
@@ -33,5 +36,4 @@ def main(tree, options):
 
 
 if __name__ == "__main__":
-    args = osbuild.api.arguments()
-    sys.exit(main(args["tree"], args["options"]))
+    sys.exit(main(osbuild.api.arguments()))
diff --git a/stages/org.osbuild.mkdir.meta.json b/stages/org.osbuild.mkdir.meta.json
index 5534120a..6cebaaf5 100644
--- a/stages/org.osbuild.mkdir.meta.json
+++ b/stages/org.osbuild.mkdir.meta.json
@@ -1,5 +1,5 @@
 {
-  "summary": "Create directories within the tree.",
+  "summary": "Create directories within the tree or mount.",
   "description": [
     "Can create one or more directories, optionally also the",
     "intermediate directories. The stage can gracefully handle",
@@ -31,8 +31,23 @@
             ],
             "properties": {
               "path": {
-                "type": "string",
-                "pattern": "^\\/?(?!\\.\\.)((?!\\/\\.\\.\\/).)+$"
+                "anyOf": [
+                  {
+                    "type": "string",
+                    "description": "Target path, if a tree",
+                    "pattern": "^\\/?(?!\\.\\.)((?!\\/\\.\\.\\/).)+$"
+                  },
+                  {
+                    "type": "string",
+                    "description": "Target path, if a mount",
+                    "pattern": "^mount://.+"
+                  },
+                  {
+                    "type": "string",
+                    "description": "Target path, if a tree",
+                    "pattern": "^tree://.+"
+                  }
+                ]
               },
               "mode": {
                 "type": "number",


Test patch:
diff --git a/stages/test/conftest.py b/stages/test/conftest.py
index f2f9d5a7..93811484 100644
--- a/stages/test/conftest.py
+++ b/stages/test/conftest.py
@@ -1,5 +1,6 @@
 import os
 import pathlib
+import subprocess
 from types import ModuleType
 
 import pytest
@@ -43,3 +44,17 @@ def stage_schema(request: pytest.FixtureRequest) -> osbuild.meta.Schema:
     root = caller_dir.parent.parent
     mod_info = osbuild.meta.ModuleInfo.load(root, "Stage", stage_name)
     return osbuild.meta.Schema(mod_info.get_schema(version=schema_version), stage_name)
+
+
+@pytest.fixture
+def tmp_path_disk_full(tmp_path, tmp_path_disk_full_size):
+    small_dir = tmp_path / "small-dir"
+    small_dir.mkdir()
+    subprocess.run(["mount",
+                    "-t", "tmpfs",
+                    "-o", f"size={tmp_path_disk_full_size}",
+                    "tmpfs",
+                    small_dir
+                    ], check=True)
+    yield small_dir
+    subprocess.run(["umount", small_dir], check=True)
diff --git a/stages/test/test_mkdir.py b/stages/test/test_mkdir.py
new file mode 100644
index 00000000..6285a380
--- /dev/null
+++ b/stages/test/test_mkdir.py
@@ -0,0 +1,89 @@
+#!/usr/bin/python3
+
+import contextlib
+import os
+import subprocess
+
+import pytest  # type: ignore
+
+from osbuild.testutil import has_executable
+
+STAGE_NAME = "org.osbuild.mkdir"
+
+
+def test_mkdir(tmp_path, stage_module):
+    tree = tmp_path / "tree"
+    tree.mkdir()
+
+    options = {
+        "paths": [
+            {"path": "/fake_dir"},
+            {"path": "fake_relative_dir"}
+        ]
+    }
+    args = {
+        "tree": f"{tree}",
+        "options": options
+    }
+    stage_module.main(args)
+    assert (tree / "fake_dir").exists()
+    assert (tree / "fake_relative_dir").exists()
+
+
+def test_mkdir_on_a_tree(tmp_path, stage_module):
+    tree = tmp_path / "tree"
+    tree.mkdir()
+
+    options = {
+        "paths": [
+            {
+                "path": "tree:///fake_parent/fake_dir",
+                "parents": 1
+            }
+        ]
+    }
+    args = {
+        "tree": f"{tree}",
+        "options": options
+    }
+    stage_module.main(args)
+    assert (tree / "fake_parent/fake_dir").exists()
+
+
+@pytest.mark.skipif(os.getuid() != 0, reason="needs root")
+@pytest.mark.skipif(not has_executable("mkfs.ext4"), reason="need mkfs.ext4")
+def test_mkdir_on_a_mount(tmp_path, stage_module):
+    tree = tmp_path / "tree"
+    tree.mkdir()
+
+    # Create fake EXT4 disk image
+    fake_disk_path = tmp_path / "fake.img"
+    with fake_disk_path.open("w") as fp:
+        fp.truncate(10 * 1024 * 1024)
+    subprocess.run(
+        ["mkfs.ext4", os.fspath(fake_disk_path)], check=True)
+
+    fake_disk_mnt = tmp_path / "mounts"
+    fake_disk_mnt.mkdir()
+
+    with contextlib.ExitStack() as cm:
+        subprocess.run(["mount", fake_disk_path, fake_disk_mnt], check=True)
+        cm.callback(subprocess.run, ["umount", fake_disk_mnt], check=True)
+
+        options = {
+            "paths": [
+                {
+                    "path": "mount:///fake_parent/fake_dir",
+                    "parents": 1
+                }
+            ]
+        }
+        args = {
+            "tree": f"{tree}",
+            "options": options,
+            "paths": {
+                "mounts": fake_disk_mnt,
+            }
+        }
+        stage_module.main(args)
+        assert (fake_disk_mnt / "fake_parent/fake_dir").exists()
diff --git a/stages/test/test_tar.py b/stages/test/test_tar.py
index 75fb0bd1..064301a9 100644
--- a/stages/test/test_tar.py
+++ b/stages/test/test_tar.py
@@ -1,6 +1,7 @@
 #!/usr/bin/python3
 
 import os.path
+import re
 import subprocess
 
 import pytest
@@ -145,3 +146,17 @@ def test_tar_transform_no_sh(tmp_path, stage_module, fake_inputs):
     with pytest.raises(subprocess.CalledProcessError) as ex:
         stage_module.main(fake_inputs, tmp_path, options)
     assert "exit status 2" in str(ex.value)
+
+
+@pytest.mark.parametrize('tmp_path_disk_full_size', [5 * 1024])
+@pytest.mark.skipif(os.getuid() != 0, reason="needs root")
+def test_tar_disk_full(stage_module, fake_inputs, tmp_path_disk_full, capfd):
+    options = {
+        "filename": "out.tar",
+    }
+
+    with pytest.raises(subprocess.CalledProcessError) as ex:
+        stage_module.main(fake_inputs, tmp_path_disk_full, options)
+
+    assert ex.value.returncode == 2
+    assert re.search(r"Wrote only \d+ of \d+ bytes", str(capfd.readouterr().err))