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

Evaluate the @root volume name also for btrfs #2324

Merged
merged 12 commits into from Jul 31, 2023

Conversation

schaefi
Copy link
Collaborator

@schaefi schaefi commented Jul 12, 2023

In a volume setup the special volume declaration
<volume name="@root=identifier"/> was only evaluated for the LVM volume manager. In case of btrfs a hardcoded root volume name '@' was used. This commit allows to specify a custom name for the root volume for btrfs as well and also allows to specify that there should be no such root volume. Example:

<volume name="@root=@"/>

Name the root volume '@'. If not specified this stays as the default to stay compatible

<volume name="@root=/"/>

Indicate no root volume is wanted. All subvolumes resides below root (/)

<volume name="@root=foo"/>

Name the root volume 'foo'

This is related to Issue #2316 and a first patch to address the requested changes

@schaefi schaefi requested a review from Conan-Kudo July 12, 2023 16:06
@Conan-Kudo
Copy link
Member

Can you add a test case for the behavior we want in Fedora so that we can verify this does what we expect?

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 12, 2023

Can you add a test case for the behavior we want in Fedora so that we can verify this does what we expect?

This is not the complete patch series we need to achieve the end result you want in Fedora. But I will add a test case that shows the volume creation without a root volume

@Conan-Kudo
Copy link
Member

Once there's a test for it, I'll be happy to approve it.

@schaefi schaefi self-assigned this Jul 12, 2023
@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch from 5823f33 to 0e1c71c Compare July 12, 2023 23:35
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 12, 2023

Once there's a test for it, I'll be happy to approve it.

There is now a unit test available. I did some real test builds and stumbled over some weird things but it's late here and my brain doesn't work anymore... don't merge it :)

@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch from 0e1c71c to e639823 Compare July 13, 2023 09:42
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 13, 2023

ok finished testing and worked for me. The unit test here is imho not enough, we also want an integration test. I'm going to setup an integration test build next

@Conan-Kudo
Copy link
Member

I'm not sure this is a good approach. Magic data can be unpredictable.

Here's my suggestion of what we should do: #2316 (comment)

@Conan-Kudo
Copy link
Member

Once we have an integration test, I'll approve and merge it.

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 16, 2023

I'm not sure this is a good approach. Magic data can be unpredictable.

Here's my suggestion of what we should do: #2316 (comment)

I added the mentioned attribute and deprecation message. I think it's a good idea to make it explicitly configured.
The optional parent setup I will add in a new pull request and we need an integration test for this code first.

I'm on it, but might take a bit since there are some tasks I have to complete next week first

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 16, 2023

Integration test setup done here:

Watch out for the Virtual profile build. Currently it's all block in obs and we need to wait.

@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch from 7df8a3d to 0ed2365 Compare July 18, 2023 16:00
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 18, 2023

@Conan-Kudo @iammattcoleman I added the parent setup and build as I hope the layout that is wanted here. Keeping all this consistent and regression free with how btrfs is used in its "default" aka suse way is really not that easy. We should have had an eye on the general implementation when it was started, but as we can't turn back the time this option does not exist. One part that will stay for the moment is how to specify the root volume. You suggested

<volume name="root" mountpoint="/"/>

and it's a nicer read yes but I have to leave this for backward compatibility to look like:

<volume name="@root=root"/>

This part I will not change.

The other suggestions were implemented the way we discussed them. I will now take a very close look to all the integration tests that we built for btrfs and see how the results look like

@Conan-Kudo
Copy link
Member

We should put it on the list of things to evaluate for KIWI 10 then.

@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch 3 times, most recently from b33a69f to 1018caa Compare July 18, 2023 16:46
@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch 3 times, most recently from 37522c0 to 2fa789b Compare July 18, 2023 19:06
For the btrfs volume management, allow to put a volume into a specific
parent volume. If not specified the volume is below the default volume
This Fixes #2316
Change the Virtual profile to build a btrfs based image
for testing respective btrfs layouts
Make the function call more robust in terms of path separation
When using btrfs with the proposed layout for testing the
delivered grub bios module for the Fedora system used to build
the integration test (FC37) is not capable to find the grub
config file. A manual call for configfile in the grub shell
fixes this with the existing kiwi created grub early-boot
script. However, it is expected that the delivered grub image
works and kiwi only creates its own one if no distro delivered
grub image was found. To make the integration test functional
for both BIOS and EFI the simple solution is to use an extra
not btrfs based boot partition. This still allows to test
the desired btrfs layout in terms of volumes and sub-volumes
and does not break on any of the boot methods.
Don't copy the same file. This case happens when rebuilding
an image using --allow-existing-root when the fallback setup
has done its job already in the first run
If the rootfs is btrfs based make sure the fstab entry for
it takes the name of the root subvolume into account
@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch from 4a55c5d to f8f1c00 Compare July 26, 2023 07:21
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 26, 2023

@schaefi Well, I've merged #2336, so we should be good for rebasing this and trying it again.

yep I'm on it

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 26, 2023

@Conan-Kudo ok image are building and the one setup as the integration test for the changes introduced here is done too. From my perspective it looks good now, please double check

osc getbinaries Virtualization:Appliances:Images:Testing_x86:fedora test-image-live-disk images x86_64 -M Virtual
kvm-cpu Broadwell-v2 -m 4096 -serial stdio kiwi-test-image-live-disk.x86_64-2.0.0-Virtual-Build77.2.qcow2

root: linux

btrfs subvol list /
mount | grep btrfs

Thanks

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Jul 26, 2023

The fstab looks right, but can we make it possible so that btrfs subvolume set-default doesn't get used during image setup? It's tricky to mount the actual parent volume to the two subvolumes because we do that. My suggestion is that we don't run that command if btrfs_create_toplevel_subvolume is set to false.

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 27, 2023

The fstab looks right, but can we make it possible so that btrfs subvolume set-default doesn't get used during image setup? It's tricky to mount the actual parent volume to the two subvolumes because we do that. My suggestion is that we don't run that command if btrfs_create_toplevel_subvolume is set to false.

Hmm, I think we shouldn't connect that implicitly. Looking into it

@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch from 5b2c135 to 8d8c14c Compare July 27, 2023 07:58
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 27, 2023

@Conan-Kudo ok I have added an attribute that allows you to configure if kiwi should set a default volume or not. The integration test has this switched off as you wanted.

With the last commit I fixed the places in kiwi which assumed there is a default volume set. With that patch the integration test will also start to build again

However, there is now another problem at boot of the test image (I did built it here locally). Not unexpected the image fails to boot and you land in a rescue shell inside of the initrd. As far as I can see the initrd simply tries to mount the root filesystem. Without any default volume configured this of course is not the correct volume. Because btrfs uses (/)
As the requested layout of the system is this:

UUID=860cc59c-936f-4c11-ab4a-c4dffdb88fb4 / btrfs defaults,subvol=root 0 1
UUID=860cc59c-936f-4c11-ab4a-c4dffdb88fb4 /home btrfs defaults,subvol=home 0 0

you have the root volume and the home volume and none of them are default when mounting.
I know at suse they have patched grub to deal with this situation and there is something like
set btrfs_relative_path="yes" in the grub early boot script and the grub package at suse.

Brings me to the question if you don't want any default, how can I make this Fedora37 based system to boot again ?
I don't consider this a kiwi thing and my gut feeling tells me that it will not be easy to find a proper and generic solution to it.

Please advise

@Conan-Kudo
Copy link
Member

Now we need to propagate rootflags=subvol=root to the bootloader config.

For example, this is what one of my installed Fedora systems has:

[root@Belldandy-Slimbook ~]# cat /boot/loader/entries/1c0b343ff0414c459df6e3b4aafe72f2-6.3.9-200.fc38.x86_64.conf
title Fedora Linux (6.3.9-200.fc38.x86_64) 38 (KDE Plasma)
version 6.3.9-200.fc38.x86_64
linux /vmlinuz-6.3.9-200.fc38.x86_64
initrd /initramfs-6.3.9-200.fc38.x86_64.img
options root=UUID=e69f9fa5-1641-4369-be7f-4a56a86ca597 ro rootflags=subvol=root resume=UUID=13169969-3d8d-4980-ba78-8e8f79a0138b rhgb quiet
grub_users $grub_users
grub_arg --unrestricted
grub_class fedora

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 27, 2023

Ah ok, that should be easy we already have a section to specify custom bootloader config settings.... I'll jump on it later as I have to do another task now

stay tuned... again :)

By default kiwi runs btrfs set-default on the volume that is
considered the default volume according to the btrfs settings
and defaults. btrfs_set_default_volume="false" allows
to deactivate this action. Along with the change also the
misleading name of the btrfs_create_toplevel_subvolume has
been changed to root_is_subvolume
The setting of a default volume is unwanted here
With the possibility to switch off setting the default volume
an issue at other parts in the kiwi code which mounted the
btrfs based system were uncovered. Without any default volume
set it's required to transport the root volume if different
from / and pass the respective subvol= option to the mount.
This commit fixes it at the places where kiwi trusted btrfs
to have a correct default volume set
@schaefi schaefi force-pushed the btrfs_no_hardcoded_toplevel_volume branch from 2e88686 to dd110f6 Compare July 27, 2023 14:19
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 27, 2023

ok works, we are getting closer :)

@Conan-Kudo
Copy link
Member

Are we in a reviewable/testable state now?

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 27, 2023

Are we in a reviewable/testable state now?

yes we are, the integration test is currently building. We should be good now. All other builds still succeed and we are backward compatible

In case of btrfs and if btrfs_set_default_volume is explicitly
switched off, we create the correct rootflags= kernel cmdline
entry to tell the system about the root volume for booting
@schaefi
Copy link
Collaborator Author

schaefi commented Jul 31, 2023

@Conan-Kudo waiting for all tests to pass...

@schaefi
Copy link
Collaborator Author

schaefi commented Jul 31, 2023

@Conan-Kudo waiting for all tests to pass...

looks good

Copy link
Member

@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.

We're finally in a good place!

@Conan-Kudo Conan-Kudo merged commit 60d7b1f into master Jul 31, 2023
12 checks passed
@Conan-Kudo Conan-Kudo deleted the btrfs_no_hardcoded_toplevel_volume branch July 31, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants