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

Prefer GPT instead of legacy MBR, even on legacy BIOS systems #4104

Closed
wants to merge 1 commit into from

Conversation

Conan-Kudo
Copy link
Contributor

As part of this change, convert the inst.gpt flag to inst.mbr to
allow optionally reverting back to the previous behavior.

@pep8speaks
Copy link

pep8speaks commented May 13, 2022

Hello @Conan-Kudo! 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 2022-05-13 16:21:47 UTC

As part of this change, convert the inst.gpt flag to inst.mbr to
allow optionally reverting back to the previous behavior.
@jkonecny12
Copy link
Member

Hi @vojtechtrefny what do you think about this change? I have a feeling that there is a reason why we are using MBR on the legacy BIOS.

@Conan-Kudo
Copy link
Contributor Author

This proposed Change is associated with this PR: https://fedoraproject.org/wiki/Changes/GPTforBIOSbyDefault

@jkonecny12
Copy link
Member

Also I would like to have @jstodola to take a look here too.

@jkonecny12
Copy link
Member

Just looking on this briefly, we definitely can't just switch kernel boot argument like that. Just because Fedora is not the only product we support and not everyone could be happy about that. We need to have both probably.

@Conan-Kudo
Copy link
Contributor Author

Just looking on this briefly, we definitely can't just switch kernel boot argument like that. Just because Fedora is not the only product we support and not everyone could be happy about that. We need to have both probably.

inst.gpt would be a no-op after this change is merged. Does that require some kind of parsing? I figure that options Anaconda doesn't know about would be ignored properly anyway.

@vojtechtrefny
Copy link
Contributor

Hi @vojtechtrefny what do you think about this change? I have a feeling that there is a reason why we are using MBR on the legacy BIOS.

Hi, I have no problem with this from the storage point of view. We'll also need to change the order of disklabels in blivet in Disklabel.get_platform_label_types.

@jkonecny12
Copy link
Member

jkonecny12 commented May 16, 2022

inst.gpt would be a no-op after this change is merged. Does that require some kind of parsing? I figure that options Anaconda doesn't know about would be ignored properly anyway.

I have to read the things in more detail before answering (especially late) :D. Yes, you are correct. In any case, we had a discussion about this in the team and here are some notes.

We have green on doing this change, seems that there is no objections.

This PR should change to a key=value solution. Instead of replacing the GPT with MBR, let's create something like inst.disklabel=<gpt|mbr> (which is consistent with kickstart) that would be more universal and

And for configuration file value, we could use disk_label_type because we already have something similar to that.

@jstodola
Copy link
Contributor

I also have no objections with the change. BTW, GPT is already used by default on BIOS systems for disks bigger than 2 TiB anyway.

A possible issue would be with existing kickstarts defining custom partitioning and not having biosboot partition specified, they will need to be updated.

Also, what would the installer behavior look like if inst.disklabel=mbr is specified on the kernel command line and the system has disks both smaller and bigger than 2 TiB? What I mean is the limitation of MBR to make use of disks bigger than 2 TiB. Perhaps inst.disklabel=mbr should only apply to disks smaller than 2 TiB?

@cmurf
Copy link
Contributor

cmurf commented May 16, 2022

What does everyone think about hybridizing all installations when inst.disklabel=gpt (including as default)? i.e. create both BIOSBoot and EFI System, and properly populate them for BIOS and UEFI boot.

This would follow the current use in Fedora Cloud edition. The argument being use of a single code path, fewer exceptions, and no regrets like in the weird cases where the user picks UEFI USB boot for installation but the CSM is active for the installed system and thus boots BIOS. Really hard for users to troubleshoot that, or for Fedora user-user support to help users navigate, and hard to document the variable firmware UIs.

Or alternatively, three options: mbr/bios, gpt/uefi, gpt/hybrid - where there is no gpt/bios (just drop it, as-in the exclusive GPT+BIOS type of installation doesn't exist, you'd have to go with gpt/hybrid to get a BIOS installation on GPT, but you'd also get an ESP properly setup too). Downstreams can choose to drop BIOS support entirely by only supporting gpt/uefi. And Fedora would use gpt/hybrid for now until it's certain BIOS is being retired.

Footnote that mbr/uefi happens with Anaconda when (a) MBR exists (b) 1+ partitions are being preserved (c) UEFI firmware is detected. This is a bit of an odd duck. It is in the UEFI spec though. I'm ambivalent whether it should be deprecated or not.

@jkonecny12
Copy link
Member

Replied on our mailing list.

@jkonecny12
Copy link
Member

Also, what would the installer behavior look like if inst.disklabel=mbr is specified on the kernel command line and the system has disks both smaller and bigger than 2 TiB? What I mean is the limitation of MBR to make use of disks bigger than 2 TiB. Perhaps inst.disklabel=mbr should only apply to disks smaller than 2 TiB?

Yes, we should follow the same behavior as we have now. The boot option would be more a hint for Anaconda than a strong rule.

@jkonecny12
Copy link
Member

I'm still most convinced for the inst.disklabel=<value> solution right now.

@poncovka
Copy link
Contributor

Hi @Conan-Kudo, will you update the pull request, or should we take it over?

@Conan-Kudo
Copy link
Contributor Author

What does everyone think about hybridizing all installations when inst.disklabel=gpt (including as default)? i.e. create both BIOSBoot and EFI System, and properly populate them for BIOS and UEFI boot.

I would like this to happen, I just don't know how to implement it.

@Conan-Kudo
Copy link
Contributor Author

Hi @Conan-Kudo, will you update the pull request, or should we take it over?

Let me give it a try to update to use the disklabel option thing you asked for, and if I can't figure it out, you can take over.

@Conan-Kudo
Copy link
Contributor Author

I haven't been able to make time to figure it out. @poncovka, @jkonecny12, can y'all take over?

@poncovka poncovka self-assigned this Jul 12, 2022
@poncovka
Copy link
Contributor

Hi! I am sorry it took so long to get back to this. I have started to implement the solution with inst.disklabel=<gpt|mbr>, but after reading the Fedora change again, I wonder why we need a new boot option anyway. It says:

After the change is merged and released, this behavior will be the default, and inst.mbr would be required to go back to the previous behavior.

The "previous behavior" is that we let Blivet to choose whatever they suggest. I would expect inst.disklabel=mbr to actually start to prefer MBR in a same way as inst.disklabel=gpt. That's a different behaviour.

If you just need something that will allow to "revert the change", then inst.gpt=0 should do the job. I have tested it with #4231 and it seems to work fine. Btw. we already have inst.nombr that is doing something very different, so inst.mbr really doesn't seem to be a good idea.

@Conan-Kudo
Copy link
Contributor Author

Conan-Kudo commented Jul 22, 2022

@poncovka The idea of the code change is to make Anaconda's baseline default change so that Fedora (and derivatives) get it because that's Anaconda's default. For all the other deliverables, we're already producing hybrid boot images that are GPT formatted already.

I actually wanted to pair this with always installing the EFI bootloader stuff on BIOS systems too so that an install can be trivially switched from BIOS to UEFI (either by moving the disk to new hardware, or re-imaging, reconfiguring the firmware, etc.). I just couldn't figure out how to do it.

@poncovka
Copy link
Contributor

@poncovka The idea of the code change is to make Anaconda's baseline default change so that Fedora (and derivatives) get it because that's Anaconda's default. For all the other deliverables, we're already producing hybrid boot images that are GPT formatted already.

@Conan-Kudo I am sorry, I don't understand what do you want me to do based on that.

I have opened a pull request for inst.disklabel=mbr at #4232.
The pull request for inst.gpt=0 is available at #4231.

I am fine with both solutions, just wanted to explain the differences.

@Conan-Kudo
Copy link
Contributor Author

Oh, I didn't see the other option, that's why I was confused. I'm closing this in favor of #4232

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