Advanced partitioning customizations (COMPOSER-2399) #1041

achilleas-k · 2024-11-13T19:40:48Z

This is the main part of #926 which I split into multiple smaller PRs. With this PR, all of the functionality from the original one is implemented, with some changes to the customization format.

This PR adds a new advanced partitioning customization to the blueprint that has the following structure:

{
  "customizations": {
    "disk": {
      "minsize": "",  # int or string
      "partitions": [
        {
          "type": "plain",
          "fs_type": "",
          "mountpoint": "",
          "label": ""
        },
        {
          "type": "lvm",
          "minsize": "",  # int or string
          "name": "",  # volume group name
          "logical_volumes": [
            {
              "name": "",  # logical volume name
              "fs_type": "",
              "minsize": ""  # int or string
            }
          ]
        },
        {
          "type": "btrfs",
          "minsize": "",  # int or string
          "subvolumes": [
            {
              "name": "",
              "mountpoint": ""
            }
          ]
        }
      ]
    }
  }
}

When the new customizations are used, the base partition table for the image type is ignored. Instead, a new "custom" partition table is constructed from scratch by iterating through the customizations and creating partitions, volume groups and logical volumes, and btrfs volumes and subvolumes as needed. Boot, EFI, and root partitions are created automatically to make the partition table valid and usable. Parameters such as boot mode and partition table type should be determined by the caller of the function, which should be based on the distro, architecture, and image type. This is, in a way, much simpler than our existing ensureLVM() and ensureBtrfs() functions, which needed to modify an existing partition table's structure in a way that preserved some parts and not others.

The custom partition table creation function (NewCustomPartitionTable()) supports all kinds of valid but perhaps unwanted configurations. For example, it's possible to create a partition table with both LVM volume groups and btrfs volumes, or multiples of each. The customizations however don't allow this, but the structure supports it (see above) in case we want to expand what we support in the future.

Customization validation happens in the blueprint. Invalid values such as duplicate mountpoints, empty btrfs subvolume names, etc, are caught by a validator in the blueprint code itself.

What's next:

Add support for defining the partition table type (dos or gpt).
Add support for swap partitions and swap logical volumes.
Add support for LUKS encryption.

DRAFT

Enable the new customizations in distros.
Add test configs.

Size values in the blueprint can be defined as either integers, representing bytes, or strings with SI unit prefixes for bytes. The conversion for the Filesystem.MinSize happens in both the JSON and TOML unmarshallers. With the new partitioning customizations we will have more conversions of the same type. This commit extracts the handling into a generic function for reusability and convenience. This changes the error messages. Tests have been updated accordingly.

- New structure that's more powerful than the existing array of FilesystemCustomization. - The general structure defines a (minimum) size for the overall disk and an array of partitions. - Each partition has one of three types: - plain: a plain disk partition, meaning partitions with traditional filesystem payloads (xfs, ext4, etc). - lvm: a partition containing an LVM volume group, which in turn will contain one or more LVM logical volumes. - btrfs: a partition containing a btrfs volume, which in turn will contain one or more subvolumes. - The new FilesystemTypedCustomization struct supports a label and fs_type and no minsize, compared to the old FilesystemCustomization. This struct is used to define both the payload of a plain partition and the payload of an LVM logical volume. - The PartitionCustomization has a type and size and embeds three substructures, one for each partition type. Decoding/unmarshalling of the PartitionCustomization is handled by a "typeSniffer" which first reads the value of the "type" field and then deserialises the whole object into a struct that only contains the fields valid for that partition type. This ensures that no fields are set for the substructure of a different type than the one defined in the "type" fields.

The unmarshaller decodes "type" and "minsize" manually and reuses the decode* functions from the JSON unmarshaller for each subtype based on the value of "type".

Add validation functions for DiskCustomization. Three types of validation are added: - Validate(): checks for functionally invalid configurations (bad mountpoint names or invalid structures, etc). The docstring lists all properties that are validated. - ValidateLayoutConstraints(): checks for limitations we imposed ourselves as a matter of policy and support on the structure of the partition table. This includes disallowing, for example, multiple LVM volume groups, multiple btrfs volumes, or combinations of the two. While these layouts are technically valid for a partitioned disk, we currently do not support them. - CheckDiskMountpointsPolicy(): Checks that none of the mountpoints are disallowed by the given PathPolicy. Tests added for all validators.

Define a nested map: [partition table type] -> [partition type name] -> [partition type ID] and use it from a private function getPartitionTypeIDfor() for convenient partition type ID selection.

New function that creates a partition table from scratch based on the new disk customization. First, it creates any necessary partitions for the selected boot mode (BIOS boot, ESP, or both). Then it determines if a /boot partition is needed based on the partitions and mountpoints defined in the disk customizations. Then, it iterates through partitions and creates each one in the order its defined. Finally, it creates a root filesystem if one wasn't already defined, resizes any necessary volumes, relayouts the table (setting all partition starts and offsets), and generates UUIDs all entities. Because of the new way the boot partition is handled, the EnsureBootPartition() function was changed to always add a boot partition and was renamed to AddBootPartition() to match the new behaviour. The tests have been updated accordingly.

Preparing the function to use the new PartitioningCustomization when set.

For all image types that don't specify a requiredPartitionSizes, add the defaults that are used in the NewPartitionTable() so that NewCustomPartitionTable() has similar behaviour. The only image type that explicitly doesn't specify minimum sizes is the iot-raw-image, which has a pre-existing empty map.

Add checks for Partitioning customizations in Fedora's checkOptions() function. The function now returns an error if: - Partitioning is defined at the same time as Filesystem - Partitioning is defined for ostree image types (commit and container). - The mountpoints in Partitioning violate the mountpoint policies for the image type.

One for each variant: - Plain - Plain + btrfs - Plain + lvm

pkg/blueprint/customizations.go

pkg/blueprint/partitioning_customizations.go

mvo5

Thank you! This is good stuff, I have some ideas/comments/suggestions inline but no blockers (I think) and I think we can land this and do the nitpicks via a followup. I did not review (most of) the tests (a 4k review is hard) but I will later today/tomorow

pkg/blueprint/partitioning_customizations.go

pkg/blueprint/filesystem_customizations.go

pkg/blueprint/partitioning_customizations.go

pkg/blueprint/filesystem_customizations_test.go

pkg/blueprint/partitioning_customizations.go

pkg/disk/partition_table.go

mvo5 · 2024-11-19T13:53:01Z

pkg/disk/partition_table.go

+func addPlainPartition(pt *PartitionTable, partition blueprint.PartitionCustomization, options *CustomPartitionTableOptions) error {
+	fstype, err := options.getfstype(partition.FSType)
+	if err != nil {
+		return fmt.Errorf("error creating partition with mountpoint %q: %w", partition.Mountpoint, err)


(nitpick) we could use this (in all add*) as an errPrefix := fmt.Sprintf("error creating partition with mountpoint %q:", partition.Mountpoint)

bcl · 2024-11-19T19:05:39Z

pkg/blueprint/partitioning_customizations.go

+// for the substructure of a different type than the one defined in the "type"
+// fields.
+func (v *PartitionCustomization) UnmarshalJSON(data []byte) error {
+	errPrefix := "JSON unmarshal:"


What's the purpose of errPrefix if it is the same for all of the errors? I think it makes the code harder to read when jumping into the middle of it, and it also makes it impossible to grep for things like 'JSON unmarshal: foo failure at position bar'

An understandable concern (not being able to grep the source for a full error message). OTOH, it is a bit tedious to have the same string repeated throughout the function in every return and risking a typo or something like that changing just one of the error message prefixes. Idk which is better tbh.

mvo5

Thank you! I did another pass over the test-cases, I think we miss a test for but we can easily add it in a followup, from my PoV this is good to go and all the comments/ideas can also be done/discussed in followups.

pkg/blueprint/partitioning_customizations_test.go

mvo5 · 2024-11-20T09:31:26Z

pkg/blueprint/partitioning_customizations_test.go

+			}`,
+			expected: &blueprint.PartitionCustomization{
+				Type:    "lvm",
+				MinSize: 99 * datasizes.GiB,


(super nitpick) I wonder if we should just use MinSize: 106300440576 here, for the^Wmost readers it's not obvious that the former translated into 99GiB

mvo5 · 2024-11-20T09:32:22Z

pkg/blueprint/partitioning_customizations_test.go

+			input:    `{"type":"not-a-partition-type"}`,
+			errorMsg: "JSON unmarshal: unknown partition type: not-a-partition-type",
+		},
+		"number": {


(super nitpick) maybe "bad-type/number" ?

pkg/blueprint/partitioning_customizations_test.go

mvo5 · 2024-11-20T09:52:38Z

pkg/disk/partition_table_test.go

+					},
+					{
+						Start:    202 * datasizes.MiB,
+						Size:     0,


This seems slightly odd, so we create partitions of size 0 without minsize constraints? I guess this will never happen in practice but is there a scenario where we would want this result or should we error if we do not get size constraints (RequiredMinSizes) for our auto-created partitions? (or am I just missing something here?)

mvo5 · 2024-11-20T10:00:25Z

pkg/disk/partition_table.go

+		pt.EnsureDirectorySizes(options.RequiredMinSizes)
+	}
+
+	pt.relayout(customizations.MinSize)


AFAICT we have no test for this value, yes? If I do:

diff --git a/pkg/disk/partition_table.go b/pkg/disk/partition_table.go index 9bed8eb61..213414340 100644 --- a/pkg/disk/partition_table.go +++ b/pkg/disk/partition_table.go @@ -1237,7 +1237,7 @@ func NewCustomPartitionTable(customizations *blueprint.DiskCustomization, option pt.EnsureDirectorySizes(options.RequiredMinSizes) } - pt.relayout(customizations.MinSize) + pt.relayout(0) pt.GenerateUUIDs(rng) return pt, nil

no test fails - should be easy enough to add though, the testing is so nice already which makes extending trivial

mvo5 · 2024-11-20T10:04:48Z

pkg/disk/partition_table_test.go

+					},
+					{
+						Start:    818 * datasizes.MiB,                                                                         // the root vg is moved to the end of the partition table by relayout()
+						Size:     3*datasizes.GiB + 16*datasizes.MiB + datasizes.MiB - (disk.DefaultSectorSize + (128 * 128)), // the sum of the LVs (rounded to the next 4 MiB extent) grows by 1 grain size (1 MiB) minus the unaligned size of the header to fit the gpt footer


(nitpick) ~~maybe instead of 128*128 just 4 * datasizes.MiB ?~~ hm, maybe I should just let go but I struggle to see where the 4MiB extend comes from here

mvo5 · 2024-11-20T10:11:11Z

pkg/disk/partition_table_test.go

+			expected: &disk.PartitionTable{
+				Type: disk.PT_GPT, // default when unspecified
+				UUID: "0194fdc2-fa2f-4cc0-81d3-ff12045b73c8",
+				Size: 818*datasizes.MiB + 16*datasizes.MiB + 3*datasizes.GiB + datasizes.MiB, // start + size of last partition (VG) + footer


(nitpick) same as above, it becomes a bit tricky to follow where the 818 comes from, the comment seems to be a bit terse, so maybe a comment like // start + efi + boot + 2 VGs + rootsize or something might slightly more helpful

mvo5 · 2024-11-20T10:14:03Z

pkg/disk/partition_table_test.go

+			expected: &disk.PartitionTable{
+				Type: disk.PT_GPT, // default when unspecified
+				UUID: "0194fdc2-fa2f-4cc0-81d3-ff12045b73c8",
+				Size: 714*datasizes.MiB + 168*datasizes.MiB + datasizes.MiB, // start + size of last partition (VG) + footer


(nitpick) It becomes a bit tricky to follow where the 714 comes from, the comment seems to be a bit terse, so maybe a comment like // start + efi + boot + 3 LVs ` or something might slightly more helpful (same below)

ondrejbudai

A handful of docs/error reporting suggestions, but nothing major, thanks for this @achilleas-k , this is awesome!

ondrejbudai · 2024-11-20T15:45:02Z

pkg/blueprint/partitioning_customizations.go

+	Partitions []PartitionCustomization `json:"partitions,omitempty" toml:"partitions,omitempty"`
+}
+
+// PartitionCustomization defines a single partition on a disk. The Type


Should the type descriptions should go to the Type field?

ondrejbudai · 2024-11-20T15:46:44Z

pkg/blueprint/partitioning_customizations.go

+	// The type of payload for the partition (optional, defaults to "plain").
+	Type string `json:"type" toml:"type"`
+
+	// Minimum size of the partition that contains the filesystem (for "plain"


What's the default MinSize for a plain partition? Should it actually be required for plain?

ondrejbudai · 2024-11-20T15:54:46Z

pkg/blueprint/partitioning_customizations.go

+	if lvAnySize.MinSize != nil {
+		size, err := decodeSize(lvAnySize.MinSize)
+		if err != nil {
+			return err


wrapping this would be nice

This commit changes some ordering and var names, very minor, test tweaks, c.f. * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment) * osbuild#1041 (comment)

This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.

This commit changes some ordering and var names, very minor, test tweaks, c.f. * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment) * #1041 (comment)

This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.

Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Follows, c.f. osbuild#1041 (comment) osbuild#1041 (comment)

Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. osbuild#1041 (comment) osbuild#1041 (comment)

This commit extracts a new `maybeAddBootPartition()` helper to make the `NewCustomPartitionTable()` a little bit more linear to read. Followup, c.f. osbuild#1041 (comment)

Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. osbuild#1041 (comment)

This commit extracts a new `maybeAddBootPartition()` helper to make the `NewCustomPartitionTable()` a little bit more linear to read. Followup, c.f. osbuild#1041 (comment)

Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. #1041 (comment)

This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. osbuild#1041 (comment)

This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.

This commit extracts a new `maybeAddBootPartition()` helper to make the `NewCustomPartitionTable()` a little bit more linear to read. Followup, c.f. #1041 (comment)

This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. osbuild#1041 (comment)

This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. #1041 (comment)

This commit makes use of the excellent work in osbuild/images#1041 and wires up support to generate LVM and btrfs volumes via the new disk customizations. It also add tests.

Small non-functional tweaks for the disk customization tests. C.f.: osbuild#1041 (comment) osbuild#1041 (comment)

achilleas-k requested review from ondrejbudai, mvo5 and supakeen November 13, 2024 19:41

achilleas-k added 12 commits November 15, 2024 00:55

blueprint: add custom TOML unmarshaller for PartitionCustomization

b9ad80d

The unmarshaller decodes "type" and "minsize" manually and reuses the decode* functions from the JSON unmarshaller for each subtype based on the value of "type".

blueprint: tests for PartitionCustomization json and toml decoding

5c0fe88

disk: extract partition type ID selection into a function

da345e6

Define a nested map: [partition table type] -> [partition type name] -> [partition type ID] and use it from a private function getPartitionTypeIDfor() for convenient partition type ID selection.

disk: add tests for NewCustomPartitionTable()

29542a1

distro/fedora: pass all customizations to getPartitionTable()

21fc141

Preparing the function to use the new PartitioningCustomization when set.

distro/fedora: enable the new partitioning functionality

58944b1

achilleas-k force-pushed the disk/NewCustomPartitionTable branch from c7b4411 to f84f305 Compare November 15, 2024 00:16

achilleas-k marked this pull request as ready for review November 15, 2024 00:17

test: add partitioning build configs

6ba8dad

One for each variant: - Plain - Plain + btrfs - Plain + lvm

achilleas-k force-pushed the disk/NewCustomPartitionTable branch from f84f305 to 6ba8dad Compare November 15, 2024 00:24

mvo5 mentioned this pull request Nov 18, 2024

datasize, blueprint: add new datasizes.Size type and use in blueprints #1049

Closed

mvo5 reviewed Nov 18, 2024

View reviewed changes

pkg/blueprint/customizations.go Show resolved Hide resolved

pkg/blueprint/partitioning_customizations.go Show resolved Hide resolved

mvo5 reviewed Nov 19, 2024

View reviewed changes

bcl reviewed Nov 19, 2024

View reviewed changes

mvo5 approved these changes Nov 20, 2024

View reviewed changes

ondrejbudai changed the title ~~Advanced partitioning customizations (COMPOSER-2355)~~ Advanced partitioning customizations (COMPOSER-2399) Nov 20, 2024

ondrejbudai approved these changes Nov 20, 2024

View reviewed changes

mvo5 enabled auto-merge November 20, 2024 16:05

mvo5 added this pull request to the merge queue Nov 20, 2024

Merged via the queue into osbuild:main with commit ead05d4 Nov 20, 2024
19 checks passed

mvo5 mentioned this pull request Nov 22, 2024

blueprint,disk: tiny style changes #1056

Merged

achilleas-k changed the title ~~Advanced partitioning customizations (COMPOSER-2399)~~ Advanced partitioning customizations (COMPOSER-2355) Nov 22, 2024

achilleas-k changed the title ~~Advanced partitioning customizations (COMPOSER-2355)~~ Advanced partitioning customizations (COMPOSER-2399) Nov 22, 2024

mvo5 mentioned this pull request Nov 25, 2024

bib: add support for the new Disk customizations osbuild/bootc-image-builder#725

Open

mvo5 mentioned this pull request Nov 25, 2024

disk: extract maybeAddBootPartition() helper #1062

Merged

mvo5 added a commit to mvo5/images that referenced this pull request Nov 25, 2024

disk: unexport AddBootPartition and AddPartitionsForBootMode

b79fdd1

Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. osbuild#1041 (comment)

github-merge-queue bot pushed a commit that referenced this pull request Nov 25, 2024

disk: unexport AddBootPartition and AddPartitionsForBootMode

56fbd91

Those two are not used outside of `disk` so let's unexport them to keep a smaller API. Followups, c.f. #1041 (comment)

mvo5 mentioned this pull request Nov 25, 2024

vendor: remove the vendored dependencies osbuild/image-builder#1418

Draft

mvo5 added a commit to mvo5/images that referenced this pull request Nov 25, 2024

blueprint: add extra comments to Validate{,LayoutConstraints}

330bb3c

This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. osbuild#1041 (comment)

mvo5 mentioned this pull request Nov 25, 2024

blueprint: add extra comments to Validate{,LayoutConstraints} #1064

Merged

mvo5 added a commit to mvo5/images that referenced this pull request Nov 25, 2024

blueprint: add extra comments to Validate{,LayoutConstraints}

e5dd018

This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. osbuild#1041 (comment)

achilleas-k pushed a commit that referenced this pull request Nov 25, 2024

blueprint: add extra comments to Validate{,LayoutConstraints}

42f6bab

This commit ensures that the roles of `Validate{,LayoutConstraints}` are clearer from the code comments. Follwup, c.f. #1041 (comment)

mvo5 added a commit to mvo5/images that referenced this pull request Nov 26, 2024

blueprint: tiny naming/style tweaks for the disk customization tests

1461a24

Small non-functional tweaks for the disk customization tests. C.f.: osbuild#1041 (comment) osbuild#1041 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced partitioning customizations (COMPOSER-2399) #1041

Advanced partitioning customizations (COMPOSER-2399) #1041

achilleas-k commented Nov 13, 2024 •

edited

Loading

mvo5 left a comment

mvo5 Nov 19, 2024

bcl Nov 19, 2024

achilleas-k Nov 25, 2024

mvo5 left a comment

mvo5 Nov 20, 2024

mvo5 Nov 20, 2024

mvo5 Nov 20, 2024

mvo5 Nov 20, 2024

mvo5 Nov 20, 2024 •

edited

Loading

mvo5 Nov 20, 2024

mvo5 Nov 20, 2024

ondrejbudai left a comment

ondrejbudai Nov 20, 2024

ondrejbudai Nov 20, 2024

ondrejbudai Nov 20, 2024

Advanced partitioning customizations (COMPOSER-2399) #1041

Advanced partitioning customizations (COMPOSER-2399) #1041

Conversation

achilleas-k commented Nov 13, 2024 • edited Loading

DRAFT

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejbudai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilleas-k commented Nov 13, 2024 •

edited

Loading

mvo5 Nov 20, 2024 •

edited

Loading