Repository: osbuild/osbuild-composer
Pull request #2373 (2022-02-28 18:04:17)

rhel86: automatically convert to LVM on fs customizations
Whenever we create a new mountpoint due to a user customization, ensure the layout uses LVM, i.e. convert plain layouts to it, if needed. It uses the existing lvm-ification code but enhances it so that we also create a `/boot` partition in case it does not yet exist.
Adjust the existing tests that assumed we can not create more than 4 partitions on mbr layouts, since that is now not true anymore.



gicmo: > Looks good. Should we add a small unit test that checks for a boot partition on the PT when it's lvmified?

good idea, maybe also specify min size for the root partition

Patch:
diff --git a/internal/disk/partition_table.go b/internal/disk/partition_table.go
index aa1385f1f..339d608c3 100644
--- a/internal/disk/partition_table.go
+++ b/internal/disk/partition_table.go
@@ -26,7 +26,10 @@ func NewPartitionTable(basePT *PartitionTable, mountpoints []blueprint.Filesyste
 			resizeEntityBranch(path, size)
 		} else {
 			if lvmify {
-				newPT.ensureLVM()
+				err := newPT.ensureLVM()
+				if err != nil {
+					return nil, err
+				}
 			}
 			if err := newPT.createFilesystem(mnt.Mountpoint, size); err != nil {
 				return nil, err
@@ -458,17 +461,24 @@ func (pt *PartitionTable) GenUUID(rng *rand.Rand) {
 
 // ensureLVM will ensure that the root partition is on an LVM volume, i.e. if
 // it currently is not, it will wrap it in one
-func (pt *PartitionTable) ensureLVM() {
+func (pt *PartitionTable) ensureLVM() error {
 
 	rootPath := entityPath(pt, "/")
 	if rootPath == nil {
 		panic("no root mountpoint for PartitionTable")
 	}
 
+	// we need a /boot partition to boot LVM, ensure one exists
+	bootPath := entityPath(pt, "/boot")
+	if bootPath == nil {
+		_, err := pt.CreateMountpoint("/boot", 512*1024*1024)
+		return err
+	}
+
 	parent := rootPath[1] // NB: entityPath has reversed order
 
 	if _, ok := parent.(*LVMLogicalVolume); ok {
-		return
+		return nil
 	} else if part, ok := parent.(*Partition); ok {
 		filesystem := part.Payload
 
@@ -496,4 +506,6 @@ func (pt *PartitionTable) ensureLVM() {
 	} else {
 		panic("unsupported parent for LVM")
 	}
+
+	return nil
 }
diff --git a/internal/distro/rhel86/distro.go b/internal/distro/rhel86/distro.go
index 82c9e30ee..2271e349f 100644
--- a/internal/distro/rhel86/distro.go
+++ b/internal/distro/rhel86/distro.go
@@ -331,6 +331,24 @@ func (t *imageType) PackageSets(bp blueprint.Blueprint) map[string]rpmmd.Package
 		bpPackages = append(bpPackages, "chrony")
 	}
 
+	// if we have file system customization that will need to a new mount point
+	// the layout is converted to LVM so we need to corresponding packages
+	if !t.rpmOstree {
+		archName := t.arch.Name()
+		pt := t.basePartitionTables[archName]
+		haveNewMountpoint := false
+
+		if fs := bp.Customizations.GetFilesystems(); fs != nil {
+			for i := 0; !haveNewMountpoint && i < len(fs); i++ {
+				haveNewMountpoint = !pt.ContainsMountpoint(fs[i].Mountpoint)
+			}
+		}
+
+		if haveNewMountpoint {
+			bpPackages = append(bpPackages, "lvm2")
+		}
+	}
+
 	// depsolve bp packages separately
 	// bp packages aren't restricted by exclude lists
 	mergedSets[blueprintPkgsKey] = rpmmd.PackageSet{Include: bpPackages}
@@ -394,7 +412,9 @@ func (t *imageType) getPartitionTable(
 
 	imageSize := t.Size(options.Size)
 
-	return disk.NewPartitionTable(&basePartitionTable, mountpoints, imageSize, false, rng)
+	lvmify := !t.rpmOstree
+
+	return disk.NewPartitionTable(&basePartitionTable, mountpoints, imageSize, lvmify, rng)
 }
 
 func (t *imageType) getDefaultImageConfig() *distro.ImageConfig {
diff --git a/internal/distro/rhel86/partition_tables.go b/internal/distro/rhel86/partition_tables.go
index b13d7e90f..7fc2e2518 100644
--- a/internal/distro/rhel86/partition_tables.go
+++ b/internal/distro/rhel86/partition_tables.go
@@ -30,6 +30,7 @@ var defaultBasePartitionTables = distro.BasePartitionTableMap{
 				},
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Type: disk.FilesystemDataGUID,
 				UUID: disk.RootPartitionUUID,
 				Payload: &disk.Filesystem{
@@ -61,6 +62,7 @@ var defaultBasePartitionTables = distro.BasePartitionTableMap{
 				},
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Type: disk.FilesystemDataGUID,
 				UUID: disk.RootPartitionUUID,
 				Payload: &disk.Filesystem{
@@ -84,6 +86,7 @@ var defaultBasePartitionTables = distro.BasePartitionTableMap{
 				Bootable: true,
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Payload: &disk.Filesystem{
 					Type:         "xfs",
 					Mountpoint:   "/",
@@ -99,6 +102,7 @@ var defaultBasePartitionTables = distro.BasePartitionTableMap{
 		Type: "dos",
 		Partitions: []disk.Partition{
 			{
+				Size:     2 * 1024 * 1024 * 1024, // 2 GiB
 				Bootable: true,
 				Payload: &disk.Filesystem{
 					Type:         "xfs",
@@ -124,6 +128,7 @@ var ec2BasePartitionTables = distro.BasePartitionTableMap{
 				UUID:     disk.BIOSBootPartitionUUID,
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Type: disk.FilesystemDataGUID,
 				UUID: disk.RootPartitionUUID,
 				Payload: &disk.Filesystem{
@@ -167,6 +172,7 @@ var ec2BasePartitionTables = distro.BasePartitionTableMap{
 				},
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Type: disk.FilesystemDataGUID,
 				UUID: disk.RootPartitionUUID,
 				Payload: &disk.Filesystem{
@@ -221,6 +227,7 @@ var edgeBasePartitionTables = distro.BasePartitionTableMap{
 				},
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Type: disk.FilesystemDataGUID,
 				UUID: disk.RootPartitionUUID,
 				Payload: &disk.Filesystem{
@@ -266,6 +273,7 @@ var edgeBasePartitionTables = distro.BasePartitionTableMap{
 				},
 			},
 			{
+				Size: 2 * 1024 * 1024 * 1024, // 2 GiB
 				Type: disk.FilesystemDataGUID,
 				UUID: disk.RootPartitionUUID,
 				Payload: &disk.Filesystem{


Test patch:
diff --git a/internal/disk/disk_test.go b/internal/disk/disk_test.go
index bd3a9b548..47f1f53ca 100644
--- a/internal/disk/disk_test.go
+++ b/internal/disk/disk_test.go
@@ -124,6 +124,45 @@ var testPartitionTables = map[string]PartitionTable{
 		},
 	},
 
+	"plain-noboot": {
+		UUID: "D209C89E-EA5E-4FBD-B161-B461CCE297E0",
+		Type: "gpt",
+		Partitions: []Partition{
+			{
+				Size:     1048576, // 1MB
+				Bootable: true,
+				Type:     BIOSBootPartitionGUID,
+				UUID:     BIOSBootPartitionUUID,
+			},
+			{
+				Size: 209715200, // 200 MB
+				Type: EFISystemPartitionGUID,
+				UUID: EFISystemPartitionUUID,
+				Payload: &Filesystem{
+					Type:         "vfat",
+					UUID:         EFIFilesystemUUID,
+					Mountpoint:   "/boot/efi",
+					Label:        "EFI-SYSTEM",
+					FSTabOptions: "defaults,uid=0,gid=0,umask=077,shortname=winnt",
+					FSTabFreq:    0,
+					FSTabPassNo:  2,
+				},
+			},
+			{
+				Type: FilesystemDataGUID,
+				UUID: RootPartitionUUID,
+				Payload: &Filesystem{
+					Type:         "xfs",
+					Label:        "root",
+					Mountpoint:   "/",
+					FSTabOptions: "defaults",
+					FSTabFreq:    0,
+					FSTabPassNo:  0,
+				},
+			},
+		},
+	},
+
 	"luks": {
 		UUID: "D209C89E-EA5E-4FBD-B161-B461CCE297E0",
 		Type: "gpt",
@@ -393,6 +432,11 @@ func TestCreatePartitionTableLVMify(t *testing.T) {
 			panic("no root mountpoint for PartitionTable")
 		}
 
+		bootPath := entityPath(mpt, "/boot")
+		if bootPath == nil {
+			panic("no boot mountpoint for PartitionTable")
+		}
+
 		parent := rootPath[1]
 		_, ok := parent.(*LVMLogicalVolume)
 		assert.True(ok, "Partition table '%s': root's parent (%q) is not an LVM logical volume", name, parent)
diff --git a/internal/distro/rhel86/distro_test.go b/internal/distro/rhel86/distro_test.go
index e9a1a45c3..1f0032e3d 100644
--- a/internal/distro/rhel86/distro_test.go
+++ b/internal/distro/rhel86/distro_test.go
@@ -704,13 +704,10 @@ func TestDistro_MountpointsWithArbitraryDepthAllowed(t *testing.T) {
 			imgType, _ := arch.GetImageType(imgTypeName)
 			testPackageSpecSets := distro_test_common.GetTestingPackageSpecSets("kernel", arch.Name(), imgType.PayloadPackageSets())
 			_, err := imgType.Manifest(bp.Customizations, distro.ImageOptions{}, nil, testPackageSpecSets, 0)
-			layout := imgType.PartitionType()
-			if layout == "" || strings.HasPrefix(imgTypeName, "edge-") {
+			if strings.HasPrefix(imgTypeName, "edge-") {
 				continue
-			} else if layout == "gpt" {
-				assert.NoError(t, err)
 			} else {
-				assert.EqualError(t, err, "failed creating volume: maximum number of partitions reached (4)")
+				assert.NoError(t, err)
 			}
 		}
 	}