[PATCH v2 storage 1/1] fix #254: iscsi: allow to configure multiple portals

Yuri Konotopov ykonotopov at gnome.org
Mon Oct 16 18:08:40 CEST 2023


16.10.2023 17:58, Dominik Csapak пишет:
> hi,

Hi, Dominik!

Thanks for your review. I will try to address all issues.

Some comments are below.

>
> a few high level comments:
>
> first:
>
> is the iscsi.cfg really necessary? couldn't we just lookup on demand 
> like we do now?

Until now I thought it is. For some reason I thought that open-iscsi 
node list is not

persistent between reboots. However it's indeed can be done without 
custom cache

file. I will rewrite that part. Thanks!


> second:
>
> i would favor an approach that uses an 'array' instead of the '-list' 
> types.
> i'm not completely sure if one can just have that as a drop in 
> replacement
> with old configs/api calls still working. if not, we could introduce
> a new 'portals' api parameter/storage config that is an array which could
> take precedence over the old 'portal' one. (that we could drop with the
> next major release) in that case we could even automatically convert from
> portal -> portals if we detect that in the api update

I'm not familiar with Proxmox formats but will try to investigate it.


>
> and a few (basic) comments inline:
>
> On 10/15/23 20:32, ykonotopov at gnome.org wrote:
>> From: Yuri Konotopov <ykonotopov at gnome.org>
>>
>
> it would be great to have a real commit message here. best would be a 
> short description
> of what the patch wants to achieve, and maybe some more tricky 
> implementation detail
> that is not obvious (most of what you wrote in the cover letter would 
> be fine, sans
> the changelog + mention of 'Here is v2 patch'

I will add it. Git's send-email interface is not something I work often :-)


>
>> Signed-off-by: Yuri Konotopov <ykonotopov at gnome.org>
>> ---
>>   PVE/API2/Storage/Scan.pm   |   2 +-
>>   PVE/Storage.pm             |  10 +-
>>   PVE/Storage/ISCSIPlugin.pm | 207 ++++++++++++++++++++++++++++++++-----
>>   3 files changed, 187 insertions(+), 32 deletions(-)
>>
>> diff --git a/PVE/API2/Storage/Scan.pm b/PVE/API2/Storage/Scan.pm
>> index d7a8743..1f9773c 100644
>> --- a/PVE/API2/Storage/Scan.pm
>> +++ b/PVE/API2/Storage/Scan.pm
>> @@ -305,7 +305,7 @@ __PACKAGE__->register_method({
>>           node => get_standard_option('pve-node'),
>>           portal => {
>>           description => "The iSCSI portal (IP or DNS name with 
>> optional port).",
>> -        type => 'string', format => 'pve-storage-portal-dns',
>> +        type => 'string', format => 'pve-storage-portal-dns-list',
>>           },
>>       },
>>       },
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index cec3996..0043507 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -1428,12 +1428,14 @@ sub resolv_portal {
>>   sub scan_iscsi {
>>       my ($portal_in) = @_;
>>   -    my $portal;
>> -    if (!($portal = resolv_portal($portal_in))) {
>> -    die "unable to parse/resolve portal address '${portal_in}'\n";
>> +    my @portals = PVE::Tools::split_list($portal_in);
>
> not a big deal, but we usually pack that into a reference immediately:
>
> my $portals = [ <...>::split_list(...) ];
>
> then you can do
> for my $portal (@$portals)
> or
> for my $portal ($portals->@*)
>
> and don't have to create a reference later on when passing to 
> iscsi_discovery
>
>> +    for my $portal (@portals) {
>> +    if (!resolv_portal($portal)) {
>> +        die "unable to parse/resolve portal address '${portal}'\n";
>> +    }
>>       }
>>   -    return PVE::Storage::ISCSIPlugin::iscsi_discovery($portal);
>> +    return PVE::Storage::ISCSIPlugin::iscsi_discovery(\@portals);
>>   }
>>     sub storage_default_format {
>> diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm
>> index a79fcb0..6f4309f 100644
>> --- a/PVE/Storage/ISCSIPlugin.pm
>> +++ b/PVE/Storage/ISCSIPlugin.pm
>> @@ -18,6 +18,65 @@ use base qw(PVE::Storage::Plugin);
>>   my $ISCSIADM = '/usr/bin/iscsiadm';
>>   $ISCSIADM = undef if ! -X $ISCSIADM;
>>   +my $iscsi_cfg = "/etc/pve/iscsi.cfg";
>> +
>> +sub read_config {
>> +    my ($filename, $raw) = @_;
>> +
>> +    my $cfg = {};
>> +
>> +    return $cfg if ! -f $iscsi_cfg;
>> +
>> +    my $content = PVE::Tools::file_get_contents($iscsi_cfg);
>> +    return $cfg if !defined($content);
>> +
>> +    my @lines = split /\n/, $content;
>> +
>> +    my $target;
>> +
>> +    for my $line (@lines) {
>> +    $line =~ s/#.*$//;
>> +    $line =~ s/^\s+//;
>> +    $line =~ s/^;.*$//;
>> +    $line =~ s/\s+$//;
>> +    next if !$line;
>> +
>> +    $target = $1 if $line =~ m/^\[(\S+)\]$/;
>> +    if (!$target) {
>> +        warn "no target - skip: $line\n";
>> +        next;
>> +    }
>> +
>> +    if (!defined($cfg->{$target})) {
>> +        $cfg->{$target} = [];
>> +    }
>> +
>> +    if ($line =~ m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)$/) {
>> +        push @{$cfg->{$target}}, $1;
>> +    }
>> +    }
>> +
>> +    return $cfg;
>> +}
>> +
>> +sub write_config {
>> +    my ($cfg) = @_;
>> +
>> +    my $out = '';
>> +
>> +    for my $target (sort keys %$cfg) {
>> +    $out .= "[$target]\n";
>> +    for my $portal (sort @{$cfg->{$target}}) {
>> +        $out .= "$portal\n";
>> +    }
>> +    $out .= "\n";
>> +    }
>> +
>> +    PVE::Tools::file_set_contents($iscsi_cfg, $out);
>> +
>> +    return;
>> +}
>
> didn't look too closely at read/write_config since i'm conviced that 
> we don't
> actually need this
>
>> +
>>   sub check_iscsi_support {
>>       my $noerr = shift;
>>   @@ -45,11 +104,10 @@ sub iscsi_session_list {
>>       eval {
>>       run_command($cmd, errmsg => 'iscsi session scan failed', 
>> outfunc => sub {
>>           my $line = shift;
>> -
>> -        if ($line =~ m/^tcp:\s+\[(\S+)\]\s+\S+\s+(\S+)(\s+\S+)?\s*$/) {
>> -        my ($session, $target) = ($1, $2);
>> +        if ($line =~ 
>> m/^tcp:\s+\[(\S+)\]\s+((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)(\s+\S+)?\s*$/) 
>> {
>
> at this point, i would love to have an example output line in a 
> comment sot that we can see what
> we want to parse...
>
>> +        my ($session, $portal, $target) = ($1, $2, $3);
>>           # there can be several sessions per target (multipath)
>> -        push @{$res->{$target}}, $session;
>> +        push @{$res->{$target}}, [$session, $portal];
>
> instead of using a list here, you could use a hash again, so that later
> instead of $res->{$target}->[0] you could use $res->{$target}->{session}
> this makes the code more readable (otherwise one always has to 
> lookup/remember what is
> in ->[0])
>
>>           }
>>       });
>>       };
>> @@ -68,42 +126,68 @@ sub iscsi_test_portal {
>>       return PVE::Network::tcp_ping($server, $port || 3260, 2);
>>   }
>>   -sub iscsi_discovery {
>> -    my ($portal) = @_;
>> +sub iscsi_portals {
>> +    my ($target) = @_;
>>         check_iscsi_support ();
>>   -    my $res = {};
>> -    return $res if !iscsi_test_portal($portal); # fixme: raise 
>> exception here?
>> -
>> -    my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 
>> 'sendtargets', '--portal', $portal];
>> +    my $res = [];
>> +    my $cmd = [$ISCSIADM, '--mode', 'node'];
>>       run_command($cmd, outfunc => sub {
>>       my $line = shift;
>>         if ($line =~ 
>> m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
>
> is this regex...
>
>> -        my $portal = $1;
>> -        my $target = $2;
>> -        # one target can have more than one portal (multipath).
>> -        push @{$res->{$target}}, $portal;
>> +        my ($portal, $portal_target) = ($1, $2);
>> +        if ($portal_target eq $target) {
>> +        push @{$res}, $portal;
>> +        }
>>       }
>>       });
>>         return $res;
>>   }
>>   +sub iscsi_discovery {
>> +    my ($portals) = @_;
>> +
>> +    check_iscsi_support ();
>> +
>> +    my $res = {};
>> +    for my $portal ($portals->@*) {
>> +    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;
>> +
>> +        if ($line =~ 
>> m/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/) {
>
> and this the same? maybe this can be factored out?

Yes, the output of commands is same, but commands are different (one is 
online and another is offline).


>
>> +        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
>
> isn't that line contradictory to the previous comment?
> we gather all portals that is resolved by any (the first?) portal, but 
> no more?

No, there is no contradiction here.

There may be multiple portals for single target, but any portal is 
suitable for

discovery.


> if we just need one, why do we push them into an list in the first place?

It looks like the return value of iscsi_discovery is not used. So we can 
drop

$res completely and regex too.


>
>> +    last;
>> +    }
>> +
>> +    return $res;
>> +}
>> +
>>   sub iscsi_login {
>> -    my ($target, $portal_in) = @_;
>> +    my ($target, $portals) = @_;
>>         check_iscsi_support();
>>   -    eval { iscsi_discovery($portal_in); };
>> +    eval { iscsi_discovery($portals); };
>>       warn $@ if $@;
>>         run_command([$ISCSIADM, '--mode', 'node', '--targetname',  
>> $target, '--login']);
>>   }
>>     sub iscsi_logout {
>> -    my ($target, $portal) = @_;
>> +    my ($target) = @_;
>>         check_iscsi_support();
>>   @@ -133,7 +217,7 @@ sub iscsi_session_rescan {
>>       }
>>         foreach my $session (@$session_list) {
>> -    my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $session, 
>> '--rescan'];
>> +    my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', 
>> $session->@[0], '--rescan'];
>
> didn't know this was valid perl syntax, normally lists are accessed 
> like this:
>
> $session->[0]
>
> (but as i said above, maybe we want to use a hash instead to have
> $session->{session} ?)
>
>>       eval { run_command($cmd, outfunc => sub {}); };
>>       warn $@ if $@;
>>       }
>> @@ -245,8 +329,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). For multipath, multiple portals can be specified.",
>> +        type => 'string', format => 'pve-storage-portal-dns-list',
>>       },
>>       };
>>   }
>> @@ -264,6 +348,33 @@ sub options {
>>     # Storage implementation
>>   +sub on_add_hook {
>> +    my ($class, $storeid, $scfg, %param) = @_;
>> +
>> +    my @portals = PVE::Tools::split_list($scfg->{portal});
>> +    my $target = $scfg->{target};
>> +
>> +    my $portal_cfg = read_config();
>> +    $portal_cfg->{$target} = \@portals;
>> +
>> +    write_config($portal_cfg);
>> +
>> +    return;
>> +}
>> +
>> +sub on_delete_hook {
>> +    my ($class, $storeid, $scfg) = @_;
>> +
>> +    my $portal_cfg = read_config();
>> +    my $target = $scfg->{target};
>> +
>> +    delete $portal_cfg->{$target};
>> +
>> +    write_config($portal_cfg);
>> +
>> +    return;
>> +}
>> +
>>   sub parse_volname {
>>       my ($class, $volname) = @_;
>>   @@ -365,6 +476,21 @@ sub iscsi_session {
>>       return $cache->{iscsi_sessions}->{$target};
>>   }
>>   +sub iscsi_session_portals {
>> +    my ($cache, $target) = @_;
>> +
>> +    my $res = [];
>> +    my $sessions = iscsi_session($cache, $target);
>> +
>> +    if (defined($sessions)) {
>> +    for my $session ($sessions->@*) {
>> +        push @{$res}, $session->@[1];
>> +    }
>> +    }
>
> this could probably be
>
> push @{$res}, $_->[1] for $sessions->@* if defined($sessions);
> or
> my $res = [ map { $_->[1] } (@$sessions // ())];
>
>
> (neither tested though)
>
>> +
>> +    return $res;
>> +}
>> +
>>   sub status {
>>       my ($class, $storeid, $scfg, $cache) = @_;
>>   @@ -379,14 +505,37 @@ sub activate_storage {
>>         return if !check_iscsi_support(1);
>>   -    my $session = iscsi_session($cache, $scfg->{target});
>>   -    if (!defined ($session)) {
>> -    eval { iscsi_login($scfg->{target}, $scfg->{portal}); };
>> +    my $sessions = iscsi_session($cache, $scfg->{target});
>> +    my $portal_cfg = read_config();
>> +
>> +    my @portals = PVE::Tools::split_list($scfg->{portal});
>> +    if (defined($portal_cfg->{$scfg->{target}})) {
>> +    @portals = @{$portal_cfg->{$scfg->{target}}};
>> +    }
>> +
>> +    if (!defined ($sessions)) {
>> +    eval { iscsi_login($scfg->{target}, \@portals); };
>>       warn $@ if $@;
>>       } else {
>> +    my @discovered_portals = @{iscsi_portals($scfg->{target})};
>> +    my @session_portals = @{iscsi_session_portals($cache, 
>> $scfg->{target})};
>
> do i understand this right that you reuse the saved portals only for 
> fresh
> sessions? since here you discover them again if there is already a 
> session?

The iscsi_portrals function is not actually discover but rather 
retrieval of last discovered

portals (during iscsi_discover).

Before I thought that we can extract discovered portals from open-iscsi 
only when we

have active session, but we can do it without sessions too.


>
>> +
>> +    for my $discovered_portal (@discovered_portals) {
>> +        if (!grep(/^\Q$discovered_portal\E$/, @session_portals)) {
>> +        eval { iscsi_login($scfg->{target}, \@discovered_portals); };
>
> isn't that already done via the iscsi_login from above when there is 
> no session?

In case some portals were unavailable during iscsi_login we will not 
have all sessions.

So we need to check that and login again when sessions are missing. 
Otherwise we will

miss multipath.


>
>
>> +        warn $@ if $@;
>> +        last;
>> +        }
>> +    }
>> +
>> +    if(join(",", sort(@discovered_portals)) ne join(",", 
>> sort(@portals))) {
>> +        $portal_cfg->{$scfg->{target}} = \@discovered_portals;
>> +        write_config($portal_cfg);
>> +    }
>> +
>>       # make sure we get all devices
>> -    iscsi_session_rescan($session);
>> +    iscsi_session_rescan($sessions);
>>       }
>>   }
>>   @@ -396,15 +545,19 @@ sub deactivate_storage {
>>       return if !check_iscsi_support(1);
>>         if (defined(iscsi_session($cache, $scfg->{target}))) {
>> -    iscsi_logout($scfg->{target}, $scfg->{portal});
>> +    iscsi_logout($scfg->{target});
>>       }
>>   }
>>     sub check_connection {
>>       my ($class, $storeid, $scfg) = @_;
>>   -    my $portal = $scfg->{portal};
>> -    return iscsi_test_portal($portal);
>> +    for my $portal (PVE::Tools::split_list($scfg->{portal})) {
>> +    my $result = iscsi_test_portal($portal);
>> +    return $result if $result;
>> +    }
>> +
>
> here you don't use the cache config at all...
>
>> +    return 0;
>>   }
>>     sub volume_resize {
>
>

-- 
Best regards, Yuri Konotopov





More information about the pve-devel mailing list