Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make UKI naming configurable #1638

Open
MariusSchiffer opened this issue Jun 21, 2023 · 26 comments
Open

Make UKI naming configurable #1638

MariusSchiffer opened this issue Jun 21, 2023 · 26 comments
Labels

Comments

@MariusSchiffer
Copy link
Contributor

I'd like to update the UKIs with systemd-sysupdate at a later point. Including the kernel version in the UKI name makes this tasks harder, as sysupdate does not support (actual asterisk) wildcards and its kernel version specifier is rather limited.
Alternatively, support for kernel "flavours" could be useful.

I do not yet have an idea for an elegant configuration option for this though.

If there is an easier option to rename generated UKIs after the fact, please tell me.

@behrmann
Copy link
Contributor

I think the easiest possibility right now would be to rename the file in the finalize script.

@Cornelicorn
Copy link
Contributor

I think the easiest possibility right now would be to rename the file in the finalize script.

UKIs only get generated after the finalize script. Running finalize after the UKI generation would break verity or imply that the finalize script cannot change the image anymore.

@DaanDeMeyer
Copy link
Contributor

I'd like to update the UKIs with systemd-sysupdate at a later point. Including the kernel version in the UKI name makes this tasks harder, as sysupdate does not support (actual asterisk) wildcards and its kernel version specifier is rather limited.
Alternatively, support for kernel "flavours" could be useful.

Isn't this something to fix in systemd-sysupdate? I'm not really following the issue here, can you give an example of why this currently doesn't work properly?

@MariusSchiffer
Copy link
Contributor Author

sysupdate could be the better place to fix this.

Mkosi generates UKI in my case to /boot/EFI/Linux/hypervisor_13.0.8-5.10.0-1057-oem.efi.
My update contains hypervisor_13.0.9-5.10.0-1060-oem.efi. This cannot really be caught with sysupdate, as only the currently booted kernel release could be used as a specifier (e.g. hypervisor_@v-%v.efi).

Alternatively I could leave out the kernel release at least in the update, but keep it in /boot/EFI/Linux, as mkosi puts the kernel release into the name.
But then sysupdate won't find any kernels other than the currently booted release, so it also won't delete old instances of the kernel.
Example: /boot/EFI/Linux/hypervisor_13.0.8-5.10.0-1057-oem.efi and update contains hypervisor_13.0.8.efi.

So the easiest solution for me would be to leave out the kernel release entirely or rename the kernel release to something I can control.

Example sysupdate.conf:

# /usr/lib/sysupdate.d/70-kernel.conf
[Transfer]
ProtectVersion=%A

[Source]
Type=regular-file
Path=/mnt/update
MatchPattern=hypervisor_@v.efi

[Target]
Type=regular-file
Path=/EFI/Linux
PathRelativeTo=boot
MatchPattern=hypervisor_@v-%v.efi
Mode=0444

@septatrix
Copy link
Contributor

I'd like to update the UKIs with systemd-sysupdate at a later point. Including the kernel version in the UKI name makes this tasks harder, as sysupdate does not support (actual asterisk) wildcards and its kernel version specifier is rather limited.
Alternatively, support for kernel "flavours" could be useful.

Isn't this something to fix in systemd-sysupdate? I'm not really following the issue here, can you give an example of why this currently doesn't work properly?

The problem is also that even with kernel version supported as patterns in sysupdate, there are still scenarios where this is not sufficient. For example there could be updates to the UKI without a change in the kernel version, and being unable to influence the UKI name forces one to adopt to the naming convention used by mkosi. In our case, however, our naming is simply ${IMAGE_ID}_${IMAGE_VERSION}.${arch}.efi which fulfills all our needs.

Also relevant for #2024

@septatrix

This comment was marked as outdated.

@septatrix
Copy link
Contributor

I also wonder why the roothash is appended. This is not something which is usually done by kernel-install. I was unable to find any documentation of why this would be necessary in the docs of sd-stub, sd-boot, sd-veritysetup-generator, or the boot loader specification. Usually this should already be handled by the cmdline embedded in the UKI? My guess would be that this is for distro-provided, presigned UKIs where adding such information to the cmdline would invalidate the signature but as said above I found no information how and by what this hash is actually used.

This is another thing where it is desirable to be able to turn it off.

@DaanDeMeyer
Copy link
Contributor

My goal here is that we make kernel-install do what we need and replace all our homegrown logic with kernel-install. We would then add the option to configure the UKI name to kernel-install.

@michaelbeaumont
Copy link

michaelbeaumont commented May 4, 2024

@DaanDeMeyer What exactly do you imagine by "do what we need"? Which logic exactly?

What advantage does kernel-install bring here? We've got a UKI image already, we're just picking a name for it now. The problem is that mkosi is assuming a filename scheme, for some reason.

Or are you proposing replacing the whole pipeline, like the calls to ukify, with calls to kernel-install?

@DaanDeMeyer
Copy link
Contributor

Indeed I would prefer to have kernel-install manage the whole thing. Its the same tool all rpm based distros use to install their kernels so using it in mkosi as well will help ensure our kernels, ukis and such are installed in the same places as kernel-install will place them. It also means we dont have to duplicate every kernel install setting in mkosi but instead just allow configuring kernel-install via the package manager trees. This would include UKI naming among other things.

@Trumeet
Copy link

Trumeet commented May 5, 2024

Also the current naming scheme (imageid-$(uname -r).efi) makes it very hard to update the UKI with rootfs ...

My rootfs label is following the scheme imageid-@v, and I would like the UKI to have the same naming scheme, so I can update the UKI along with the root image.

The current scheme makes it really hard.

@mcassaniti
Copy link
Contributor

So I'm back again. The only issue I'm running into this time is UKI naming and how it affects systemd-sysupdate. This one is a bit long I'm afraid.

I've followed the git history back to see what has changed around the output file naming for UKIs and also the move to putting more into /boot.

The decision to add the kernel version to the UKI name was done in #1633 by @MariusSchiffer to fix #1631 and it makes enough sense. If you build an image and there are multiple kernel versions (kvers) in the image then you need a separate file name for each kver. This is independent of the image version set (ImageVersion=).

Later on the image version was completely removed from the UKI file name in #2154 by @DaanDeMeyer to cater more towards package managers (at least that's what I gather). Again this makes sense too since package managers want the kernel version in the file name since that is what they use to denote a version change.

The downside to using kernel versions as mentioned above is that it doesn't play well with systemd-sysupdate. IMHO we need an option for mkosi that enforces using the image version and configures everything to play nicely with systemd-sysupdate. The rest from here is my opinionated view:

  • multiple kernel versions packaged as UKIs in the one image are not compatible with systemd-sysupdate. This is because you can really only push out a single UKI version in an update. If you were doing something like a virtual and standard kernel image but keeping the same image version (not kver) that would work, but mkosi isn't really designed for that either.
  • A type 1 boot setup might work with systemd-sysupdate but the amount of effort required compared to a UKI makes it majorly painful so let's mark that as not supported either
  • The only identifying version that is useful between systemd-sysupdate, systemd-repart and mkosi is the image version. That version is stored:
    • In the UKI itself
    • On disk in /etc/os-release
    • In the GPT partition names for A/B versioned partition updates
    • In the file name of the UKI
    • In the names of the output files so that a version string can be picked up (yes these are "broken" too)

I don't know if having mkosi assume ImageVersion=... means that the user wants an image and the artifacts usable with systemd-sysupdate, but there needs to be something to indicate this fact and it means adding the image version in several places it isn't at the moment. It also means complaining when multiple kernel versions are encountered IMO.

Remember that systemd-sysupdate is designed to be quite simple. The "standard" way of using it means:

  • Versioning is done using IMAGE_VERSION= from /etc/os-release
  • An update requires a new UKI, usr image and usr-verity image
  • The GPT partition names have the version in the name
  • There is space to fit at least a second UKI, usr and usr-verity image which means two /usr partitions and two usr-verity partitions
  • The base image generated by mkosi has put versions in the right place (UKI file name, /etc/os-release, partition table) ready to be updated later
  • The kernel command line has usrhash=... configured to find the right partitions and the partitions have the right UUIDs set

I'm happy to write an integration test that:

  • Generates an image
  • Bumps the version and generates another image
  • Attempts to update the original image using systemd-sysupdate
  • Boots and confirms the image is updated

Do we need a new issue? Should we rename this one?

@mcassaniti
Copy link
Contributor

I have to retract part of my statement, the split artifacts output is not broken. If I configure SplitName = %A.%U.%t in my repart.d definitions then things work just fine.

@mcassaniti
Copy link
Contributor

I think I should add that all we should need to change is:

  • The file name of the UKI stored at /boot/EFI/Linux
  • The file name stored alongside the image

@MariusSchiffer
Copy link
Contributor Author

Yes, I added the kernel version to the name and then noticed a little bit later that I shot myself in the foot, as I also use sysupdate. I'm currently using a patched version as there was no elegant path forward yet.

@mcassaniti
Copy link
Contributor

I think I should add that all we should need to change is:

* The file name of the UKI stored at /boot/EFI/Linux

* The file name stored alongside the image

Hey @bluca, @DaanDeMeyer and @behrmann, can one of you weigh in here on making the changes above, adding some sort of I want sysupdate mode and adding tests? I'm happy to do the work, but I don't want to start on something that won't get traction.

@behrmann
Copy link
Contributor

I haven't yet used sysupdate, so I can't really speak to the needs here and I can't say I fully grasp the problem:

I have to retract part of my statement, the split artifacts output is not broken. If I configure SplitName = %A.%U.%t in my repart.d definitions then things work just fine.

Does that mean there is a way to configure your image such that it already works fine as is—then this is a documentation problem—, or do we need something like a switch to enforce sysupdate-compatible output?

@mcassaniti
Copy link
Contributor

We need a change made for UKI file names, and I'd highly recommend a change to the main output file name (i.e.: image.raw) so that they include the image version and do not include any other version information. The main file is highly recommended but not a hard requirement while the UKI file names do need changing.

Like I said, happy to write an integration test and make the appropriate changes. The test should provide a canonical example.

@michaelbeaumont
Copy link

As far as I can tell

I think I should add that all we should need to change is:

The file name of the UKI stored at /boot/EFI/Linux

still stands. There's just no way to influence the naming of the UKI on the disk to make it match a given repart.d/sysupdate.d schema, its schema is static. So I can use repart.d and SplitArtifacts to get a nice filename in mkosi.output that I can use with sysupdate but the on-disk file won't match it. My workaround is to mount the image after mkosi is done and just rename the file:

  mkosi_output="$(mkosi --json summary | jq -r .Images[0].Output)"

  # mount the image on a loopback dev at ./scratch...

  path=$(bootctl list --root=scratch --esp-path=/ --json=short | jq -r .[0].linux)
  sudo mv "./scratch/${path}" "./scratch/$(dirname ${path})/${mkosi_output}.efi"

AFAICT it's also not possible to do the rename in mkosi.repart/esp.conf because the UKI has the roothash in the filename and there's no specifier we can use to get this value, so we can't match with CopyFiles.

@septatrix
Copy link
Contributor

The part which needs changes is this:

mkosi/mkosi/__init__.py

Lines 2315 to 2319 in 87c900f

if roothash:
_, _, h = roothash.partition("=")
boot_binary = context.root / f"boot/EFI/Linux/{token}-{kver}-{h}{boot_count}.efi"
else:
boot_binary = context.root / f"boot/EFI/Linux/{token}-{kver}{boot_count}.efi"

This results in files like /efi//EFI/Linux/DSPlayerOS-6.8.9-300.fc40.x86_64-0d6f9cad07e5929986d04f66aa9186532383c5cee8587c99dbb06087266fd35f.efi that cannot reasonably be managed by sysupdate.

  1. Why is the root hash included? It seems to make no sense as it is already part of the cmdline (see my unanswered comment above)
  2. Add an option to omit the kernel version. This allows user to fully control the destination of the uki by creating /etc/kernel/entry-token. This breaks down when multiple kernel versions are present but that makes little sense in the case of sysupdate.

Potential changes to kernel-install which would help here:

  1. Include boot count as part of the output from inspect
  2. Some mechanism to adjust the name in a cleverer way than abusing the entry token?

Potential changes to bootctl which would simplify the code:

  1. Bootctl should learn to install shims and other EFI applications (see Missing features for deploying image-based systems systemd#29722). This is mostly for the if-block above the code I referenced.

Potential changes to sysupdate which would make this easier:

  1. Allow a wildcard selector to match and ignore the kernel version. I think there was already an issue about this but I am unable to find it.

@mcassaniti
Copy link
Contributor

@septatrix The root hash change was introduced in #923 by @DaanDeMeyer but I can't see in the PR why. I'm curious to know the answer to that one too.

@DaanDeMeyer
Copy link
Contributor

@mcassaniti We had it before that as well. I was originally introduced by @poettering and I simply kept it intact. Maybe @poettering can tell us whether it's OK to remove the roothash from the UKI name?

@NekkoDroid
Copy link
Contributor

and I'd highly recommend a change to the main output file name (i.e.: image.raw) so that they include the image version and do not include any other version information.

I am unsure what exactly you mean with this. The output files by default do include the version in the name if its set, but if you specify a custom Output= then you need to manually add it (e.g %i_%v).

mkosi/mkosi/config.py

Lines 691 to 697 in 87c900f

def config_default_output(namespace: argparse.Namespace) -> str:
output = namespace.image or namespace.image_id or "image"
if namespace.image_version:
output += f"_{namespace.image_version}"
return output

@mcassaniti
Copy link
Contributor

@NekkoDroid That will output something like 04-desktop_001 because it picks up the image name first. I thought the same thing initially too until I tried it.

@NekkoDroid
Copy link
Contributor

NekkoDroid commented May 28, 2024

That is if you use mkosi.images, where having multiple images refering to the same ImageId is somewhat likely (output names would clash). In which case you can still can manually set it to Output=%i_%v to have it only consist of {ImageId}_{ImageVersion}, so this isn't something too crazy to do.

But this is neither here nor there, since the version is still included.

@mcassaniti
Copy link
Contributor

I've tested #2731 and can confirm it works using the partial snippet of configuration below. The output controls the UKI output while UnifiedKernelImageName controls the file name within the ESP/XBOOTLDR partition.

[Output]
ImageId = ...
ImageVersion = ...
Output = %i_%v

[Content]
UnifiedKernelImageName = %i_%v

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9 participants