Re: [PATCH manager v2] fix #4130: external metric: better handle failed connections

["Lukas Sichert" <l.sichert@proxmox.com>]

Thanks for looking into it. Comments are inline.

On 2026-05-06 19:00, Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> Am 19.02.26 um 13:34 schrieb Lukas Sichert:
>> When an external metric server configured to use TCP is unreachable, the
>> storage and VM indicators of the cluster nodes in the UI turn gray. This
>> is because, currently, a failed connection attempt raises an unhandled
>> exception, which aborts the status update flow.  As the connection
>> attempts happen at the beginning of the update process, status
>> information is then not broadcasted within the system or across the
>> cluster. After five minutes without updates, the frontend marks the
>> indicators as gray.
>> 
>> To catch connection errors, wrap connection establishment in an eval
>> block. The implementation ensures that other connections to external
>> metric servers are still established, even if one fails.
>> 
>> Signed-off-by: Lukas Sichert <l.sichert@proxmox.com>
>> ---
>> 
>> Notes:
>>     changes from v1 to v2:
>>     -add the SafeSyslog import required for syslog()
>>     -correct bug ID: #4911 -> #4130
>>     -move the push operation outside the eval block as suggested by Thomas
>>     
>>     Regarding catching the errors at a higher level: Since this function
>>     is iterated through the plugins, not catching the error here would mean,
>>     that not all the plugins are checked.
>> 
>>  PVE/ExtMetric.pm | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/PVE/ExtMetric.pm b/PVE/ExtMetric.pm
>> index ebc2817b..18815efd 100644
>> --- a/PVE/ExtMetric.pm
>> +++ b/PVE/ExtMetric.pm
>> @@ -7,6 +7,7 @@ use PVE::Status::Plugin;
>>  use PVE::Status::Graphite;
>>  use PVE::Status::InfluxDB;
>>  use PVE::Status::OpenTelemetry;
>> +use PVE::SafeSyslog;
>>  
>>  PVE::Status::Graphite->register();
>>  PVE::Status::InfluxDB->register();
>> @@ -52,8 +53,12 @@ sub transactions_start {
>>          $cfg,
>>          sub {
>>              my ($plugin, $id, $plugin_config) = @_;
>> -
>> -            my $connection = $plugin->_connect($plugin_config, $id);
>> +            
>> +            my $connection = eval { $plugin->_connect($plugin_config, $id);}; 
>
> there are various whitespace/code format issues here, please run the top-level
> "make tidy" target or call promxox-perltidy manually on the files to fix this.
Thank you for pointing it out. I will fix it in a v3.
>
>> +            if (my $err = $@) {
>> +                syslog( "warning", "connection for plugin '$id' failed: $err");
>> +                return;
>
> This now returns an undef for transation, so the call sides in pvestatd probably
> need to be adapted too to:
>
> if (defined(my $transactions = PVE::ExtMetric::transactions_start($status_cfg))) {
>     # do something with $transaction
> }
>
> As otherwise this could cause warnings about accessing an undef value.

The '$transactions' variable is initialized as an empty array earlier.
The 'return;' is inside the closure passed to 'foreach_plug', so it only
returns from that closure and prevents the 'push @$transactions, ...'
below from being executed for the current plugin. The 'foreach_plug'
loop then continues with the next plugin in '$cfg'. After 'foreach_plug'
finishes, '$transactions' is returned from 'transactions_start'.
Please tell me if I am misunderstanding some Perl closure intricacy
here.

>
>> +            }
>>  
>>              push @$transactions,
>>                  {