[pve-devel] [PATCH storage v2] iscsi: disable Open-iSCSI login retries

[Mira Limbeck]

On 10/11/24 14:58, Friedrich Weber wrote:
> Since 90c1b10 ("fix #254: iscsi: add support for multipath targets"),
> iSCSI storage activation checks whether a session exists for each
> discovered portal. If there is a discovered portal without a session,
> it performs a discovery and login in the hope of establishing a
> session to the portal. If the portal is unreachable when trying to log
> in, Open-iSCSI's default behavior is to retry for up to 2 minutes, as
> explained in /etc/iscsi/iscid.conf:
> 
>> # The default node.session.initial_login_retry_max is 8 and
>> # node.conn[0].timeo.login_timeout is 15 so we have:
>> #
>> # node.conn[0].timeo.login_timeout * \
>> node.session.initial_login_retry_max = 120s
> 
> If pvestatd is activating the storage, it will be blocked during that
> time, which is undesirable. This is particularly unfortunate if the
> target announces portals that the host permanently cannot reach. In
> that case, every pvestatd iteration will take 2 minutes. While it can
> be argued that such setups are misconfigured, it is still desirable to
> keep the fallout of that misconfiguration as low as possible.
> 
> In order to reduce the time Open-iSCSI tries to log in, instruct
> Open-ISCSI to not perform login retries for that target. For this, set
> node.session.initial_login_retry_max for the target to 0. This setting
> is stored in Open-iSCSI's records under /etc/iscsi/nodes. As these
> records are overwritten with the defaults from /etc/iscsi/iscsid.conf
> on discovery, the setting needs to be applied after discovery.
> 
> With this setting, one login attempt should take at most 15 seconds.
> This is still higher than pvestatd's iteration time of 10 seconds, but
> more tolerable. Logins will still be continuously retried by pvestatd
> in every iteration until there is a session to each discovered portal.
> 
> Signed-off-by: Friedrich Weber <f.weber at proxmox.com>
> Tested-by: Mira Limbeck <m.limbeck at proxmox.com>
> Reviewed-by: Mira Limbeck <m.limbeck at proxmox.com>
> ---
> 
> Notes:
>     changes since v1:
>     - dropped "to avoid blocking pvestatd" from commit title, as pvestatd
>       will still be blocked, just for a shorter time
>     - changed formatting
>     - added Mira's R-b and T-b (thx!). Took the liberty to include them
>       although the code, strictly speaking, changed
>     
>     v1: https://lore.proxmox.com/pve-devel/20241011093744.25545-1-f.weber@proxmox.com/
> 
>  src/PVE/Storage/ISCSIPlugin.pm | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/src/PVE/Storage/ISCSIPlugin.pm b/src/PVE/Storage/ISCSIPlugin.pm
> index 2bdd9a2..2e3d6ee 100644
> --- a/src/PVE/Storage/ISCSIPlugin.pm
> +++ b/src/PVE/Storage/ISCSIPlugin.pm
> @@ -132,6 +132,20 @@ sub iscsi_login {
>      eval { iscsi_discovery($portals); };
>      warn $@ if $@;
>  
> +    # Disable retries to avoid blocking pvestatd for too long, next iteration will retry anyway
> +    eval {
> +	my $cmd = [
> +	    $ISCSIADM,
> +	    '--mode', 'node',
> +	    '--targetname', $target,
> +	    '--op', 'update',
> +	    '--name', 'node.session.initial_login_retry_max',
> +	    '--value', '0',
> +	];
> +	run_command($cmd);
> +    };
> +    warn $@ if $@;
> +
>      run_command([$ISCSIADM, '--mode', 'node', '--targetname',  $target, '--login']);
>  }
>  
I tested the v2 again with the same setup, so my `Tested-by` is now true
for this one as well. The commit message and the array formatting are
now clearer.
So the `Tested-by` and `Reviewed-by` included are fine from my side.