Re: [pve-devel] [PATCH pve-installer v3 2/7] move RAID setup checks to RAID level enum implementations

["Christoph Heiss" <c.heiss@proxmox.com>]

On Thu Jun 26, 2025 at 5:11 PM CEST, Michael Köppl wrote:
[..]
> 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
[..]
>  #[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());
> +        }
>      }

These unit tests should be moved to `proxmox_installer_common::options`,
if the implementation of these methods also resides there.

>  }
> 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> {

Maybe rename this to something more expressive, e.g.
check_raid_disks_setup()?

check_disks() by itself is a rather "opaque" method name and would (at
least to me, if I didn't know the implementation) suggests that the
actual disks are checked, not just the RAID configuration and sizes.

> +        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> {

^ Same here as above.

> +        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(())
> +    }
>  }


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