[pve-devel] [RFC PATCH pve-installer 3/6] close #5887: add sanity check for LVM swapsize and maxroot

["Michael Köppl" <m.koeppl@proxmox.com>]

Check that the configured swapsize is not greater than the total size
of the disk and that maxroot is at most hdsize / 4. Define the
behavior for the auto-installer as well as the TUI and GUI installers.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
The changes implemented in this patch regarding swap size only avoid
rather obvious misconfigurations. However, users could still configure a
32GB swap volume on a 32GB hard disk and the installer would fail later
on. Users could benefit another check that ensures the swap volume size
does not go beyond a certain threshold. One option could be to set the
limit similar to the 8GB maximum when the swap volume size is calculated
from the size of the memory. OTOH, this might also be
considered too restrictive. Would love some input on this.

Note regarding the change around set_hdsize in proxinstall: Before this
change, the hdsize was only set if the user manually changed it. If the
user did not change it, any checks against hdsize would check against
undefined.


 Proxmox/Install.pm                            | 14 +++++++++++++
 proxinstall                                   |  5 ++++-
 proxmox-auto-installer/src/utils.rs           | 13 ++++++++++++
 proxmox-auto-installer/tests/parse-answer.rs  |  4 +++-
 .../lvm_maxroot_greater_than_maximum.json     |  3 +++
 .../lvm_maxroot_greater_than_maximum.toml     | 16 +++++++++++++++
 .../lvm_swapsize_greater_than_hdsize.json     |  3 +++
 .../lvm_swapsize_greater_than_hdsize.toml     | 16 +++++++++++++++
 proxmox-installer-common/src/disk_checks.rs   | 20 ++++++++++++++++++-
 proxmox-tui-installer/src/views/bootdisk.rs   |  4 +++-
 10 files changed, 94 insertions(+), 4 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index f673604..d93a54b 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -592,6 +592,20 @@ sub compute_swapsize {
     return $swapsize_kb;
 }
 
+sub swapsize_check {
+    my ($hdsize) = @_;
+    my $swapsize = Proxmox::Install::Config::get_swapsize();
+    die "swapsize cannot be greater than hdsize\n"
+	if $swapsize > $hdsize;
+}
+
+sub maxroot_size_check {
+    my ($hdsize) = @_;
+    my $maxroot = Proxmox::Install::Config::get_maxroot();
+    die "maxroot cannot be greater than hdsize / 4\n"
+	if $maxroot > ($hdsize/4);
+}
+
 my sub chroot_chown {
     my ($root, $path, %param) = @_;
 
diff --git a/proxinstall b/proxinstall
index bc9ade6..e9ff6e8 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1406,7 +1406,7 @@ sub create_hdoption_view {
 
     my $tmp;
 
-    if (($tmp = &$get_float($spinbutton_hdsize)) && ($tmp != $hdsize)) {
+    if (defined($tmp = &$get_float($spinbutton_hdsize))) {
 	Proxmox::Install::Config::set_hdsize($tmp);
     } else {
 	Proxmox::Install::Config::set_hdsize(undef);
@@ -1521,9 +1521,12 @@ sub create_hdsel_view {
 	    $target_hds = [ map { $_->[1] } @$devlist ];
 	} else {
 	    my $target_hd = Proxmox::Install::Config::get_target_hd();
+	    my $hdsize = Proxmox::Install::Config::get_hdsize();
 	    eval {
 		my $target_block_size = Proxmox::Sys::Block::logical_blocksize($target_hd);
 		Proxmox::Install::legacy_bios_4k_check($target_block_size);
+		Proxmox::Install::swapsize_check($hdsize);
+		Proxmox::Install::maxroot_size_check($hdsize);
 	    };
 	    if (my $err = $@) {
 		Proxmox::UI::message("Warning: $err\n");
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index d6bc6e3..85a1f52 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -396,6 +396,19 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
             );
         }
     }
+
+    if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
+        if lvm.swapsize > lvm.hdsize {
+            bail!("LVM swapsize cannot be greater than hdsize");
+        }
+
+        if let Some((maxroot, hdsize)) = lvm.maxroot.zip(lvm.hdsize) {
+            if maxroot > hdsize / 4.0 {
+                bail!("LVM maxroot cannot be greater than hdsize / 4");
+            }
+        }
+    }
+
     Ok(())
 }
 
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 92dba63..e615672 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -7,7 +7,7 @@ use proxmox_auto_installer::udevinfo::UdevInfo;
 use proxmox_auto_installer::utils::parse_answer;
 
 use proxmox_installer_common::setup::{
-    LocaleInfo, RuntimeInfo, SetupInfo, load_installer_setup_files, read_json,
+    load_installer_setup_files, read_json, LocaleInfo, RuntimeInfo, SetupInfo,
 };
 
 fn get_test_resource_path() -> Result<PathBuf, String> {
@@ -145,6 +145,8 @@ mod tests {
             btrfs_raid_single_disk,
             fqdn_from_dhcp_no_default_domain,
             fqdn_hostname_only,
+            lvm_maxroot_greater_than_maximum,
+            lvm_swapsize_greater_than_hdsize,
             no_fqdn_from_dhcp,
             no_root_password_set,
             short_password,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
new file mode 100644
index 0000000..f05d6ee
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
@@ -0,0 +1,3 @@
+{
+  "error": "LVM maxroot cannot be greater than hdsize / 4"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
new file mode 100644
index 0000000..e934d29
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.toml
@@ -0,0 +1,16 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "ext4"
+lvm.maxroot = 8.01
+lvm.hdsize = 32
+disk-list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
new file mode 100644
index 0000000..562d834
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
@@ -0,0 +1,3 @@
+{
+  "error": "LVM swapsize cannot be greater than hdsize"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
new file mode 100644
index 0000000..a29c36a
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.toml
@@ -0,0 +1,16 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "btrfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "ext4"
+lvm.swapsize = 33
+lvm.hdsize = 32
+disk-list = ["sda"]
diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index 1d17e2c..bb33baf 100644
--- a/proxmox-installer-common/src/disk_checks.rs
+++ b/proxmox-installer-common/src/disk_checks.rs
@@ -1,6 +1,6 @@
 use std::collections::HashSet;
 
-use crate::options::{BtrfsRaidLevel, Disk, ZfsRaidLevel};
+use crate::options::{BtrfsRaidLevel, Disk, LvmBootdiskOptions, ZfsRaidLevel};
 use crate::setup::BootType;
 
 /// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,6 +49,24 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
     Ok(())
 }
 
+/// Checks whether a user-supplied LVM setup is valid or not, such as the swapsize or maxroot not
+/// exceeding certain thresholds.
+///
+/// # Arguments
+///
+/// * `bootdisk_opts` - The LVM options set by the user.
+pub fn check_lvm_bootdisk_opts(bootdisk_opts: &LvmBootdiskOptions) -> Result<(), &str> {
+    if bootdisk_opts.swap_size > Some(bootdisk_opts.total_size) {
+        return Err("Swap size cannot be greater than total size");
+    }
+
+    if bootdisk_opts.max_root_size > Some(bootdisk_opts.total_size / 4.0) {
+        return Err("Max root size cannot be greater than (total size / 4)");
+    }
+
+    Ok(())
+}
+
 /// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
 /// number of disks.
 ///
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 313a3c9..60d5316 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -19,7 +19,7 @@ use crate::options::FS_TYPES;
 use proxmox_installer_common::{
     disk_checks::{
         check_btrfs_raid_config, check_disks_4kn_legacy_boot, check_for_duplicate_disks,
-        check_zfs_raid_config,
+        check_lvm_bootdisk_opts, check_zfs_raid_config,
     },
     options::{
         AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
@@ -264,6 +264,8 @@ impl AdvancedBootdiskOptionsView {
                 .get_values()
                 .ok_or("Failed to retrieve advanced bootdisk options")?;
 
+            check_lvm_bootdisk_opts(&advanced).map_err(|err| format!("{fstype}: {err}"))?;
+
             Ok(BootdiskOptions {
                 disks: vec![disk],
                 fstype,
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel