Re: [pve-devel] [PATCH storage 1/1] iscsi: allow to configure multiple portals

[Fiona Ebner <f.ebner@proxmox.com>]

Hi,
thank you for the contribution! Please prepend the commit title with
"fix #254: ".

Am 16.08.23 um 13:56 schrieb ykonotopov@gnome.org:
> From: Yuri Konotopov <ykonotopov@gnome.org>
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=254

To accept contributions, we need your Signed-off-by trailer here and you
need to agree to the Harmony CLA (assuming you haven't sent it to us
already):
https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright

> ---
>  PVE/Storage/ISCSIPlugin.pm | 44 +++++++++++++++++++++++---------------
>  PVE/Storage/Plugin.pm      | 18 ++++++++++++++++
>  2 files changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm
> index a79fcb0..e056393 100644
> --- a/PVE/Storage/ISCSIPlugin.pm
> +++ b/PVE/Storage/ISCSIPlugin.pm
> @@ -69,24 +69,30 @@ sub iscsi_test_portal {
>  }
>  
>  sub iscsi_discovery {
> -    my ($portal) = @_;
> +    my @portals = @{$_[0]};

Style nit: Usually we'd still write this as

my ($portals) = @_;

and then use $portals->@* whenever we need to access the array below.
Like that it's nicer to extend if a new parameter is added.

>  
>      check_iscsi_support ();
>  
>      my $res = {};
> -    return $res if !iscsi_test_portal($portal); # fixme: raise exception here?
> +    foreach my $portal (@portals)
> +    {

Style nit: We use 'for' instead of 'foreach' for new code and the
opening bracket should be on the same line

> +	next if !iscsi_test_portal($portal); # fixme: raise exception here?
>  
> -    my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
> -    run_command($cmd, outfunc => sub {
> -	my $line = shift;
> +	my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
> +	run_command($cmd, outfunc => sub {
> +	    my $line = shift;
>  
> -	if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
> -	    my $portal = $1;
> -	    my $target = $2;
> -	    # one target can have more than one portal (multipath).
> -	    push @{$res->{$target}}, $portal;
> -	}
> -    });
> +	    if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
> +		my $portal = $1;
> +		my $target = $2;
> +		# one target can have more than one portal (multipath).
> +		push @{$res->{$target}}, $portal;
> +	    }
> +	});
> +
> +        # In case of multipath we want to exit on any portal available> +        last;
> +    }
>  
>      return $res;
>  }
> @@ -96,7 +102,7 @@ sub iscsi_login {
>  
>      check_iscsi_support();
>  
> -    eval { iscsi_discovery($portal_in); };
> +    eval { iscsi_discovery(PVE::Storage::Plugin::get_portals($portal_in)); };
>      warn $@ if $@;
>  
>      run_command([$ISCSIADM, '--mode', 'node', '--targetname',  $target, '--login']);
> @@ -245,8 +251,8 @@ sub properties {
>  	    type => 'string',
>  	},
>  	portal => {
> -	    description => "iSCSI portal (IP or DNS name with optional port).",
> -	    type => 'string', format => 'pve-storage-portal-dns',
> +	    description => "iSCSI portal (IP or DNS name with optional port). Multiple portals can be separated by comma (for multipath).",

Since separation for lists is (mostly) consistent in our API, you could
also just write:
"For multipath, multiple portals can be specified."

> +	    type => 'string', format => 'pve-storage-portals-dns',

Please note that for lists, you can just use the existing format and
append "-list" in our schemas, i.e. using 'pve-storage-portal-dns-list'
should give you what you want already and you don't need to register a
new format.

>  	},
>      };
>  }
> @@ -403,8 +409,12 @@ sub deactivate_storage {
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
>  
> -    my $portal = $scfg->{portal};
> -    return iscsi_test_portal($portal);
> +    foreach my $portal (@{PVE::Storage::Plugin::get_portals($scfg->{portal})}) {
> +	my $result = iscsi_test_portal($portal);
> +	return $result if $result;
> +    }
> +
> +    return 0;
>  }
>  
>  sub volume_resize {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index c323085..ee69b14 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -205,6 +205,13 @@ sub content_hash_to_string {
>      return join(',', @cta);
>  }
>  
> +sub get_portals {
> +    my $portal_cfg = shift;
> +    my $portals = [split(',', $portal_cfg)];
> +    map { s/^\s+|\s+$//g; } @{$portals};

You can use the PVE::Tools::split_list() helper instead of this one.

> +    return $portals;
> +}
> +
>  sub valid_content_types {
>      my ($type) = @_;
>  
> @@ -301,6 +308,17 @@ sub verify_portal_dns {
>      return $portal;
>  }
>  
> +PVE::JSONSchema::register_format('pve-storage-portals-dns', \&verify_portals_dns);
> +sub verify_portals_dns {
> +    my ($portal_in, $noerr) = @_;
> +
> +    foreach my $portal (@{get_portals($portal_in)}) {
> +	verify_portal_dns($portal, $noerr);
> +    }
> +
> +    return $portal_in;
> +}
> +
>  PVE::JSONSchema::register_format('pve-storage-content', \&verify_content);
>  sub verify_content {
>      my ($ct, $noerr) = @_;

Best Regards,
Fiona