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

Disable systemd-boot on live media #5172

Closed

Conversation

jlinton
Copy link

@jlinton jlinton commented Sep 14, 2023

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=2237327, by adding the btrfs subvolumes like zipl and extlinux to the systemd-boot strings. Grub has special cased this in grub-tools, otherwise a better fix would have been to get the btrfs subvolume boot string into boot_args when BTRFS was selected rather than depending on each bootloader to discover and fix them up.

It also partially fixes https://bugzilla.redhat.com/show_bug.cgi?id=2234638 by providing a nicer message on live installs. This fix isn't ideal because 1: The webui apparently doesn't have a way to actually display storage/bootloader errors like the old anaconda UI does. 2: Its a bit later in the installation than I would have liked, but moving it earlier tends to blow up the webUI in even less ideal ways.

Suggestions are welcome...

@github-actions github-actions bot added the f40 label Sep 14, 2023
@M4rtinK
Copy link
Contributor

M4rtinK commented Sep 14, 2023

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

@jlinton thanks for the proposed fixes.

@KKoukiou I wonder, do you think that we can do something about the storage error propagation in web UI?

@KKoukiou
Copy link
Contributor

@jlinton thanks for the proposed fixes.

@KKoukiou I wonder, do you think that we can do something about the storage error propagation in web UI?

Where exactly these errors you talk about here come from?

Copy link
Contributor

@VladimirSlavik VladimirSlavik left a comment

Choose a reason for hiding this comment

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

First commit is OK.

Second commit needs more discussion, I think. There's a bit of a problem, in that sdboot can be selected on live by using kickstart... Which is not a thing that would be really supported in the future. So, perhaps it's actually ok, because the whole scheme will need to be revisited anyway.

Generally speaking, the error should be happening at different time of the installation process. We know we're on live and what the "inputs" are, so we don't have to wait until installation time. We can act on the problem as soon as we have the data, either on setting the default bootloader from conf, or when processing the kickstart.

However, I think this might be actually good enough. Check my logic please?

The options here are:

  • Selected via kickstart on live = unsupported = hard error too late is somewhat admissible in unsupported state
  • Selected via config file on live = supported, BUT conf and media type is known in advance, so the media author knows they are creating an impossible combination = hard error late is sort-of ok too, this should come up when testing the media before circulation
  • If sdboot starts being supported on live, the error is no longer happening, so does not matter.

pyanaconda/modules/storage/bootloader/installation.py Outdated Show resolved Hide resolved
@VladimirSlavik
Copy link
Contributor

Where exactly these errors you talk about here come from?

These would manifest as D-Bus errors that bubble up from there via InstallBootloaderWithTasks or GenerateInitramfsWithTasks, which are hardwired into the installation queue. In other words, this, as written now, would stop the installation by erroring it out.

BTW, there was originally some discussion about "known errors" and "unknown errors". This sounds like a known error, but does not behave like one - it's raised at install time, even if it's known in advance. I'm not too sure if there is an existing better place to plug this, though...

@VladimirSlavik
Copy link
Contributor

One more thought is that we might want to fall back to grub instead. But I'm not sure if it does any good. Personally I'm ignorant of situations where sdboot would be desired, unsupported, and selected, unless the user is a hardcore tester / power user and intentionally chooses to do this. But in such case, the experience does not have to be polished at cost of additional code complexity.

@VladimirSlavik
Copy link
Contributor

@poncovka I'd appreciate any thoughts about

  • the validity of my reasoning
  • if you have an idea where to plug the error earlier
  • if it's a valid action to have a fallback for bootloader choice

@poncovka
Copy link
Contributor

poncovka commented Sep 18, 2023

The webui apparently doesn't have a way to actually display storage/bootloader errors like the old anaconda UI does.

@jlinton Do you have a screenshot of the error in the old UI? There are multiple error handlers and I am not sure which one you mean. Or if you could just point us at the place in the code where you tried to raise the error.

@poncovka
Copy link
Contributor

Some background. This is not exactly about support, but about missing installation dependencies of the selected bootloader. While are able to verify these kind of dependencies for RPM installations, we never had time to implement support for other types of payloads like live images and rpm ostree. Btw, these checks are also done during the installation process, so I wouldn't worry about that too much.

@jlinton
Copy link
Author

jlinton commented Sep 18, 2023

One more thought is that we might want to fall back to grub instead. But I'm not sure if it does any good. Personally I'm ignorant of situations where sdboot would be desired, unsupported, and selected, unless the user is a hardcore tester / power user and intentionally chooses to do this. But in such case, the experience does not have to be polished at cost of additional code complexity.

IMHO, its better to simply stop the install if inst.sdboot/etc is selected against a live image that already has grub rather than (silently?) falling back to grub. That way the user isn't surprised when the machine boots with grub.

@jlinton
Copy link
Author

jlinton commented Sep 18, 2023

The webui apparently doesn't have a way to actually display storage/bootloader errors like the old anaconda UI does.

@jlinton Do you have a screenshot of the error in the old UI? There are multiple error handlers and I am not sure which one you mean. Or if you could just point us at the place in the code where you tried to raise the error.

Screen shot of what the above change looks like with the graphical UI.

anaconda_systemd_error

Or where I was trying to handle the error before the current PR? I tried a couple of places, one of the nicer ones was just in the init routine of SystemdBoot() that one IIRC just reset the webUI into an infinite loop.

@jlinton jlinton force-pushed the systemd_disable_live_fix_btrfs branch from 07e49a1 to 3874ce5 Compare September 18, 2023 19:56
@rvykydal
Copy link
Contributor

@jlinton thanks for the proposed fixes.
@KKoukiou I wonder, do you think that we can do something about the storage error propagation in web UI?

Where exactly these errors you talk about here come from?

I think that at this point we should try solve this issue wrt to GUI and then later (at a separte issue) look at how the solution works with WebUI and think if/how we should handle this and similar cases. I've got a feeling that including WebUI into considerations now could make resolution of the issue too complicated.

@jlinton
Copy link
Author

jlinton commented Sep 20, 2023

Well, AFAIK its only really an issue on the live media (webUI) and OStree, which needs another fix. I would like to get the btrfs fix in place because that will allow everything media to work out of the box, rather than requiring an alternate FS, or user interaction during the boot?

Should I split these to commits so the first one can land sooner?

@poncovka
Copy link
Contributor

Should I split these to commits so the first one can land sooner?

Yeah, that sounds like a good idea :-)

@jlinton jlinton force-pushed the systemd_disable_live_fix_btrfs branch from 3874ce5 to 7a0e5cb Compare September 20, 2023 19:45
@jlinton jlinton changed the title Disable systemd-boot on live media and fix btrfs boot options Disable systemd-boot on live media Sep 20, 2023
@jlinton
Copy link
Author

jlinton commented Sep 20, 2023

Moved systemd/btrfs commit to #5194 and rebased.

@jlinton
Copy link
Author

jlinton commented Sep 20, 2023

Uh, this isn't any better with the latest f39 builds, I rebased to those and it fires too early too, and doesn't get caught properly.

@jlinton
Copy link
Author

jlinton commented Sep 20, 2023

Ok, so I moved into an overridden is_valid_stage1_device() and that sorta works, but is ugly too. The storage config gets an error, so if you go into the storage config, and click the not obvious link to see what the error is, it give a couple pages of the error message about systemd-boot not being supported on live media.

@jlinton jlinton force-pushed the systemd_disable_live_fix_btrfs branch from 7a0e5cb to 0f79309 Compare September 20, 2023 23:59
@pep8speaks
Copy link

pep8speaks commented Sep 20, 2023

Hello @jlinton! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-09-21 00:01:14 UTC

Live images are already installed with grub2 and cannot be changed on
the fly to systemd-boot (well at least without a lot of extra
work). In case the user passed one of the systemd-boot options to
anaconda, we should be a bit more graceful than failing to run the
updateloader entries script as currently happens.

This change, when run on non webUI installs results in a popup
notifying the user there is an error in the bootloader configuration
and asking if they would like to ignore it.

Resolves: 2234638
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
@jlinton jlinton force-pushed the systemd_disable_live_fix_btrfs branch from 0f79309 to 8626d88 Compare September 21, 2023 00:01
@jlinton
Copy link
Author

jlinton commented Sep 21, 2023

Ok, I'm pretty happy with this one. Its one of the "infinite loop" ones with the webUI, because the user gets stuck in the storage configuration clicking next without any notification of what is wrong.

OTOH: In f39, which i'm assuming is the immediate target it looks like:
anaconda_storage_error
followed by:
anaconda_storage_err

@jlinton
Copy link
Author

jlinton commented Sep 21, 2023

Also, should I create a separate PR for the f39 branch (once this is merged), or can you guys merge this commit to both branches?

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Since master now is defaulting to the new Web UI (see new Fedora rawhide anaconda UI) this needs to be re-tested on the new UI.

I will try if I can test this.


def is_valid_stage1_device(self, device, early=False):
valid = True
if conf.system.provides_liveuser:
Copy link
Member

Choose a reason for hiding this comment

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

Hi, we should probably make the special casing include also ostree based installations. However, I fear the logic needs to change a bit more thanks to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's more correct then to check the payload type? If the payload type is DNF then all good, otherwise error out.

@KKoukiou
Copy link
Contributor

/build-image --live --webui

@github-actions
Copy link

Images built based on commit 8626d88:

  • Live: failure

Download the images from the bottom of the job status page.

@jlinton
Copy link
Author

jlinton commented Oct 9, 2023

I'm frankly having second thoughts about disabling this. I've been building systemd-boot live media (in various states of functionality but its getting closer AFAIK).

So, really it doesn't look like a huge lift to create live systemd-boot media. But it does require dedicated images (rather than a single ex workstation image that can flip between bootloaders). Right now I have a fedora-live-systemd.ks that builds live media with systemd-boot where the intention is to install that to to the end machine with systemd-boot.

Meaning, if/when that gets cleaned up this code should be detecting whether the final media is a systemd-boot or grub before making the decision to complain about it. And I expect its going to be a similar situation for ostree too.

@jlinton
Copy link
Author

jlinton commented Oct 26, 2023

I'm thinking about, but haven't yet managed to test, the idea that instead of aborting on live media installs/etc it only aborts if it is running from the live media and it can't find updateloaderentries, although maybe a better plan is actually to query something in systemd-boot itself, but i'm not yet sure what a good "is this image systemd-boot" flag is, short of actually checking to see if it exists in the ESP.

@poncovka
Copy link
Contributor

poncovka commented Nov 2, 2023

@jlinton I have opened a new pull request with an alternative solution. The team would like to get it to Fedora 39 soon. Can you look at it, please? The idea is to support only payloads where we are able to verify the systemd-boot requirements. At this moment, that's only the package installation. The check can be eventually extended to support other types of payloads. #5297

@VladimirSlavik
Copy link
Contributor

@poncovka as the alternative is merged, should this be closed?

@jlinton
Copy link
Author

jlinton commented Dec 4, 2023

Why not, i'm closing it.

@jlinton jlinton closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9 participants