From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 9B7E91FF146 for ; Tue, 26 May 2026 10:49:05 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C836C16DDC; Tue, 26 May 2026 10:49:02 +0200 (CEST) Message-ID: Date: Tue, 26 May 2026 10:48:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 storage 09/15] iscsi: remove stale sessions in non-mapping case To: Thomas Lamprecht , Proxmox VE development discussion References: <20260430173220.441001-1-m.limbeck@proxmox.com> <20260430173220.441001-10-m.limbeck@proxmox.com> <20260523212856.2822353-7-t.lamprecht@proxmox.com> Content-Language: en-US From: Mira Limbeck In-Reply-To: <20260523212856.2822353-7-t.lamprecht@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1779785314161 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.350 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [iscsiplugin.pm] Message-ID-Hash: H6OQYGHZBM5TY2YR4NPJN7TMDFFH32CF X-Message-ID-Hash: H6OQYGHZBM5TY2YR4NPJN7TMDFFH32CF X-MailFrom: m.limbeck@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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.