search

OSInside / kiwi

KIWI - Appliance Builder Next Generation
https://osinside.github.io/kiwi
GNU General Public License v3.0
300 stars 152 forks source link

Prevent extra volume mount/umount on btrfs #2530

Closed schaefi closed 5 months ago

schaefi commented 5 months ago

For setting up the read-only property an extra mount of the btrfs sub-volumes was issued. However, all volumes are mounted at that time. Thus it's not required to mount them again, resulting in a busy state because of the auto-snapshot mounts which does not get umounted and keeps a busy state until the lazy umount kicks in. This Fixes #2529

schaefi commented 5 months ago

I added the don't merge label because I'd like Ludwig to double check if this fixes it for him

lnussel commented 5 months ago

It fixes the busy issue but the real problem is still lurking. sync_target is actually the root subvolume rather than snapshot 1. therefore the property ends up set on the wrong subvolume

lnussel commented 5 months ago

I think it needs sync_target = self.get_mountpoint() instead of sync_target = self.mountpoint

schaefi commented 5 months ago

yes this makes sense. I'll give it a test

lnussel commented 5 months ago

yup, with self.get_mountpoint() it works

schaefi commented 5 months ago

@lnussel also tested this successfully if the bootloader is not custom. To make sure there are no regressions by this change I will create a Staging build after this one has been merged.

Thanks

schaefi commented 5 months ago

waiting for staging builds to complete

schaefi commented 5 months ago

integration tests looks good

lnussel commented 5 months ago

any ETA? currently systemd-boot based images in Factory fail

schaefi commented 5 months ago

any ETA? currently systemd-boot based images in Factory fail

I was waiting for an approval from at least one involved person. I think we are good and I have a version in Staging and all integration test builds looks good. If you or Neal agrees I'll continue and make a submission today.

Conan-Kudo commented 5 months ago

Won't this mess up non-Btrfs stuff (e.g. LVM disks)? I vaguely recall that we have this specifically to ensure that everything cleans up properly...

schaefi commented 5 months ago

Won't this mess up non-Btrfs stuff (e.g. LVM disks)? I vaguely recall that we have this specifically to ensure that everything cleans up properly...

I was also concerned about the other volume manager but due to the refactoring into the context managers we have proper cleanup which allowed to cleanup some places of explicit mount/umount calls. There is an integration test for testing LVM and I did a closer look to that build to check if there are any regressions. I could not find one.