#2365 builder: handle btrfs subvolumes in ApplianceTask
Merged 2 years ago by tkopecek. Opened 2 years ago by dcavalca.
dcavalca/koji btrfs  into  master

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

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,

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.

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

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

Gack, I forgot about that...

@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

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

Doesn't the last one win?

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

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

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.

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

2 years ago

Rebased this on master in the meantime

Well, 12 days, but sure... I can carry a patch for fedora, I just wanted to make sure the general approach was fine for upstream.

yep, works for me (was offline for three weeks)

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Maybe it makes sense to reverse this condition so that it doesn't short-circuit the other way?

Ignore that comment, I'm dumb, it'd be the same disk either way...

Commit 1aa4fb5 fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done

2 years ago