On 3/18/25 17:11, Friedrich Weber wrote:
> 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 <linuxman...@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].
We parse it with `(\S+)` in iscsi_session_list(). But since it looks
like it can only be ASCII [0][1], limiting it to `[0-9]` should be better.
>
>> + 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)?
After talking off-list about this, we do want to fall back to the
tcp_ping if the session is not logged in. For discovery no session is
needed. So even without a login, it might still be reachable via ping
and a discovery possible.
>> + }
>> + }
>> + # 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
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
I think with Friedrich's suggested changes it should to be good to merge.
[0] https://linux.die.net/man/8/iscsiadm
[1] https://www.kernel.org/doc/Documentation/filesystems/sysfs.txt
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
