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
