PR#2365: builder: handle btrfs subvolumes in ApplianceTask - koji

koji

#2365 builder: handle btrfs subvolumes in ApplianceTask

Merged 3 years ago by tkopecek. Opened 3 years ago by dcavalca.

dcavalca/koji btrfs into master

builder: handle btrfs subvolumes in ApplianceTask

Davide Cavalca • 3 years ago

01c52b2

builder/kojid

file modified

		`@@ -3181,6 +3181,10 @@`
		`for part in self.ks.handler.partition.partitions:`
		`if part.mountpoint == '/':`
		`return part.disk`
		`+ elif part.fstype == 'btrfs':`
		`+ for s in self.ks.handler.btrfs.btrfsList:`
		`+ if s.subvol and s.mountpoint == '/':`
		`+ return part.disk`
ngompa commented 3 years ago Maybe it makes sense to reverse this condition so that it doesn't short-circuit the other way?
		`raise koji.ApplianceError('kickstart lacks a "/" mountpoint')`

		`def handler(self, name, version, release, arch, target_info,`

dcavalca commented 3 years ago

When using btrfs, the / mountpoint can be associated with a subvolume; if that's the case, return the btrfs partition as the root device. Note that this implementation assumes there's only one btrfs partition defined in kickstart.

This is a fix for https://koji.fedoraproject.org/koji/taskinfo?taskID=47074262 which in turn should fix https://bugzilla.redhat.com/show_bug.cgi?id=1855034 . See the kickstart attached to that bug for an example config.

ngompa commented 3 years ago

:thumbsup:

ngompa commented 3 years ago

FYI @kevin: We need this patch deployed on Fedora Koji builders, please. :smile:

kevin commented 3 years ago

I'd like to see an ack that this is the right approach at least before updating our production servers (we have no staging currently to test this with. )

ngompa commented 3 years ago

Gack, I forgot about that...

ngompa commented 3 years ago

@mikem @tkopecek Can either of you look at this ASAP? We need this in for appliance image builds using Btrfs...

Cf. https://pagure.io/fedora-infrastructure/issue/9138

julian8628 commented 3 years ago

a trivial question: what should be expected if the kickstart file contains both root mountpoint and btrfs root sub-mountpoint?

part / ...
part btrfs.007 --fstype="btrfs" --size=5000
...
btrfs / --subvol --name=root LABEL=fedora

ngompa commented 3 years ago

Doesn't the last one win?

dcavalca commented 3 years ago

I'm not sure if that's a valid kickstart configuration in the first place. But either way, it doesn't matter for getRootDevice -- the comment says it's supposed to return the device name for /, which in both cases would be the device backing btrfs.007 (both for the partition itself and for the subvolume).

Edited 3 years ago by dcavalca

julian8628 commented 3 years ago

Thanks for the answer. The code LGTM, If the order doesn't matter or it is actually invalid.

julian8628 commented 3 years ago

How urgent could this issue be? We are in koji 1.22 code freeze now, so we probably have to put this into 1.22.1 or 1.23 release.

ngompa commented 3 years ago

ARM image builds for Rawhide have been broken for weeks now. We need this to fix that. So unfortunately it's quite urgent.

rebased onto 01c52b2

3 years ago

dcavalca commented 3 years ago

Rebased this on master in the meantime

kevin commented 3 years ago