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

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

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