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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 08/06/2022 um 17:34 schrieb Stefan Hrdlicka:
> It is possible to set the number of spares and the size of
> data stripes via draidspares & dreaddata parameters.
> 

looks functional but din't test it, some api design suggestions and small style nits
inline.

> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  PVE/API2/Disks/ZFS.pm | 44 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index eeb9f48..571bc90 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -299,12 +299,28 @@ __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',
> +		],
>  	    },
>  	    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 => 'The number of data devices per redundancy group. (dRAID)',
> +	    },
> +	    draidspares => {
> +		type => 'integer',
> +		minimum => 0,
> +		optional => 1,
> +		description => 'Number of dRAID spares.',
> +	    },

I'd like above two to be a single format string parameter to avoid bloating the API too much
with specialized/context-dependent params.

my $draid_config_format = {
    spares => {
        type => 'integer',
        minimum => 0,
        description => 'Number of dRAID spares.',
    },
    data => {
        ....
    },
};

(note that I made the format options non-optional by design).

'draid-config' => {
    type => 'string',
    format => $draid_config_format,
    optional => 1,
}

could be also more generically named than draid specific, but I don't see that
many settings in the same spirit still coming in the future.
FWIW, we could also deprecate raidlevel and use a new "raid" named parameter with "type" + the
above config keys as parameter, but that's creeping up the scope of this a bit too much, can
also be something for a next major version.

>  	    ashift => {
>  		type => 'integer',
>  		minimum => 9,
> @@ -339,6 +355,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 +372,9 @@ __PACKAGE__->register_method ({
>  	    raidz => 3,
>  	    raidz2 => 4,
>  	    raidz3 => 5,
> +	    draid => 3,
> +	    draid2 => 4,
> +	    draid3 => 5,
>  	};
>  
>  	# sanity checks
> @@ -366,6 +387,22 @@ __PACKAGE__->register_method ({
>  	die "$raidlevel needs at least $mindisks->{$raidlevel} disks\n"
>  	    if $numdisks < $mindisks->{$raidlevel};
>  
> +	# draid checks
> +	if ($raidlevel =~ m/^draid/) {
> +	    # bare minimum 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;
> +	} else {
> +	    die "draidspares and/or draiddata set without using dRAID"
> +		if ($draid_spares or $draid_data);

style nit, parenthesis are not required for such simple post-if and we
more commonly use `||` over `or`

> +	}
> +
>  	my $code = sub {
>  	    for my $dev (@$devs) {
>  		PVE::Diskmanage::assert_disk_unused($dev);
> @@ -402,6 +439,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;
>  	    }