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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7E2839AF45 for ; Mon, 16 Oct 2023 15:59:26 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5F5F918C39 for ; Mon, 16 Oct 2023 15:58:56 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 16 Oct 2023 15:58:54 +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 0905A417CF; Mon, 16 Oct 2023 15:58:54 +0200 (CEST) Message-ID: <0314d5f0-a08b-41dc-b027-90ff8a44c041@proxmox.com> Date: Mon, 16 Oct 2023 15:58:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: ykonotopov@gnome.org, pve-devel@lists.proxmox.com References: <20231015183216.86220-1-ykonotopov@gnome.org> <20231015183216.86220-2-ykonotopov@gnome.org> From: Dominik Csapak In-Reply-To: <20231015183216.86220-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.011 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 Subject: Re: [pve-devel] [PATCH v2 storage 1/1] fix #254: iscsi: allow to configure multiple portals 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: Mon, 16 Oct 2023 13:59:26 -0000 hi, a few high level comments: first: is the iscsi.cfg really necessary? couldn't we just lookup on demand like we do now? *if* we want to cache that list, we should only do this on a per node level, since there is no guarantee that this is the same for all nodes. (e.g. use /var/run/..../scsi.cache) but really i'm in favor of dropping that altogether (except you can provide a good argument why it's necessary) this would remove a lot of code from this patch 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 and a few (basic) comments inline: On 10/15/23 20:32, ykonotopov@gnome.org wrote: > From: Yuri Konotopov > 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' > Signed-off-by: Yuri Konotopov > --- > 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? > + 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? if we just need one, why do we push them into an list in the first place? > + 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? > + > + 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? > + 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 {