Repository: osbuild/osbuild
Pull request #738 (2021-07-20 17:56:22)

Mounts are arrays
The order of entries in a dictionary is not specified by the JSON standard and hard to control when marshalling dictionaries in Go. Since the order of mounts is important and the wrong order leads to wrong mount trees change the `mounts` field to an array. This breaks existing manifests but after careful deliberation it was concluded that the original schema with mounts as dictionaries is not something we want to support. Apologies to everyone.

gicmo: Alternative for #737 if we are ok with changing `mounts` to be an array and thus breaking backwards compatibility (just one release though).
teg: I think this is ok. Needs a rebase though.
achilleas-k: Looking at it again alongside the changes in composer, is the name for the mount necessary now?
gicmo: > Looking at it again alongside the changes in composer, is the name for the mount necessary now?

Yeah, since I kept the internal structure the same, where we still have the dict. I guess it is technically not necessary but it makes things nicer.

Patch:
diff --git a/mounts/org.osbuild.noop b/mounts/org.osbuild.noop
new file mode 100755
index 00000000..15a4bfa9
--- /dev/null
+++ b/mounts/org.osbuild.noop
@@ -0,0 +1,47 @@
+#!/usr/bin/python3
+"""
+No-op mount service
+
+Does not mount anything, but only creates an empty directory.
+Useful for testing.
+
+Host commands used: mount
+"""
+
+import os
+import sys
+from typing import Dict
+
+from osbuild import mounts
+
+
+SCHEMA = """
+"additionalProperties": false
+"""
+
+
+class NoOpMount(mounts.MountService):
+
+    def mount(self, _source: str, root: str, target: str, _options: Dict):
+
+        mountpoint = os.path.join(root, target.lstrip("/"))
+
+        os.makedirs(mountpoint, exist_ok=True)
+        self.mountpoint = mountpoint
+
+        return mountpoint
+
+    def umount(self):
+        self.mountpoint = None
+
+    def sync(self):
+        pass
+
+
+def main():
+    service = NoOpMount.from_args(sys.argv[1:])
+    service.main()
+
+
+if __name__ == '__main__':
+    main()
diff --git a/osbuild/formats/v2.py b/osbuild/formats/v2.py
index ec660c0b..43310351 100644
--- a/osbuild/formats/v2.py
+++ b/osbuild/formats/v2.py
@@ -75,8 +75,9 @@ def describe(manifest: Manifest, *, with_id=False) -> Dict:
 
     def describe_mount(mnt):
         desc = {
+            "name": mnt.name,
             "type": mnt.info.name,
-            "device": mnt.device.name,
+            "source": mnt.device.name,
             "target": mnt.target
         }
 
@@ -85,10 +86,10 @@ def describe(manifest: Manifest, *, with_id=False) -> Dict:
         return desc
 
     def describe_mounts(mounts: Dict):
-        desc = {
-            name: describe_mount(mnt)
-            for name, mnt in mounts.items()
-        }
+        desc = [
+            describe_mount(mnt)
+            for mnt in mounts.values()
+        ]
         return desc
 
     def describe_stage(s: Stage):
@@ -214,10 +215,15 @@ def load_input(name: str, description: Dict, index: Index, stage: Stage, manifes
         ip.add_reference(r, desc)
 
 
-def load_mount(name: str, description: Dict, index: Index, stage: Stage):
+def load_mount(description: Dict, index: Index, stage: Stage):
     mount_type = description["type"]
     info = index.get_module_info("Mount", mount_type)
 
+    name = description["name"]
+
+    if name in stage.mounts:
+        raise ValueError(f"Duplicated mount '{name}'")
+
     source = description["source"]
     target = description["target"]
 
@@ -245,9 +251,9 @@ def load_stage(description: Dict, index: Index, pipeline: Pipeline, manifest: Ma
     for name, desc in ips.items():
         load_input(name, desc, index, stage, manifest, source_refs)
 
-    mounts = description.get("mounts", {})
-    for name, desc in mounts.items():
-        load_mount(name, desc, index, stage)
+    mounts = description.get("mounts", [])
+    for mount in mounts:
+        load_mount(mount, index, stage)
 
     return stage
 
@@ -394,6 +400,10 @@ def validate(manifest: Dict, index: Index) -> ValidationResult:
     def validate_stage_modules(klass, stage, path):
         group = ModuleInfo.MODULES[klass]
         items = stage.get(group, {})
+
+        if isinstance(items, list):
+            items = {i["name"]: i for i in items}
+
         for name, mod in items.items():
             validate_module(mod, klass, path + [group, name])
 
diff --git a/schemas/osbuild2.json b/schemas/osbuild2.json
index d7e7beb0..5261b751 100644
--- a/schemas/osbuild2.json
+++ b/schemas/osbuild2.json
@@ -26,7 +26,7 @@
     "device": {
       "title": "Device for a stage",
       "additionalProperties": false,
-      "required": ["type", "options"],
+      "required": ["type"],
       "properties": {
         "type": { "type": "string" },
         "options": {
@@ -63,16 +63,16 @@
 
     "mounts": {
       "title": "Collection of mount points for a stage",
-      "additionalProperties": {
-        "$ref": "#/definitions/mount"
-      }
+      "type": "array",
+      "items": { "$ref": "#/definitions/mount"}
     },
 
     "mount": {
       "title": "Mount point for a stage",
       "additionalProperties": false,
-      "required": ["type", "source", "target"],
+      "required": ["name", "type", "source", "target"],
       "properties": {
+        "name": { "type": "string" },
         "type": { "type": "string" },
         "source": {
           "type": "string"
diff --git a/stages/org.osbuild.copy b/stages/org.osbuild.copy
index 15e3cd7d..19330968 100755
--- a/stages/org.osbuild.copy
+++ b/stages/org.osbuild.copy
@@ -64,8 +64,7 @@ SCHEMA_2 = r"""
   "additionalProperties": true
 },
 "mounts": {
-  "type": "object",
-  "additionalProperties": true
+  "type": "array"
 },
 "inputs": {
   "type": "object",
diff --git a/stages/org.osbuild.noop b/stages/org.osbuild.noop
index 4b4d1ea8..e68c72da 100755
--- a/stages/org.osbuild.noop
+++ b/stages/org.osbuild.noop
@@ -17,8 +17,14 @@ SCHEMA_2 = """
 "options": {
   "additionalProperties": true
 },
+"devices": {
+  "additionalProperties": true
+},
 "inputs": {
   "additionalProperties": true
+},
+"mounts": {
+  "additionalProperties": true
 }
 """
 
diff --git a/stages/org.osbuild.zipl.inst b/stages/org.osbuild.zipl.inst
index d8bacff6..f5c9721d 100755
--- a/stages/org.osbuild.zipl.inst
+++ b/stages/org.osbuild.zipl.inst
@@ -47,15 +47,8 @@ SCHEMA_2 = r"""
   }
 },
 "mounts": {
-  "type": "object",
-  "additionalProperties": true,
-  "required": ["root"],
-  "properties": {
-    "root": {
-      "type": "object",
-      "additionalProperties": true
-    }
-  }
+  "type": "array",
+  "minItems": 1
 }
 """
 


Test patch:
diff --git a/test/data/manifests/fedora-ostree-bootiso.json b/test/data/manifests/fedora-ostree-bootiso.json
index 3f43bb97..3df7747a 100644
--- a/test/data/manifests/fedora-ostree-bootiso.json
+++ b/test/data/manifests/fedora-ostree-bootiso.json
@@ -1763,13 +1763,14 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.ext4",
               "source": "root",
               "target": "/"
             }
-          }
+          ]
         }
       ]
     },
@@ -1935,13 +1936,14 @@
               }
             }
           },
-          "mounts": {
-            "efi": {
+          "mounts": [
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.copy",
diff --git a/test/data/manifests/fedora-ostree-bootiso.mpp.json b/test/data/manifests/fedora-ostree-bootiso.mpp.json
index b2c55024..732eefe8 100644
--- a/test/data/manifests/fedora-ostree-bootiso.mpp.json
+++ b/test/data/manifests/fedora-ostree-bootiso.mpp.json
@@ -534,13 +534,14 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.ext4",
               "source": "root",
               "target": "/"
             }
-          }
+          ]
         }
       ]
     },
@@ -720,13 +721,14 @@
               }
             }
           },
-          "mounts": {
-            "efi": {
+          "mounts": [
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.copy",
diff --git a/test/data/manifests/fedora-ostree-image.json b/test/data/manifests/fedora-ostree-image.json
index c1ecb768..18c9e20b 100644
--- a/test/data/manifests/fedora-ostree-image.json
+++ b/test/data/manifests/fedora-ostree-image.json
@@ -1080,23 +1080,26 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.xfs",
               "source": "root",
               "target": "/"
             },
-            "boot": {
+            {
+              "name": "boot",
               "type": "org.osbuild.ext4",
               "source": "boot",
               "target": "/boot"
             },
-            "efi": {
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/boot/efi"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.grub2.inst",
diff --git a/test/data/manifests/fedora-ostree-image.mpp.json b/test/data/manifests/fedora-ostree-image.mpp.json
index 00ad6a60..4b8309c5 100644
--- a/test/data/manifests/fedora-ostree-image.mpp.json
+++ b/test/data/manifests/fedora-ostree-image.mpp.json
@@ -534,23 +534,26 @@
               }
             }
           },
-          "mounts": {
-            "root": {
+          "mounts": [
+            {
+              "name": "root",
               "type": "org.osbuild.xfs",
               "source": "root",
               "target": "/"
             },
-            "boot": {
+            {
+              "name": "boot",
               "type": "org.osbuild.ext4",
               "source": "boot",
               "target": "/boot"
             },
-            "efi": {
+            {
+              "name": "efi",
               "type": "org.osbuild.fat",
               "source": "efi",
               "target": "/boot/efi"
             }
-          }
+          ]
         },
         {
           "type": "org.osbuild.grub2.inst",
diff --git a/test/mod/test_fmt_v2.py b/test/mod/test_fmt_v2.py
index 6e5c8aaf..43df8fad 100644
--- a/test/mod/test_fmt_v2.py
+++ b/test/mod/test_fmt_v2.py
@@ -2,6 +2,7 @@
 # Tests specific for version 2 of the format
 #
 
+import copy
 import os
 import unittest
 
@@ -65,7 +66,35 @@ BASIC_PIPELINE = {
                                 "name:tree": {}
                             }
                         }
-                    }
+                    },
+                    "devices": {
+                        "root": {
+                            "type": "org.osbuild.loopback",
+                            "options": {
+                                "filename": "empty.img"
+                            }
+                        },
+                        "boot": {
+                            "type": "org.osbuild.loopback",
+                            "options": {
+                                "filename": "empty.img"
+                            }
+                        },
+                    },
+                    "mounts": [
+                        {
+                            "name": "root",
+                            "type": "org.osbuild.noop",
+                            "source": "root",
+                            "target": "/",
+                        },
+                        {
+                            "name": "boot",
+                            "type": "org.osbuild.noop",
+                            "source": "boot",
+                            "target": "/boot",
+                        }
+                    ]
                 }
             ]
         }
@@ -75,35 +104,36 @@ BASIC_PIPELINE = {
 BAD_SHA = "sha256:15a654d32efaa75b5df3e2481939d0393fe1746696cc858ca094ccf8b76073cd"
 
 BAD_REF_PIPELINE = {
-  "version": "2",
-  "sources": {
-    "org.osbuild.curl": {
-      "items": {
-        "sha256:c540ca8c5e21ba5f063286c94a088af2aac0b15bc40df6fd562d40154c10f4a1": "",
-      }
-    }
-  },
-  "pipelines": [
-    {
-      "name": "build",
-      "stages": [
-        {
-          "type": "org.osbuild.rpm",
-          "inputs": {
-            "packages": {
-              "type": "org.osbuild.files",
-              "origin": "org.osbuild.source",
-              "references": {
-                BAD_SHA: {}
-              }
+    "version": "2",
+    "sources": {
+        "org.osbuild.curl": {
+            "items": {
+                "sha256:c540ca8c5e21ba5f063286c94a088af2aac0b15bc40df6fd562d40154c10f4a1": "",
             }
-          }
         }
-      ]
-    }
-  ]
+    },
+    "pipelines": [
+        {
+            "name": "build",
+            "stages": [
+                {
+                    "type": "org.osbuild.rpm",
+                    "inputs": {
+                        "packages": {
+                            "type": "org.osbuild.files",
+                            "origin": "org.osbuild.source",
+                            "references": {
+                                BAD_SHA: {}
+                            }
+                        }
+                    }
+                }
+            ]
+        }
+    ]
 }
 
+
 class TestFormatV1(unittest.TestCase):
     def setUp(self):
         self.index = osbuild.meta.Index(os.curdir)
@@ -191,4 +221,88 @@ class TestFormatV1(unittest.TestCase):
             fmt.load(desc, self.index)
 
         self.assertTrue(str(ex.exception).find(BAD_SHA) > -1,
-            "The unknown source reference is not included in the exception")
+                        "The unknown source reference is not included in the exception")
+
+    def test_mounts(self):
+        BASE = {
+            "version": "2",
+
+            "pipelines": [
+                {
+                    "name": "test",
+                    "runner": "org.osbuild.linux",
+                    "stages": [
+                        {
+                            "type": "org.osbuild.noop",
+                            "options": {"zero": 0},
+                            "devices": {
+                                "root": {
+                                    "type": "org.osbuild.loopback",
+                                    "options": {
+                                        "filename": "empty.img"
+                                    }
+                                }
+                            },
+                            "mounts": []
+                        }
+                    ]
+                }
+            ]
+        }
+
+        # verify the device
+        pipeline = copy.deepcopy(BASE)
+        stage = pipeline["pipelines"][0]["stages"][0]
+        mounts = stage["mounts"]
+
+        mounts.extend([{
+            "name": "root",
+            "type": "org.osbuild.noop",
+            "source": "root",
+            "target": "/",
+        }])
+
+        manifest, _ = self.load_manifest(pipeline)
+        self.assertIsNotNone(manifest)
+        test = manifest["test"]
+        self.assertIsNotNone(test)
+        stage = test.stages[0]
+        root = stage.mounts["root"]
+        self.assertIsNotNone(root)
+        self.assertIsNotNone(root.device)
+        self.assertEqual(root.device.name, "root")
+
+        # duplicated mount
+        pipeline = copy.deepcopy(BASE)
+        stage = pipeline["pipelines"][0]["stages"][0]
+        mounts = stage["mounts"]
+
+        mounts.extend([{
+            "name": "root",
+            "type": "org.osbuild.noop",
+            "source": "root",
+            "target": "/",
+        }, {
+            "name": "root",
+            "type": "org.osbuild.noop",
+            "source": "root",
+            "target": "/",
+        }])
+
+        with self.assertRaises(ValueError):
+            self.load_manifest(pipeline)
+
+        # mount without a device
+        pipeline = copy.deepcopy(BASE)
+        stage = pipeline["pipelines"][0]["stages"][0]
+        mounts = stage["mounts"]
+
+        mounts.extend([{
+            "name": "boot",
+            "type": "org.osbuild.noop",
+            "source": "boot",
+            "target": "/boot",
+        }])
+
+        with self.assertRaises(ValueError):
+            self.load_manifest(pipeline)