Repository: osbuild/osbuild
Pull request #1724 (2024-04-11 13:04:33)

tools/osbuild-depsolve-dnf(5): require releasever
Originally, I made releasever required only when root_dir was set.  This was initially done to maintain backwards compatibility but we broke that already and osbuild/images will always include releasever in the request.

This will conflict with #1721 so maybe we can pull this commit into that PR (@mvo5?).

mvo5: > Originally, I made releasever required only when root_dir was set. This was initially done to maintain backwards compatibility but we broke that already and osbuild/images will always include releasever in the request.
> 
> This will conflict with #1721 so maybe we can pull this commit into that PR (@mvo5?).

Sure thing! I cherry picked it in there. The fun part is that cherry picking is really simple, I can just discard the (identical) changes to osbuild-depsolve-dnf5 :)

Patch:
diff --git a/mounts/org.osbuild.bind b/mounts/org.osbuild.bind
new file mode 100755
index 00000000..54d6f8d1
--- /dev/null
+++ b/mounts/org.osbuild.bind
@@ -0,0 +1,86 @@
+#!/usr/bin/python3
+"""
+Bind mount service
+
+Can (r)bind mount mounts to the tree.
+"""
+
+import os.path
+import subprocess
+import sys
+from typing import Dict
+from urllib.parse import urlparse
+
+from osbuild import mounts
+
+SCHEMA_2 = """
+"additionalProperties": false,
+"required": ["name", "type", "target"],
+"properties": {
+  "name": { "type": "string" },
+  "type": { "type": "string" },
+  "target": {
+    "type": "string",
+    "pattern": "^tree://"
+  },
+  "options": {
+    "required": ["source"],
+    "source": {
+      "type": "string",
+      "pattern": "^mount://"
+    }
+  }
+}
+"""
+
+
+def parse_location(location, tree, mountroot: str) -> str:
+    # we cannot use "osutil.util.parsing" here because it is too
+    # tightly coupled with how arguments for stages are passed
+    url = urlparse(location)
+    path = url.netloc
+    if url.scheme == "tree":
+        return os.path.join(tree, path.rstrip("/"))
+    if url.scheme == "mount":
+        return os.path.join(mountroot, path.rstrip("/"))
+    raise ValueError(f"unsupported schema {url.scheme} for {location}")
+
+
+class BindMount(mounts.MountService):
+
+    def __init__(self, args):
+        super().__init__(args)
+        self.mountpoint = ""
+
+    def mount(self, args: Dict):
+        tree = args["tree"]
+        mountroot = args["root"]
+        target = args["target"]
+        # we cannot use args["sources"] here because the osbuild code makes
+        # many assumptions about that it must link back to a "Device" so
+        # we follow the pattern from org.osbuild.ostree.deployment here
+        # and put it into "options"
+        options = args["options"]
+        source = parse_location(options.get("source"), tree, mountroot)
+        self.mountpoint = parse_location(target, tree, mountroot)
+        subprocess.run([
+            "mount",
+            "--rbind", source, self.mountpoint,
+        ], check=True)
+
+    def umount(self):
+        if self.mountpoint:
+            subprocess.run(["umount", "-R", "-v", self.mountpoint], check=True)
+            self.mountpoint = ""
+
+    def sync(self):
+        pass
+
+
+def main():
+    service = BindMount.from_args(sys.argv[1:])
+    service.main()
+
+
+if __name__ == '__main__':
+    main()
diff --git a/tools/osbuild-depsolve-dnf b/tools/osbuild-depsolve-dnf
index 18592b72..dbffdb59 100755
--- a/tools/osbuild-depsolve-dnf
+++ b/tools/osbuild-depsolve-dnf
@@ -63,8 +63,7 @@ class Solver():
         self.base.conf.cachedir = cache_dir
         self.base.conf.substitutions['arch'] = arch
         self.base.conf.substitutions['basearch'] = dnf.rpm.basearch(arch)
-        if releasever:
-            self.base.conf.substitutions['releasever'] = releasever
+        self.base.conf.substitutions['releasever'] = releasever
         if proxy:
             self.base.conf.proxy = proxy
 
@@ -445,7 +444,11 @@ def validate_request(request):
             "kind": "InvalidRequest",
             "reason": "no 'module_platform_id' specified"
         }
-
+    if not request.get("releasever"):
+        return {
+            "kind": "InvalidRequest",
+            "reason": "no 'module_platform_id' specified"
+        }
     arguments = request.get("arguments")
     if not arguments:
         return {
@@ -459,16 +462,6 @@ def validate_request(request):
             "reason": "no 'repos' or 'root_dir' specified"
         }
 
-    # if root_dir is used, we also need releasever
-    # pylint: disable=fixme
-    # TODO: Make releasever mandatory in all cases.
-    # We temporarily keep it tied to root_dir for short-term backwards compatibility
-    if arguments.get("root_dir") and not request.get("releasever"):
-        return {
-            "kind": "InvalidRequest",
-            "reason": "'root_dir' requires setting 'releasever'"
-        }
-
     return None
 
 
diff --git a/tools/osbuild-depsolve-dnf5 b/tools/osbuild-depsolve-dnf5
index 252c2a15..afac612e 100755
--- a/tools/osbuild-depsolve-dnf5
+++ b/tools/osbuild-depsolve-dnf5
@@ -570,7 +570,11 @@ def validate_request(request):
             "kind": "InvalidRequest",
             "reason": "no 'module_platform_id' specified"
         }
-
+    if not request.get("releasever"):
+        return {
+            "kind": "InvalidRequest",
+            "reason": "no 'module_platform_id' specified"
+        }
     arguments = request.get("arguments")
     if not arguments:
         return {
@@ -584,16 +588,6 @@ def validate_request(request):
             "reason": "no 'repos' or 'root_dir' specified"
         }
 
-    # if root_dir is used, we also need releasever
-    # pylint: disable=fixme
-    # TODO: Make releasever mandatory in all cases.
-    # We temporarily keep it tied to root_dir for short-term backwards compatibility
-    if arguments.get("root_dir") and not request.get("releasever"):
-        return {
-            "kind": "InvalidRequest",
-            "reason": "'root_dir' requires setting 'releasever'"
-        }
-
     return None
 
 
diff --git a/tox.ini b/tox.ini
index 18f5e3e5..fac477d2 100644
--- a/tox.ini
+++ b/tox.ini
@@ -24,7 +24,7 @@ deps =
     requests
 
 setenv =
-    LINTABLES = osbuild/ assemblers/* devices/* inputs/*.* mounts/* runners/* sources/*.* stages/*.* inputs/test/*.py stages/test/*.py sources/test/*.py test/ tools/
+    LINTABLES = osbuild/ assemblers/* devices/* inputs/*.* mounts/*.* mounts/test/*.py runners/* sources/*.* stages/*.* inputs/test/*.py stages/test/*.py sources/test/*.py test/ tools/
     LINTABLES_EXCLUDES = "*.json,*.sh"
     LINTABLES_EXCLUDES_RE = ".*\.json$,.*\.sh"
     TYPEABLES = osbuild


Test patch:
diff --git a/mounts/test/conftest.py b/mounts/test/conftest.py
new file mode 100644
index 00000000..28f926b2
--- /dev/null
+++ b/mounts/test/conftest.py
@@ -0,0 +1,33 @@
+import os
+import pathlib
+from types import ModuleType
+
+import pytest
+
+from osbuild import mounts, testutil
+from osbuild.testutil.imports import import_module_from_path
+
+
+@pytest.fixture(name="mounts_module")
+def mounts_module_fixture(request: pytest.FixtureRequest) -> ModuleType:
+    """mounts_module is a fixture that imports a stage module by its name
+    defined in MOUNTS_NAME in the test module.
+    """
+    if not hasattr(request.module, "MOUNTS_NAME"):
+        raise ValueError("mounts_module fixture must be used in a test module that defines MOUNTS_NAME")
+
+    mounts_name = request.module.MOUNTS_NAME
+    caller_dir = pathlib.Path(request.node.fspath).parent
+    module_path = caller_dir.parent / mounts_name
+    return import_module_from_path("mounts", os.fspath(module_path))
+
+
+@pytest.fixture
+def mounts_service(mounts_module) -> ModuleType:
+    """mounts_service is a fixture that imports a mounts module by its name
+    defined in MOUNTS_NAME in the test module and returns a MountsService
+    """
+    service_cls = testutil.find_one_subclass_in_module(mounts_module, mounts.MountService)
+    fd = testutil.make_fake_service_fd()
+    srv_obj = service_cls.from_args(["--service-fd", str(fd)])
+    return srv_obj
diff --git a/mounts/test/test_bind.py b/mounts/test/test_bind.py
new file mode 100644
index 00000000..2a3c701e
--- /dev/null
+++ b/mounts/test/test_bind.py
@@ -0,0 +1,100 @@
+#!/usr/bin/python3
+
+import os
+import pathlib
+from unittest.mock import patch
+
+import pytest
+
+import osbuild.meta
+from osbuild import testutil
+
+MOUNTS_NAME = "org.osbuild.bind"
+
+
+@patch("subprocess.run")
+def test_bind_mount_simple(mocked_run, mounts_service):
+    fake_tree_path = "/tmp/osbuild/.osbuild/tmp/buildroot-tmp-_ym0rt5a/mounts/"
+    fake_mountroot_path = "/tmp/osbuild/.osbuild/stage/uuid-e7443c948dff406d88bcbb7658a31f0f/data/tree/"
+
+    fake_args = {
+        "tree": fake_tree_path,
+        "root": fake_mountroot_path,
+        "target": "tree://",
+        "options": {
+            "source": "mount://",
+        }
+    }
+    mounts_service.mount(fake_args)
+    assert len(mocked_run.call_args_list) == 1
+    args, kwargs = mocked_run.call_args_list[0]
+    assert args[0] == ["mount", "--rbind", fake_mountroot_path, fake_tree_path]
+    assert kwargs == {"check": True}
+
+
+@patch("subprocess.run")
+def test_bind_umount_simple(mocked_run, mounts_service):
+    # nothing mounted yet, nothing is done
+    mounts_service.umount()
+    assert len(mocked_run.call_args_list) == 0
+    # pretend a mount
+    mounts_service.mountpoint = "/something"
+    mounts_service.umount()
+    assert len(mocked_run.call_args_list) == 1
+    args, kwargs = mocked_run.call_args_list[0]
+    assert args[0] == ["umount", "-R", "-v", "/something"]
+    assert kwargs == {"check": True}
+    # mountpoint is cleared
+    assert mounts_service.mountpoint == ""
+    # ensure we do not umount twice
+    mounts_service.umount()
+    assert len(mocked_run.call_args_list) == 1
+
+
+@pytest.mark.parametrize("test_data,expected_err", [
+    # bad
+    ({}, "'name' is a required property"),
+    ({}, "'target' is a required property"),
+    ({}, "'source' is a required property"),
+    # good
+    ({"name": "bind", "target": "tree://", "options": {"source": "mount://"}}, ""),
+])
+def test_parameters_validation(test_data, expected_err):
+    root = pathlib.Path(__file__).parent.parent.parent
+    mod_info = osbuild.meta.ModuleInfo.load(root, "Mount", MOUNTS_NAME)
+    schema = osbuild.meta.Schema(mod_info.get_schema(), MOUNTS_NAME)
+    test_input = {
+        "type": MOUNTS_NAME,
+        "options": {}
+    }
+    test_input.update(test_data)
+    res = 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)
+
+
+@pytest.mark.skipif(os.getuid() != 0, reason="needs root")
+def test_bind_mount_integration(tmp_path, mounts_service):
+    fake_tree_path = tmp_path / "mounts"
+    fake_tree_path.mkdir(parents=True, exist_ok=True)
+
+    fake_mountroot_path = tmp_path / "uuid-1234/data/tree"
+    fake_mountroot_path.mkdir(parents=True, exist_ok=True)
+    (fake_mountroot_path / "in-src-mnt").write_text("", encoding="utf8")
+
+    fake_args = {
+        "tree": fake_tree_path,
+        "root": fake_mountroot_path,
+        "target": "tree://",
+        "options": {
+            "source": "mount://",
+        }
+    }
+    assert not (fake_tree_path / "in-src-mnt").exists()
+    mounts_service.mount(fake_args)
+    assert (fake_tree_path / "in-src-mnt").exists()
+    mounts_service.umount()
+    assert not (fake_tree_path / "in-src-mnt").exists()
diff --git a/tools/test/test_depsolve.py b/tools/test/test_depsolve.py
index 4cfe53e3..13db8b37 100644
--- a/tools/test/test_depsolve.py
+++ b/tools/test/test_depsolve.py
@@ -2,6 +2,7 @@ import configparser
 import importlib
 import json
 import os
+import pathlib
 import socket
 import subprocess as sp
 from tempfile import TemporaryDirectory
@@ -182,7 +183,23 @@ test_cases = [
 ]
 
 
-def config_combos(servers):
+def make_dnf_scafolding(base_dir):
+    root_dir = pathlib.Path(TemporaryDirectory(dir=base_dir).name)
+
+    repos_dir = root_dir / "etc/yum.repos.d"
+    repos_dir.mkdir(parents=True)
+    keys_dir = root_dir / "etc/pki/rpm-gpg"
+    keys_dir.mkdir(parents=True)
+    vars_dir = root_dir / "etc/dnf/vars"
+    vars_dir.mkdir(parents=True)
+
+    vars_path = vars_dir / "customvar"
+    vars_path.write_text(CUSTOMVAR, encoding="utf8")
+
+    return root_dir, repos_dir, keys_dir
+
+
+def config_combos(tmp_path, servers):
     """
     Return all configurations for the provided repositories, either as config files in a directory or as repository
     configs in the depsolve request, or a combination of both.
@@ -207,43 +224,31 @@ def config_combos(servers):
                 "rhsm": False,
                 "gpgkeys": [TEST_KEY + server["name"]],
             })
-        with TemporaryDirectory() as root_dir:
-            repos_dir = os.path.join(root_dir, "etc/yum.repos.d")
-            os.makedirs(repos_dir)
-            keys_dir = os.path.join(root_dir, "etc/pki/rpm-gpg")
-            os.makedirs(keys_dir)
-            vars_dir = os.path.join(root_dir, "etc/dnf/vars")
-            os.makedirs(vars_dir)
-
-            vars_path = os.path.join(vars_dir, "customvar")
-            with open(vars_path, "w", encoding="utf-8") as vars_file:
-                vars_file.write(CUSTOMVAR)
-
-            for idx in combo[1]:  # servers to be configured through root_dir
-                server = servers[idx]
-                name = server["name"]
-                # Use the gpgkey to test both the key reading and the variable substitution.
-                # For this test, it doesn't need to be a real key.
-                key_url = f"file:///etc/pki/rpm-gpg/RPM-GPG-KEY-$releasever-$basearch-$customvar-{name}"
-
-                key_path = os.path.join(keys_dir, f"RPM-GPG-KEY-{RELEASEVER}-{ARCH}-{CUSTOMVAR}-{name}")
-                with open(key_path, "w", encoding="utf-8") as key_file:
-                    key_file.write(TEST_KEY + name)
-                parser = configparser.ConfigParser()
-                parser.add_section(name)
-                # Set some options in a specific order in which they tend to be
-                # written in repo files.
-                parser.set(name, "name", name)
-                parser.set(name, "baseurl", server["address"])
-                parser.set(name, "enabled", "1")
-                parser.set(name, "gpgcheck", "1")
-                parser.set(name, "sslverify", "0")
-                parser.set(name, "gpgkey", key_url)
-
-                with open(f"{repos_dir}/{name}.repo", "w", encoding="utf-8") as repo_file:
-                    parser.write(repo_file, space_around_delimiters=False)
-
-            yield repo_configs, root_dir
+        root_dir, repos_dir, keys_dir = make_dnf_scafolding(tmp_path)
+        for idx in combo[1]:  # servers to be configured through root_dir
+            server = servers[idx]
+            name = server["name"]
+            # Use the gpgkey to test both the key reading and the variable substitution.
+            # For this test, it doesn't need to be a real key.
+            key_url = f"file:///etc/pki/rpm-gpg/RPM-GPG-KEY-$releasever-$basearch-$customvar-{name}"
+
+            key_path = keys_dir / f"RPM-GPG-KEY-{RELEASEVER}-{ARCH}-{CUSTOMVAR}-{name}"
+            key_path.write_text(TEST_KEY + name, encoding="utf8")
+            parser = configparser.ConfigParser()
+            parser.add_section(name)
+            # Set some options in a specific order in which they tend to be
+            # written in repo files.
+            parser.set(name, "name", name)
+            parser.set(name, "baseurl", server["address"])
+            parser.set(name, "enabled", "1")
+            parser.set(name, "gpgcheck", "1")
+            parser.set(name, "sslverify", "0")
+            parser.set(name, "gpgkey", key_url)
+
+            with (repos_dir / f"{name}.repo").open("w", encoding="utf-8") as fp:
+                parser.write(fp, space_around_delimiters=False)
+
+        yield repo_configs, os.fspath(root_dir)
 
 
 @pytest.fixture(name="cache_dir", scope="session")
@@ -252,10 +257,10 @@ def cache_dir_fixture(tmpdir_factory):
 
 
 @pytest.mark.parametrize("test_case", test_cases)
-def test_depsolve(repo_servers, test_case, cache_dir):
+def test_depsolve(tmp_path, repo_servers, test_case, cache_dir):
     pks = test_case["packages"]
 
-    for repo_configs, root_dir in config_combos(repo_servers):
+    for repo_configs, root_dir in config_combos(tmp_path, repo_servers):
         res = depsolve(pks, repo_configs, root_dir, cache_dir, "./tools/osbuild-depsolve-dnf")
         assert {pkg["name"] for pkg in res["packages"]} == test_case["results"]["packages"]
         assert res["repos"].keys() == test_case["results"]["reponames"]
@@ -265,10 +270,10 @@ def test_depsolve(repo_servers, test_case, cache_dir):
 
 @pytest.mark.skipif(not has_dnf5(), reason="libdnf5 not available")
 @pytest.mark.parametrize("test_case", test_cases)
-def test_depsolve_dnf5(repo_servers, test_case, cache_dir):
+def test_depsolve_dnf5(tmp_path, repo_servers, test_case, cache_dir):
     pks = test_case["packages"]
 
-    for repo_configs, repos_dir in config_combos(repo_servers):
+    for repo_configs, repos_dir in config_combos(tmp_path, repo_servers):
         res = depsolve(pks, repo_configs, repos_dir, cache_dir, "./tools/osbuild-depsolve-dnf5")
         assert {pkg["name"] for pkg in res["packages"]} == test_case["results"]["packages"]
         assert res["repos"].keys() == test_case["results"]["reponames"]