Re: [pve-devel] [PATCH pve-storage 1/3] fix #3967: enable ZFS dRAID creation via API

[Dominik Csapak <d.csapak@proxmox.com>]

some comments inline

On 6/2/22 13:22, Stefan Hrdlicka wrote:
> It is possible to set the number of spares and the size of
> data stripes via draidspares & dreaddata parameters.
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>   PVE/API2/Disks/ZFS.pm | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index eeb9f48..63946d2 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -299,12 +299,27 @@ __PACKAGE__->register_method ({
>   	    raidlevel => {
>   		type => 'string',
>   		description => 'The RAID level to use.',
> -		enum => ['single', 'mirror', 'raid10', 'raidz', 'raidz2', 'raidz3'],
> +		enum => ['single', 'mirror',
> +		    'raid10', 'raidz', 'raidz2', 'raidz3',
> +		    'draid', 'draid2', 'draid3',
> +		],

i'd rather have the first options also on a single line, so

enum => [
     'single', 'mirror',
     ...
],

makes it even easier to read

>   	    },
>   	    devices => {
>   		type => 'string', format => 'string-list',
>   		description => 'The block devices you want to create the zpool on.',
>   	    },
> +	    draiddata => {
> +		type => 'integer',
> +		minimum => 1,
> +		optional => 1,
> +		description => 'Number of dRAID data stripes.',

isn't that the 'number of data devices per redundancy group' ?
it sounds here like its the number of groups, not the number
of disks per group

> +	    },
> +	    draidspares => {
> +		type => 'integer',
> +		minimum => 0,
> +		optional => 1,
> +		description => 'Number of dRAID spares.',
> +	    },
>   	    ashift => {
>   		type => 'integer',
>   		minimum => 9,
> @@ -339,6 +354,8 @@ __PACKAGE__->register_method ({
>   	my $devs = [PVE::Tools::split_list($param->{devices})];
>   	my $raidlevel = $param->{raidlevel};
>   	my $compression = $param->{compression} // 'on';
> +	my $draid_data = $param->{draiddata};
> +	my $draid_spares = $param->{draidspares};
>   
>   	for my $dev (@$devs) {
>   	    $dev = PVE::Diskmanage::verify_blockdev_path($dev);
> @@ -354,6 +371,9 @@ __PACKAGE__->register_method ({
>   	    raidz => 3,
>   	    raidz2 => 4,
>   	    raidz3 => 5,
> +	    draid => 3,
> +	    draid2 => 4,
> +	    draid3 => 5,
>   	};
>   
>   	# sanity checks
> @@ -366,6 +386,19 @@ __PACKAGE__->register_method ({
>   	die "$raidlevel needs at least $mindisks->{$raidlevel} disks\n"
>   	    if $numdisks < $mindisks->{$raidlevel};
>   
> +	# draid checks
> +	if ($raidlevel =~ m/^draid/) {
> +	    # bare minium would be two drives:
> +	    # one parity & one data drive this code doesn't allow that because
> +	    # it makes no sense, at least one spare disk should be used
> +	    my $draidmin = $mindisks->{$raidlevel} - 2;
> +	    $draidmin += $draid_data if $draid_data;
> +	    $draidmin += $draid_spares if $draid_spares;
> +
> +	    die "At least $draidmin disks needed for current dRAID config\n"
> +		if $numdisks < $draidmin;
> +	}
> +

maybe check here in the else path if the draiddata/spares were set and
throw an error? since they don't make any sense
(i know the gui does not show them, but an api user might misunderstand)

>   	my $code = sub {
>   	    for my $dev (@$devs) {
>   		PVE::Diskmanage::assert_disk_unused($dev);
> @@ -402,6 +435,11 @@ __PACKAGE__->register_method ({
>   		}
>   	    } elsif ($raidlevel eq 'single') {
>   		push @$cmd, $devs->[0];
> +	    } elsif ($raidlevel =~ m/^draid/) {
> +		my $draid_cmd = $raidlevel;
> +		$draid_cmd .= ":${draid_data}d" if $draid_data;
> +		$draid_cmd .= ":${draid_spares}s" if $draid_spares;
> +		push @$cmd, $draid_cmd, @$devs;
>   	    } else {
>   		push @$cmd, $raidlevel, @$devs;
>   	    }