* Re: [pve-devel] [PATCH v2 storage 1/1] fix #254: iscsi: allow to configure multiple portals
[not found] ` <20231015183216.86220-2-ykonotopov@gnome.org>
@ 2023-10-16 13:58 ` Dominik Csapak
[not found] ` <caccd18c-120d-4734-9d28-f4541f322d1e@gnome.org>
0 siblings, 1 reply; 2+ messages in thread
From: Dominik Csapak @ 2023-10-16 13:58 UTC (permalink / raw)
To: ykonotopov, pve-devel
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 <ykonotopov@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'
> Signed-off-by: Yuri Konotopov <ykonotopov@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?
> + 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 {
^ permalink raw reply [flat|nested] 2+ messages in thread