disk: Add support for raw partitions, custom types and UKIBoot partition layout

[martinezjavier]

This pull-request adds the capability to create raw (unformatted) partitions, for custom boot setups that require partitions with specific GPT types but without a filesystem (e.g: ukiboot).

It also adds the specific GPT type UUIDs for UKIBoot (UKIBootPartitionUUID) and the UKIBoot control (UKIBootCtlPartitionUUID) partition types. And the logic to properly map these types with the UKIBoot partitions, based on their labels.

Having this support will allow bootc-image-builder to embedded customization files to generate images that can be booted using UKIBoot.

[thozza]

Thank you for your contribution.

Unfortunately, I'm not a fan of this approach and the changes done to addPlainPartition(). My suggestion would be to allow users to define this scenario by explicit options in the FS customizations, instead of having a special case logic if specific options are not set at all (since it could be just a user error, which would now get caught by this special case).

I would also like you to extend unit tests to cover cases such as no partition label being set, etc...

[mvo5]

Thanks, this looks nice, two tiny (optional) suggestions and a question inline. We also want @achilleas-k review here as he is the expert in this area :)

[martinezjavier]

@thozza @mvo5 I've updated my branch with changes to address your feedback and suggestions.

The changes for v2 are the following:

• Introduces an explicit FSType: "none" to make the intent clear when a plain partition should be left unformatted instead of doing it implicitly when fs_type and mountpoint were not defined.
• Refactor addPlainPartition() to remove the if/else logic and handle the new "none" type in a switch statement.
• Add a check in validatePlain() to return an error if a "none" partition has a mountpoint specified.
• Modify GenUUID() to skip UUID generation for the "none" type (since there's no filesystem in the partition, the filesystem UUID should not be set).
• Add and update unit tests to cover the new type and its constraints.
• Fix the formatting issues reported by ./tools/prepare-source.sh

[achilleas-k]

Thanks. LGTM. One very small comment, but not a blocker. Feel free to ignore.

[achilleas-k]

Tiniest of nitpicks:

I'd prefer if the ukiboot_a and ukiboot_b labels mapping to the ukiboot type happened here instead of in the getPartitionTypeIDfor() function. It's not very important, but I think of the types that get passed to getPartitionTypeIDfor() to be common names for the types of partitions we handle. In fact, I was considering changing that to an enum with type names matching the names as they appear in fdisk. So with that logic, the getPartitionTypeIDfor() function would only know those types and it's up to the caller to make the decision based on mountpoint or label.

So this would then be:

case fstype == "none":
    switch partition.Label {
    case "ukiboot_a", "ukiboot_b":
        typeName = "ukiboot"
    case "ukibootctl":
        typeName = "ukibootctl"
    default:
        typeName = "data"
    }

[martinezjavier]

@achilleas-k I've pushed a v3 addressing this comment. The full diff between v2 and v3 is the following:

diff --git a/pkg/disk/disk.go b/pkg/disk/disk.go
index 4c4b206ba99b..7e8d2d498af2 100644
--- a/pkg/disk/disk.go
+++ b/pkg/disk/disk.go
@@ -175,7 +175,7 @@ func getPartitionTypeIDfor(ptType PartitionTableType, partTypeName string, archi
                        default:
                                return "", fmt.Errorf("unknown or unsupported architecture enum value: %d", architecture)
                        }
-               case "ukiboot_a", "ukiboot_b":
+               case "ukiboot":
                        return UKIBootPartitionUUID, nil
                case "ukibootctl":
                        return UKIBootCtlPartitionUUID, nil
diff --git a/pkg/disk/partition_table.go b/pkg/disk/partition_table.go
index dce7382434bf..92be355c21bd 100644
--- a/pkg/disk/partition_table.go
+++ b/pkg/disk/partition_table.go
@@ -1428,9 +1428,12 @@ func addPlainPartition(pt *PartitionTable, partition blueprint.PartitionCustomiz
                case partition.Mountpoint == "/boot":
                        typeName = "boot"
                case fstype == "none":
-                       if partition.Label != "" {
-                               typeName = partition.Label
-                       } else {
+                       switch partition.Label {
+                       case "ukiboot_a", "ukiboot_b":
+                               typeName = "ukiboot"
+                       case "ukibootctl":
+                               typeName = "ukibootctl"
+                       default:
                                typeName = "data"
                        }
                case fstype == "swap":

[achilleas-k]

Thanks. Your diff here looks good, but I don't see the change in the PR diff.

[martinezjavier]

@achilleas-k yeah, because since that change @mvo5 and @thozza had more comments and at the end the mapping was gone :)

[mvo5]

Thanks, lots to like here - but I do wonder if we really need to add knowledge about the uki boot here or if adding "fstype" none is enough. I mean, it might be fine, how widely used is it?

[thozza]

Thank you for all the changes, this looks much better now and almost perfect (please see my inline comment).

[mvo5]

Thanks! Two tiny question/suggestion inline, if you are busy we can take care of it. This may need a decision by e.g. @achilleas-k if we allow payload to be nil or add a new disk.Raw{} structure

[mvo5]

I love how simple this has become, one tiny suggestion about a comment but approving already as I think that its optional (but would be nice I think)

[thozza]

Nice, LGTM. I appreciate your patience.

[alexlarsson]

Seems like these failures are some random registry issues. Can we re-run them?

[supakeen]

Seems like these failures are some random registry issues. Can we re-run them?

Sadly no, Fedora's infra is having a bunch of issues at the moment. I'm asking around if we can make (some of) these checks non-required while this is going on.

[achilleas-k]

Love how this turned out. Thanks for being so patient with our endless reviews.

LGTM!

[martinezjavier]

Love how this turned out. Thanks for being so patient with our endless reviews.

LGTM!

@achilleas-k @thozza @mvo5 on the contrary, thanks a lot folks for all your feedback and comments!