From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 92A9F9D1F8 for ; Wed, 25 Oct 2023 15:45:41 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6E59114D58 for ; Wed, 25 Oct 2023 15:45:41 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 25 Oct 2023 15:45:37 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 7898845F2A; Wed, 25 Oct 2023 15:45:37 +0200 (CEST) Message-ID: Date: Wed, 25 Oct 2023 15:45:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Yuri Konotopov , pve-devel@lists.proxmox.com References: <20231023174508.385682-1-ykonotopov@gnome.org> <20231023174508.385682-2-ykonotopov@gnome.org> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20231023174508.385682-2-ykonotopov@gnome.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.012 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [iscsiplugin.pm, storage.pm] Subject: Re: [pve-devel] [PATCH v5 storage 1/1] fix #254: iscsi: add support for multipath iscsi targets X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Oct 2023 13:45:41 -0000 One nit inline, but that could be fixed up when applying too probably? Also, and that was previously also the case, if the first (or any) portal is not reachable, we get a big warning about iscsiadm --login not being able to login every 10 seconds in pvestatd. It's nothing your patch changes, but now it also can happen when the storage works, making it more unlikely that the admin will check the logs for an error. We could extend the storage status from a simple binary result to an enum/list with e.g. ok/warning/fail and that could be shown in the ui for such cases. (Again, this has nothing directly to do with your patch, i just wanted to mention it, since this change emphasizes the need for it IMO) Otherwise consider this patch: Reviewed-By: Dominik Csapak Tested-By: Dominik Csapak On 10/23/23 19:45, Yuri Konotopov wrote: > With this patch Proxmox now tries to login to all discovered portals in > case some of them are not logged yet. > In case of multipath configuration when initially configured portal is > missing for some reason Proxmox don't lose iscsi storage now and can > succesfully restore iscsi connection between reboots. > > Signed-off-by: Yuri Konotopov > --- > src/PVE/Storage.pm | 2 +- > src/PVE/Storage/ISCSIPlugin.pm | 117 +++++++++++++++++++++++++-------- > 2 files changed, 89 insertions(+), 30 deletions(-) > > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index a4d85e1..ec5f5bd 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -1432,7 +1432,7 @@ sub scan_iscsi { > die "unable to parse/resolve portal address '${portal_in}'\n"; > } > > - return PVE::Storage::ISCSIPlugin::iscsi_discovery($portal); > + return PVE::Storage::ISCSIPlugin::iscsi_discovery([ $portal ]); > } > > sub storage_default_format { > diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm > index a79fcb0..b4ab1dd 100644 > --- a/src/PVE/Storage/ISCSIPlugin.pm > +++ b/src/PVE/Storage/ISCSIPlugin.pm > @@ -18,6 +18,9 @@ use base qw(PVE::Storage::Plugin); > my $ISCSIADM = '/usr/bin/iscsiadm'; > $ISCSIADM = undef if ! -X $ISCSIADM; > > +# Example: 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f > +my $ISCSI_TARGET_RE = qr/^((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s*$/; > + > sub check_iscsi_support { > my $noerr = shift; > > @@ -45,11 +48,12 @@ 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); > + # example: tcp: [1] 192.168.122.252:3260,1 iqn.2003-01.org.linux-iscsi.proxmox-nfs.x8664:sn.00567885ba8f (non-flash) > + if ($line =~ m/^tcp:\s+\[(\S+)\]\s+((?:$IPV4RE|\[$IPV6RE\]):\d+)\,\S+\s+(\S+)\s+\S+?\s*$/) { > + my ($session_id, $portal, $target) = ($1, $2, $3); > # there can be several sessions per target (multipath) > - push @{$res->{$target}}, $session; > + my %session = ( session_id => $session_id, portal => $portal ); > + push @{$res->{$target}}, \%session; nit: since you don't need the hash afterwards anymore, you could inline that with: --- push @${res->{$target}}, { session_id => $session_id, portal => $portal }; --- bot no hard feelings for that, and could potentially be fixed up. > } > }); > }; > @@ -68,42 +72,77 @@ sub iscsi_test_portal { > return PVE::Network::tcp_ping($server, $port || 3260, 2); > } > > -sub iscsi_discovery { > - my ($portal) = @_; > +sub iscsi_portals { > + my ($target, $portal_in) = @_; > > check_iscsi_support (); > > - my $res = {}; > - return $res if !iscsi_test_portal($portal); # fixme: raise exception here? > + my $res = []; > + my $cmd = [$ISCSIADM, '--mode', 'node']; > + eval { > + 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 =~ $ISCSI_TARGET_RE) { > + my ($portal, $portal_target) = ($1, $2); > + if ($portal_target eq $target) { > + push @{$res}, $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; > - } > - }); > + if ($@) { > + warn $@; > + return [ $portal_in ]; > + } > + > + 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]; > + eval { > + run_command($cmd, outfunc => sub { > + my $line = shift; > + > + if ($line =~ $ISCSI_TARGET_RE) { > + my ($portal, $target) = ($1, $2); > + # one target can have more than one portal (multipath) > + # and sendtargets should return all of them in single call > + push @{$res->{$target}}, $portal; > + } > + }); > + }; > + > + # In case of multipath we can stop after receiving targets from any available portal > + last if scalar(keys %$res) > 0; > + } > > 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 +172,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->{session_id}, '--rescan']; > eval { run_command($cmd, outfunc => sub {}); }; > warn $@ if $@; > } > @@ -379,14 +418,28 @@ sub activate_storage { > > return if !check_iscsi_support(1); > > - my $session = iscsi_session($cache, $scfg->{target}); > + my $sessions = iscsi_session($cache, $scfg->{target}); > + my $portals = iscsi_portals($scfg->{target}, $scfg->{portal}); > + my $do_login = !defined($sessions); > > - if (!defined ($session)) { > - eval { iscsi_login($scfg->{target}, $scfg->{portal}); }; > + if (!$do_login) { > + # We should check that sessions for all portals are available > + my $session_portals = [ map { $_->{portal} } (@$sessions) ]; > + > + for my $portal (@$portals) { > + if (!grep(/^\Q$portal\E$/, @$session_portals)) { > + $do_login = 1; > + last; > + } > + } > + } > + > + if ($do_login) { > + eval { iscsi_login($scfg->{target}, $portals); }; > warn $@ if $@; > } else { > # make sure we get all devices > - iscsi_session_rescan($session); > + iscsi_session_rescan($sessions); > } > } > > @@ -396,15 +449,21 @@ 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); > + my $portals = iscsi_portals($scfg->{target}, $scfg->{portal}); > + > + for my $portal (@$portals) { > + my $result = iscsi_test_portal($portal); > + return $result if $result; > + } > + > + return 0; > } > > sub volume_resize {