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
