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 #4232

Merged
merged 4 commits into from Aug 8, 2022

Conversation

poncovka
Copy link
Contributor

@poncovka poncovka commented Jul 22, 2022

Use the inst.disklabel boot option to specify a preferred disk label type. Specify
gpt to prefer creation of GPT disk labels. Specify mbr to prefer creation of MBR
disk labels if supported. The inst.gpt boot option is deprecated and will be removed
in future releases.

The default value of the preferred disk label type is specified by the disk_label_type
option in the Anaconda configuration files. The gpt configuration option is no longer
supported.

Fedora Linux systems installed on legacy x86 BIOS systems should get GPT partitioning by
default instead of legacy MBR partitioning. This should be a new default for all products.

See: https://fedoraproject.org/wiki/Changes/GPTforBIOSbyDefault

@poncovka poncovka added manual testing required This issue can't be merged without manual testing blocked Don't merge this pull request! f37 Fedora 37 labels Jul 22, 2022
@Conan-Kudo
Copy link
Contributor

This looks good, but is there a reason we want Anaconda's core default to remain MBR?

Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Looks good to me code-wise, thanks! :)

Copy link
Contributor

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

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

Oh, I see. We now use Blivet's setting by default.

@poncovka poncovka changed the title Introduce the inst.disklabel boot option Prefer GPT instead of legacy MBR on Fedora (inst.disklabel version) Jul 22, 2022
Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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.

Looks good to me. Thank you!

@poncovka
Copy link
Contributor Author

poncovka commented Aug 1, 2022

This looks good, but is there a reason we want Anaconda's core default to remain MBR?

No idea. @jkonecny12? @jstodola?

@jstodola
Copy link
Contributor

jstodola commented Aug 2, 2022

I'm not aware of any reason why MBR should remain the default. IMO, it makes sense to make GPT the default for all future products, not just Fedora. The disk label would be unified across all systems with all disk sizes.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me and I'm approving this. Thanks!

@poncovka poncovka removed the blocked Don't merge this pull request! label Aug 4, 2022
@poncovka poncovka changed the title Prefer GPT instead of legacy MBR on Fedora (inst.disklabel version) Prefer GPT instead of legacy MBR on Fedora Aug 4, 2022
Use the `inst.disklabel` boot option to specify a preferred disk label type.
Specify `gpt` to prefer creation of GPT disk labels. Specify `mbr` to prefer
creation of MBR disk labels if supported.

The `inst.gpt` boot option is deprecated and will be removed in future releases.
Fedora Linux systems installed on legacy x86 BIOS systems should get
GPT partitioning by default instead of legacy MBR partitioning. This
should be a new default for all products.

See: https://fedoraproject.org/wiki/Changes/GPTforBIOSbyDefault
Implement new unit tests for the `_set_default_label_type` function.
Document all changes related to the default disk label type.
@poncovka poncovka changed the title Prefer GPT instead of legacy MBR on Fedora Prefer GPT instead of legacy MBR Aug 4, 2022
@poncovka poncovka removed the manual testing required This issue can't be merged without manual testing label Aug 4, 2022
@poncovka
Copy link
Contributor Author

poncovka commented Aug 4, 2022

Updated and tested.

@poncovka
Copy link
Contributor Author

poncovka commented Aug 4, 2022

/kickstart-test --testtype smoke

@poncovka poncovka marked this pull request as ready for review August 4, 2022 16:01
@poncovka poncovka merged commit 91415dd into rhinstaller:master Aug 8, 2022
12 checks passed
@AdamWill
Copy link
Contributor

This PR arguably does rather a lot more than was actually requested, and broke at least one thing. See https://bugzilla.redhat.com/show_bug.cgi?id=2092091#c6 and https://bugzilla.redhat.com/show_bug.cgi?id=2209760 . Why was this implemented this way, instead of by just tweaking the existing logic in blivet?

@poncovka
Copy link
Contributor Author

This PR arguably does rather a lot more than was actually requested, and broke at least one thing. See https://bugzilla.redhat.com/show_bug.cgi?id=2092091#c6 and https://bugzilla.redhat.com/show_bug.cgi?id=2209760 . Why was this implemented this way, instead of by just tweaking the existing logic in blivet?

@AdamWill I don't remember all details, but this was the original proposal #4104, so we just went from there. It looks like @vojtechtrefny actually suggested to change the order of disk labels in Blivet at #4104 (comment), but we never followed up on that. I don't know why.

Anyway, it should be pretty straightforward to fix this. If Blivet changes the order of disk labels, Anaconda can re-enable Blivet's defaults in the Anaconda configuration file via the disk_label_type option. @AdamWill @vojtechtrefny Would it make sense now?

@AdamWill
Copy link
Contributor

Sure, that's what I was going to propose too, if there wasn't any non-obvious reason not to do it. Here's the blivet PR: storaged-project/blivet#1132

AdamWill added a commit to AdamWill/anaconda that referenced this pull request May 26, 2023
rhinstaller#4232 made anaconda
prefer GPT disk labels by default. This was intended to implement
the Change called "Install Using GPT on x86_64 BIOS by Default":
https://fedoraproject.org/wiki/Changes/GPTforBIOSbyDefault
https://bugzilla.redhat.com/show_bug.cgi?id=2092091
However, it actually does more than that Change requested. It
prefers GPT in other cases - it's quite hard to parse out every
possible case this changes, but for example, it also prefers GPT
on ppc64le installs, which previously preferred MBR. It's also
just an odd approach, when blivet already goes to some lengths
specifically to provide a list of supported label types in
a preferred order. It makes much more sense for anaconda to keep
respecting the order of the list blivet provides, and adjust
blivet to list GPT first on x86_64. The config setting and
`inst.disklabel` boot option can still be used to force a
preference; this just makes the default config value unset rather
than 'gpt'.

This goes with this blivet PR that implements preferring GPT on
x86_64 (even on BIOS):
storaged-project/blivet#1132

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/anaconda that referenced this pull request May 26, 2023
rhinstaller#4232 made anaconda
prefer GPT disk labels by default. This was intended to implement
the Change called "Install Using GPT on x86_64 BIOS by Default":
https://fedoraproject.org/wiki/Changes/GPTforBIOSbyDefault
https://bugzilla.redhat.com/show_bug.cgi?id=2092091
However, it actually does more than that Change requested. It
prefers GPT in other cases - it's quite hard to parse out every
possible case this changes, but for example, it also prefers GPT
on ppc64le installs, which previously preferred MBR. It's also
just an odd approach, when blivet already goes to some lengths
specifically to provide a list of supported label types in
a preferred order. It makes much more sense for anaconda to keep
respecting the order of the list blivet provides, and adjust
blivet to list GPT first on x86_64. The config setting and
`inst.disklabel` boot option can still be used to force a
preference; this just makes the default config value unset rather
than 'gpt'.

This goes with this blivet PR that implements preferring GPT on
x86_64 (even on BIOS):
storaged-project/blivet#1132

Signed-off-by: Adam Williamson <awilliam@redhat.com>
VladimirSlavik pushed a commit to AdamWill/anaconda that referenced this pull request May 31, 2023
rhinstaller#4232 made anaconda
prefer GPT disk labels by default. This was intended to implement
the Change called "Install Using GPT on x86_64 BIOS by Default":
https://fedoraproject.org/wiki/Changes/GPTforBIOSbyDefault
https://bugzilla.redhat.com/show_bug.cgi?id=2092091
However, it actually does more than that Change requested. It
prefers GPT in other cases - it's quite hard to parse out every
possible case this changes, but for example, it also prefers GPT
on ppc64le installs, which previously preferred MBR. It's also
just an odd approach, when blivet already goes to some lengths
specifically to provide a list of supported label types in
a preferred order. It makes much more sense for anaconda to keep
respecting the order of the list blivet provides, and adjust
blivet to list GPT first on x86_64. The config setting and
`inst.disklabel` boot option can still be used to force a
preference; this just makes the default config value unset rather
than 'gpt'.

This goes with this blivet PR that implements preferring GPT on
x86_64 (even on BIOS):
storaged-project/blivet#1132

Signed-off-by: Adam Williamson <awilliam@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f37 Fedora 37
8 participants