[pve-devel] [PATCH storage v4] fix #957 iscsi: improve iscsi_test_portal logic

[Friedrich Weber]

Hi, thanks for the new version! I think this is shaping up nicely. Some
comments inline below, but only minor ones. So it may make sense to wait
a couple of days for additional comments from others before sending a
new version. I'll also run a few more tests this week and report back.

> don't check tcp connection directly if there are already
> sessions.
> 
> pvestatd is calling check_connection every 10 seconds.
> This check produces a lot of noise at the iscsi server logging.
> 
> Signed-off-by: Victor Seva <linuxmaniac at torreviejawireless.org>
> ---
> 
> changes since v3:
> 
> * iscsi_test_session():
>   read /sys/class/iscsi_session/session${sid}/state instead of
>   $ISCSIADM call
> 
> * iscsi_test_portal():
>   add optional parameters target and cache. If target is defined,
>   used it to check session status instead of check tcp port
> 
> * iscsi_discovery():
>   add optional parameters target and cache and pass it to
>   iscsi_test_portal call
> 
> * iscsi_login():
>   add optional parameter cache and pass it to iscsi_discovery call
> 
> * activate_storage():
>   pass cache parameter to iscsi_login call
> 
> * check_connection():
>   add target and cache parameters to iscsi_test_portal call  

Thank you for adding these!

> 
>  src/PVE/Storage.pm             |  2 +-
>  src/PVE/Storage/ISCSIPlugin.pm | 41 ++++++++++++++++++++++++++--------
>  2 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 3b4f041..c5d4ff8 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1479,7 +1479,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(undef, [ $portal ]);
>  }

On first sight I wanted to suggest to keep $portals as the first
argument and add $target and $cache as optional second+third argument,
but now I see that `$target, $portals, $cache` allows us to have similar
argument orderings for `iscsi_discovery`, `iscsi_login` and
`iscsi_test_portal`, so I think this is fine.

>  
>  sub storage_default_format {
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm<
> index eb70453..c7bacea 100644
> --- a/src/PVE/Storage/ISCSIPlugin.pm
> +++ b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -58,9 +58,32 @@ sub iscsi_session_list {
>      return $res;
>  }
>  
> -sub iscsi_test_portal {
> -    my ($portal) = @_;
> +sub iscsi_test_session {
> +    my ($sid) = @_;
>  
> +    if ($sid !~ m/^\d+$/) {

It's safer to change \d to [0-9], see [1].

> +	die "session_id: '$sid' is not a number\n";
> +    }
> +    my $state = file_read_firstline("/sys/class/iscsi_session/session${sid}/state");
> +    if ($state eq 'LOGGED_IN') {
> +	return 1;
> +    }
> +    return 0;
> +}

Can be shortened to

	return $state eq 'LOGGED_IN';

but it would be better to also check for definedness because
file_read_firstline returns undef if the file does not exist, which is
unlikely but not impossible:

	return defined($state) && $state eq 'LOGGED_IN';

> +
> +sub iscsi_test_portal {
> +    my ($target, $portal, $cache) = @_;
> +    $cache //= {};
> +
> +    if (defined($target)) {

Right, good catch, the $target == undef case is needed to allow initial
discovery.

> +	# check session state instead if available
> +	my $sessions = iscsi_session($cache, $target);
> +	for my $session ($sessions->@*) {
> +	    next if $session->{portal} ne $portal;
> +	    return iscsi_test_session($session->{session_id});

So if we have a session but it is not LOGGED_IN, we return 0.
I know this is what I suggested in my v3 comment, but now I'm not so
sure anymore. Couldn't it be the case that the session is broken for
some reason, but discovery would still works? In such a case, we would
now consider the portal offline. We could instead fall back to a TCP ping:
	
	my $state = iscsi_test_session($session->{session_id});
	return $state if $state;

Any opinions (from others)?

> +	}
> +    }
> +    # check portal via tcp
>      my ($server, $port) = PVE::Tools::parse_host_and_port($portal);
>      return 0 if !$server;
>      return PVE::Network::tcp_ping($server, $port || 3260, 2);
> @@ -97,13 +120,13 @@ sub iscsi_portals {
>  }
>  
>  sub iscsi_discovery {
> -    my ($portals) = @_;
> +    my ($target, $portals, $cache) = @_;

There is already a $target inside the deeply-nested `if`. To avoid
confusion, it would be better to avoid variable shadowing. Probably the
top-level one could be renamed to $discovery_target or $target_in or
similar.

>  
>      assert_iscsi_support();
>  
>      my $res = {};
>      for my $portal ($portals->@*) {
> -	next if !iscsi_test_portal($portal); # fixme: raise exception here?
> +	next if !iscsi_test_portal($target, $portal, $cache); # fixme: raise exception here?
>  
>  	my $cmd = [$ISCSIADM, '--mode', 'discovery', '--type', 'sendtargets', '--portal', $portal];
>  	eval {
> @@ -127,11 +150,11 @@ sub iscsi_discovery {
>  }
>  
>  sub iscsi_login {
> -    my ($target, $portals) = @_;
> +    my ($target, $portals, $cache) = @_;
>  
>      assert_iscsi_support();
>  
> -    eval { iscsi_discovery($portals); };
> +    eval { iscsi_discovery($target, $portals, $cache); };
>      warn $@ if $@;
>  
>      # Disable retries to avoid blocking pvestatd for too long, next iteration will retry anyway
> @@ -445,7 +468,7 @@ sub activate_storage {
>      }
>  
>      if ($do_login) {
> -	eval { iscsi_login($scfg->{target}, $portals); };
> +	eval { iscsi_login($scfg->{target}, $portals, $cache); };
>  	warn $@ if $@;
>      } else {
>  	# make sure we get all devices
> @@ -559,11 +582,11 @@ sub activate_volume {
>  
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
> -
> +    my $cache = {};
>      my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
>  
>      for my $portal (@$portals) {
> -	my $result = iscsi_test_portal($portal);
> +	my $result = iscsi_test_portal($scfg->{target}, $portal, $cache);
>  	return $result if $result;
>      }
>  
> -- 
> 2.43.0
> 
> 


[1] https://pve.proxmox.com/wiki/Perl_Style_Guide#Matching_Digits_Fallacy