Add the btrfs partitioning mode #694

ondrejbudai · 2024-05-16T15:31:58Z

(This is a revived #422.)

This PR has basically three parts:

The first commits fix already existing btrfs code in the library (it was never used before). Just tiny fixes. I tried to add tests where they had been missing before.
The "middle section" adds missing parts for btrfs subvolume. The scaffolding was there, but the translation from an abstract partition table to osbuild stages was missing.
The final section is the actual addition of a btrfs partitioning mode. When this mode is used, a root partition from the base partition table is converted to a btrfs one with a subvolume. The code for adding more btrfs subvolumes based on blueprint mountpoints was already there.

Weird parts:

When a btrfs partitioning mode is used, the compression is hard-coded to zstd:1. It would be great if this could be configured per an image type/distro/arch, but the partitioning code currently cannot do such specific things. In the end, this should be probably also user-configurable. However, I think that this default value is alright for the starters, we can always iterate on this.
Setting min-size for extra mountpoints is a weird thing, because the extra mountpoints are translated to subvolumes... Maybe we should just error out if the min size is set with btrfs and depend just on the total size customization... Happy to discuss this!

TODO:

more tests (especially for ensureBtrfs)

achilleas-k · 2024-05-16T15:42:35Z

When a btrfs partitioning mode is used, the compression is hard-coded to zstd:1. It would be great if this could be configured per an image type/distro/arch, but the partitioning code currently cannot do such specific things. In the end, this should be probably also user-configurable. However, I think that this default value is alright for the starters, we can always iterate on this.

I can't say for sure (I don't remember), but I bet we gave little consideration for this when adding it. It was probably a decision to add it and figure out the config later. Like you said, it's a sane enough default.

Setting min-size for extra mountpoints is a weird thing, because the extra mountpoints are translated to subvolumes... Maybe we should just error out if the min size is set with btrfs and depend just on the total size customization... Happy to discuss this!

I remember thinking about this, going back and forth on ideas for how to handle sizing. One idea was to sum up the min sizes and set the whole volume as the sum. Erroring out on sizing for subvols also makes sense though.

mvo5

I did not got very far but some nitpicks/comments ideas inline before I call it a day :) Thanks for the work on this, looks very nice so far and I really think this will help bootc!

pkg/disk/btrfs_test.go

pkg/disk/btrfs.go

pkg/disk/disk_test.go

pkg/disk/partition_table.go

mvo5

Thank you! This looks very nice! I focused on the code in the review and did not dig into how fedora uses btrfs and how that maps but it seems also fine. Great stuff, some ideas/nitpicks inline but no blockers (from my POV).

pkg/disk/partition_table.go

pkg/disk/partition_table_test.go

pkg/osbuild/device_test.go

pkg/osbuild/disk_test.go

bcl · 2024-05-20T18:53:57Z

Setting min-size for extra mountpoints is a weird thing, because the extra mountpoints are translated to subvolumes... Maybe we should just error out if the min size is set with btrfs and depend just on the total size customization... Happy to discuss this!

Or only error if the min-size is larger than the total volume size?

pkg/disk/btrfs.go

pkg/disk/partition_table.go

github-actions · 2024-06-22T04:00:20Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2024-06-29T04:00:25Z

This PR was closed because it has been stalled for 30+7 days with no activity.

achilleas-k

LGTM but I have some git commit and history related nitpicks:

There are a couple of commits with long messages.
Can we get the history cleaned up a bit? For instance squashing the small follow-up commits into the ones that introduced the small issue.

pkg/osbuild/disk_test.go

pkg/disk/disk_test.go

mvo5

Fwiw, I like it but my review only counts +0.5 as I did a bunch of tweaks to it. If this gets merged I will also look at a small followup to see if #694 (comment) makes sense and I am thinking about testing this more, maybe via #732 even - we give the partition code a bunch of inputs and see if we get the expected btrfs (but maybe I'm overthinking things here :)

achilleas-k · 2024-07-03T16:17:24Z

Merge conflicts

Prior this commit, the GenUUID method of Btrfs didn't propagate the generated UUID to a subvolume. Thus, when GetFSSpec was called on the subvolume, an empty UUID was returned. Thus, invalid kernel arguments were generated (for root fs), and the system rendered to be unbootable.

ForEachMountable is called for every subvolume, thus if there was one btrfs partition with multiple subvolumes, the code generated multiple mkfs.btrfs stages for one partition, breaking the build. This commit adds a tiny bit of logic for ensuring that we call mkfs just once per each partition.

So the code just panicked before. Let's generate a device name in a similar way as we do for a luks container.

This commit also adds a test that covers both mkfs.btrfs (see the previous commit) and creating subvolumes.

This commit also adds a semi-fake partition table with a Btrfs setup.

This commits also removes the generic opts (MntOpts) from btrfs subvolumes. They were not used anywhere, and I think it's much better to make all supported mount options explicit, rather than storing them in a generic array.

TODO: needs tests The conversion code from raw partitioning is basically the same as the one for LVM. The main difference is that LVM just embeds an already defined filesystem, but btrfs-ication needs to do some guessing because the code actually creates a new partition: specifically, I just hardcoded the compression to zstd:1. This is not ideal, but it's the Fedora's default, so it's imho fine as the first iteration.

This is weird, because we just hardcode zstd:1, but it's good enough for the starters (and also inspired by Fedora). I also introduced a constant for the default btrfs compression.

If the base partition table uses a read-only root filesystem (ostree), let's respect that when converting the root to a btrfs subvolume.

Add new TestGenImageKernelOptionsBtrfsNotRootCmdlineGenerated() to check that no commandline is generated for non "/" partitions.

A new test TestCreatePartitionTableBtrfsify was added. This test also uncovered a small bug in ensureBtrfs() that got fixed along the way.

mvo5 · 2024-07-04T07:34:29Z

Merge conflicts

Should be fixed now.

achilleas-k

Approving but the error from the linter should be fixed to avoid weird loop side-effects.

mvo5 · 2024-07-04T14:14:39Z

Approving but the error from the linter should be fixed to avoid weird loop side-effects.

Linter error is addressed.

ondrejbudai

fwiw, the changes from Michael are wonderful, thanks!

ondrejbudai mentioned this pull request May 16, 2024

Add support for btrfs as root fs osbuild/bootc-image-builder#439

Merged

achilleas-k self-requested a review May 16, 2024 15:38

mvo5 reviewed May 16, 2024

View reviewed changes

mvo5 previously approved these changes May 17, 2024

View reviewed changes

achilleas-k reviewed May 21, 2024

View reviewed changes

pkg/disk/btrfs.go Show resolved Hide resolved

achilleas-k reviewed May 21, 2024

View reviewed changes

pkg/disk/partition_table.go Outdated Show resolved Hide resolved

github-actions bot added the Stale label Jun 22, 2024

github-actions bot closed this Jun 29, 2024

mvo5 reopened this Jul 2, 2024

mvo5 dismissed their stale review via 2e709ef July 2, 2024 11:44

mvo5 removed the Stale label Jul 2, 2024

achilleas-k reviewed Jul 2, 2024

View reviewed changes

pkg/osbuild/disk_test.go Show resolved Hide resolved

pkg/disk/disk_test.go Show resolved Hide resolved

mvo5 force-pushed the btrfs3 branch from 2e709ef to 140c16c Compare July 2, 2024 17:38

mvo5 requested a review from achilleas-k July 2, 2024 17:39

mvo5 force-pushed the btrfs3 branch from 140c16c to b7d0d40 Compare July 3, 2024 11:20

mvo5 previously approved these changes Jul 3, 2024

View reviewed changes

ondrejbudai added 9 commits July 4, 2024 09:29

osbuild: fix missing device name for btrfs partitions

b80cbd0

So the code just panicked before. Let's generate a device name in a similar way as we do for a luks container.

osbuild: add creating btrfs subvolumes

b31e3d0

This commit also adds a test that covers both mkfs.btrfs (see the previous commit) and creating subvolumes.

osbuild: allow mounting btrfs subvolumes

3b88ee7

This commit also adds a semi-fake partition table with a Btrfs setup.

disk: generate cmdline for root on a btrfs subvolume

30320a4

disk: add support for btrfs compression

21b26b0

This commits also removes the generic opts (MntOpts) from btrfs subvolumes. They were not used anywhere, and I think it's much better to make all supported mount options explicit, rather than storing them in a generic array.

disk: create new btrfs subvolumes with zstd:1

f28b416

This is weird, because we just hardcode zstd:1, but it's good enough for the starters (and also inspired by Fedora). I also introduced a constant for the default btrfs compression.

ondrejbudai and others added 5 commits July 4, 2024 09:29

pkg/osbuild: add btrfs support for bootupd

2f3bf18

disk: clone the ro option to / when ensuring btrfs

183968e

If the base partition table uses a read-only root filesystem (ostree), let's respect that when converting the root to a btrfs subvolume.

disk: tweak error when creating /boot mountpoint (thanks to Achilleas)

414bbdb

osbuild: tweak TestMountsDeviceFromBrfs with expectedName

339ff9f

osbuild: go fmt on disk_test.go

dd60c76

mvo5 dismissed their stale review via a8632b1 July 4, 2024 07:30

mvo5 force-pushed the btrfs3 branch from b7d0d40 to a8632b1 Compare July 4, 2024 07:30

mvo5 added 7 commits July 4, 2024 09:31

osbuild: add new pkg/disk/dist_test.go

3617149

Add new TestGenImageKernelOptionsBtrfsNotRootCmdlineGenerated() to check that no commandline is generated for non "/" partitions.

disk: tweak TestBtrfsSubvolume_GetFSTabOptions to cover all cases

2e2c00d

disk: panic when BtrfsSubvolume is called without a Name

a6c0825

disk: tweak TestFSTabOptionsReadOnly ro->expctedRO in table

c43fa33

disk: move ensureBtrfs into the same line as ensureLVM

1fd7690

disk: bugfix/test for ensureBtrfs()

d6e5dbd

A new test TestCreatePartitionTableBtrfsify was added. This test also uncovered a small bug in ensureBtrfs() that got fixed along the way.

osbuild: update TestGenBootupdDevicesMountsHappyBtrfs

91e9acb

mvo5 force-pushed the btrfs3 branch from a8632b1 to 91e9acb Compare July 4, 2024 07:33

achilleas-k previously approved these changes Jul 4, 2024

View reviewed changes

osbuild: fix G601 in genMountsForBootupd()

81d8dfa

mvo5 dismissed achilleas-k’s stale review via 81d8dfa July 4, 2024 14:01

mvo5 requested a review from achilleas-k July 4, 2024 14:14

mvo5 approved these changes Jul 4, 2024

View reviewed changes

mvo5 enabled auto-merge July 4, 2024 14:15

achilleas-k approved these changes Jul 4, 2024

View reviewed changes

ondrejbudai commented Jul 4, 2024

View reviewed changes

mvo5 added this pull request to the merge queue Jul 4, 2024

Merged via the queue into osbuild:main with commit d28a8a8 Jul 4, 2024
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the btrfs partitioning mode #694

Add the btrfs partitioning mode #694

ondrejbudai commented May 16, 2024 •

edited

Loading

achilleas-k commented May 16, 2024

mvo5 left a comment

mvo5 left a comment

bcl commented May 20, 2024

github-actions bot commented Jun 22, 2024

github-actions bot commented Jun 29, 2024

achilleas-k left a comment

mvo5 left a comment

achilleas-k commented Jul 3, 2024 •

edited

Loading

mvo5 commented Jul 4, 2024

achilleas-k left a comment •

edited

Loading

mvo5 commented Jul 4, 2024

ondrejbudai left a comment

Add the btrfs partitioning mode #694

Add the btrfs partitioning mode #694

Conversation

ondrejbudai commented May 16, 2024 • edited Loading

achilleas-k commented May 16, 2024

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

bcl commented May 20, 2024

github-actions bot commented Jun 22, 2024

github-actions bot commented Jun 29, 2024

achilleas-k left a comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Jul 3, 2024 • edited Loading

mvo5 commented Jul 4, 2024

achilleas-k left a comment • edited Loading

Choose a reason for hiding this comment

mvo5 commented Jul 4, 2024

ondrejbudai left a comment

Choose a reason for hiding this comment

ondrejbudai commented May 16, 2024 •

edited

Loading

achilleas-k commented Jul 3, 2024 •

edited

Loading

achilleas-k left a comment •

edited

Loading