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

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

On 7/7/25 13:47, Christoph Heiss wrote:
>> +
>> +        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.

First of all, thanks for having a look at this! True, I forgot to move
the tests as well. Will move them for v4. Right now, both sets of unit
tests (for proxmox_installer_common::options and
proxmox_installer_common::disk_checks) use the dummy_disks and
dummy_disk helpers. I think simply copying the implementation of these
over to the options.rs tests is fine since the implementation is not
exactly complicated and it seems a bit overkill to introduce some sort
of test utils for this. What do you think?
> 
>>  }
>> 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.

I agree that check_disks() suggests that the function does
more/something different than it actually does. Thanks for your
suggestion, I'll adapt it to check_raid_disks_setup() in v4.

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