Support swap partitions and logical volumes in DiskCustomization (COMPOSER-2400) #1072

achilleas-k · 2024-11-27T18:12:35Z

This PR adds support for swap partitions and logical volumes when using the new disk blueprint customizations.

Internally, swap space is represented by a new disk.Swap type (a disk.Element type) which implements the disk.PayloadEntity interface. I also added a new interface: disk.FSTabEntity. This new entity variant is for representing items that can appear in the fstab file. It differs from disk.Mountable in that it doesn't have a mountpoint, which makes it useful for distinguishing between filesystems and swap areas. It will also be useful for defining swap files as part of the partitioning layout (even though it is not, strictly speaking, part of the partition table).

In the customizations, swap is represented by setting the filesystem type to "swap". This is valid for both plain partitions and lvm logical volumes. When the fs type is "swap", the mountpoint must always be empty.

thozza

Great work. I've found a few minor issues, but overall, this looks very nice 👍

pkg/disk/swap.go

pkg/osbuild/mkfs_stages_test.go

mvo5

Thank you! This is very nice! I have a few inline suggestions/ideas/questions but I don't think there are any blockers, fine to merge to move this forward (or we can iterate here as you prefer).

A bit of a meta comment - splitting 1f9e2a2 (the testdisk move) out beforehand would be something I would have preferred, it would be a very simple review as it just shuffles things around. Then this PR would have been smaller.

pkg/disk/btrfs.go

mvo5 · 2024-11-28T08:26:14Z

pkg/blueprint/disk_customizations.go

+	if p.FSType == "swap" {
+		// make sure the mountpoint is empty and return
+		if p.Mountpoint != "" {
+			return fmt.Errorf("mountpoint for swap partition must be empty")


(super nitpick) maybe add the mountpoint so that the issue is easier to locate (e.g. mountpoint for swap partition must be empty, got %q", p.Mountpoint))

mvo5 · 2024-11-28T08:30:52Z

pkg/disk/filesystem.go

@@ -55,6 +55,10 @@ func (fs *Filesystem) GetMountpoint() string {
 	return fs.Mountpoint
 }

+func (fs *Filesystem) GetFSFile() string {


Same comment as for btrfs, we probably want:

diff --git a/pkg/disk/filesystem_test.go b/pkg/disk/filesystem_test.go index c97b811c9..a6cc67c09 100644 --- a/pkg/disk/filesystem_test.go +++ b/pkg/disk/filesystem_test.go @@ -9,4 +9,5 @@ import ( func TestImplementsInterfacesCompileTimeCheckFilesystem(t *testing.T) { var _ = disk.Mountable(&disk.Filesystem{}) var _ = disk.UniqueEntity(&disk.Filesystem{}) + var _ = disk.FSTabEntity(&disk.Filesystem{}) }

pkg/disk/partition_table.go

mvo5 · 2024-11-28T09:03:21Z

pkg/disk/partition_table.go

+
+	newpart := Partition{
+		Type:     partType,
+		Bootable: false,


(nitpick) as false is the default, we could just remove it here

mvo5 · 2024-11-28T10:08:04Z

pkg/disk/disk_test.go

@@ -164,6 +164,7 @@ func TestCreatePartitionTable(t *testing.T) {
 			// /boot and subdirectories is exempt from this rule
 			return nil
 		}
+


(nitpick) This change in the disk: add a test partition table with swap looks a bit unrelated(?)

I disagree. It's definitely completely unrelated.

mvo5 · 2024-11-28T10:11:34Z

pkg/osbuild/fstab_stage_test.go

+			// populate UUIDs
+			pt.GenerateUUIDs(rng)
+
+			// print an informative failure message if a new test partition


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

pkg/osbuild/mkswap_stage.go

+
+func (MkswapStageOptions) isStageOptions() {}
+
+func NewMkswapStage(options *MkswapStageOptions, devices map[string]Device) *Stage {


(niptick/followup) - I know its a may sound a bit silly but a tiny test like:

package osbuild import ( "encoding/json" "testing" "github.com/stretchr/testify/assert" ) var expectedJSON = `{ "type": "org.osbuild.mkswap", "options": { "uuid": "8a1fc521-02a0-4917-92a9-90a44d7e6503", "label": "some-label" }, "devices": { "root": { "type": "org.osbuild.loopback" } } }` func TestNewMkswapStage(t *testing.T) { devices := make(map[string]Device) devices["root"] = Device{ Type: "org.osbuild.loopback", } options := MkswapStageOptions{ UUID: "8a1fc521-02a0-4917-92a9-90a44d7e6503", Label: "some-label", } stage := NewMkswapStage(&options, devices) b, err := json.MarshalIndent(stage, "", " ") assert.NoError(t, err) assert.Equal(t, expectedJSON, string(b)) }

(also in the tests-as-documentation sense)

mvo5 · 2024-11-28T10:22:06Z

pkg/disk/partition_table_test.go

@@ -1259,6 +1259,105 @@ func TestNewCustomPartitionTable(t *testing.T) {
 				},
 			},
 		},
+		"plain+swap": {


mvo5 · 2024-11-28T10:44:11Z

pkg/osbuild/mkfs_stage.go

-				UUID:  fsSpec.UUID,
-				Label: fsSpec.Label,
+			stages = append(stages, NewMkfsBtrfsStage(options, stageDevices))
+			// Handle subvolumes here directly instead of collecting them in


Maybe add a line about the /why/ here as well? I assume just because of simplify? Or is there a deeper reason? Maybe // Handle subvolumes directly here instead of in their own case because we can reuse the parent easier this way (I totally made this up :)

mvo5 · 2024-11-28T11:05:56Z

pkg/osbuild/mkfs_stage.go

+	genStage := func(ent disk.Entity, path []disk.Entity) error {
+		switch e := ent.(type) {
+		case *disk.Filesystem:
+			// TODO: extract last device renaming into helper


Fwiw, I did a quick experiemnt on this in fa11558 - I would love to see this slightly cleaner (the linked patch is still not nice but avoids some of the repetition and is hopefully a starting point)

I left this as a TODO because I didn't want to add extra things and it could use some investigation. Sometimes we need to rename the leaf device and other times we don't, but maybe renaming it always is a good general approach and we can just add it tot he existing function. Alternatively we can have a "rename last device" function. Another alternative is to add an argument to the function to rename the last device to whatever the caller specifies.

I don't know which is cleaner, so I'd rather look into it separately :)

Yeah, total in a followup (or prereq - not here). Sorry if that wasn't clear!

A new interface FSTabEntity which describes entities that can appear in the /etc/fstab file. This new interface replaces most of the Mountable interface, in which it is now embedded. The new interface will be useful to separate entries that cannot be mounted but do appear in the fstab file (namely, swap areas and swap files). All current Mountables (Filesystem and BtrfsSubvolume) implement GetFSFile() now as an alias to GetMountpoint().

This function is the same idea as ForEachMountable() but for the new FSTabEntity interface. The new function is used in the NewFSTabStageOptions() now which is more appropriate. The distinction will be useful with the addition of swap areas that should appear in /etc/fstab (and therefore, the org.osbuild.fstab stage options) but should not be considered for the mkfs stage generation.

New PayloadEntity for Swap partitions. It resembles a Filesystem, but with fewer fields and fixed values for the fstab entries (fs_file, type, freq, and passno).

Add the SwapPartitionGUID for GPT and DosSwapID for DOS. Register both in the idMap. Refs: - https://www.freedesktop.org/wiki/Specifications/DiscoverablePartitionsSpec/ - https://tldp.org/HOWTO/Partition-Mass-Storage-Definitions-Naming-HOWTO/x190.html

Add a new partition table to the TestPartitionTables that includes a swap partition. These partition tables are used in multiple tests in disk_test.go.

Move the TestPartitionTables from the disk tests to the internal/testdisk package. This makes the test partition tables reusable across the project, but also makes it impossible to use them in internal disk tests (import cycle). The PartitionTableFeatures test is therefore moved to the disk_test package instead and the function is exported for testing in the pkg/disk/export_test.go file.

Test the stage option generator for the org.osbuild.fstab stage using the TestPartitionTables. Note that the btrfs subvolumes were missing a name, making the stage option generation fail, so this is also fixed.

Replace the GenMkfsStages() function with a more general GenFsStages(). This new function iterates through all entities, not just mountables, and handles, in addition to filesystem creation (org.osbuild.mkfs.*), btrfs subvolume creation (org.osbuild.btrfs.subvol) and swap (org.osbuild.mkswap). This removes the need for calling the separate GenBtrfsSubVolStage() when generating image pipelines. This change has a minor effect on the org.osbuild.btrfs.subvol stage options in manifests: it changes the name of the device (which is not a functional change) and locks the loopback device (which is not necessary but shouldn't cause issues). Tests have been updated accordingly.

Support setting the fileystem type for a FilesystemTypedCustomization to "swap". When this is set, the Mountpoint should be empty. This makes it possible to create swap areas on either plain partitions or on logical volumes.

When creating plain partitions of logical volumes, if the fstype is "swap", create a Swap payload instead of a Filesystem. Added a new test case for plain with swap and included swap in the btrfs case and a swap logical volume in the lvm case.

mvo5

Thank you!

ondrejbudai

This is pure awesomeness, thank you! ❤️

This update includes support for swap partitions and logical volumes. See: osbuild/images#1072

This commit adds an integration test for the new swap disk customization that got added to the `images` library in PR osbuild/images#1072

This commit adds an integration test for the new swap disk customization that got added to the `images` library in PR osbuild/images#1072 Also extends the LVM test to include swap on lvm.

achilleas-k requested review from ondrejbudai, mvo5 and thozza November 27, 2024 18:12

achilleas-k force-pushed the disk/swap branch from f96c80d to d9448fe Compare November 27, 2024 19:32

achilleas-k changed the title ~~Support swap partitions and logical volumes in DiskCustomization~~ Support swap partitions and logical volumes in DiskCustomization (COMPOSER-2400) Nov 27, 2024

thozza reviewed Nov 28, 2024

View reviewed changes

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

pkg/osbuild/mkfs_stages_test.go Outdated Show resolved Hide resolved

mvo5 previously approved these changes Nov 28, 2024

View reviewed changes

mvo5 reviewed Nov 28, 2024

View reviewed changes

achilleas-k and others added 17 commits November 28, 2024 14:05

disk: new PayloadEntity: Swap

8bbbf4a

New PayloadEntity for Swap partitions. It resembles a Filesystem, but with fewer fields and fixed values for the fstab entries (fs_file, type, freq, and passno).

disk: add Swap to partitionTableFeatures

f649085

disk: add a test partition table with swap

650e0bc

Add a new partition table to the TestPartitionTables that includes a swap partition. These partition tables are used in multiple tests in disk_test.go.

osbuild: test NewFSTabStageOptions function

96dfc58

Test the stage option generator for the org.osbuild.fstab stage using the TestPartitionTables. Note that the btrfs subvolumes were missing a name, making the stage option generation fail, so this is also fixed.

osbuild: add org.osbuild.mkswap stage

882d135

osbuild: add LVM test for GenFsStages

74ef416

osbuild: add swap to all GenFSStages() tests

3651b92

test: add swap to all the partitioning test configs

1e78f3d

disk: test ForEachFSTabEntity()

c1b9afe

osbuild: add test for NewMkswapStage()

de9034d

achilleas-k dismissed mvo5’s stale review via de9034d November 28, 2024 14:19

achilleas-k force-pushed the disk/swap branch from d9448fe to de9034d Compare November 28, 2024 14:19

This was referenced Nov 28, 2024

disk: cover all entities in PartitionTable.features() #1074

Merged

disk: test ForEachMountable() #1075

Merged

mvo5 approved these changes Nov 28, 2024

View reviewed changes

ondrejbudai approved these changes Nov 28, 2024

View reviewed changes

ondrejbudai added this pull request to the merge queue Nov 28, 2024

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

achilleas-k added a commit to achilleas-k/bootc-image-builder that referenced this pull request Nov 29, 2024

go.mod: update osbuild/images to v0.103.0

202abb2

This update includes support for swap partitions and logical volumes. See: osbuild/images#1072

achilleas-k mentioned this pull request Nov 29, 2024

go.mod: update osbuild/images to v0.103.0 osbuild/bootc-image-builder#737

Closed

achilleas-k added a commit to achilleas-k/bootc-image-builder that referenced this pull request Dec 3, 2024

go.mod: update osbuild/images to v0.103.0

b2b7c81

This update includes support for swap partitions and logical volumes. See: osbuild/images#1072

achilleas-k deleted the disk/swap branch December 3, 2024 14:19

mvo5 pushed a commit to mvo5/bootc-image-builder that referenced this pull request Dec 3, 2024

go.mod: update osbuild/images to v0.103.0

3857e5e

This update includes support for swap partitions and logical volumes. See: osbuild/images#1072

github-merge-queue bot pushed a commit to osbuild/bootc-image-builder that referenced this pull request Dec 4, 2024

go.mod: update osbuild/images to v0.103.0

ea030f0

This update includes support for swap partitions and logical volumes. See: osbuild/images#1072

github-merge-queue bot pushed a commit to osbuild/bootc-image-builder that referenced this pull request Dec 4, 2024

go.mod: update osbuild/images to v0.103.0

48f65ce

This update includes support for swap partitions and logical volumes. See: osbuild/images#1072

mvo5 mentioned this pull request Dec 5, 2024

test: add integration test for the new swap customization osbuild/bootc-image-builder#745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support swap partitions and logical volumes in DiskCustomization (COMPOSER-2400) #1072

Support swap partitions and logical volumes in DiskCustomization (COMPOSER-2400) #1072

achilleas-k commented Nov 27, 2024

thozza left a comment

mvo5 left a comment

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

achilleas-k Nov 28, 2024

mvo5 Nov 28, 2024

mvo5 left a comment

ondrejbudai left a comment


		func (MkswapStageOptions) isStageOptions() {}

		func NewMkswapStage(options MkswapStageOptions, devices map[string]Device) Stage {

Support swap partitions and logical volumes in DiskCustomization (COMPOSER-2400) #1072

Support swap partitions and logical volumes in DiskCustomization (COMPOSER-2400) #1072

Conversation

achilleas-k commented Nov 27, 2024

thozza left a 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

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

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

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

ondrejbudai left a comment

Choose a reason for hiding this comment