Re: [PATCH v2 storage 09/15] iscsi: remove stale sessions in non-mapping case

[Mira Limbeck <m.limbeck@proxmox.com>]

On 5/23/26 11:28 PM, Thomas Lamprecht wrote:
> On Thu, 30 Apr 2026 19:27:07 +0200, Mira Limbeck wrote:
>> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
>> @@ -736,6 +736,41 @@ sub activate_storage {
>> +    # log out of stale sessions
>> +    # should only be needed for the non-mapping case
>> +    if (!$local_cfg->{is_mapping}) {
>> +        for my $target (keys $targets->%*) {
>> +            for my $session ($targets->{$target}->{sessions}->@*) {
>> +                my $portals = $targets->{$target}->{portals};
>> +                if (!grep(/^\Q$session->{portal}\E$/, $portals->@*)) {
>> +                    my $sid = $session->{session_id};
>> +                    my $portal = $session->{portal};
>> +                    print "logging out of stale iscsi session: $sid for $target via $portal\n";
>> +                    my $cmd = [
>> +                        $ISCSIADM, '--mode', 'session', '--sid', $sid, '--logout',
>> +                    ];
>> +                    eval { run_command($cmd); };
>> +                    warn "failed to log out of stale session: $@\n" if $@;
> 
> In the non-mapping case $portals is whatever discovery returned this cycle
> (see 07/15) - driven by the SAN's sendtargets output, or kept as-is if
> discovery failed. So if the SAN momentarily stops announcing a portal (or
> discovery partially fails), its live session here gets logged out and the
> node entry deleted. Under shared LVM-on-iSCSI that can pull a PV out from an
> active VG; under multipath it can remove the last surviving path. It also
> runs counter to the "keep configured entries, clean up manually" caution
> added in 07/15.
> 
> I'd suggest dropping this for now, or reworking it to be opt-in, to act only
> after a successful full discovery against all portals, to never remove the
> last path, and to never remove a session that is a member of an active
> multipath map.

It shouldn't be able to ever remove the last one, since in
get_local_config() it should fall back to all node entries when none can
be discovered. So at least one session should always remain until the
next reboot.

This is actually the only patch I'm not so sure about, since it changes
the behavior we had so far, and we usually don't clean up connections
for storages (see NFS) when it could lead to issues.
That's why I mentioned it is optional (no patch after it depends on it),
and controversial.
So I'm fine with just dropping it entirely and keeping cleaning up the
responsibility of the admin.