Make EFI partition size configurable at install time #2105

davidcassany · 2024-06-17T10:55:57Z

davidcassany · 2024-06-17T10:59:43Z

In addition to that we could also consider having way higher default value (what about 1G or even 2G?) So if we ever support systemd-boot or unified kernel image (which require having kernel and initrd within the EFI partition) we could eventually consider upgrades without having to reinstall from scratch.

anmazzotti

Looks good, only one minor remark to provide an example.

anmazzotti · 2024-06-17T12:31:30Z

config.yaml.example

@@ -19,7 +19,7 @@ install:
  #     size: 300

  # default partitions
-  # only 'oem', 'recovery', 'state' and 'persistent' objects allowed
+  # only 'efi', 'oem', 'recovery', 'state' and 'persistent' objects allowed


Just nitpicking, but it would be best to also include an efi partition example, just to highlight the fact that the filesystem to use should be vfat. Or can it be omitted even? I don't truly know if we use the defaults or not.

vfat filesystem is set by default and I was wondering to force it or not (error out if not set to vfat), but then I thought about better giving some flexibility as this is pretty hidden in the setup and if one is actually touching this is probably for a good reason. For instance I don't know if for RPi it is mandatory to boot from vfat or it can be another filesystem.
What we probably could consider in the future is renaming this EFI partition to some firmware neutral name (boot partition?), so there is no explicit assumption we are using EFI firmware. But I did not want to touch that at this time because it feels like a risky change, there might be some corner cases hard to foresee.

I would not force it as well, but it could be an idea. What I missed is an example for the end-users to follow, let's say I do want to use this feature, then I would miss (without looking at the source code) how can I extend the partition size, and what do I need to configure in order to do that.
I was wondering is that fat, fat32, etc. before looking at the source, and if it's optional is even better, so I could just code:

efi: size: 1000

Indeed this is the case you can provide only size and this will only modify the default size, but keep all other parameters. About partition sizes we definitely need some docs on that topic and we already have an issue for that rancher/elemental-docs#345

or instance I don't know if for RPi it is mandatory to boot from vfat or it can be another filesystem.

It is mandatory. It cannot be another filesystem.
(At least for the firmware and bootloader files. Once GRUB takes over, things probably look a bit brighter.)

kkaempf · 2024-06-17T13:51:08Z

As said in #2104, calling it EFI partition is misleading on Raspberry (and might as well other ARM platforms).
Raspberry doesn't boot from EFI but also needs a vfat formatted boot partition.

It might not matter implementation wise, but I'm somewhat concerned about mixing topics.

davidcassany · 2024-06-17T15:50:25Z

As said in #2104, calling it EFI partition is misleading on Raspberry (and might as well other ARM platforms). Raspberry doesn't boot from EFI but also needs a vfat formatted boot partition.

It might not matter implementation wise, but I'm somewhat concerned about mixing topics.

All right, this is a fair point and probably we can start by exposing it with a different name but keep all variables and constants as they are now, which is probably already misleading. Lets start having a look.

davidcassany · 2024-06-17T16:12:11Z

I also dislike the boot name as this can easily be confusing with /boot 🤔 Shall we call it firmware partition? Something like:

install:
  partitions:
    firmware:
      size: 512
    oem:
      ...

We could easily name EFI to firmware in UX and probably change the EFI variable name to FW or something like that, however there still many constant literals that refer to efi (e.g. the partition is mounted at /run/elemental/efi).

Note that in toolkit we are only effectively supporting EFI firmware. So to some extend it makes sense keeping the default constant literals referring to efi (e.g. /run/elemental/efi).

Shall we try to do such a rename? How far should we attempt to go.

kkaempf · 2024-06-18T07:14:23Z

firmware isn't clear either, as there's "boot firmware" (like in RPi) and "kernel firmware" (all the *-firmware rpms).

Yeah, naming is hard 😆

davidcassany · 2024-06-18T07:42:52Z

Probably bootloader partition is more accurate? Could be meaningful for BIOS, EFI and UBOOT-like with EFI Grub

kkaempf · 2024-06-18T09:47:38Z

Probably bootloader partition is more accurate? Could be meaningful for BIOS, EFI and UBOOT-like with EFI Grub

Let's go for bootloader !

kkaempf · 2024-06-18T09:48:36Z

cmd/config/fixtures/config/config.yaml

@@ -4,6 +4,9 @@ cloud-init-paths:
 - "some/alternate/path"

 install:
+  partitions:
+    efi:
+      size: 512


How/where do we document the size unit ? (I'd assume it's in megabytes ;-) )

Yes everything is in megabytes, it is already added as a comment in the example config file and also in elemental-toolkit docs.

Signed-off-by: David Cassany <dcassany@suse.com>

davidcassany · 2024-06-18T10:08:09Z

config.yaml.example

@@ -19,7 +19,7 @@ install:
  #     size: 300

  # default partitions
-  # only 'oem', 'recovery', 'state' and 'persistent' objects allowed
+  # only 'bootloader', 'oem', 'recovery', 'state' and 'persistent' objects allowed
  # size in MiB


Here is were we document all sizes are in megabytes. It is also documented in elemental-toolkit docs.

davidcassany · 2024-06-18T10:08:53Z

cmd/config/fixtures/config/config.yaml

@@ -4,6 +4,9 @@ cloud-init-paths:
 - "some/alternate/path"

 install:
+  partitions:
+    efi:
+      size: 512


Yes everything is in megabytes, it is already added as a comment in the example config file and also in elemental-toolkit docs.

davidcassany · 2024-06-18T10:10:55Z

pkg/constants/constants.go

-	EfiLabel           = "COS_GRUB"
-	EfiPartName        = "efi"
+	BootLabel          = "COS_GRUB"
+	BootPartName       = "efi"


Note this PR changes the variable and constant names, but none of the values. This to ensure no regressions are introduced. If we consider changes values to I'd do it in a separate and follow up PR.

davidcassany · 2024-06-18T10:11:48Z

pkg/types/config.go

@@ -502,7 +502,7 @@ func (pl PartitionList) GetByNameOrLabel(name, label string) *Partition {

 type ElementalPartitions struct {
 	BIOS       *Partition
-	EFI        *Partition
+	Boot       *Partition `yaml:"bootloader,omitempty" mapstructure:"bootloader"`


bootloader partition is named as Boot in the struct, just for the sake of short names.

kkaempf

Ok to merge 🚀

kkaempf · 2024-06-19T07:55:56Z

I cannot add

partitions:
  bootloader:
    size: 128

via the Rancher UI 😕

kkaempf · 2024-06-19T09:31:53Z

When trying to edit via kubectl I get

> kubectl edit -n fleet-default MachineRegistration rpi-cluster-nodes
error: machineregistrations.elemental.cattle.io "rpi-cluster-nodes" is invalid
A copy of your changes has been stored to "/tmp/kubectl-edit-rw5pf.yaml"
error: Edit cancelled, no valid changes were saved.

kkaempf · 2024-06-21T13:02:15Z

Confirmed working, thanks !

davidcassany requested a review from a team as a code owner June 17, 2024 10:55

davidcassany force-pushed the make_efi_partition_size_configurable branch from 86f6bc3 to 07d4633 Compare June 17, 2024 11:10

davidcassany self-assigned this Jun 17, 2024

anmazzotti approved these changes Jun 17, 2024

View reviewed changes

davidcassany marked this pull request as draft June 17, 2024 15:50

kkaempf reviewed Jun 18, 2024

View reviewed changes

davidcassany added 5 commits June 18, 2024 12:05

Make EFI partition size configurable at install time

5504179

Signed-off-by: David Cassany <dcassany@suse.com>

Add a custom EFI size test

7b3f0ac

Signed-off-by: David Cassany <dcassany@suse.com>

Rename efi to bootloader partition in config.yaml

48e7e54

Signed-off-by: David Cassany <dcassany@suse.com>

Rename EFI variable to Boot

be377aa

Signed-off-by: David Cassany <dcassany@suse.com>

Rename constants

4f6e8c1

Signed-off-by: David Cassany <dcassany@suse.com>

davidcassany force-pushed the make_efi_partition_size_configurable branch from 43b37d3 to 4f6e8c1 Compare June 18, 2024 10:06

davidcassany commented Jun 18, 2024

View reviewed changes

davidcassany marked this pull request as ready for review June 18, 2024 10:09

davidcassany commented Jun 18, 2024

View reviewed changes

kkaempf approved these changes Jun 18, 2024

View reviewed changes

davidcassany merged commit 15a10cc into rancher:main Jun 19, 2024
29 checks passed

davidcassany deleted the make_efi_partition_size_configurable branch June 19, 2024 04:23

kkaempf mentioned this pull request Jun 19, 2024

COS_GRUB too small on ARM / RPi #2104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make EFI partition size configurable at install time #2105

Make EFI partition size configurable at install time #2105

davidcassany commented Jun 17, 2024

davidcassany commented Jun 17, 2024

anmazzotti left a comment

anmazzotti Jun 17, 2024

davidcassany Jun 17, 2024

anmazzotti Jun 17, 2024

davidcassany Jun 17, 2024

kkaempf Jun 17, 2024

kkaempf commented Jun 17, 2024

davidcassany commented Jun 17, 2024

davidcassany commented Jun 17, 2024

kkaempf commented Jun 18, 2024

davidcassany commented Jun 18, 2024

kkaempf commented Jun 18, 2024

kkaempf Jun 18, 2024

davidcassany Jun 18, 2024

davidcassany Jun 18, 2024

davidcassany Jun 18, 2024

davidcassany Jun 18, 2024

davidcassany Jun 18, 2024

kkaempf left a comment

kkaempf commented Jun 19, 2024

kkaempf commented Jun 19, 2024

kkaempf commented Jun 21, 2024

Make EFI partition size configurable at install time #2105

Make EFI partition size configurable at install time #2105

Conversation

davidcassany commented Jun 17, 2024

davidcassany commented Jun 17, 2024

anmazzotti 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

kkaempf commented Jun 17, 2024

davidcassany commented Jun 17, 2024

davidcassany commented Jun 17, 2024

kkaempf commented Jun 18, 2024

davidcassany commented Jun 18, 2024

kkaempf commented Jun 18, 2024

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

kkaempf left a comment

Choose a reason for hiding this comment

kkaempf commented Jun 19, 2024

kkaempf commented Jun 19, 2024

kkaempf commented Jun 21, 2024