Repository: osbuild/osbuild
Pull request #1519 (2023-12-22 16:39:37)

create org.osbuild.bootupd stage
Add the bootupd stage to install GRUB on both BIOS and UEFI systems,
ensuring that your bootloader stays up-to-date.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>               


dustymabe: The 2nd commit here will be dropped. The 3rd and 4th commits are optional and stem from a [previous conversation](https://github.com/osbuild/osbuild/pull/1475#discussion_r1429789726) between @achilleas-k @cgwalters and I. Over there I don't think we fully fleshed out if the optimization was completely out of the question or not. Maybe I should just open a separate PR with that optimization so we could focus discussion on it, but at the same time I don't want to introduce options to the `bootupd` stage here if we'd remove them later.
achilleas-k: > Maybe I should just open a separate PR with that optimization so we could focus discussion on it, but at the same time I don't want to introduce options to the `bootupd` stage here if we'd remove them later.

That would be preferable, yeah.  Can you open it separately so we can talk about that there and then keep this specific to bootupd?  Thanks!
dustymabe: I broke out the controversial commits into https://github.com/osbuild/osbuild/pull/1527

You can now review just the first commit in this PR. The 2nd commit is just there to make tests pass until we can update the registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos next week when a new build of FCOS is out that we can use.
dustymabe: @achilleas-k can you pull `quay.io/fedora/fedora-coreos:testing` and push it to `registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:testing` and I'll update this so tests pass and we can hopefully merge.

You can delete `registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:stable` if you want as it should be unused now.
achilleas-k: > @achilleas-k can you pull `quay.io/fedora/fedora-coreos:testing` and push it to `registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:testing` and I'll update this so tests pass and we can hopefully merge.


Done: https://gitlab.com/redhat/services/products/image-builder/ci/images/container_registry/5710257
dustymabe: > > @achilleas-k can you pull `quay.io/fedora/fedora-coreos:testing` and push it to `registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:testing` and I'll update this so tests pass and we can hopefully merge.
> 
> Done: https://gitlab.com/redhat/services/products/image-builder/ci/images/container_registry/5710257

Thanks! Pushed a new update and rebased on top of latest `main`. Hopefully the tests pass!
dustymabe: > 1. The last two commits have subjects that are too long.  Can we keep them under 72 chars?

Fixed.

> 
> 2. Can we also clean up the history a bit? (see comment below).

Tried to fix.

> 
> 3. The `disk` option under `bios`: Making enforcing the name through the schema for the `devices` object would make it consistent with how we do it in other stages, but if we prefer to keep it explicit, I'd change the name of the option to make it clear that it maps to a "device".

I was learning your way on this, but then Renata and I found a gap in
our coverage here (see the newest commit) where on ppc64le we need to
be able to install into a partition of a device so I added the
`partition` key too. Otherwise we could have made `bios: true/false` a
boolean.

achilleas-k: > I was learning your way on this, but then Renata and I found a gap in
> our coverage here (see the newest commit) where on ppc64le we need to
> be able to install into a partition of a device so I added the
> `partition` key too. Otherwise we could have made `bios: true/false` a
> boolean.

You could specify the `start` and `size` options of the device to create a loopback that points to the specific partition.  In the test case for example it look like this:
```json
        {
          "type": "org.osbuild.bootupd",
          "options": {
            "bios": true,
            "static-configs": true,
            "deployment": {
              "osname": "fedora-coreos",
              "ref": "ostree/1/1/0"
            }
          },
          "devices": {
            "disk": {
              "type": "org.osbuild.loopback",
              "options": {
                "filename": "disk.img",
                "start": 4096,
                "size": 260096
              }
            }
          }
        }
```

Would that work?
dustymabe: > Would that work?

unfortunately not. the `grub2-install` stuff really doesn't like it when you map in a slice of a device and not the whole device itself :( 
achilleas-k: > > Would that work?
> 
> unfortunately not. the `grub2-install` stuff really doesn't like it when you map in a slice of a device and not the whole device itself :(

Right, of course -_-

Patch:
diff --git a/stages/org.osbuild.bootupd b/stages/org.osbuild.bootupd
new file mode 100755
index 00000000..224bd6d4
--- /dev/null
+++ b/stages/org.osbuild.bootupd
@@ -0,0 +1,149 @@
+#!/usr/bin/python3
+"""
+Install GRUB on both BIOS and UEFI systems,
+ensuring that your bootloader stays up-to-date.
+
+Bootupd supports updating GRUB and shim for
+UEFI firmware on x86_64 and aarch64,
+and GRUB for BIOS firmware on x86_64.
+The project is deployed in Fedora CoreOS and derivatives
+"""
+
+import contextlib
+import json
+import os
+import subprocess
+import sys
+
+import osbuild.api
+from osbuild.util import ostree
+
+SCHEMA_2 = r"""
+"devices": {
+  "type": "object",
+  "additionalProperties": true
+},
+"mounts": {
+  "type": "array"
+},
+"options": {
+  "additionalProperties": false,
+  "properties": {
+    "deployment": {
+      "type": "object",
+      "additionalProperties": false,
+      "required": ["osname", "ref"],
+      "properties": {
+        "osname": {
+          "description": "Name of the stateroot to be used in the deployment",
+          "type": "string"
+        },
+        "ref": {
+          "description": "OStree ref to create and use for deployment",
+          "type": "string"
+        },
+        "serial": {
+          "description": "The deployment serial (usually '0')",
+          "type": "number",
+          "default": 0
+        }
+      }
+    },
+    "static-configs": {
+      "description": "Install the grub configs defined for Fedora CoreOS",
+      "type": "boolean"
+    },
+    "bios": {
+      "additionalProperties": false,
+      "type": "object",
+      "required": ["device"],
+      "properties": {
+        "device": {
+            "description": "Name of stage device to install GRUB for BIOS-based systems.",
+            "type": "string"
+        },
+        "partition": {
+            "description": "The partition on the stage device to install to, if installing to a partition",
+            "type": "number"
+        }
+      }
+    }
+  }
+}
+"""
+
+
+@contextlib.contextmanager
+def bind_mounts(sources, dest_root):
+    for src in sources:
+        dst = os.path.join(dest_root, src.lstrip("/"))
+        subprocess.run(["mount", "--rbind", src, dst], check=True)
+    try:
+        yield
+    finally:
+        for src in reversed(sources):
+            dst = os.path.join(dest_root, src.lstrip("/"))
+            subprocess.run(["umount", "--recursive", dst], check=False)
+
+
+def main(args, options):
+    deployment = options.get("deployment", None)
+    static_configs = options.get("static-configs", False)
+    bios = options.get("bios", {})
+    device = bios.get("device", "")
+    partition = bios.get("partition", None)
+
+    # Get the path where the filesystems are mounted
+    mounts = args["paths"]["mounts"]
+
+    # Get the deployment root. For non-OSTree this is simply
+    # the root location of the mount points. For OSTree systems
+    # we'll call ostree.deployment_path() helper to find it for us.
+    root = mounts
+    if deployment:
+        osname = deployment["osname"]
+        ref = deployment["ref"]
+        serial = deployment.get("serial", 0)
+        root = ostree.deployment_path(mounts, osname, ref, serial)
+
+    bootupd_args = []
+    if device:
+        # The value passed by the user is the name of the device
+        # as specified in the devices array (also passed in by the
+        # user). Let's map that name to the actual loopback device
+        # that now backs it.
+        target = args["devices"][device]["path"]
+        # If we are targeting a partition on the device rather than
+        # the whole device itself (i.e. MBR) then let's find that
+        # partition's device.
+        if partition:
+            cp = subprocess.run(["sfdisk", "--json", target],
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.STDOUT,
+                                encoding='utf8',
+                                check=True)
+            disk_table = json.loads(cp.stdout)["partitiontable"]
+            disk_parts = disk_table["partitions"]
+            index = partition - 1  # partition index starts at 0
+            target = disk_parts[index]["node"]
+        bootupd_args.append(f"--device={target}")
+    if static_configs:
+        bootupd_args.append("--with-static-configs")
+
+    # We want to run the bootupctl command from the target (i.e. we
+    # want to make sure the version used matches the target and not
+    # risk any inconsistencies with the build root). Let's set up
+    # and chroot to run the bootupctl command from the target.
+    with bind_mounts(['/dev', '/proc', '/sys', '/run', '/var', '/tmp'], root):
+        cmd = ['chroot', root, '/usr/bin/bootupctl', 'backend', 'install']
+        cmd.extend(bootupd_args)
+        cmd.append(mounts)
+        subprocess.run(cmd, check=True)
+
+    return 0
+
+
+if __name__ == '__main__':
+    _args = osbuild.api.arguments()
+    r = main(_args, _args["options"])
+    sys.exit(r)


Test patch:
diff --git a/stages/test/test_bootupd.py b/stages/test/test_bootupd.py
new file mode 100644
index 00000000..9df845d3
--- /dev/null
+++ b/stages/test/test_bootupd.py
@@ -0,0 +1,118 @@
+#!/usr/bin/python3
+
+import os.path
+from unittest.mock import call, patch
+
+import pytest
+
+import osbuild.meta
+from osbuild.testutil.imports import import_module_from_path
+
+
+@pytest.mark.parametrize("test_data,expected_err", [
+    # bad
+    ({"deployment": "must-be-object"}, "'must-be-object' is not of type 'object'"),
+    ({"deployment": {"osname": "some-os"}}, "'ref' is a required property"),
+    ({"deployment": {"ref": "some-ref"}}, "'osname' is a required property"),
+    ({"deployment": {"osname": "some-os", "ref": "some-ref", "serial": "must-be-number"}},
+     "'must-be-number' is not of type 'number'"),
+    ({"random": "property"}, "Additional properties are not allowed"),
+    ({"bios": {}}, "'device' is a required property"),
+    ({"bios": "must-be-object"}, "'must-be-object' is not of type 'object'"),
+    # good
+    ({}, ""),
+    ({
+        "deployment":
+        {
+            "osname": "some-os",
+            "ref": "some-ref",
+            "serial": 1,
+        },
+        "static-configs": True,
+        "bios":
+        {
+            "device": "/dev/sda",
+        },
+    }, "")
+
+])
+def test_bootupd_schema_validation(test_data, expected_err):
+    name = "org.osbuild.bootupd"
+    root = os.path.join(os.path.dirname(__file__), "../..")
+    mod_info = osbuild.meta.ModuleInfo.load(root, "Stage", name)
+    schema = osbuild.meta.Schema(mod_info.get_schema(version="2"), name)
+
+    test_input = {
+        "type": "org.osbuild.bootupd",
+        "options": {
+        }
+    }
+    test_input["options"].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
+        assert len(res.errors) == 1, [e.as_dict() for e in res.errors]
+        err_msgs = [e.as_dict()["message"] for e in res.errors]
+        assert expected_err in err_msgs[0]
+
+
+@pytest.mark.parametrize("test_options,expected_bootupd_opts", [
+    ({}, []),
+    ({"bios": {"device": "/dev/sda"}}, ["--device=/mapped/dev/sda"]),
+    ({"static-configs": True}, ["--with-static-configs"]),
+])
+@patch("subprocess.run")
+def test_bootupd_defaults(mocked_run, test_options, expected_bootupd_opts):
+    stage_path = os.path.join(os.path.dirname(__file__), "../org.osbuild.bootupd")
+    stage = import_module_from_path("bootupd_stage", stage_path)
+
+    args = {
+        "paths": {
+            "mounts": "/run/osbuild/mounts",
+        },
+        "devices": {
+            "/dev/sda": {
+                "path": "/mapped/dev/sda",
+            },
+        },
+    }
+    options = test_options
+    with patch.object(stage, "bind_mounts") as mocked_bind_mounts:
+        stage.main(args, options)
+
+    assert len(mocked_bind_mounts.call_args_list) == 1
+    assert mocked_run.call_args_list == [call(["chroot",
+                                               "/run/osbuild/mounts",
+                                               "/usr/bin/bootupctl",
+                                               "backend",
+                                               "install"] + expected_bootupd_opts + ["/run/osbuild/mounts"],
+                                              check=True),
+                                         ]
+
+
+@patch("subprocess.run")
+def test_bootupd_bind_mounts(mocked_run):
+    stage_path = os.path.join(os.path.dirname(__file__), "../org.osbuild.bootupd")
+    stage = import_module_from_path("bootupd_stage", stage_path)
+
+    dst = "/run/osbuild/mounts/"
+    with stage.bind_mounts(['/dev', '/proc', '/sys', '/run', '/var', '/tmp'], dst):
+        pass
+    assert mocked_run.call_args_list == [
+        call(["mount", "--rbind", "/dev", "/run/osbuild/mounts/dev"], check=True),
+        call(["mount", "--rbind", "/proc", "/run/osbuild/mounts/proc"], check=True),
+        call(["mount", "--rbind", "/sys", "/run/osbuild/mounts/sys"], check=True),
+        call(["mount", "--rbind", "/run", "/run/osbuild/mounts/run"], check=True),
+        call(["mount", "--rbind", "/var", "/run/osbuild/mounts/var"], check=True),
+        call(["mount", "--rbind", "/tmp", "/run/osbuild/mounts/tmp"], check=True),
+        # and umount in reverse order
+        call(["umount", "--recursive", "/run/osbuild/mounts/tmp"], check=False),
+        call(["umount", "--recursive", "/run/osbuild/mounts/var"], check=False),
+        call(["umount", "--recursive", "/run/osbuild/mounts/run"], check=False),
+        call(["umount", "--recursive", "/run/osbuild/mounts/sys"], check=False),
+        call(["umount", "--recursive", "/run/osbuild/mounts/proc"], check=False),
+        call(["umount", "--recursive", "/run/osbuild/mounts/dev"], check=False),
+    ]
diff --git a/test/data/manifests/fedora-coreos-container.json b/test/data/manifests/fedora-coreos-container.json
index e253a61e..2dbb5617 100644
--- a/test/data/manifests/fedora-coreos-container.json
+++ b/test/data/manifests/fedora-coreos-container.json
@@ -504,8 +504,8 @@
               "type": "org.osbuild.containers",
               "origin": "org.osbuild.source",
               "references": {
-                "sha256:3bdf633fb6c027a01688aecf1d3f33bbecf8975b138c1861cb89ae20f3e3e7db": {
-                  "name": "registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:stable"
+                "sha256:66d1012cb68ea1ec0c38a830499feadb2c9e015162a5acb818f65afb6f25d0e5": {
+                  "name": "registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:testing"
                 }
               }
             }
@@ -529,25 +529,6 @@
               "ref": "ostree/1/1/0"
             }
           }
-        },
-        {
-          "type": "org.osbuild.grub2",
-          "options": {
-            "rootfs": {
-              "label": "root"
-            },
-            "bootfs": {
-              "label": "boot"
-            },
-            "uefi": {
-              "vendor": "fedora",
-              "install": true
-            },
-            "legacy": "i386-pc",
-            "write_defaults": false,
-            "greenboot": false,
-            "ignition": true
-          }
         }
       ]
     },
@@ -715,23 +696,49 @@
           ]
         },
         {
-          "type": "org.osbuild.grub2.inst",
+          "type": "org.osbuild.bootupd",
           "options": {
-            "platform": "i386-pc",
-            "filename": "disk.img",
-            "location": 2048,
-            "core": {
-              "type": "mkimage",
-              "partlabel": "gpt",
-              "filesystem": "ext4"
+            "bios": {
+              "device": "disk"
             },
-            "prefix": {
-              "type": "partition",
-              "partlabel": "gpt",
-              "number": 2,
-              "path": "/grub2"
+            "static-configs": true,
+            "deployment": {
+              "osname": "fedora-coreos",
+              "ref": "ostree/1/1/0"
             }
-          }
+          },
+          "devices": {
+            "disk": {
+              "type": "org.osbuild.loopback",
+              "options": {
+                "filename": "disk.img",
+                "partscan": true
+              }
+            }
+          },
+          "mounts": [
+            {
+              "name": "root",
+              "type": "org.osbuild.xfs",
+              "source": "disk",
+              "partition": 4,
+              "target": "/"
+            },
+            {
+              "name": "boot",
+              "type": "org.osbuild.ext4",
+              "source": "disk",
+              "partition": 3,
+              "target": "/boot"
+            },
+            {
+              "name": "efi",
+              "type": "org.osbuild.fat",
+              "source": "disk",
+              "partition": 2,
+              "target": "/boot/efi"
+            }
+          ]
         }
       ]
     },
@@ -902,6 +909,49 @@
               "target": "/boot/efi"
             }
           ]
+        },
+        {
+          "type": "org.osbuild.bootupd",
+          "options": {
+            "static-configs": true,
+            "deployment": {
+              "osname": "fedora-coreos",
+              "ref": "ostree/1/1/0"
+            }
+          },
+          "devices": {
+            "disk": {
+              "type": "org.osbuild.loopback",
+              "options": {
+                "filename": "disk.img",
+                "partscan": true,
+                "sector-size": 4096
+              }
+            }
+          },
+          "mounts": [
+            {
+              "name": "root",
+              "type": "org.osbuild.xfs",
+              "source": "disk",
+              "partition": 4,
+              "target": "/"
+            },
+            {
+              "name": "boot",
+              "type": "org.osbuild.ext4",
+              "source": "disk",
+              "partition": 3,
+              "target": "/boot"
+            },
+            {
+              "name": "efi",
+              "type": "org.osbuild.fat",
+              "source": "disk",
+              "partition": 2,
+              "target": "/boot/efi"
+            }
+          ]
         }
       ]
     },
@@ -1562,10 +1612,10 @@
     },
     "org.osbuild.skopeo": {
       "items": {
-        "sha256:3bdf633fb6c027a01688aecf1d3f33bbecf8975b138c1861cb89ae20f3e3e7db": {
+        "sha256:66d1012cb68ea1ec0c38a830499feadb2c9e015162a5acb818f65afb6f25d0e5": {
           "image": {
             "name": "registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos",
-            "digest": "sha256:f0ebfc8bb813e45225a93e76780c44439fac11a24ba8fb83001ec96a61ebbb12"
+            "digest": "sha256:2ba8999ef74d45f39c601c20783d61a1c632636eb3e8ead9c020e29bfbbae8bf"
           }
         }
       }
diff --git a/test/data/manifests/fedora-coreos-container.mpp.yaml b/test/data/manifests/fedora-coreos-container.mpp.yaml
index aefda0d9..27983e54 100644
--- a/test/data/manifests/fedora-coreos-container.mpp.yaml
+++ b/test/data/manifests/fedora-coreos-container.mpp.yaml
@@ -112,7 +112,7 @@ pipelines:
             mpp-resolve-images:
               images:
                 - source: registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos
-                  tag: stable
+                  tag: testing
       - type: org.osbuild.ostree.aleph
         options:
           coreos_compat: true
@@ -124,19 +124,6 @@ pipelines:
           deployment:
             osname: fedora-coreos
             ref: ostree/1/1/0
-      - type: org.osbuild.grub2
-        options:
-          rootfs:
-            label: root
-          bootfs:
-            label: boot
-          uefi:
-            vendor: fedora
-            install: true
-          legacy: i386-pc
-          write_defaults: false
-          greenboot: false
-          ignition: true
   - name: raw-image
     build: name:build
     stages:
@@ -231,23 +218,39 @@ pipelines:
             partition:
               mpp-format-int: '{image.layout[''EFI-SYSTEM''].partnum}'
             target: /boot/efi
-      - type: org.osbuild.grub2.inst
+      - type: org.osbuild.bootupd
         options:
-          platform: i386-pc
-          filename: disk.img
-          location:
-            mpp-format-int: '{image.layout[''BIOS-BOOT''].start}'
-          core:
-            type: mkimage
-            partlabel: gpt
-            filesystem: ext4
-          prefix:
-            type: partition
-            partlabel:
-              mpp-format-string: '{image.layout.label}'
-            number:
-              mpp-format-int: '{image.layout[''boot''].index}'
-            path: /grub2
+          bios:
+            device: disk
+          static-configs: true
+          deployment:
+            osname: fedora-coreos
+            ref: ostree/1/1/0
+        devices:
+          disk:
+            type: org.osbuild.loopback
+            options:
+              filename: disk.img
+              partscan: true
+        mounts:
+          - name: root
+            type: org.osbuild.xfs
+            source: disk
+            partition:
+              mpp-format-int: '{image.layout[''root''].partnum}'
+            target: /
+          - name: boot
+            type: org.osbuild.ext4
+            source: disk
+            partition:
+              mpp-format-int: '{image.layout[''boot''].partnum}'
+            target: /boot
+          - name: efi
+            type: org.osbuild.fat
+            source: disk
+            partition:
+              mpp-format-int: '{image.layout[''EFI-SYSTEM''].partnum}'
+            target: /boot/efi
   - name: raw-4k-image
     build: name:build
     stages:
@@ -352,6 +355,39 @@ pipelines:
             partition:
               mpp-format-int: '{image4k.layout[''EFI-SYSTEM''].partnum}'
             target: /boot/efi
+      - type: org.osbuild.bootupd
+        options:
+          static-configs: true
+          deployment:
+            osname: fedora-coreos
+            ref: ostree/1/1/0
+        devices:
+          disk:
+            type: org.osbuild.loopback
+            options:
+              filename: disk.img
+              partscan: true
+              sector-size:
+                  mpp-format-int: "{four_k_sector_size}"
+        mounts:
+          - name: root
+            type: org.osbuild.xfs
+            source: disk
+            partition:
+              mpp-format-int: '{image4k.layout[''root''].partnum}'
+            target: /
+          - name: boot
+            type: org.osbuild.ext4
+            source: disk
+            partition:
+              mpp-format-int: '{image4k.layout[''boot''].partnum}'
+            target: /boot
+          - name: efi
+            type: org.osbuild.fat
+            source: disk
+            partition:
+              mpp-format-int: '{image4k.layout[''EFI-SYSTEM''].partnum}'
+            target: /boot/efi
   - name: raw-metal-image
     build: name:build
     stages: