all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Friedrich Weber <f.weber@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH storage v3] fix #957 iscsi: improve check_connection logic
Date: Wed, 12 Mar 2025 12:45:46 +0100	[thread overview]
Message-ID: <d60c122f-6d2f-4441-b004-24a160a8f7a7@proxmox.com> (raw)
In-Reply-To: <mailman.992.1741716020.293.pve-devel@lists.proxmox.com>

Hi, thanks for the v3! I looked into this together with Mira and we have
some further comments.

Some general comments:

- The indentation settings look off, please have a look at our Perl
style guide [1].

- If you haven't done so yet, please make sure to send a CLA [0].

First, I want to point out that this patch will not completely get rid
of the "TCP ping"s that trigger the messages on the target, because
after booting the PVE node, iscsi_login/iscsi_discovery will do a TCP
ping. You're probably aware of that, I just wanted to mention it.

I was testing this patch and noticed we still get periodic messages on
the target in the following situation:

- configure two portals A and B, B is permanently down
- reboot PVE node
- as there is no session to B, every 10 seconds the iSCSI plugin calls
iscsi_login, then iscsi_discovery, then iscsi_test_portal for portal A:

sub iscsi_discovery {
    my ($portals) = @_;

    assert_iscsi_support();

    my $res = {};
    for my $portal ($portals->@*) {
        next if !iscsi_test_portal($portal); # fixme: raise exception here?

I guess also here, we don't need to make a TCP ping against portal A if
we already have a LOGGED_IN session with A, similarly to what this patch
does in `check_connection`.

But then we already have this pattern twice, and it might make sense to
instead rewrite `iscsi_test_portal` so that

- it takes a $target, $portal and optionally a $cache
- it calls iscsi_session to get the current list of sessions (passing
$cache to avoid double work)
- it checks whether there is a session to $portal, if yes:
  - if it is LOGGED_IN, return 1, otherwise 0
- otherwise, it triggers a tcp_ping against the portal and returns its
result

Then, we could use this new `iscsi_test_portal` as a drop-in replacement
for the current one. To be able to pass the cache in both call sites, we
would need to modify iscsi_discovery to optionally take a cache as
second argument (note that iscsi_discovery has a second call site in
PVE/Storage.pm that wouldn't pass a cache), and modify iscsi_login to
accept and pass down the cache to iscsi_discovery, and finally
activate_storage to pass down the cache to iscsi_login.

What do you think?

> don't check tcp connection directly if there are already
> sessions. Use iscsiadm command to check the sessions
> status instead.
> 
> 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@torreviejawireless.org>
> ---

It would be great if you could add a short summary here (under the ---)
of the changes since the last revision. They don't go into the final
commit, but make reviewing easier. See [2] for more details.

>  src/PVE/Storage/ISCSIPlugin.pm | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> index eb70453..89fc871 100644
> --- a/src/PVE/Storage/ISCSIPlugin.pm
> +++ b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -66,6 +66,23 @@ sub iscsi_test_portal {
>      return PVE::Network::tcp_ping($server, $port || 3260, 2);
>  }
>  
> +sub iscsi_test_session {
> +    my ($sid) = @_;
> +    my $cmd = [$ISCSIADM, '--mode', 'session', '--sid', $sid, '-P1'];

Instead of doing a relatively expensive subprocess invocation, we could
try reading the connection state from sysfs. From what I could gather,
`/sys/class/iscsi_session/session$sid/state` should work:

# cat /sys/class/iscsi_session/session1/state
LOGGED_IN

`file_read_firstline` should work for reading this. We should probably
also double-check that $sid is actually just a number, just to be sure
we avoid path traversal.

What do you think?

> +
> +    my $res = 0;
> +    eval {
> +        run_command($cmd, errmsg => 'iscsi session test failed', outfunc => sub {
> +            my $line = shift;
> +            if ($line =~ m/^\s+iSCSI Session State: LOGGED_IN\s*$/) {
> +                $res = 1;
> +            }
> +        });
> +    };
> +    die "session check failed: $@\n" if $@;
> +    return $res;
> +}
> +
>  sub iscsi_portals {
>      my ($target, $portal_in) = @_;
>  
> @@ -560,11 +577,22 @@ sub activate_volume {
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
>  
> +    my $cache = {};
> +    my $sessions = iscsi_session($cache, $scfg->{target});
>      my $portals = iscsi_portals($scfg->{target}, $scfg->{portal});
>  
> -    for my $portal (@$portals) {
> -	my $result = iscsi_test_portal($portal);
> -	return $result if $result;
> +    for my $portal ($portals->@*) {
> +        my $session_exists = 0;
> +        for my $session ($sessions->@*) {
> +            next if $session->{portal} ne $portal;
> +            $session_exists = 1;
> +            my $result = iscsi_test_session($session->{session_id});
> +            return $result if $result;
> +        }
> +        next if $session_exists;
> +        # no sessions, check portal via tcp
> +        my $result = iscsi_test_portal($portal);
> +        return $result if $result;
>      }
>  
>      return 0;
> -- 
> 2.43.0
> 
> 

[0]
https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright
[1] https://pve.proxmox.com/wiki/Perl_Style_Guide#Indentation
[2] https://pve.proxmox.com/wiki/Developer_Documentation#Preparing_Patches


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


      reply	other threads:[~2025-03-12 11:46 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-11 17:59 Victor Seva via pve-devel
2025-03-12 11:45 ` Friedrich Weber [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d60c122f-6d2f-4441-b004-24a160a8f7a7@proxmox.com \
    --to=f.weber@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal