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.

Sign up for GitHub

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

Jump to bottom

payload/rpmostree: Add support for bootupd #5298

Closed
wants to merge 1 commit into from
Closed

payload/rpmostree: Add support for bootupd #5298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions pyanaconda/modules/payloads/payload/rpm_ostree/installation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,8 @@ def _run(self):
be fixed to *copy* data into /boot at install time, instead of shipping it in the RPM).
"""
bootloader = STORAGE.get_proxy(BOOTLOADER)
if bootloader.use_bootupd:
return
is_efi = bootloader.IsEFI()

physboot = self._physroot + '/boot'
Expand Down Expand Up @@ -496,8 +498,10 @@ def name(self):
return "Configure OSTree bootloader"

def run(self):
self._move_grub_config()
self._set_kargs()
bootloader = STORAGE.get_proxy(BOOTLOADER)
if not bootloader.use_bootupd:
Comment on lines +501 to +502
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you're calling the interface, so CamelCase UseBootupd once added as explained above.

self._move_grub_config()
self._set_kargs(bootloader)

def _move_grub_config(self):
"""If using GRUB2, move its config file, also with a compatibility symlink."""
Expand All @@ -509,7 +513,7 @@ def _move_grub_config(self):
os.rename(boot_grub2_cfg, target_grub_cfg)
os.symlink('../loader/grub.cfg', boot_grub2_cfg)

def _set_kargs(self):
def _set_kargs(self, bootloader):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add this option, please add it to docstring, it's a "proxy to bootloader". However, as it's just a Pythonified wrapper for D-Bus, it's equal to grabbing it anew as it was below, so just not doing this change is an option too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a procedural level please feel free to just force-push fixups. I haven't yet spun up an Anaconda dev/test environment.

"""Set kernel arguments via OSTree-specific utils.

OSTree owns the bootloader configuration, so here we give it an argument list computed
Expand All @@ -520,7 +524,6 @@ def _set_kargs(self):
if conf.target.is_directory:
return

bootloader = STORAGE.get_proxy(BOOTLOADER)
device_tree = STORAGE.get_proxy(DEVICE_TREE)

root_name = device_tree.GetRootDevice()
Expand Down Expand Up @@ -749,3 +752,9 @@ def run(self):
deployment = deployments[0]
deployment_path = sysroot.get_deployment_directory(deployment)
set_system_root(deployment_path.get_path())

have_bootupd = os.path.exists(deployment_path.get_path() + '/usr/bin/bootupctl')
if have_bootupd:
log.info("Found bootupd in target root, disabling Anaconda bootloader installation")
bootloader = STORAGE.get_proxy(BOOTLOADER)
bootloader.set_use_bootupd()
Comment on lines +759 to +760
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the current anaconda architecture really, and I was wondering if it really works to make "cross actor" dynamic calls or not like this. Basically here the bootloader state can only be fully computed once we've written the payload today. Hopefully this can work.

Copy link
Contributor

@VladimirSlavik VladimirSlavik Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work. The place doing return [SomeTask()] is called before calling the run() that checks if there is bootupd installed. The whole installation task queue is gathered before starting the tasks.

Is there some way of detecting this from the payload before installation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VladimirSlavik That's what I though as well, but the installation queue still uses workarounds from the time when the payload wasn't fully migrated, so these tasks are actually collected on demand during the installation. That makes me sad, because it means that this will work until we remove the workarounds.

On the other side, we can hook the bootloader tasks to the bootloader mode change signal and update their states on change. Alternatively, we can wrap them into another task. It won't be so difficult to support this use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way of detecting this from the payload before installation?

Not today. we could add something to the container image metadata for it, and then have a flow that does:

  • fetch metadata (i.e. container image manifest)
  • set up kickstart state
  • perform installation

This topic intersects with #5197 a bit...

What I think would be really neat is generalized support for embedding kickstart fragments in the container image. This seems most viable then if it's done in the metadata instead of the payload, because otherwise in the general case we'd need to buffer the whole payload into RAM before an install.

If we did that, then we could add e.g. inst.bootc=quay.io/examplecorp/someos:latest on the kernel commandline which could then entirely replace inst.ks - the ergonomic improvement there would be huge in the general case.

(But this general case again is for "using pre-generated os/distro ISO with custom payload", like Fedora today, but not like what we want for a custom Image Builder flow)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, actually, not having a kickstart option and state would be preferable. If there's any way to read this from the payload once it's "known", before being "installed", that would be best.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep agree that's preferable! In this stubbed out code we detect this state by noticing that /usr/bin/bootupctl is in the target root...that's after "install" but should still be before the bootloader stage, right?

Failing that, I think we can add a metadata property (LABEL bootupd.enabled=true) or so in the container image manifest. But implementing a separate "discovery" phase would be some new code.

(Not difficult code, just need to fork off skopeo inspect <container image reference>)

20 changes: 19 additions & 1 deletion pyanaconda/modules/storage/bootloader/bootloader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
from pyanaconda.modules.storage.bootloader.bootloader_interface import BootloaderInterface
from pyanaconda.modules.storage.bootloader.installation import ConfigureBootloaderTask, \
InstallBootloaderTask, FixZIPLBootloaderTask, FixBTRFSBootloaderTask, RecreateInitrdsTask, \
CreateRescueImagesTask, CreateBLSEntriesTask
CreateRescueImagesTask, CreateBLSEntriesTask, InstallBootloaderTaskViaBootupd
from pyanaconda.modules.storage.constants import BootloaderMode, ZIPLSecureBoot
from pyanaconda.modules.storage.storage_subscriber import StorageSubscriberModule

Expand Down Expand Up @@ -84,6 +84,9 @@ def __init__(self):
self._password = ""
self._password_is_encrypted = False

# the ostree payload may turn this on if detected
self._use_bootupd = False

def publish(self):
"""Publish the module."""
DBus.publish_object(BOOTLOADER.object_path, BootloaderInterface(self))
Expand Down Expand Up @@ -447,6 +450,16 @@ def collect_requirements(self):

return requirements

@property
def use_bootupd(self):
"""Whether bootupd is enabled"""
return self._use_bootupd

def set_use_bootupd(self):
"""Install the bootloader using https://github.com/coreos/bootupd"""
self._use_bootupd = True
self.set_bootloader_mode(BootloaderMode.SKIPPED)

Comment on lines +453 to +462
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the "implementation", in the bootloader "module", but it needs to be added to the "interface", too. In the file bootlader_interface.py, add the same thing, CamelCase-named, touching self.implementation.<name-from-module>. Also, it's a D-Bus property, so in the interface should be a property, not a function. Check bootloader_mode and BootloaderMode to see an example how this is done.

(The interface members are translated to D-Bus automagically)

def install_bootloader_with_tasks(self, payload_type, kernel_versions):
"""Install the bootloader with a list of tasks.

Expand All @@ -456,6 +469,11 @@ def install_bootloader_with_tasks(self, payload_type, kernel_versions):
:param kernel_versions: a list of kernel versions
:return: a list of tasks
"""
if self._use_bootupd:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that setting the bootloader mode to SKIPPED is enough to disable the tasks in the else branch (there are conditions in their run methods that will stop the tasks before any action). So the only effect of the use_bootupd flag on this module is running the bootupctl tool.

I am wondering if the bootupd code shouldn't stay in the rpm ostree module and the bootupctl tool shoudn't be called from there. This functionality is specific to ostree installations, right? Or is there a possibility of expansion to other types of payloads in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality is specific to ostree installations, right?

Today yes.

Or is there a possibility of expansion to other types of payloads in the future?

In theory it's possible, in practice it'd be a giant change and not really worth it.

return [
InstallBootloaderTaskViaBootupd(
storage=self.storage, sysroot=conf.target.system_root)
]
return [
CreateRescueImagesTask(
payload_type=payload_type,
Expand Down
28 changes: 28 additions & 0 deletions pyanaconda/modules/storage/bootloader/installation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,34 @@ def run(self):
raise BootloaderInstallationError(str(e)) from None


class InstallBootloaderTaskViaBootupd(Task):
"""Installation task for the bootloader via bootupd"""

def __init__(self, storage, sysroot):
"""Create a new task."""
super().__init__()
self._storage = storage
self._sysroot = sysroot

@property
def name(self):
return "Install the bootloader"

def run(self):
"""Run the task.

:raise: BootloaderInstallationError if the installation fails
"""
rc = execWithRedirect(
"bootupctl",
["backend", "install", "--auto", "--with-static-configs",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting another big difference here vs how Anaconda works by default is that we use a static grub config. This means we don't run grub2-mkconfig for every kernel update - which in turns we don't run os-prober. And IMO os-prober is one of the worst chunks of code we ship that runs as root, scanning all your block devices to see if there's e.g. a BeOS or Windows partition there every time you just want to update your kernel...

"--device", self._storage.root_device, self._sysroot],
root=self._sysroot)
if rc:
raise BootloaderInstallationError(
"failed to write boot loader configuration")


class CreateBLSEntriesTask(Task):
"""The installation task that creates BLS entries."""

Expand Down
Loading