Support custom mountpoints under /var (COMPOSER-2301) #601

achilleas-k · 2024-08-09T18:31:51Z

This PR adds initial support for specifying custom mountpoints while creating a disk image. We define a new mountpoint policy internal to BIB, separate and more restrictive than the ostree mountpoint policies in osbuild/images. The rules are:

/ and /boot are supported for modifying their size.
Custom mountpoints under /var that do not become symlinks to other paths are supported.
/var itself is not.

The combination of 2. and 3. isn't directly supported in our policy checkers, so instead the policy in this PR initially allows for /var and its subdirectories, but then makes an explicit check that /var itself isn't in the custom filesystems in the blueprint.

Btrfs subvolumes are not supported because they are mounted with the parent disk at the location where they are created, so it is not possible to run bootc install to-filesystem with an empty root fileystem (see also osbuild/images#846). Since mountpoints cannot be created at paths that will contain data on or before first boot, a workaround for this would be to create subvolumes on first boot (manually or otherwise). We could enable this with firstboot scripts if necessary.

The discussion in #579 should provide further context for the limitations.

Trivial tweak for the errors returned from PathPolicies.Check() so that they are a bit more user friendly. See also osbuild/bootc-image-builder#601

mvo5

Thank you! This looks nice and will be a super nice feature for our users. I have some small suggestion inline mostly around the error handling/comments. It's fine to merge as is of course as my suggestions are all cosmetic.

bib/cmd/bootc-image-builder/image.go

test/test_build.py

achilleas-k · 2024-08-12T10:58:33Z

The error messages we test for will change once osbuild/images#853 is merged and pulled in here, but we can deal with that separately if this gets merged first.

Trivial tweak for the errors returned from PathPolicies.Check() so that they are a bit more user friendly. See also osbuild/bootc-image-builder#601

achilleas-k · 2024-08-12T17:15:57Z

I think everything's sorted now.

cgwalters · 2024-08-12T17:54:36Z

I tried to dig into this a bit but maybe I'm missing it...what's the expected UX here? What would some sample blueprints look like?

cgwalters · 2024-08-12T17:55:15Z

IOW it'd be helpful I think if the PR included changes to the docs in this repo at least.

achilleas-k · 2024-08-12T18:20:42Z

I've updated the README with information on the new customizations.

mvo5

Thanks for the updated version and the excellent README update. It seems some unit test is failing so I won't appove just yet but it looks very close. Some small suggestions/questions inline for your consideration.

bib/cmd/bootc-image-builder/image.go

achilleas-k · 2024-08-13T11:55:24Z

It seems some unit test is failing so I won't appove just yet but it looks very close.

Yeah, I spotted those too but was waiting for the full result to address them (running locally was taking too long last night and I had to stop). I'll get to them now.

README.md

achilleas-k · 2024-08-13T14:51:10Z

It seems some unit test is failing so I won't appove just yet but it looks very close.

The issue was the fallback logic for the root filesystem type in cross-arch builds. I made some changes to the internal logic there, making it an error if it's not set from outside the Manifest() call. I think I prefer the explicitness.

mvo5

Thank you

cgwalters · 2024-08-13T15:44:28Z

I think this change stands alone and makes sense on its own; but we did also discuss supporting fully custom toplevel mount points like /data or whatever - is that a followup?

There's also the case for e.g. /mnt (a default empty toplevel directory)

achilleas-k · 2024-08-13T15:55:40Z

I think this change stands alone and makes sense on its own; but we did also discuss supporting fully custom toplevel mount points like /data or whatever - is that a followup?

Yes, after our discussion in #579, I decided the quickest way to get something working is this. Top-level mounts will need to be handled differently (.mount units?), so once we figure all that out, we can have it work the same way for everything.

achilleas-k · 2024-08-13T17:11:46Z

Needs a bit more work for fs type handling and cross-arch builds.

With the changes in 5f70417, the minimum size for the root filesystem, calculated as double the size of the base container, was not being considered at all—the Filesystems property of the ManifestConfig was being ignored. Also, the rootfs type from the container was not being applied to the root filesystem in the base partition table. Replace the Filesystems property with a simple RootfsMinsize to clarify its purpose and use it to update or add to the user-defined customizations. Add a new function, setRootfsType(), which sets the root filesystem type based on the rootfs option set either by the container config or the user option. The function replicates the checks that were moved to TestBasePartitionTablesHaveRoot(). The checks in the test code will catch any modifications to the static partition tables defined in code. Running the same checks while modifying the partition table will also catch any issues caused by the partition table being modified before the rootfs type is applied.

achilleas-k · 2024-08-14T13:51:10Z

I applied @mvo5's patch for splitting out the PT handling and adds tests.
I also rebased on main and resolved the conflicts.

When setting the rootfs type, make sure it's not an empty string, which would be invalid. This makes it a hard requirement that the ManifestConfig.RootFSType is set when calling manifestForDiskImage() (i.e. Manifest() with ManifestConfig.BuildType == BuildTypeDisk).

Before this change, the RootFSType validation would set the option to an empty string when an unsupported type was used for cross-arch builds. This left the type for the root filesystem unchanged from the default when generating the partition table for the manifest. Now that the RootFSType is required, let's be explicit about what we're doing and set it to the only (currently) supported type for cross-arch builds: ext4.

Iterate over each Mountable in the partition table (each filesystem) and change the filesystem type to match the rootfs, with two exceptions: - /boot/efi is never changed - /boot is not changed when rootfs is btrfs Do this *after* the partition table with the customizations has been generated, so that all filesystems in the final partition table are properly set. This also fixes osbuild#407.

Small tweak to make the partition table generation eaiser to test in a "integration" type of way and adding some tests.

Add a test that checks that filesystem customizations appear in the manifest in the fstab stage. Checks that: - All filesystems, where appropriate, have the same type as the desired rootfs. - All filesystems listed in the customizations appear in the fstab stage.

Add the same test as in the previous commit but for cross-arch manifests. Cross-arch builds only support ext4 filesystems. Any selected root fstype (whether from the container itself or from the --rootfs option) is ignored (with a warning)

mvo5

Lovely, thank you!

mvo5 · 2024-08-15T09:54:20Z

It fails right now with:

____ test_image_boots[quay.io/centos-bootc/centos-bootc:stream9,raw,arm64] _____
...
            # NOTE: xfs filesystems are ~40 MiB and ext4 ~26 MiB smaller than the partition - reduce minsize by 100 MiB
            minsize -= 100 * 2 ** 20
>           assert minsize < mountpoint_sizes[mountpoint]
E           assert 3116367872 < 3094126592

test/test_build.py:483: AssertionError

in

The size comparison in the build test is bad. Customizations control the size of a partition but the function was comparing the usable size of a filesystem, which can be very different. Attempts were made to cover the difference, by simply reducing the expected size by a fixed amount, but were ultimately inadequate. We can do better.

mvo5

Thanks, this is fine! We could look into cleaning up the commit history a bit, but it's probably fine as is.

achilleas-k · 2024-08-15T10:15:54Z

It fails right now with:

____ test_image_boots[quay.io/centos-bootc/centos-bootc:stream9,raw,arm64] _____
...
            # NOTE: xfs filesystems are ~40 MiB and ext4 ~26 MiB smaller than the partition - reduce minsize by 100 MiB
            minsize -= 100 * 2 ** 20
>           assert minsize < mountpoint_sizes[mountpoint]
E           assert 3116367872 < 3094126592

test/test_build.py:483: AssertionError

Yup, that size comparison isn't great. Was a bad idea to just take a chunk out of the expected size and call it a day. I dropped the check and we can replace it with something a little smarter (see last commit), but I think we should leave that for another PR.

The partition sizing is either way tested at the manifest level. The boot test now checks that all mountpoints exist when the system is booted.

achilleas-k · 2024-08-15T10:18:40Z

Thanks, this is fine! We could look into cleaning up the commit history a bit, but it's probably fine as is.

It did get a bit long, I agree. The last commit could be squashed, but let's keep my stupid attempt at a size comparison heuristic in there for posterity.

achilleas-k requested review from ondrejbudai, cgwalters, mvo5 and thozza August 9, 2024 18:31

achilleas-k force-pushed the partitions branch 3 times, most recently from 4642953 to a4ade40 Compare August 9, 2024 18:49

mvo5 added a commit to mvo5/images that referenced this pull request Aug 12, 2024

pathpolicy: tweak error messages and add test

48a701c

Trivial tweak for the errors returned from PathPolicies.Check() so that they are a bit more user friendly. See also osbuild/bootc-image-builder#601

mvo5 mentioned this pull request Aug 12, 2024

pathpolicy: tweak error messages and add test osbuild/images#853

Merged

mvo5 previously approved these changes Aug 12, 2024

View reviewed changes

achilleas-k dismissed mvo5’s stale review via 48e9207 August 12, 2024 10:40

achilleas-k force-pushed the partitions branch from a4ade40 to 48e9207 Compare August 12, 2024 10:40

achilleas-k force-pushed the partitions branch from a9a80b8 to 3743975 Compare August 12, 2024 17:15

mvo5 reviewed Aug 13, 2024

View reviewed changes

bib/cmd/bootc-image-builder/image.go Show resolved Hide resolved

bib/cmd/bootc-image-builder/image.go Outdated Show resolved Hide resolved

bib/cmd/bootc-image-builder/image.go Outdated Show resolved Hide resolved

cgwalters reviewed Aug 13, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

achilleas-k force-pushed the partitions branch from 67307cd to c115bf7 Compare August 13, 2024 14:47

achilleas-k requested a review from mvo5 August 13, 2024 14:47

mvo5 previously approved these changes Aug 13, 2024

View reviewed changes

achilleas-k dismissed mvo5’s stale review via 6bfa04e August 13, 2024 19:29

achilleas-k added 2 commits August 14, 2024 14:56

bib: test UpdateFilesystemSizes()

8de2b40

achilleas-k force-pushed the partitions branch 2 times, most recently from e987d13 to 7809d9b Compare August 14, 2024 13:50

achilleas-k requested a review from mvo5 August 14, 2024 13:51

achilleas-k added 5 commits August 14, 2024 20:19

bib: fix typo in warning: prefered -> preferred

f6df7ce

bib: update the comment about filesystem types in cross-arch builds

96c60f8

achilleas-k force-pushed the partitions branch from 7809d9b to 3e390f5 Compare August 14, 2024 18:19

mvo5 and others added 5 commits August 14, 2024 20:23

image: extract genPartitionTable() helper and add unit tests

eafeb74

Small tweak to make the partition table generation eaiser to test in a "integration" type of way and adding some tests.

README: document the filesystem customizations

33adb87

README: rootfs changes all filesystem types

1ea9764

achilleas-k force-pushed the partitions branch from 3e390f5 to 1ea9764 Compare August 14, 2024 18:23

mvo5 previously approved these changes Aug 14, 2024

View reviewed changes

mvo5 enabled auto-merge August 14, 2024 19:19

achilleas-k dismissed mvo5’s stale review via 48eab3d August 15, 2024 10:10

mvo5 approved these changes Aug 15, 2024

View reviewed changes

mvo5 added this pull request to the merge queue Aug 15, 2024

Merged via the queue into osbuild:main with commit 2713962 Aug 15, 2024
8 of 10 checks passed

achilleas-k deleted the partitions branch August 15, 2024 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom mountpoints under /var (COMPOSER-2301) #601

Support custom mountpoints under /var (COMPOSER-2301) #601

achilleas-k commented Aug 9, 2024

mvo5 left a comment

achilleas-k commented Aug 12, 2024

achilleas-k commented Aug 12, 2024

cgwalters commented Aug 12, 2024

cgwalters commented Aug 12, 2024

achilleas-k commented Aug 12, 2024

mvo5 left a comment

achilleas-k commented Aug 13, 2024

achilleas-k commented Aug 13, 2024

mvo5 left a comment

cgwalters commented Aug 13, 2024

achilleas-k commented Aug 13, 2024

achilleas-k commented Aug 13, 2024

achilleas-k commented Aug 14, 2024

mvo5 left a comment

mvo5 commented Aug 15, 2024

mvo5 left a comment

achilleas-k commented Aug 15, 2024

achilleas-k commented Aug 15, 2024

Support custom mountpoints under /var (COMPOSER-2301) #601

Support custom mountpoints under /var (COMPOSER-2301) #601

Conversation

achilleas-k commented Aug 9, 2024

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Aug 12, 2024

achilleas-k commented Aug 12, 2024

cgwalters commented Aug 12, 2024

cgwalters commented Aug 12, 2024

achilleas-k commented Aug 12, 2024

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Aug 13, 2024

achilleas-k commented Aug 13, 2024

mvo5 left a comment

Choose a reason for hiding this comment

cgwalters commented Aug 13, 2024

achilleas-k commented Aug 13, 2024

achilleas-k commented Aug 13, 2024

achilleas-k commented Aug 14, 2024

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 commented Aug 15, 2024

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Aug 15, 2024

achilleas-k commented Aug 15, 2024