Repository: osbuild/osbuild
Pull request #1872 (2024-08-28 11:27:17)

stages: add a new mkswap stage
A stage very similar to the existing mkfs ones (actually, I started from
the xfs one). It creates a swap area on a given device.

I wonder if we should be more strict with the UUID format, but no stage schema currently contains any UUID-format validation, so I did the same.

The test is very similar to the xfs one, which makes me wonder whether we can somehow deduplicate them, but I'm slightly in favor of leaving the duplication in. I like that the tests are currently quite simple.


Patch:
diff --git a/osbuild/util/chroot.py b/osbuild/util/chroot.py
index 661a7a8f..682988e5 100644
--- a/osbuild/util/chroot.py
+++ b/osbuild/util/chroot.py
@@ -2,7 +2,7 @@ import os
 import subprocess
 
 
-class ChrootProcDevSys:
+class Chroot:
     """
     Sets up mounts for the virtual filesystems inside a root tree, preparing it for running commands using chroot. This
     should be used whenever a stage needs to run a command against the root tree but doesn't support a --root option or
@@ -21,21 +21,29 @@ class ChrootProcDevSys:
                 print(f"Making missing chroot directory: {d}")
                 os.makedirs(self.root + d)
 
-        subprocess.check_call(["/usr/bin/mount",
-                               "-t", "proc",
-                               "-o", "nosuid,noexec,nodev",
-                               "proc", f"{self.root}/proc"])
-        subprocess.check_call(["/usr/bin/mount",
-                               "-t", "devtmpfs",
-                               "-o", "mode=0755,noexec,nosuid,strictatime",
-                               "devtmpfs", f"{self.root}/dev"])
-        subprocess.check_call(["/usr/bin/mount",
-                               "-t", "sysfs",
-                               "-o", "nosuid,noexec,nodev",
-                               "sysfs", f"{self.root}/sys"])
+        subprocess.run(["/usr/bin/mount", "-t", "proc", "-o", "nosuid,noexec,nodev",
+                        "proc", f"{self.root}/proc"],
+                       check=True)
+
+        subprocess.run(["/usr/bin/mount", "-t", "devtmpfs", "-o", "mode=0755,noexec,nosuid,strictatime",
+                        "devtmpfs", f"{self.root}/dev"],
+                       check=True)
+
+        subprocess.run(["/usr/bin/mount", "-t", "sysfs", "-o", "nosuid,noexec,nodev",
+                        "sysfs", f"{self.root}/sys"],
+                       check=True)
 
         return self
 
     def __exit__(self, exc_type, exc_value, tracebk):
+        failed_umounts = []
         for d in ["/proc", "/dev", "/sys"]:
-            subprocess.check_call(["/usr/bin/umount", self.root + d])
+            if subprocess.run(["/usr/bin/umount", "--lazy", self.root + d], check=False).returncode != 0:
+                failed_umounts.append(d)
+        if failed_umounts:
+            print(f"Error unmounting paths from chroot: {failed_umounts}")
+
+    def run(self, cmd, **kwargs):
+        cmd = ["/usr/sbin/chroot", self.root] + cmd
+        # pylint: disable=subprocess-run-check
+        subprocess.run(cmd, **kwargs)  # noqa: PLW1510
diff --git a/stages/org.osbuild.dracut b/stages/org.osbuild.dracut
index 44e3c9ca..05a56682 100755
--- a/stages/org.osbuild.dracut
+++ b/stages/org.osbuild.dracut
@@ -1,9 +1,8 @@
 #!/usr/bin/python3
-import subprocess
 import sys
 
 import osbuild.api
-from osbuild.util.chroot import ChrootProcDevSys
+from osbuild.util.chroot import Chroot
 
 
 def yesno(name: str, value: bool) -> str:
@@ -83,12 +82,8 @@ def main(tree, options):
         if initoverlayfs:
             initfs_bin = "/usr/bin/initoverlayfs-install"
 
-        with ChrootProcDevSys(tree):
-            subprocess.run(["/usr/sbin/chroot", tree, initfs_bin,
-                            "--no-hostonly",
-                            "--kver", kver]
-                           + opts,
-                           check=True)
+        with Chroot(tree) as chroot:
+            chroot.run([initfs_bin, "--no-hostonly", "--kver", kver] + opts, check=True)
 
     return 0
 
diff --git a/stages/org.osbuild.mkswap b/stages/org.osbuild.mkswap
new file mode 100755
index 00000000..3e2d192c
--- /dev/null
+++ b/stages/org.osbuild.mkswap
@@ -0,0 +1,25 @@
+#!/usr/bin/python3
+import subprocess
+import sys
+
+import osbuild.api
+
+
+def main(devices, options):
+    device = devices["device"]["path"]
+
+    uuid = options["uuid"]
+    label = options.get("label")
+    opts = []
+
+    if label:
+        opts = ["--label", label]
+
+    subprocess.run(["mkswap", "--uuid", uuid] + opts + [device],
+                   encoding='utf8', check=True)
+
+
+if __name__ == '__main__':
+    args = osbuild.api.arguments()
+    ret = main(args["devices"], args["options"])
+    sys.exit(ret)
diff --git a/stages/org.osbuild.mkswap.meta.json b/stages/org.osbuild.mkswap.meta.json
new file mode 100644
index 00000000..63aecc1f
--- /dev/null
+++ b/stages/org.osbuild.mkswap.meta.json
@@ -0,0 +1,40 @@
+{
+  "summary": "Construct a swap area via mkswap(8)",
+  "description": [
+    "Construct a swap area with the given options at the device",
+    "specified via `device`.",
+    "Buildhost commands used: `mkswap`."
+  ],
+  "schema_2": {
+    "devices": {
+      "type": "object",
+      "additionalProperties": true,
+      "required": [
+        "device"
+      ],
+      "properties": {
+        "device": {
+          "type": "object",
+          "additionalProperties": true
+        }
+      }
+    },
+    "options": {
+      "additionalProperties": false,
+      "required": [
+        "uuid"
+      ],
+      "properties": {
+        "uuid": {
+          "description": "UUID for the file system",
+          "type": "string"
+        },
+        "label": {
+          "description": "Label for the file system",
+          "type": "string",
+          "maxLength": 12
+        }
+      }
+    }
+  }
+}
diff --git a/stages/org.osbuild.update-crypto-policies b/stages/org.osbuild.update-crypto-policies
index b174497b..c0ea8e32 100755
--- a/stages/org.osbuild.update-crypto-policies
+++ b/stages/org.osbuild.update-crypto-policies
@@ -1,23 +1,20 @@
 #!/usr/bin/python3
-import subprocess
 import sys
 
 from osbuild import api
-from osbuild.util.chroot import ChrootProcDevSys
+from osbuild.util.chroot import Chroot
 
 
 def main(tree, options):
     policy = options["policy"]
 
-    with ChrootProcDevSys(tree):
+    with Chroot(tree) as chroot:
         # update-crypto-polciies uses /proc/self/mountinfo to find and verify that fips paths have been mounted to their
         # expected locations by searching for the following path suffixes:
         #   /crypto-policies/default-fips-config
         #   /crypto-policies/back-ends/FIPS
-        cmd = ["/usr/sbin/chroot", tree,
-               "/usr/bin/update-crypto-policies", "--set", policy]
-
-        subprocess.run(cmd, check=True)
+        cmd = ["/usr/bin/update-crypto-policies", "--set", policy]
+        chroot.run(cmd, check=True)
 
     return 0
 


Test patch:
diff --git a/stages/test/test_dracut.py b/stages/test/test_dracut.py
index 7e019f8b..c8c1d7ed 100644
--- a/stages/test/test_dracut.py
+++ b/stages/test/test_dracut.py
@@ -22,8 +22,9 @@ def test_dracut_with_initoverlayfs(mocked_run, tmp_path, stage_module, with_init
 
     stage_module.main(str(tmp_path), options)
 
-    assert len(mocked_run.call_args_list) == 1
-    args, kwargs = mocked_run.call_args_list[0]
+    # We expect 7 calls to run(): 3 mount + chroot + 3 umount
+    assert len(mocked_run.call_args_list) == 7
+    args, kwargs = mocked_run.call_args_list[3]  # chroot is the 4th call
     assert kwargs.get("check") is True
     run_argv = args[0]
     assert run_argv[0] == "/usr/sbin/chroot"
diff --git a/stages/test/test_mkswap.py b/stages/test/test_mkswap.py
new file mode 100644
index 00000000..d729f80a
--- /dev/null
+++ b/stages/test/test_mkswap.py
@@ -0,0 +1,105 @@
+#!/usr/bin/python3
+
+import os.path
+import subprocess
+import uuid
+from unittest import mock
+
+import pytest
+
+from osbuild import testutil
+from osbuild.testutil import has_executable
+
+STAGE_NAME = "org.osbuild.mkswap"
+
+
+# Prepare dataset containing good and bad API call parameters
+@pytest.mark.parametrize("test_data, expected_err", [
+    # Bad API parameters
+    ({}, "'uuid' is a required property"),
+    ({"uuid": 123}, "123 is not of type 'string'"),
+    ({"uuid": "text", "label": 1234}, "1234 is not of type 'string'"),
+    ({"uuid": "text", "label": "12345678901234567"}, "is too long"),
+    # Good API parameters
+    # mkswap requires valid UUID, but API will accept any string right now.
+    # Bellow will fail when API will be more restricted in the future.
+    ({"uuid": "text"}, ""),
+    ({"uuid": "text", "label": "123456789012"}, ""),
+])
+# This test validates only API calls using correct and incorrect queries
+def test_schema_validation_mkswap(stage_schema, test_data, expected_err):
+    test_input = {
+        "type": STAGE_NAME,
+        "devices": {
+            "device": {
+                "path": "some-path",
+            },
+        },
+        "options": {
+        }
+    }
+    test_input["options"].update(test_data)
+    res = stage_schema.validate(test_input)
+
+    if expected_err == "":
+        assert res.valid is True, f"err: {[e.as_dict() for e in res.errors]}"
+    else:
+        assert res.valid is False
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
+
+
+# This test creates dummy swap using predefined parameters and verifies
+# that end result has those parameters set
+@pytest.mark.skipif(not has_executable("mkswap") or not has_executable("swaplabel"), reason="need mkswap and swaplabel")
+def test_mkswap_integration(tmp_path, stage_module):
+    fake_disk_path = tmp_path / "fake.img"
+    with fake_disk_path.open("w") as fp:
+        fp.truncate(300 * 1024 * 1024)
+    devices = {
+        "device": {
+            "path": fake_disk_path,
+        },
+    }
+    fake_uuid = str(uuid.uuid4())
+    fake_label = "osbuild"
+    options = {
+        "uuid": fake_uuid,
+        "label": fake_label,
+    }
+    stage_module.main(devices, options)
+    assert os.path.exists(fake_disk_path)
+    output = subprocess.check_output([
+        "swaplabel", fake_disk_path], encoding="utf-8")
+    assert f'UUID:  {fake_uuid}' in output, \
+        f'expected UUID not found in: {output}'
+    assert f'LABEL: {fake_label}' in output, \
+        f'expected label not found in: {output}'
+
+
+@pytest.mark.parametrize("test_input, expected", [
+    ({}, []),
+    ({"label": "osbuild"}, ["--label", "osbuild"]),
+])
+@mock.patch("subprocess.run")
+def test_mkswap_cmdline(mock_run, stage_module, test_input, expected):
+    fake_disk_path = "/dev/xxd1"
+    devices = {
+        "device": {
+            "path": fake_disk_path,
+        },
+    }
+    fake_uuid = str(uuid.uuid4())
+    options = {
+        "uuid": fake_uuid,
+    }
+    options.update(test_input)
+    stage_module.main(devices, options)
+
+    expected = [
+        "mkswap",
+        "--uuid",
+        fake_uuid,
+    ] + expected + [
+        fake_disk_path,
+    ]
+    mock_run.assert_called_once_with(expected, encoding="utf8", check=True)
diff --git a/test/mod/test_util_chroot.py b/test/mod/test_util_chroot.py
new file mode 100644
index 00000000..1f4134bf
--- /dev/null
+++ b/test/mod/test_util_chroot.py
@@ -0,0 +1,42 @@
+#
+# Test for util/chroot.py
+#
+
+import os
+from unittest.mock import call, patch
+
+from osbuild.util.chroot import Chroot
+
+
+class RunReturn:
+    """
+    Class to be returned from mocked run() call so that the returncode is always 0.
+    """
+
+    @property
+    def returncode(self):
+        return 0
+
+
+@patch("subprocess.run", return_value=RunReturn())
+def test_chroot_context(mocked_run, tmp_path):
+
+    with Chroot(os.fspath(tmp_path)) as chroot:
+        chroot.run(["/bin/true"], check=True)
+        chroot.run(["/bin/false"], check=False)
+
+    assert mocked_run.call_args_list == [
+        call(["/usr/bin/mount", "-t", "proc", "-o", "nosuid,noexec,nodev",
+              "proc", os.fspath(tmp_path / "proc")], check=True),
+        call(["/usr/bin/mount", "-t", "devtmpfs", "-o", "mode=0755,noexec,nosuid,strictatime",
+              "devtmpfs", os.fspath(tmp_path / "dev")], check=True),
+        call(["/usr/bin/mount", "-t", "sysfs", "-o", "nosuid,noexec,nodev",
+              "sysfs", os.fspath(tmp_path / "sys")], check=True),
+
+        call(["/usr/sbin/chroot", os.fspath(tmp_path), "/bin/true"], check=True),
+        call(["/usr/sbin/chroot", os.fspath(tmp_path), "/bin/false"], check=False),
+
+        call(["/usr/bin/umount", "--lazy", os.fspath(tmp_path / "proc")], check=False),
+        call(["/usr/bin/umount", "--lazy", os.fspath(tmp_path / "dev")], check=False),
+        call(["/usr/bin/umount", "--lazy", os.fspath(tmp_path / "sys")], check=False),
+    ]