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

Use the volume UUID to search for the GRUB config in btrfs partitions #3227

Merged
merged 1 commit into from Mar 10, 2021

Conversation

martinezjavier
Copy link
Contributor

For UEFI installs, a minimal GRUB config file is created in the EFI System
Partition that is used to load the main config file that is located in the
/boot/grub2 directory. The partition that contains the latter, is found by
the minimal config searching a device with a given filesystem UUID.

But for this to work the stage2 device must have a filesystem UUID set and
blivet doesn't set this for partitions formatted with a btrfs filesystem.

Because each btrfs volume will have its own UUID, the UUID of the volume
that's mounted according to /etc/fstab must be used to search the device.

Resolves: rhbz#1930567

Signed-off-by: Javier Martinez Canillas javierm@redhat.com

@pep8speaks
Copy link

pep8speaks commented Mar 8, 2021

Hello @martinezjavier! 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 2021-03-08 19:28:23 UTC

For UEFI installs, a minimal GRUB config file is created in the EFI System
Partition that is used to load the main config file that is located in the
/boot/grub2 directory. The partition that contains the latter, is found by
the minimal config searching a device with a given filesystem UUID.

But for this to work the stage2 device must have a filesystem UUID set and
blivet doesn't set this for partitions formatted with a btrfs filesystem.

Because each btrfs volume will have its own UUID, the UUID of the volume
that's mounted according to /etc/fstab must be used to search the device.

Resolves: rhbz#1930567

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
@cmurf
Copy link
Contributor

cmurf commented Mar 8, 2021

@vojtechtrefny what do you think? Does Btrfs need to be special cased because a subvolume is created for any mountpoint only on Btrfs? Or could it be treated the same as ext4 and XFS when it comes to FS UUID?

Related: https://bugzilla.redhat.com/show_bug.cgi?id=1930567#c7

@martinezjavier
Copy link
Contributor Author

For reference this was changed in blivet by the following commit storaged-project/blivet@b3042c63e93f.

I would like to understand why that change was made in the first place.

@vojtechtrefny
Copy link
Contributor

I'm not sure why we have a special "exception" for btrfs here. Having the UUID set correctly for every subvolume and using format.uuid for all filesystems should be possible. I'm not sure why the change with vol_uuid was implemented. @dwlehman do you remember what was the reason for it? It's possible the reason behind this is to have the UUID unique in blivet and not having having multiple devices with the same UUID (internally we treat btrfs subvolumes as devices).

@martinezjavier
Copy link
Contributor Author

It's possible the reason behind this is to have the UUID unique in blivet and not having having multiple devices with the same UUID (internally we treat btrfs subvolumes as devices)

Yes, that was my assumption since I noticed that BTRFSVolumeDevice inherits from BTRFSDevice which in turn inherits from StorageDevice.

@poncovka poncovka added the master Please, use the `f39` label instead. label Mar 9, 2021
@poncovka
Copy link
Contributor

poncovka commented Mar 9, 2021

/kickstart-test --testtype smoke

@poncovka
Copy link
Contributor

poncovka commented Mar 9, 2021

/test

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

The code looks good to me. Thanks!

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.

+1 Looks good to me. Thank you!

@poncovka
Copy link
Contributor

Tested.

@poncovka poncovka merged commit 7ca4664 into rhinstaller:f34-devel Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f34 master Please, use the `f39` label instead.
6 participants