Add support for the new partitioning customizations to RHEL and CentOS #1077

achilleas-k · 2024-11-29T20:42:53Z

Enabling the new partitioning customizations for RHEL and CentOS.

I also found some duplication in Fedora that I cleaned up.

mvo5

Thank you for adding the new customizations to all image types.

The duplication makes me slightly sad but I guess there is no way with the current design to do it differently, I would still love to explore ideas here. I was thinking about at least suggesting a comment like "// keep in sync with the other distros" but I guess that is implied already so redundant.

mvo5 · 2024-12-02T08:42:27Z

pkg/distro/rhel/imagetype.go

+	if err != nil {
+		return nil, err
+	}
+	if partitioning != nil {


We could be defensive here and double check that customizations.Filesystem and partitioning is never both non-nil and raise an internal error. But maybe overkill.

mvo5 · 2024-12-02T08:49:09Z

pkg/distro/rhel/imagetype.go

+			partitioning.MinSize = imageSize
+		}
+
+		partOptions := &disk.CustomPartitionTableOptions{


I guess the fact that this is duplicated with fedor/imagetype.go is unavoidable currently (and consistent with the rest of the code)(?)

mvo5 · 2024-12-02T08:51:33Z

pkg/distro/rhel/rhel9/options.go

+		return warnings, err
+	}
+
+	if err := partitioning.ValidateLayoutConstraints(); err != nil {


Seeing this here three times makes me wonder if we could consolidate this longer term, might be nice to have a single "validate" helper somewhere so that it's less duplicated lines.

achilleas-k · 2024-12-02T11:10:25Z

The duplication makes me slightly sad but I guess there is no way with the current design to do it differently

If you're referring to the checkOptions() parts, I agree, there's a tonne of duplication there and it's also quite hard to trace. There have been several attempts to make it nicer, but none of them reached completion.
(Curious what you think about one of my more recent attempts from earlier this year: main...achilleas-k:images:config-validation).

The way it's structured currently, it's a bit of a necessary evil because for some (but perhaps not most) customizations, the requirements are different between distributions and major versions. Perhaps extracting the common ones would be enough to get the ball rolling on some of the simplification and to avoid diverging where possible.

achilleas-k · 2024-12-02T11:12:12Z

RHEL 8 aarch64 builds are failing. 🤔

Digging.

achilleas-k · 2024-12-02T13:48:14Z

A comment in @ondrejbudai's #1080 PR got me thinking:

This test is only performed on image types with a partition table, thus it doesn't run on e.g. installers and ostree commits.

Instead of listing all supported customizations for each image type like I tried to do in that linked branch, it might be simpler (and more readable) to have properties and capabilities for each image type, and then customizations would be valid or invalid for a given property. For example "has partition table" is an image type property, which internally could just be a method that checks if the base image type defines a partition table or not. Defining disk or filesystem customizations for any image type that doesn't have a PT is invalid.

This could probably be cleanly extended to all blueprint options: "has package payload" (if false, don't allow packages), "has ostree payload" (requires ostree commit), "can oscap"...

Those payload ones in particular will simplify things a lot. Right now, when we're dealing with ostree types, we first check for imageType.RPMOstree (bool), which is true for all ostree image types. But that's bad and wrong, because edge-commit has a package payload (and therefore installing packages is supported) but edge-ami and edge-installer have an ostree commit payload, so they require an ostree commit and disallow packages.

achilleas-k · 2024-12-02T21:23:37Z

RHEL 8 aarch64 builds are failing. 🤔

Digging.

I managed to reproduce the issue. When building a RHEL 8 image on a Fedora host, it seems the swap partition doesn't get created properly, or at least there's some incompatibility with the RHEL 8 kernel, resulting in:

Unable to find swap-space signature

when trying to enable the swap space.

Building the same image on RHEL 8 works fine.

It's strange though that this only happens on aarch64.

achilleas-k · 2024-12-02T21:31:39Z

Might be time to switch to running each build on the same host as the target. Not a huge change to set up, but might need some changes in the test scripts that were written to run on newer python versions.

OTOH, maybe running stuff on RHEL 8 doesn't make sense. We're not targeting RHEL 8 any more (no more releases), so it might make more sense to test RHEL 8 builds on RHEL 9.

mvo5 · 2024-12-03T11:43:00Z

RHEL 8 aarch64 builds are failing. 🤔
Digging.

I managed to reproduce the issue. When building a RHEL 8 image on a Fedora host, it seems the swap partition doesn't get created properly, or at least there's some incompatibility with the RHEL 8 kernel, resulting in:
Unable to find swap-space signature
when trying to enable the swap space.

Building the same image on RHEL 8 works fine.

It's strange though that this only happens on aarch64.

I suspect the issue is the pagesize on aarch64. Apparently this got changed recently(ish).c.f. https://bugzilla.redhat.com/show_bug.cgi?id=1978730 and https://bugzilla.redhat.com/show_bug.cgi?id=2001569 - I guess we could try to be smart about it in images too and add a "mkswap -p 64k" option if we target arm and rhel < 9.2(?)

achilleas-k · 2024-12-03T13:17:10Z

We decided to not enable support for disk customizations in RHEL 8 and CentOS 8 for now.

mvo5 · 2024-12-03T17:48:25Z

pkg/distro/rhel/imagetype.go

+		partOptions := &disk.CustomPartitionTableOptions{
+			PartitionTableType: basePartitionTable.Type, // PT type is not customizable, it is determined by the base PT for an image type or architecture
+			BootMode:           t.BootMode(),
+			DefaultFSType:      disk.FS_EXT4, // default fs type for Fedora


Hm, this is RHEL here, right? so should this be updated to XFS and comment twekaed?

Copy pastaaaaaa!!

Pass the whole customizations struct so we can then use the Disk customization if it's set.

Read the new Partitioning/Disk customizations in the RHEL partition table generators. Validate the customizations and check that they're not used at the same time as FilesystemCustomization. We are not enabling disk customizations on RHEL 8 currently because of issues with kernel page size differences in some versions on aarch64.

mvo5

Thank you

mvo5 previously approved these changes Dec 2, 2024

View reviewed changes

ondrejbudai mentioned this pull request Dec 3, 2024

Fix boot modes and ESP partitions for RHEL 8 and 9 (RHEL-69628) #1080

Merged

achilleas-k dismissed mvo5’s stale review via 9896461 December 3, 2024 13:16

achilleas-k force-pushed the partitioning/rhel branch from 777a6f2 to 9896461 Compare December 3, 2024 13:16

mvo5 reviewed Dec 3, 2024

View reviewed changes

fedora: read and validate the Partitioning only once

d524c2f

achilleas-k force-pushed the partitioning/rhel branch from 9896461 to e463a57 Compare December 3, 2024 17:53

achilleas-k added 3 commits December 3, 2024 19:11

distro/rhel: pass customizations to GetPartitionTable()

c47e5c9

Pass the whole customizations struct so we can then use the Disk customization if it's set.

test: test custom partitioning on RHEL and CentOS 9 and 10

4e5f0c1

achilleas-k force-pushed the partitioning/rhel branch from e463a57 to 4e5f0c1 Compare December 3, 2024 18:12

mvo5 approved these changes Dec 3, 2024

View reviewed changes

supakeen added this pull request to the merge queue Dec 4, 2024

Merged via the queue into osbuild:main with commit 54e1ff1 Dec 4, 2024
19 checks passed

achilleas-k deleted the partitioning/rhel branch December 5, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the new partitioning customizations to RHEL and CentOS #1077

Add support for the new partitioning customizations to RHEL and CentOS #1077

achilleas-k commented Nov 29, 2024 •

edited

Loading

mvo5 left a comment

mvo5 Dec 2, 2024

mvo5 Dec 2, 2024

mvo5 Dec 2, 2024

achilleas-k Dec 2, 2024

achilleas-k commented Dec 2, 2024

achilleas-k commented Dec 2, 2024

achilleas-k commented Dec 2, 2024 •

edited

Loading

achilleas-k commented Dec 2, 2024

achilleas-k commented Dec 2, 2024

mvo5 commented Dec 3, 2024

achilleas-k commented Dec 3, 2024

mvo5 Dec 3, 2024

achilleas-k Dec 3, 2024

mvo5 left a comment

Add support for the new partitioning customizations to RHEL and CentOS #1077

Add support for the new partitioning customizations to RHEL and CentOS #1077

Conversation

achilleas-k commented Nov 29, 2024 • edited Loading

mvo5 left a comment

Choose a reason for hiding this comment

mvo5 Dec 2, 2024

Choose a reason for hiding this comment

mvo5 Dec 2, 2024

Choose a reason for hiding this comment

mvo5 Dec 2, 2024

Choose a reason for hiding this comment

achilleas-k Dec 2, 2024

Choose a reason for hiding this comment

achilleas-k commented Dec 2, 2024

achilleas-k commented Dec 2, 2024

achilleas-k commented Dec 2, 2024 • edited Loading

achilleas-k commented Dec 2, 2024

achilleas-k commented Dec 2, 2024

mvo5 commented Dec 3, 2024

achilleas-k commented Dec 3, 2024

mvo5 Dec 3, 2024

Choose a reason for hiding this comment

achilleas-k Dec 3, 2024

Choose a reason for hiding this comment

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k commented Nov 29, 2024 •

edited

Loading

achilleas-k commented Dec 2, 2024 •

edited

Loading