Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(storage): add option to explicitly omit the bootloader installation #2126

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aamkye
Copy link

@aamkye aamkye commented Jan 11, 2025

While playing with Subiquity, I've found quite an extraordinary edge-case, there is basically no way of purposely omitting the bootloader installation (grub). And while the intention of having it in 99/100 cases is solid, it seems like an exception from this behavior could be appreciated by the ZFS community.

Let's take a look at this particular example:

autoinstall:
  version: 1

  early-commands:
    - echo "admin123" > /zfs.key
    - sudo apt-get install -y zfsutils-linux curl efibootmgr

  network:
    network:
      renderer: networkd
      version: 2
      ethernets:
        interface0:
          match:
            name: ens*
          dhcp4: true

  storage:
    version: 2
    swap:
      size: 0
    config:
      # DISK
      - {type: disk, id: d1, ptable: gpt, serial: QEMU_HARDDISK_101,
         wipe: superblock-recursive, grub_device: false, preserve: false}

      # /EFI
      - {type: partition, id: d1p1, number: 1, size: 1G, device: d1, flag: boot,
         wipe: superblock, grub_device: false, partition_type: ef00}
      - {type: format, id: d1p1_format, label: efi, fstype: fat32, volume: d1p1}
      - {type: mount, id: d1p1_mount, device: d1p1_format, path: /efi}

      # /
      - {type: partition, id: d1p2, number: 2, size: -1, device: d1,
         grub_device: false, partition_type: bf00}
      - {type: zpool, id: d1_rpool, pool: rpool, vdevs: [d1p2], mountpoint: none,
         pool_properties: {ashift: 12, version: null, feature@encryption: enabled},
         fs_properties: {acltype: posixacl, relatime: on, canmount: on,
                         compression: lz4, devices: off, xattr: sa, encryption: on,
                         keylocation: file:///zfs.key, keyformat: passphrase}}

      - {type: zfs, id: d1_rpool_data, pool: d1_rpool, volume: data,
         properties: {mountpoint: /, canmount: on}}

  late-commands:
    ### ZFS Boot Menu ###
    - sudo mkdir -p /target/efi/EFI/ZBM
    - sudo curl -o /target/efi/EFI/ZBM/VMLINUZ.EFI -L https://get.zfsbootmenu.org/efi
    - sudo cp /target/efi/EFI/ZBM/VMLINUZ.EFI /target/efi/EFI/ZBM/VMLINUZ-BACKUP.EFI
    - >-
      sudo efibootmgr -c -d "/dev/sda" -p "1" \
        -L "ZFSBootMenu (Backup)" \
        -l '\EFI\ZBM\VMLINUZ-BACKUP.EFI'
    - >-
      sudo efibootmgr -c -d "/dev/sda" -p "1" \
        -L "ZFSBootMenu" \
        -l '\EFI\ZBM\VMLINUZ.EFI'
    - zfs set canmount=noauto rpool

  shutdown: reboot

  # (...)

ZFS Boot Manager handles it all, regarding booting the actual system. All partitions and disks have grub_device: false which should be interpreted by Curtin as grub.install_devices: []. The only missing part is the logic in this statement - which blocks the installation due to the missing bootloader. And having in mind that the grub part is not needed in this case, it should be a possible to skip the grub installation part.

The skip should be possible by enlisting all disks with grub_device: false and having the following logic implemented:

# subiquity/models/filesystem.py:2363
if (len(self._all(type="disk")) > 0 and len(self._all(type="disk")) == len(self._all(type="disk", grub_device=False))):
  return False

@dbungert
Copy link
Collaborator

Hi @aamkye , thanks for the PR.

I have to admit to being a bit torn about this one.

On one hand it's interesting that we're able to have an alternate boot system with so small of changes, and I want to be able to enable expert users to run this sort of experiment.

On the other hand, it creates a foot-gun for autoinstall users who are intending to install grub but not quite configuring the storage config in the needed manner. It is a deliberate feature for Subiquity that we don't allow the install to proceed unless we see a path for how the target system is going to be able to boot, and this breaks that feature. The currently failing unit tests were written with that expectation in mind.

By coincidence there is some ongoing work in curtin to relax the assumption of grub.

Let me think on this one a bit. Again, I want to enable this feature you have in mind, but want to do so without making it seem like someone's manual partitioning config is valid when it isn't.

@aamkye
Copy link
Author

aamkye commented Jan 14, 2025

@dbungert, thanks for an additional perspective, it's always valuable to look at the problem and solutions from different angles.

Playing with arguments that have default values, and making them explicit False will not work in this case. IDK why, but I've assumed that this field could be “not specified / not set”, thanks for pinpointing that from the test perspective.

However, I've tried to align to what you have said, and maybe adding an extra parameter to the disk could be accepted:

autoinstall:
  storage:
    config:
      - {type: disk, id: d1, skip_bootloader=true}

or on the other hand, the EXPERIMENTAL block in the storage section:

autoinstall:
  storage:
    # During tests
    experimental:
      skip_bootloader: true

    # After tests
    skip_bootloader: true

    config:
      - {type: disk, id: d1}

Please let me know what do you think about it.

@dbungert
Copy link
Collaborator

or on the other hand, the EXPERIMENTAL block

I like your experimental section proposal, it has a feature-flag like feel to it and contains the changes to autoinstall to this area. I'd like to amend it slightly to not have the experimental section under storage - we may choose to have more experimental features in the future, so I think this needs to be a higher level declaration.

autoinstall:
  experimental:
    skip_bootloader: true
  storage:
    config:
      - {type: disk, id: d1}

code along these lines should do it

experimental = self.app.autoinstall_config.get("experimental")
skip_bootloader = experimental.get("skip_bootloader")

I don't however want to offer outside of experimental a skip_bootloader, rather it should take some form of making it supported. Let's say we wanted to offer ZFS boot menu properly, as a supported feature. We won't have that go through a skip_bootloader setting, we'd have that through some mechanism here yet to be designed for autoinstall, along with matching logic in curtin providing the equivalent of your late-commands. So I see this experimental: {skip_bootloader: true} as a stopgap prior to that being fully implemented, allowing testing.

Normally when autoinstall changes are done I request documentation changes, however I specifically don't want documentation on anything under experimental.

@aamkye
Copy link
Author

aamkye commented Jan 23, 2025

Okay, some action plan would be appreciated.

But let's summarize (from my POV):

  • We have to wait for “curtin relaxing on assumption of grub” anyway.
  • After that, the decision if the approach would be “skip the bootloader[1]” or “support of ZBM[2]” has to be made.
  • After the approach is settled, we can start the actual development.
    • In approach [1] we would skip the docs. But implement test.
    • In approach [2] we must prepare docs, tests, and actual support for ZBM.
  • After the development is done, docs are approved, and tests pass, this change would be a part of the earliest (from the future perspective) ubuntu-server release publicly available.

Is that correct @dbungert?

From my side, I could offer to make necessary changes in Subiquity. However, some documentation related to building custom images from scratch with custom Subiquity Snap might be needed.

@dbungert
Copy link
Collaborator

dbungert commented Feb 3, 2025

@aamkye While I've mentioned potential curtin changes, I'm not asking you to do those curtin changes. I'm only trying to imagine what an eventual non-experimental integration might look like, and leave room for that.

So a step 1 for now just relaxing the bootable check if opted in by an autoinstall flag is fine and allows that experimentation.

We have to wait for “curtin relaxing on assumption of grub” anyway.

Nothing needs to be waited on here.

After the development is done, docs are approved, and tests pass, this change would be a part of the earliest (from the future perspective) ubuntu-server release publicly available.

We're looking at main branch, so this would appear in Ubuntu 25.04, and earlier releases can opt-in to that branch for testing purposes.

However, some documentation related to building custom images from scratch with custom Subiquity Snap might be needed

It is a bit of a challenge to actually test changes in an ISO, but we do have some tools that can help. The easiest to get going is scripts/quick-test-this-branch.sh which takes 2 args, an existing ISO (use one from here https://cdimage.ubuntu.com/ubuntu-server/daily-live/) and an output ISO file name. You can then boot that output ISO in your VM of choice, which will contain a "rebuilt" snap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants