[pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks

[pve-devel] [PATCH installer v2 0/6] add early disk and network sanity checks

[Michael Köppl]

The goal of this series is to add additional sanity checks to the
auto-installer and the TUI and GUI installers. The following checks were
added:
* Btrfs / ZFS RAID: check if the required number of disks is available
* LVM: check if swapsize < hdsize
* LVM: check if maxroot < hdsize/4
* Duplicate disks in answer file disk selection
* Networking: check if IPv4 address is valid within subnet (RFC)

The disk-related checks aim to close [0], whereas the IPv4 check would
partially close [1].

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5887
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=5757

Changes since v1:
- Removed rustfmt patch
- Removed patch that changed check_zfs_raid_config (replaced with patch
  below)
- Added patch that moves implementation of RAID setup checks to 
  implementation of ZfsRaidLevel and BtrfsRaidLevel
- Adapted tests for RAID setup validity
- Added configured values in GiB to error messages for invalid 
  swapsize and maxroot
- Changed maximum allowed swapsize to hdsize/8
- De-duplicated swapsize and maxroot check implementations in
  autoinstaller utils and common's disk_checks 
- Added patch that adds a check for duplicate disks in answer file disk
  selection
- Refactored Display implementation of CidrAddressParseError
- Simplified Deserialize implementation of CidrAddress
- Explicitly allow /31 and /32 subnets in CIDR validity check

Thanks to @Christoph for reviewing v1 and his suggestions.

pve-installer:

Michael Köppl (6):
  auto: add early answer file sanity check for RAID configurations
  move RAID setup checks to RAID level enum implementations
  close #5887: add sanity check for LVM swapsize and maxroot
  auto: add check for duplicate disks in answer file
  common: add more descriptive errors for invalid network configs
  common: add checks for valid subnet mask and IPv4 address within
    subnet

 Proxmox/Install.pm                            |  16 ++
 proxinstall                                   |   5 +-
 proxmox-auto-install-assistant/src/main.rs    |   7 +-
 proxmox-auto-installer/src/utils.rs           |  46 ++++-
 proxmox-auto-installer/tests/parse-answer.rs  |  12 +-
 .../parse_answer/ipv4_and_subnet_31.json      |  19 ++
 .../parse_answer/ipv4_and_subnet_31.toml      |  18 ++
 .../btrfs_raid_single_disk.json               |   3 +
 .../btrfs_raid_single_disk.toml               |  15 ++
 .../parse_answer_fail/duplicate_disk.json     |   3 +
 .../parse_answer_fail/duplicate_disk.toml     |  15 ++
 .../ipv4_and_subnet_addr_is_network.json      |   3 +
 .../ipv4_and_subnet_addr_is_network.toml      |  18 ++
 .../ipv4_and_subnet_mask_33.json              |   3 +
 .../ipv4_and_subnet_mask_33.toml              |  18 ++
 .../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 ++
 .../zfs_raid_single_disk.json                 |   3 +
 .../zfs_raid_single_disk.toml                 |  15 ++
 proxmox-installer-common/src/disk_checks.rs   | 174 +++++++-----------
 proxmox-installer-common/src/options.rs       |  91 +++++++++
 proxmox-installer-common/src/utils.rs         |  84 +++++++--
 proxmox-tui-installer/src/views/bootdisk.rs   |  13 +-
 25 files changed, 482 insertions(+), 137 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
 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
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml


Summary over all repositories:
  25 files changed, 482 insertions(+), 137 deletions(-)

-- 
Generated by git-murpp 0.8.0


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

[pve-devel] [PATCH pve-installer v2 1/6] auto: add early answer file sanity check for RAID configurations

[Michael Köppl]

The GUI and TUI installers already implement checks to ensure systems
have the minimum required number of disks available for the various RAID
configurations (min 2 disks for RAID1, min 4 disks for RAID10, etc).
This change adds an early check of the answer file to the
auto-installer, improving the user experience by avoiding failure during
the actual installation.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 proxmox-auto-install-assistant/src/main.rs    |  7 ++--
 proxmox-auto-installer/src/utils.rs           | 18 ++++++++++-
 proxmox-auto-installer/tests/parse-answer.rs  |  4 ++-
 .../btrfs_raid_single_disk.json               |  3 ++
 .../btrfs_raid_single_disk.toml               | 15 +++++++++
 .../zfs_raid_single_disk.json                 |  3 ++
 .../zfs_raid_single_disk.toml                 | 15 +++++++++
 proxmox-installer-common/src/options.rs       | 32 +++++++++++++++++++
 8 files changed, 92 insertions(+), 5 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml

diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index b64623b..3cf3abc 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -14,9 +14,9 @@ use proxmox_auto_installer::{
     answer::{Answer, FilterMatch},
     sysinfo::SysInfo,
     utils::{
-        AutoInstSettings, FetchAnswerFrom, HttpOptions, get_matched_udev_indexes, get_nic_list,
-        get_single_udev_index, verify_email_and_root_password_settings, verify_first_boot_settings,
-        verify_locale_settings,
+        get_matched_udev_indexes, get_nic_list, get_single_udev_index, verify_disks_settings,
+        verify_email_and_root_password_settings, verify_first_boot_settings,
+        verify_locale_settings, AutoInstSettings, FetchAnswerFrom, HttpOptions,
     },
 };
 use proxmox_installer_common::{FIRST_BOOT_EXEC_MAX_SIZE, FIRST_BOOT_EXEC_NAME};
@@ -597,6 +597,7 @@ fn parse_answer(path: impl AsRef<Path> + fmt::Debug) -> Result<Answer> {
     match toml::from_str(&contents) {
         Ok(answer) => {
             verify_locale_settings(&answer, &serde_json::from_str(LOCALE_INFO)?)?;
+            verify_disks_settings(&answer)?;
             verify_first_boot_settings(&answer)?;
             verify_email_and_root_password_settings(&answer)?;
             println!("The answer file was parsed successfully, no errors found!");
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 365e01a..d6bc6e3 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -6,7 +6,8 @@ use std::{collections::BTreeMap, process::Command};
 
 use crate::{
     answer::{
-        self, Answer, FirstBootHookSourceMode, FqdnConfig, FqdnExtendedConfig, FqdnSourceMode,
+        self, Answer, DiskSelection, FirstBootHookSourceMode, FqdnConfig, FqdnExtendedConfig,
+        FqdnSourceMode,
     },
     udevinfo::UdevInfo,
 };
@@ -384,6 +385,20 @@ pub fn verify_email_and_root_password_settings(answer: &Answer) -> Result<()> {
     }
 }
 
+pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
+    if let DiskSelection::Selection(selection) = &answer.disks.disk_selection {
+        let min_disks = answer.disks.fs_type.get_min_disks();
+        if selection.len() < min_disks {
+            bail!(
+                "{} requires at least {} disks",
+                answer.disks.fs_type,
+                min_disks
+            );
+        }
+    }
+    Ok(())
+}
+
 pub fn verify_first_boot_settings(answer: &Answer) -> Result<()> {
     info!("Verifying first boot settings");
 
@@ -414,6 +429,7 @@ pub fn parse_answer(
     let network_settings = get_network_settings(answer, udev_info, runtime_info, setup_info)?;
 
     verify_locale_settings(answer, locales)?;
+    verify_disks_settings(answer)?;
     verify_email_and_root_password_settings(answer)?;
     verify_first_boot_settings(answer)?;
 
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index 34bc969..92dba63 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -142,11 +142,13 @@ mod tests {
             run_named_fail_parse_test,
             // Keep below entries alphabetically sorted
             both_password_and_hashed_set,
+            btrfs_raid_single_disk,
             fqdn_from_dhcp_no_default_domain,
             fqdn_hostname_only,
             no_fqdn_from_dhcp,
             no_root_password_set,
-            short_password
+            short_password,
+            zfs_raid_single_disk
         );
     }
 }
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
new file mode 100644
index 0000000..37f35fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.json
@@ -0,0 +1,3 @@
+{
+  "error": "BTRFS (RAID1) requires at least 2 disks"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
new file mode 100644
index 0000000..e96fb16
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/btrfs_raid_single_disk.toml
@@ -0,0 +1,15 @@
+[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 = "btrfs"
+btrfs.raid = "raid1"
+disk-list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
new file mode 100644
index 0000000..9a8cc90
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.json
@@ -0,0 +1,3 @@
+{
+  "error": "ZFS (RAID10) requires at least 4 disks"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
new file mode 100644
index 0000000..94ae748
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/zfs_raid_single_disk.toml
@@ -0,0 +1,15 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "zfs-raid-single-disk.fail.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root-password = "12345678"
+
+[network]
+source = "from-dhcp"
+
+[disk-setup]
+filesystem = "zfs"
+zfs.raid = "raid10"
+disk-list = ["sda"]
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9cc4ee0..9271b8b 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -20,6 +20,16 @@ pub enum BtrfsRaidLevel {
     Raid10,
 }
 
+impl BtrfsRaidLevel {
+    pub fn get_min_disks(&self) -> usize {
+        match self {
+            BtrfsRaidLevel::Raid0 => 1,
+            BtrfsRaidLevel::Raid1 => 2,
+            BtrfsRaidLevel::Raid10 => 4,
+        }
+    }
+}
+
 serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
 
 #[derive(Copy, Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
@@ -48,6 +58,19 @@ pub enum ZfsRaidLevel {
     RaidZ3,
 }
 
+impl ZfsRaidLevel {
+    pub fn get_min_disks(&self) -> usize {
+        match self {
+            ZfsRaidLevel::Raid0 => 1,
+            ZfsRaidLevel::Raid1 => 2,
+            ZfsRaidLevel::Raid10 => 4,
+            ZfsRaidLevel::RaidZ => 3,
+            ZfsRaidLevel::RaidZ2 => 4,
+            ZfsRaidLevel::RaidZ3 => 5,
+        }
+    }
+}
+
 serde_plain::derive_display_from_serialize!(ZfsRaidLevel);
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -67,6 +90,15 @@ impl FsType {
     pub fn is_lvm(&self) -> bool {
         matches!(self, FsType::Ext4 | FsType::Xfs)
     }
+
+    pub fn get_min_disks(&self) -> usize {
+        match self {
+            FsType::Ext4 => 1,
+            FsType::Xfs => 1,
+            FsType::Zfs(level) => level.get_min_disks(),
+            FsType::Btrfs(level) => level.get_min_disks(),
+        }
+    }
 }
 
 impl fmt::Display for FsType {
-- 
2.39.5



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

[pve-devel] [PATCH pve-installer v2 2/6] move RAID setup checks to RAID level enum implementations

[Michael Köppl]

Instead of having parts of the RAID setup checks scattered in multiple
places, move the core of the checks to implementations of the
ZfsRaidLevel and BtrfsRaidLevel enums.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 proxmox-installer-common/src/disk_checks.rs | 156 ++++----------------
 proxmox-installer-common/src/options.rs     |  59 ++++++++
 proxmox-tui-installer/src/views/bootdisk.rs |  13 +-
 3 files changed, 96 insertions(+), 132 deletions(-)

diff --git a/proxmox-installer-common/src/disk_checks.rs b/proxmox-installer-common/src/disk_checks.rs
index ecc43bd..d535837 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::Disk;
 use crate::setup::BootType;
 
 /// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,94 +49,10 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
     Ok(())
 }
 
-/// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
-/// number of disks.
-///
-/// # Arguments
-///
-/// * `level` - The targeted ZFS RAID level by the user.
-/// * `disks` - List of disks designated as RAID targets.
-pub fn check_zfs_raid_config(level: ZfsRaidLevel, disks: &[Disk]) -> Result<(), String> {
-    // See also Proxmox/Install.pm:get_zfs_raid_setup()
-
-    let check_mirror_size = |disk1: &Disk, disk2: &Disk| {
-        if (disk1.size - disk2.size).abs() > disk1.size / 10. {
-            Err(format!(
-                "Mirrored disks must have same size:\n\n  * {disk1}\n  * {disk2}"
-            ))
-        } else {
-            Ok(())
-        }
-    };
-
-    match level {
-        ZfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
-        ZfsRaidLevel::Raid1 => {
-            check_raid_min_disks(disks, 2)?;
-            for disk in disks {
-                check_mirror_size(&disks[0], disk)?;
-            }
-        }
-        ZfsRaidLevel::Raid10 => {
-            check_raid_min_disks(disks, 4)?;
-
-            if disks.len() % 2 != 0 {
-                return Err(format!(
-                    "Needs an even number of disks, currently selected: {}",
-                    disks.len(),
-                ));
-            }
-
-            // Pairs need to have the same size
-            for i in (0..disks.len()).step_by(2) {
-                check_mirror_size(&disks[i], &disks[i + 1])?;
-            }
-        }
-        // For RAID-Z: minimum disks number is level + 2
-        ZfsRaidLevel::RaidZ => {
-            check_raid_min_disks(disks, 3)?;
-            for disk in disks {
-                check_mirror_size(&disks[0], disk)?;
-            }
-        }
-        ZfsRaidLevel::RaidZ2 => {
-            check_raid_min_disks(disks, 4)?;
-            for disk in disks {
-                check_mirror_size(&disks[0], disk)?;
-            }
-        }
-        ZfsRaidLevel::RaidZ3 => {
-            check_raid_min_disks(disks, 5)?;
-            for disk in disks {
-                check_mirror_size(&disks[0], disk)?;
-            }
-        }
-    }
-
-    Ok(())
-}
-
-/// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
-/// number of disks.
-///
-/// # Arguments
-///
-/// * `level` - The targeted Btrfs RAID level by the user.
-/// * `disks` - List of disks designated as RAID targets.
-pub fn check_btrfs_raid_config(level: BtrfsRaidLevel, disks: &[Disk]) -> Result<(), String> {
-    // See also Proxmox/Install.pm:get_btrfs_raid_setup()
-
-    match level {
-        BtrfsRaidLevel::Raid0 => check_raid_min_disks(disks, 1)?,
-        BtrfsRaidLevel::Raid1 => check_raid_min_disks(disks, 2)?,
-        BtrfsRaidLevel::Raid10 => check_raid_min_disks(disks, 4)?,
-    }
-
-    Ok(())
-}
-
 #[cfg(test)]
 mod tests {
+    use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
+
     use super::*;
 
     fn dummy_disk(index: usize) -> Disk {
@@ -194,50 +110,38 @@ mod tests {
     fn btrfs_raid() {
         let disks = dummy_disks(10);
 
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &[]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks[..1]).is_ok());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid0, &disks).is_ok());
-
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &[]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..1]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks[..2]).is_ok());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid1, &disks).is_ok());
-
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &[]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..3]).is_err());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks[..4]).is_ok());
-        assert!(check_btrfs_raid_config(BtrfsRaidLevel::Raid10, &disks).is_ok());
+        let btrfs_raid_variants = [
+            BtrfsRaidLevel::Raid0,
+            BtrfsRaidLevel::Raid1,
+            BtrfsRaidLevel::Raid10,
+        ];
+
+        for v in btrfs_raid_variants {
+            assert!(v.check_disks(&[]).is_err());
+            assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
+            assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
+            assert!(v.check_disks(&disks).is_ok());
+        }
     }
 
     #[test]
     fn zfs_raid() {
         let disks = dummy_disks(10);
 
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &[]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks[..1]).is_ok());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid0, &disks).is_ok());
-
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &[]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks[..2]).is_ok());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid1, &disks).is_ok());
-
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &[]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &dummy_disks(4)).is_ok());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::Raid10, &disks).is_ok());
-
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &[]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..2]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks[..3]).is_ok());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ, &disks).is_ok());
-
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &[]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..3]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks[..4]).is_ok());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ2, &disks).is_ok());
-
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &[]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..4]).is_err());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks[..5]).is_ok());
-        assert!(check_zfs_raid_config(ZfsRaidLevel::RaidZ3, &disks).is_ok());
+        let zfs_raid_variants = [
+            ZfsRaidLevel::Raid0,
+            ZfsRaidLevel::Raid1,
+            ZfsRaidLevel::Raid10,
+            ZfsRaidLevel::RaidZ,
+            ZfsRaidLevel::RaidZ2,
+            ZfsRaidLevel::RaidZ3,
+        ];
+
+        for v in zfs_raid_variants {
+            assert!(v.check_disks(&[]).is_err());
+            assert!(v.check_disks(&disks[..v.get_min_disks() - 1]).is_err());
+            assert!(v.check_disks(&disks[..v.get_min_disks()]).is_ok());
+            assert!(v.check_disks(&disks).is_ok());
+        }
     }
 }
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9271b8b..0552954 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -6,6 +6,7 @@ use std::str::FromStr;
 use std::sync::OnceLock;
 use std::{cmp, fmt};
 
+use crate::disk_checks::check_raid_min_disks;
 use crate::setup::{LocaleInfo, NetworkInfo, RuntimeInfo, SetupInfo};
 use crate::utils::{CidrAddress, Fqdn};
 
@@ -28,6 +29,17 @@ impl BtrfsRaidLevel {
             BtrfsRaidLevel::Raid10 => 4,
         }
     }
+
+    /// Checks whether a user-supplied Btrfs RAID setup is valid or not, such as minimum
+    /// number of disks.
+    ///
+    /// # Arguments
+    ///
+    /// * `disks` - List of disks designated as RAID targets.
+    pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
+        check_raid_min_disks(disks, self.get_min_disks())?;
+        Ok(())
+    }
 }
 
 serde_plain::derive_display_from_serialize!(BtrfsRaidLevel);
@@ -69,6 +81,53 @@ impl ZfsRaidLevel {
             ZfsRaidLevel::RaidZ3 => 5,
         }
     }
+
+    fn check_mirror_size(&self, disk1: &Disk, disk2: &Disk) -> Result<(), String> {
+        if (disk1.size - disk2.size).abs() > disk1.size / 10. {
+            Err(format!(
+                "Mirrored disks must have same size:\n\n  * {disk1}\n  * {disk2}"
+            ))
+        } else {
+            Ok(())
+        }
+    }
+
+    /// Checks whether a user-supplied ZFS RAID setup is valid or not, such as disk sizes andminimum
+    /// number of disks.
+    ///
+    /// # Arguments
+    ///
+    /// * `disks` - List of disks designated as RAID targets.
+    pub fn check_disks(&self, disks: &[Disk]) -> Result<(), String> {
+        check_raid_min_disks(disks, self.get_min_disks())?;
+
+        match self {
+            ZfsRaidLevel::Raid0 => {}
+            ZfsRaidLevel::Raid10 => {
+                if disks.len() % 2 != 0 {
+                    return Err(format!(
+                        "Needs an even number of disks, currently selected: {}",
+                        disks.len(),
+                    ));
+                }
+
+                // Pairs need to have the same size
+                for i in (0..disks.len()).step_by(2) {
+                    self.check_mirror_size(&disks[i], &disks[i + 1])?;
+                }
+            }
+            ZfsRaidLevel::Raid1
+            | ZfsRaidLevel::RaidZ
+            | ZfsRaidLevel::RaidZ2
+            | ZfsRaidLevel::RaidZ3 => {
+                for disk in disks {
+                    self.check_mirror_size(&disks[0], disk)?;
+                }
+            }
+        }
+
+        Ok(())
+    }
 }
 
 serde_plain::derive_display_from_serialize!(ZfsRaidLevel);
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 313a3c9..e87b040 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -17,10 +17,7 @@ use crate::InstallerState;
 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,
-    },
+    disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
     options::{
         AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
         Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
@@ -275,7 +272,9 @@ impl AdvancedBootdiskOptionsView {
                 .ok_or("Failed to retrieve advanced bootdisk options")?;
 
             if let FsType::Zfs(level) = fstype {
-                check_zfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?;
+                level
+                    .check_disks(&disks)
+                    .map_err(|err| format!("{fstype}: {err}"))?;
             }
 
             Ok(BootdiskOptions {
@@ -289,7 +288,9 @@ impl AdvancedBootdiskOptionsView {
                 .ok_or("Failed to retrieve advanced bootdisk options")?;
 
             if let FsType::Btrfs(level) = fstype {
-                check_btrfs_raid_config(level, &disks).map_err(|err| format!("{fstype}: {err}"))?;
+                level
+                    .check_disks(&disks)
+                    .map_err(|err| format!("{fstype}: {err}"))?;
             }
 
             Ok(BootdiskOptions {
-- 
2.39.5



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

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

[Michael Köppl]

Check that the configured swapsize is not greater than hdsize / 8 as
stated in the admin guide [0]. Additionally check that maxroot is at
most hdsize / 4. Define the behavior for the auto-installer as well as
the TUI and GUI installers.

[0] https://pve.proxmox.com/pve-docs/pve-admin-guide.html#advanced_lvm_options

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
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                            | 16 ++++++
 proxinstall                                   |  5 +-
 proxmox-auto-installer/src/utils.rs           | 16 ++++++
 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   | 52 ++++++++++++++++++-
 proxmox-tui-installer/src/views/bootdisk.rs   |  6 ++-
 10 files changed, 133 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..7c55d1f 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -592,6 +592,22 @@ sub compute_swapsize {
     return $swapsize_kb;
 }
 
+sub swapsize_check {
+    my ($hdsize) = @_;
+    my $swapsize = Proxmox::Install::Config::get_swapsize();
+    my $threshold = $hdsize / 8;
+    die "swap size ${swapsize} GiB cannot be greater than ${threshold} GiB (hard disk size / 8)\n"
+	if $swapsize > $threshold;
+}
+
+sub maxroot_size_check {
+    my ($hdsize) = @_;
+    my $maxroot = Proxmox::Install::Config::get_maxroot();
+    my $threshold = $hdsize / 4;
+    die "maximum root volume size ${maxroot} GiB cannot be greater than ${threshold} GiB (hard disk size / 4)\n"
+	if $maxroot > $threshold;
+}
+
 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..75696bd 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -13,6 +13,7 @@ use crate::{
 };
 use proxmox_installer_common::{
     ROOT_PASSWORD_MIN_LENGTH,
+    disk_checks::{check_maxroot, check_swapsize},
     options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption, email_validate},
     setup::{
         InstallBtrfsOption, InstallConfig, InstallFirstBootSetup, InstallRootPassword,
@@ -396,6 +397,21 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
             );
         }
     }
+
+    if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
+        if let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize) {
+            if let Err(err) = check_swapsize(swapsize, hdsize) {
+                bail!(err);
+            }
+        }
+
+        if let Some((maxroot, hdsize)) = lvm.maxroot.zip(lvm.hdsize) {
+            if let Err(err) = check_maxroot(maxroot, hdsize) {
+                bail!(err);
+            }
+        }
+    }
+
     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..bab12e6
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
@@ -0,0 +1,3 @@
+{
+  "error": "Maximum root volume size 8.01 GiB cannot be greater than 8 GiB (hard disk size / 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..aa4f7fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
@@ -0,0 +1,3 @@
+{
+  "error": "Swap size 4.01 GiB cannot be greater than 4 GiB (hard disk size / 8)"
+}
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..ffe16dc
--- /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 = 4.01
+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 d535837..77104ae 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::Disk;
+use crate::options::{Disk, LvmBootdiskOptions};
 use crate::setup::BootType;
 
 /// Checks a list of disks for duplicate entries, using their index as key.
@@ -49,6 +49,56 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
     Ok(())
 }
 
+/// Checks whether the configured swap size exceeds the allowed threshold.
+///
+/// # Arguments
+///
+/// * `swapsize` - The size of the swap in GiB
+/// * `hdsize` - The total size of the hard disk in GiB
+pub fn check_swapsize(swapsize: f64, hdsize: f64) -> Result<(), String> {
+    let threshold = hdsize / 8.0;
+    if swapsize > threshold {
+        return Err(format!(
+            "Swap size {swapsize} GiB cannot be greater than {threshold} GiB (hard disk size / 8)",
+        ));
+    }
+    Ok(())
+}
+
+/// Checks whether the configured root volume size exceeds the allowed threshold.
+///
+/// # Arguments
+///
+/// * `maxroot` - The size of the root volume in GiB
+/// * `hdsize` - The total size of the hard disk in GiB
+pub fn check_maxroot(maxroot: f64, hdsize: f64) -> Result<(), String> {
+    let threshold = hdsize / 4.0;
+    if maxroot > threshold {
+        return Err(format!(
+            "Maximum root volume size {maxroot} GiB cannot be greater than {threshold} GiB (hard disk size / 4)",
+        ));
+    }
+    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<(), String> {
+    if let Some(swap_size) = bootdisk_opts.swap_size {
+        check_swapsize(swap_size, bootdisk_opts.total_size)?;
+    }
+
+    if let Some(max_root_size) = bootdisk_opts.max_root_size {
+        check_maxroot(max_root_size, bootdisk_opts.total_size)?;
+    }
+
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index e87b040..b94cf38 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -17,7 +17,9 @@ use crate::InstallerState;
 use crate::options::FS_TYPES;
 
 use proxmox_installer_common::{
-    disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
+    disk_checks::{
+        check_disks_4kn_legacy_boot, check_for_duplicate_disks, check_lvm_bootdisk_opts,
+    },
     options::{
         AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
         Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
@@ -261,6 +263,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

[pve-devel] [PATCH pve-installer v2 4/6] auto: add check for duplicate disks in answer file

[Michael Köppl]

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 proxmox-auto-installer/src/utils.rs               | 12 +++++++++++-
 proxmox-auto-installer/tests/parse-answer.rs      |  1 +
 .../parse_answer_fail/duplicate_disk.json         |  3 +++
 .../parse_answer_fail/duplicate_disk.toml         | 15 +++++++++++++++
 4 files changed, 30 insertions(+), 1 deletion(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml

diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 75696bd..3a1573e 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -2,7 +2,10 @@ use anyhow::{Context, Result, bail};
 use clap::ValueEnum;
 use glob::Pattern;
 use log::info;
-use std::{collections::BTreeMap, process::Command};
+use std::{
+    collections::{BTreeMap, HashSet},
+    process::Command,
+};
 
 use crate::{
     answer::{
@@ -396,6 +399,13 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
                 min_disks
             );
         }
+
+        let mut disk_set = HashSet::new();
+        for disk in selection {
+            if !disk_set.insert(disk) {
+                bail!("List of disks contains duplicate disk {disk}");
+            }
+        }
     }
 
     if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index e615672..f560483 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -143,6 +143,7 @@ mod tests {
             // Keep below entries alphabetically sorted
             both_password_and_hashed_set,
             btrfs_raid_single_disk,
+            duplicate_disk,
             fqdn_from_dhcp_no_default_domain,
             fqdn_hostname_only,
             lvm_maxroot_greater_than_maximum,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
new file mode 100644
index 0000000..d01fabe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.json
@@ -0,0 +1,3 @@
+{
+  "error": "List of disks contains duplicate disk sda"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
new file mode 100644
index 0000000..bff1631
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/duplicate_disk.toml
@@ -0,0 +1,15 @@
+[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 = "btrfs"
+btrfs.raid = "raid1"
+disk-list = ["sda", "sdb", "sda"]
-- 
2.39.5



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

[pve-devel] [PATCH pve-installer v2 5/6] common: add more descriptive errors for invalid network configs

[Michael Köppl]

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 proxmox-installer-common/src/utils.rs | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 8adcec0..1fe6a74 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -18,6 +18,20 @@ pub enum CidrAddressParseError {
     InvalidMask(Option<ParseIntError>),
 }
 
+impl fmt::Display for CidrAddressParseError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let msg = match &self {
+            CidrAddressParseError::NoDelimiter => {
+                String::from("No delimiter for separating address and mask was found")
+            }
+            CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
+            CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
+        };
+
+        write!(f, "Invalid CIDR: {msg}")
+    }
+}
+
 /// An IP address (IPv4 or IPv6), including network mask.
 ///
 /// See the [`IpAddr`] type for more information how IP addresses are handled.
@@ -109,8 +123,7 @@ impl<'de> Deserialize<'de> for CidrAddress {
         D: serde::Deserializer<'de>,
     {
         let s: String = Deserialize::deserialize(deserializer)?;
-        s.parse()
-            .map_err(|_| serde::de::Error::custom("invalid CIDR"))
+        s.parse().map_err(serde::de::Error::custom)
     }
 }
 
-- 
2.39.5



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

[pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet

[Michael Köppl]

Add checks for valid subnet mask (greater than /0 and at most /32 for
IPv4). In addition, check if the address entered by the user is valid
within the given subnet, i.e. not a network address or broadcast
address. /31 is considered an exception in accordance with RFC3021 [0],
considering any of the 2 available addresses to be valid host addresses.

[0] https://datatracker.ietf.org/doc/html/rfc3021

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Some input / discussion would be much appreciated here, since this might
again be considered too restrictive.

 proxmox-auto-installer/tests/parse-answer.rs  |  3 +
 .../parse_answer/ipv4_and_subnet_31.json      | 19 +++++
 .../parse_answer/ipv4_and_subnet_31.toml      | 18 +++++
 .../ipv4_and_subnet_addr_is_network.json      |  3 +
 .../ipv4_and_subnet_addr_is_network.toml      | 18 +++++
 .../ipv4_and_subnet_mask_33.json              |  3 +
 .../ipv4_and_subnet_mask_33.toml              | 18 +++++
 proxmox-installer-common/src/utils.rs         | 81 ++++++++++++++-----
 8 files changed, 143 insertions(+), 20 deletions(-)
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
 create mode 100644 proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml

diff --git a/proxmox-auto-installer/tests/parse-answer.rs b/proxmox-auto-installer/tests/parse-answer.rs
index f560483..6f627a0 100644
--- a/proxmox-auto-installer/tests/parse-answer.rs
+++ b/proxmox-auto-installer/tests/parse-answer.rs
@@ -127,6 +127,7 @@ mod tests {
             fqdn_from_dhcp_no_dhcp_domain_with_default_domain,
             full_fqdn_from_dhcp_with_default_domain,
             hashed_root_password,
+            ipv4_and_subnet_31,
             minimal,
             nic_matching,
             specific_nic,
@@ -146,6 +147,8 @@ mod tests {
             duplicate_disk,
             fqdn_from_dhcp_no_default_domain,
             fqdn_hostname_only,
+            ipv4_and_subnet_addr_is_network,
+            ipv4_and_subnet_mask_33,
             lvm_maxroot_greater_than_maximum,
             lvm_swapsize_greater_than_hdsize,
             no_fqdn_from_dhcp,
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
new file mode 100644
index 0000000..2a475fe
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.json
@@ -0,0 +1,19 @@
+{
+  "autoreboot": 1,
+  "cidr": "10.10.10.10/31",
+  "country": "at",
+  "dns": "10.10.10.1",
+  "domain": "testinstall",
+  "filesys": "ext4",
+  "gateway": "10.10.10.1",
+  "hdsize": 223.57088470458984,
+  "existing_storage_auto_rename": 1,
+  "hostname": "pveauto",
+  "keymap": "de",
+  "mailto": "mail@no.invalid",
+  "mngmt_nic": "enp129s0f1np1",
+  "root_password": { "plain": "12345678" },
+  "target_hd": "/dev/sda",
+  "timezone": "Europe/Vienna",
+  "first_boot": { "enabled": 0 }
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
new file mode 100644
index 0000000..0ca6ca5
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer/ipv4_and_subnet_31.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/31"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
new file mode 100644
index 0000000..b28699d
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.json
@@ -0,0 +1,3 @@
+{
+  "parse-error": "error parsing answer.toml: Invalid CIDR: address is a network address"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
new file mode 100644
index 0000000..7a9141b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_addr_is_network.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.32/27"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
new file mode 100644
index 0000000..6b2888b
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.json
@@ -0,0 +1,3 @@
+{
+    "parse-error": "error parsing answer.toml: Invalid CIDR: mask cannot be greater than 32"
+}
diff --git a/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
new file mode 100644
index 0000000..16a560f
--- /dev/null
+++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/ipv4_and_subnet_mask_33.toml
@@ -0,0 +1,18 @@
+[global]
+keyboard = "de"
+country = "at"
+fqdn = "pveauto.testinstall"
+mailto = "mail@no.invalid"
+timezone = "Europe/Vienna"
+root_password = "12345678"
+
+[network]
+source = "from-answer"
+cidr = "10.10.10.10/33"
+dns = "10.10.10.1"
+gateway = "10.10.10.1"
+filter.ID_NET_NAME = "enp129s0f1np1"
+
+[disk-setup]
+filesystem = "ext4"
+disk_list = ["sda"]
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 1fe6a74..fea98db 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -1,7 +1,7 @@
 use std::{
+    error::Error,
     fmt,
     net::{AddrParseError, IpAddr},
-    num::ParseIntError,
     str::FromStr,
 };
 
@@ -15,20 +15,26 @@ pub enum CidrAddressParseError {
     /// The IP address part could not be parsed.
     InvalidAddr(AddrParseError),
     /// The mask could not be parsed.
-    InvalidMask(Option<ParseIntError>),
+    InvalidMask(Box<dyn Error>),
+    /// The IP address is a network address.
+    IsNetworkAddr,
+    /// The IP address is a broadcast address.
+    IsBroadcastAddr,
 }
 
 impl fmt::Display for CidrAddressParseError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let msg = match &self {
+        write!(f, "Invalid CIDR: ")?;
+
+        match self {
             CidrAddressParseError::NoDelimiter => {
-                String::from("No delimiter for separating address and mask was found")
+                write!(f, "no delimiter for separating address and mask was found")
             }
-            CidrAddressParseError::InvalidAddr(addr_parse_error) => format!("{addr_parse_error}"),
-            CidrAddressParseError::InvalidMask(parse_int_error) => format!("{:?}", parse_int_error),
-        };
-
-        write!(f, "Invalid CIDR: {msg}")
+            CidrAddressParseError::InvalidAddr(err) => write!(f, "{err}"),
+            CidrAddressParseError::InvalidMask(err) => write!(f, "{err}"),
+            CidrAddressParseError::IsNetworkAddr => write!(f, "address is a network address"),
+            CidrAddressParseError::IsBroadcastAddr => write!(f, "address is a broadcast address"),
+        }
     }
 }
 
@@ -61,11 +67,10 @@ impl CidrAddress {
     pub fn new<T: Into<IpAddr>>(addr: T, mask: usize) -> Result<Self, CidrAddressParseError> {
         let addr = addr.into();
 
-        if mask > mask_limit(&addr) {
-            Err(CidrAddressParseError::InvalidMask(None))
-        } else {
-            Ok(Self { addr, mask })
-        }
+        check_mask_limit(&addr, mask)?;
+        check_addr_valid_in_subnet(&addr, mask)?;
+
+        Ok(Self { addr, mask })
     }
 
     /// Returns only the IP address part of the address.
@@ -101,13 +106,12 @@ impl FromStr for CidrAddress {
 
         let mask = mask
             .parse()
-            .map_err(|err| CidrAddressParseError::InvalidMask(Some(err)))?;
+            .map_err(|err| CidrAddressParseError::InvalidMask(Box::new(err)))?;
 
-        if mask > mask_limit(&addr) {
-            Err(CidrAddressParseError::InvalidMask(None))
-        } else {
-            Ok(Self { addr, mask })
-        }
+        check_mask_limit(&addr, mask)?;
+        check_addr_valid_in_subnet(&addr, mask)?;
+
+        Ok(Self { addr, mask })
     }
 }
 
@@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
     if addr.is_ipv4() { 32 } else { 128 }
 }
 
+fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+    return if mask > mask_limit(&addr) {
+        Err(CidrAddressParseError::InvalidMask(
+            "mask cannot be greater than 32".into(),
+        ))
+    } else {
+        Ok(())
+    };
+}
+
+fn check_addr_valid_in_subnet(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
+    match &addr {
+        IpAddr::V4(addr_v4) => {
+            if mask >= 31 {
+                return Ok(());
+            }
+
+            let num_host_bits = 32 - mask;
+            let host_part_mask = (1u32 << num_host_bits) - 1;
+
+            let ip_bits = u32::from_be_bytes(addr_v4.octets());
+
+            let network_addr = ip_bits & !host_part_mask;
+            let broadcast_addr = network_addr | host_part_mask;
+
+            if ip_bits == network_addr {
+                Err(CidrAddressParseError::IsNetworkAddr)
+            } else if ip_bits == broadcast_addr {
+                Err(CidrAddressParseError::IsBroadcastAddr)
+            } else {
+                Ok(())
+            }
+        }
+        IpAddr::V6(_) => Ok(()),
+    }
+}
+
 /// Possible errors that might occur when parsing FQDNs.
 #[derive(Debug, Eq, PartialEq)]
 pub enum FqdnParseError {
-- 
2.39.5



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

Re: [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet

[Christoph Heiss]

Apart from the one inline comment, looks good to me.

IMO it's not too restrictive, since IPv4 network/broadcast addresses
shouldn't really be used (and probably introduce subtle breakages)
outside of /31 prefixes anyway.

On Tue Apr 29, 2025 at 4:09 PM CEST, Michael Köppl wrote:
> Add checks for valid subnet mask (greater than /0 and at most /32 for
> IPv4). In addition, check if the address entered by the user is valid
> within the given subnet, i.e. not a network address or broadcast
> address. /31 is considered an exception in accordance with RFC3021 [0],
> considering any of the 2 available addresses to be valid host addresses.
>
> [0] https://datatracker.ietf.org/doc/html/rfc3021
>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> [..]
> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
> index 1fe6a74..fea98db 100644
> --- a/proxmox-installer-common/src/utils.rs
> +++ b/proxmox-installer-common/src/utils.rs
> [..]
> @@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
>      if addr.is_ipv4() { 32 } else { 128 }
>  }
>
> +fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
> +    return if mask > mask_limit(&addr) {
> +        Err(CidrAddressParseError::InvalidMask(
> +            "mask cannot be greater than 32".into(),

s/32/mask_limit(&addr)/g

> +        ))
> +    } else {
> +        Ok(())
> +    };
> +}


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

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

[Christoph Heiss]

After testing this change and thinking about the maxroot change again,
$hdsize / 4 doesn't really make sense. E.g. for an (unrealistically
small, but still) disk of 8 GiB; if its unset, pve-root will be ~6.5 GiB
in size, with the limit of 2 GiB, the installation fails due to
ENOSPACE.

The default calculations try really hard to make installations possible
even on small disks, in Proxmox/Install.pm:create_lvm_volumes()

So I'm not sure if we really should restrict it that much, or rather
relax it in the documentation.

On Tue Apr 29, 2025 at 4:09 PM CEST, Michael Köppl wrote:
> Check that the configured swapsize is not greater than hdsize / 8 as
> stated in the admin guide [0]. Additionally check that maxroot is at
> most hdsize / 4. Define the behavior for the auto-installer as well as
> the TUI and GUI installers.
>
> [0] https://pve.proxmox.com/pve-docs/pve-admin-guide.html#advanced_lvm_options
>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> 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                            | 16 ++++++
>  proxinstall                                   |  5 +-
>  proxmox-auto-installer/src/utils.rs           | 16 ++++++
>  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   | 52 ++++++++++++++++++-
>  proxmox-tui-installer/src/views/bootdisk.rs   |  6 ++-
>  10 files changed, 133 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..7c55d1f 100644
> --- a/Proxmox/Install.pm
> +++ b/Proxmox/Install.pm
> @@ -592,6 +592,22 @@ sub compute_swapsize {
>      return $swapsize_kb;
>  }
>
> +sub swapsize_check {
> +    my ($hdsize) = @_;
> +    my $swapsize = Proxmox::Install::Config::get_swapsize();
> +    my $threshold = $hdsize / 8;
> +    die "swap size ${swapsize} GiB cannot be greater than ${threshold} GiB (hard disk size / 8)\n"
> +	if $swapsize > $threshold;
> +}
> +
> +sub maxroot_size_check {
> +    my ($hdsize) = @_;
> +    my $maxroot = Proxmox::Install::Config::get_maxroot();
> +    my $threshold = $hdsize / 4;
> +    die "maximum root volume size ${maxroot} GiB cannot be greater than ${threshold} GiB (hard disk size / 4)\n"
> +	if $maxroot > $threshold;
> +}
> +
>  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..75696bd 100644
> --- a/proxmox-auto-installer/src/utils.rs
> +++ b/proxmox-auto-installer/src/utils.rs
> @@ -13,6 +13,7 @@ use crate::{
>  };
>  use proxmox_installer_common::{
>      ROOT_PASSWORD_MIN_LENGTH,
> +    disk_checks::{check_maxroot, check_swapsize},
>      options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption, email_validate},
>      setup::{
>          InstallBtrfsOption, InstallConfig, InstallFirstBootSetup, InstallRootPassword,
> @@ -396,6 +397,21 @@ pub fn verify_disks_settings(answer: &Answer) -> Result<()> {
>              );
>          }
>      }
> +
> +    if let answer::FsOptions::LVM(lvm) = &answer.disks.fs_options {
> +        if let Some((swapsize, hdsize)) = lvm.swapsize.zip(lvm.hdsize) {
> +            if let Err(err) = check_swapsize(swapsize, hdsize) {
> +                bail!(err);
> +            }
> +        }
> +
> +        if let Some((maxroot, hdsize)) = lvm.maxroot.zip(lvm.hdsize) {
> +            if let Err(err) = check_maxroot(maxroot, hdsize) {
> +                bail!(err);
> +            }
> +        }
> +    }
> +
>      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..bab12e6
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_maxroot_greater_than_maximum.json
> @@ -0,0 +1,3 @@
> +{
> +  "error": "Maximum root volume size 8.01 GiB cannot be greater than 8 GiB (hard disk size / 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..aa4f7fe
> --- /dev/null
> +++ b/proxmox-auto-installer/tests/resources/parse_answer_fail/lvm_swapsize_greater_than_hdsize.json
> @@ -0,0 +1,3 @@
> +{
> +  "error": "Swap size 4.01 GiB cannot be greater than 4 GiB (hard disk size / 8)"
> +}
> 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..ffe16dc
> --- /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 = 4.01
> +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 d535837..77104ae 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::Disk;
> +use crate::options::{Disk, LvmBootdiskOptions};
>  use crate::setup::BootType;
>
>  /// Checks a list of disks for duplicate entries, using their index as key.
> @@ -49,6 +49,56 @@ pub fn check_disks_4kn_legacy_boot(boot_type: BootType, disks: &[Disk]) -> Resul
>      Ok(())
>  }
>
> +/// Checks whether the configured swap size exceeds the allowed threshold.
> +///
> +/// # Arguments
> +///
> +/// * `swapsize` - The size of the swap in GiB
> +/// * `hdsize` - The total size of the hard disk in GiB
> +pub fn check_swapsize(swapsize: f64, hdsize: f64) -> Result<(), String> {
> +    let threshold = hdsize / 8.0;
> +    if swapsize > threshold {
> +        return Err(format!(
> +            "Swap size {swapsize} GiB cannot be greater than {threshold} GiB (hard disk size / 8)",
> +        ));
> +    }
> +    Ok(())
> +}
> +
> +/// Checks whether the configured root volume size exceeds the allowed threshold.
> +///
> +/// # Arguments
> +///
> +/// * `maxroot` - The size of the root volume in GiB
> +/// * `hdsize` - The total size of the hard disk in GiB
> +pub fn check_maxroot(maxroot: f64, hdsize: f64) -> Result<(), String> {
> +    let threshold = hdsize / 4.0;
> +    if maxroot > threshold {
> +        return Err(format!(
> +            "Maximum root volume size {maxroot} GiB cannot be greater than {threshold} GiB (hard disk size / 4)",
> +        ));
> +    }
> +    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<(), String> {
> +    if let Some(swap_size) = bootdisk_opts.swap_size {
> +        check_swapsize(swap_size, bootdisk_opts.total_size)?;
> +    }
> +
> +    if let Some(max_root_size) = bootdisk_opts.max_root_size {
> +        check_maxroot(max_root_size, bootdisk_opts.total_size)?;
> +    }
> +
> +    Ok(())
> +}
> +
>  #[cfg(test)]
>  mod tests {
>      use crate::options::{BtrfsRaidLevel, ZfsRaidLevel};
> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
> index e87b040..b94cf38 100644
> --- a/proxmox-tui-installer/src/views/bootdisk.rs
> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
> @@ -17,7 +17,9 @@ use crate::InstallerState;
>  use crate::options::FS_TYPES;
>
>  use proxmox_installer_common::{
> -    disk_checks::{check_disks_4kn_legacy_boot, check_for_duplicate_disks},
> +    disk_checks::{
> +        check_disks_4kn_legacy_boot, check_for_duplicate_disks, check_lvm_bootdisk_opts,
> +    },
>      options::{
>          AdvancedBootdiskOptions, BTRFS_COMPRESS_OPTIONS, BootdiskOptions, BtrfsBootdiskOptions,
>          Disk, FsType, LvmBootdiskOptions, ZFS_CHECKSUM_OPTIONS, ZFS_COMPRESS_OPTIONS,
> @@ -261,6 +263,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,



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

Re: [pve-devel] [RFC PATCH pve-installer v2 6/6] common: add checks for valid subnet mask and IPv4 address within subnet

[Michael Köppl]

On 5/6/25 11:21, Christoph Heiss wrote:
> Apart from the one inline comment, looks good to me.
> 
> IMO it's not too restrictive, since IPv4 network/broadcast addresses
> shouldn't really be used (and probably introduce subtle breakages)
> outside of /31 prefixes anyway.
> 
> On Tue Apr 29, 2025 at 4:09 PM CEST, Michael Köppl wrote:
>> Add checks for valid subnet mask (greater than /0 and at most /32 for
>> IPv4). In addition, check if the address entered by the user is valid
>> within the given subnet, i.e. not a network address or broadcast
>> address. /31 is considered an exception in accordance with RFC3021 [0],
>> considering any of the 2 available addresses to be valid host addresses.
>>
>> [0] https://datatracker.ietf.org/doc/html/rfc3021
>>
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>> [..]
>> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
>> index 1fe6a74..fea98db 100644
>> --- a/proxmox-installer-common/src/utils.rs
>> +++ b/proxmox-installer-common/src/utils.rs
>> [..]
>> @@ -133,6 +137,43 @@ fn mask_limit(addr: &IpAddr) -> usize {
>>      if addr.is_ipv4() { 32 } else { 128 }
>>  }
>>
>> +fn check_mask_limit(addr: &IpAddr, mask: usize) -> Result<(), CidrAddressParseError> {
>> +    return if mask > mask_limit(&addr) {
>> +        Err(CidrAddressParseError::InvalidMask(
>> +            "mask cannot be greater than 32".into(),
> 
> s/32/mask_limit(&addr)/g

Will update this bit when I send a v3 including a better solution for
the maxroot check. Thanks!

> 
>> +        ))
>> +    } else {
>> +        Ok(())
>> +    };
>> +}



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

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

[Michael Köppl]

On 5/6/25 13:48, Christoph Heiss wrote:
> After testing this change and thinking about the maxroot change again,
> $hdsize / 4 doesn't really make sense. E.g. for an (unrealistically
> small, but still) disk of 8 GiB; if its unset, pve-root will be ~6.5 GiB
> in size, with the limit of 2 GiB, the installation fails due to
> ENOSPACE.
> 
> The default calculations try really hard to make installations possible
> even on small disks, in Proxmox/Install.pm:create_lvm_volumes()
> 
> So I'm not sure if we really should restrict it that much, or rather
> relax it in the documentation.

Thought a bit about this. I agree that the sanity check should not
entirely stop users from creating setups that would work. An alternative
approach might be not to enforce the maxroot limit of $hdsize / 4 for
smaller disks. create_lvm_volumes() considers 12 GiB to be small, if I'm
not mistaken, and basically does a best-effort install. I'm not sure if
I like setting such an arbitrary limit for a sanity check, though. It
makes the sanity check intransparent if users cannot configure a root
volume size greater than 4 GiB if their hdsize is 16 GiB, but at the
same time are free to set it to 6 GiB if the hdsize is 8 GiB. What do
you think?

Another alternative might be to simply display a warning that doesn't
stop users from installing, i.e. "Recommended maximum root volume size
is hdsize / 4".

In both cases the documentation should be updated to reflect that.


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

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

[Michael Köppl]

Ping, still applies

On 5/9/25 13:07, Michael Köppl wrote:
> On 5/6/25 13:48, Christoph Heiss wrote:
>> After testing this change and thinking about the maxroot change again,
>> $hdsize / 4 doesn't really make sense. E.g. for an (unrealistically
>> small, but still) disk of 8 GiB; if its unset, pve-root will be ~6.5 GiB
>> in size, with the limit of 2 GiB, the installation fails due to
>> ENOSPACE.
>>
>> The default calculations try really hard to make installations possible
>> even on small disks, in Proxmox/Install.pm:create_lvm_volumes()
>>
>> So I'm not sure if we really should restrict it that much, or rather
>> relax it in the documentation.
> 
> Thought a bit about this. I agree that the sanity check should not
> entirely stop users from creating setups that would work. An alternative
> approach might be not to enforce the maxroot limit of $hdsize / 4 for
> smaller disks. create_lvm_volumes() considers 12 GiB to be small, if I'm
> not mistaken, and basically does a best-effort install. I'm not sure if
> I like setting such an arbitrary limit for a sanity check, though. It
> makes the sanity check intransparent if users cannot configure a root
> volume size greater than 4 GiB if their hdsize is 16 GiB, but at the
> same time are free to set it to 6 GiB if the hdsize is 8 GiB. What do
> you think?
> 
> Another alternative might be to simply display a warning that doesn't
> stop users from installing, i.e. "Recommended maximum root volume size
> is hdsize / 4".
> 
> In both cases the documentation should be updated to reflect that.
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



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