From 2670f4857de17c0558015be167c1e168d9524bd8 Mon Sep 17 00:00:00 2001 From: Olivier Gayot Date: Thu, 29 Feb 2024 16:26:20 +0100 Subject: [PATCH] filesystem: accept to place the rootfs on remote storage for NVMe-o-TCP Now that curtin more or less supports NVMe over TCP with the rootfs on remote storage, relax the contraints set by subiquity. We still need the /boot (and /boot/efi) partitions on local storage though. Signed-off-by: Olivier Gayot --- examples/answers/nvme-over-tcp-rootfs.yaml | 58 +++++++++++++++++++++ snapcraft.yaml | 2 +- subiquity/models/filesystem.py | 23 +++++++- subiquity/ui/mount.py | 14 ++--- subiquity/ui/views/filesystem/filesystem.py | 7 +++ subiquity/ui/views/filesystem/partition.py | 12 ++--- 6 files changed, 99 insertions(+), 17 deletions(-) create mode 100644 examples/answers/nvme-over-tcp-rootfs.yaml diff --git a/examples/answers/nvme-over-tcp-rootfs.yaml b/examples/answers/nvme-over-tcp-rootfs.yaml new file mode 100644 index 000000000..c948ad1d8 --- /dev/null +++ b/examples/answers/nvme-over-tcp-rootfs.yaml @@ -0,0 +1,58 @@ +#machine-config: examples/machines/nvme-over-tcp.json +Source: + source: ubuntu-server +Welcome: + lang: en_US +Refresh: + update: no +Keyboard: + layout: us +Network: + accept-default: yes +Proxy: + proxy: "" +Mirror: + mirror: "http://fr.archive.ubuntu.com" +Filesystem: + # 1. Reformat both disks + # 2. Create the root filesystem on disk 1 (which is a remote disk) + # 3. Create the /boot filesystem on disk 0 (which is a local disk) + manual: + - obj: [disk index 0] + action: REFORMAT + - obj: [disk index 1] + action: REFORMAT + - obj: [disk index 1] + action: PARTITION + data: + fstype: ext4 + mount: / + - obj: [disk index 0] + action: PARTITION + data: + fstype: ext4 + mount: /boot + - action: done +Identity: + realname: Ubuntu + username: ubuntu + hostname: ubuntu-server + # ubuntu + password: '$6$wdAcoXrU039hKYPd$508Qvbe7ObUnxoj15DRCkzC3qO7edjH0VV7BPNRDYK4QR8ofJaEEF2heacn0QgD.f8pO8SNp83XNdWG6tocBM1' +UbuntuPro: + token: "" +SSH: + install_server: true + pwauth: false + authorized_keys: + - | + ssh-rsa AAAAAAAAAAAAAAAAAAAAAAAAA # ssh-import-id lp:subiquity +SnapList: + snaps: + hello: + channel: stable + classic: false +InstallProgress: + reboot: yes +Drivers: + install: no diff --git a/snapcraft.yaml b/snapcraft.yaml index 8433475a9..1c8b4dd7b 100644 --- a/snapcraft.yaml +++ b/snapcraft.yaml @@ -70,7 +70,7 @@ parts: source: https://git.launchpad.net/curtin source-type: git - source-commit: 237053d9d18916dd72cf861280474d4df0e9fd24 + source-commit: 5094d95fa5b8506412979e21548069bc12681dc7 override-pull: | craftctl default diff --git a/subiquity/models/filesystem.py b/subiquity/models/filesystem.py index 4e299195d..d149ab6b6 100644 --- a/subiquity/models/filesystem.py +++ b/subiquity/models/filesystem.py @@ -2235,8 +2235,27 @@ def _mount_for_path(self, path): def is_root_mounted(self): return self._mount_for_path("/") is not None - def can_install(self): - return self.is_root_mounted() and not self.needs_bootloader_partition() + def is_rootfs_on_remote_storage(self) -> bool: + return self._mount_for_path("/").device.volume.on_remote_storage() + + def is_boot_mounted(self) -> bool: + return self._mount_for_path("/boot") is not None + + def is_bootfs_on_remote_storage(self) -> bool: + return self._mount_for_path("/boot").device.volume.on_remote_storage() + + def can_install(self) -> bool: + if not self.is_root_mounted(): + return False + + if self.is_rootfs_on_remote_storage(): + if not self.is_boot_mounted() or self.is_bootfs_on_remote_storage(): + return False + + if self.needs_bootloader_partition(): + return False + + return True def should_add_swapfile(self): mount = self._mount_for_path("/") diff --git a/subiquity/ui/mount.py b/subiquity/ui/mount.py index 649dd68e0..990771ac4 100644 --- a/subiquity/ui/mount.py +++ b/subiquity/ui/mount.py @@ -79,16 +79,16 @@ def enable_common_mountpoints(self): if isinstance(opt.value, str): opt.enabled = opt.value in common_mountpoints - def disable_all_mountpoints_but_home(self): - """Currently, we only want filesystems of non-local disks mounted at - /home. This is not completely enforced by the UI though. Users can - still select "other" and type "/", "/boot", "/var" or anything else.""" + def disable_boot_boot_efi_mountpoints(self): + """Currently, we want /boot and /boot/efi filesystems to be stored on + local disks. This is not completely enforced by the UI though. + Note that users can still select "other" and type "/boot", + "/boot/efi".""" for opt in self._selector._options: if not isinstance(opt.value, str): continue - if opt.value == "/home": - continue - opt.enabled = False + if opt.value == "/boot" or opt.value == "/boot/efi": + opt.enabled = False def _showhide_other(self, show): if show and not self._other_showing: diff --git a/subiquity/ui/views/filesystem/filesystem.py b/subiquity/ui/views/filesystem/filesystem.py index 9ba17d2af..60d564ebe 100644 --- a/subiquity/ui/views/filesystem/filesystem.py +++ b/subiquity/ui/views/filesystem/filesystem.py @@ -488,6 +488,13 @@ def _guidance(self): todos = [] if not self.model.is_root_mounted(): todos.append(_("Mount a filesystem at /")) + elif self.model.is_rootfs_on_remote_storage(): + # We need a /boot partition on local storage + if ( + not self.model.is_boot_mounted() + or self.model.is_bootfs_on_remote_storage() + ): + todos.append(_("Mount a local filesystem at /boot")) if self.model.needs_bootloader_partition(): todos.append(_("Select a boot disk")) if not todos: diff --git a/subiquity/ui/views/filesystem/partition.py b/subiquity/ui/views/filesystem/partition.py index 1f14c5066..807e40e33 100644 --- a/subiquity/ui/views/filesystem/partition.py +++ b/subiquity/ui/views/filesystem/partition.py @@ -180,8 +180,6 @@ def __init__( if ofstype: self.existing_fs_type = ofstype initial_path = initial.get("mount") - if not initial_path and remote_storage: - initial["mount"] = "/home" self.mountpoints = { m.path: m.device.volume for m in self.model.all_mounts() @@ -209,7 +207,7 @@ def select_fstype(self, sender, fstype): self.mount.widget.enable_common_mountpoints() self.mount.value = self.mount.value if self.remote_storage: - self.mount.widget.disable_all_mountpoints_but_home() + self.mount.widget.disable_boot_boot_efi_mountpoints() self.mount.value = self.mount.value if fstype is None: if self.existing_fs_type == "swap": @@ -312,14 +310,14 @@ def validate_mount(self): ).format(mountpoint=mount), ) ) - if self.remote_storage and mount != "/home": + if self.remote_storage and mount in ("/boot", "/boot/efi"): self.mount.show_extra( ( "info_error", _( - "This filesystem is on remote storage. It is usually a " - "bad idea to mount it anywhere but at /home, proceed " - "only with caution." + f"The filesystem for {mount} should be stored on local" + " storage. Storing it on remote storage will likely" + " prevent the system for booting." ), ) )