Re: [PATCH cluster v3 4/8] add functions to determine warning level for high token timeouts

["Michael Köppl" <m.koeppl@proxmox.com>]

On Tue May 19, 2026 at 8:59 AM CEST, Fabian Grünbichler wrote:
> On May 18, 2026 5:39 pm, Michael Köppl wrote:
>> On Mon May 18, 2026 at 4:11 PM CEST, Fabian Grünbichler wrote:
>>> On April 27, 2026 7:05 pm, Michael Köppl wrote:
>>>> High token timeouts can lead to stability problems in clusters. To
>>>> inform users about the timeout in their current setup (or expected
>>>> timeouts when adding nodes) and give recommendations regarding the token
>>>> coefficient setting, introduce function to calculate the timeout as well
>>>> as determine the warning / recommendation levels.
>>>> 
>>>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>>>> ---
>>>>  src/PVE/Corosync.pm | 50 +++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 50 insertions(+)
>>>> 
>>>> diff --git a/src/PVE/Corosync.pm b/src/PVE/Corosync.pm
>>>> index aef0d31..45a1f71 100644
>>>> --- a/src/PVE/Corosync.pm
>>>> +++ b/src/PVE/Corosync.pm
>>>> @@ -534,4 +534,54 @@ sub resolve_hostname_like_corosync {
>>>>      return $match_ip_and_version->($resolved_ip);
>>>>  }
>>>>  
>>>> +sub calculate_membership_recovery_timeout {
>>>> +    my ($totemcfg, $node_count) = @_;
>>>> +
>>>> +    my $token_timeout = $totemcfg->{token} // 3000;
>>>> +    my $token_coefficient = $totemcfg->{token_coefficient} // 650;
>>>> +
>>>> +    my $expected_token_timeout = $token_timeout;
>>>> +    if ($node_count > 2) {
>>>> +        $expected_token_timeout += ($node_count - 2) * $token_coefficient;
>>>> +    }
>>>> +
>>>> +    my $expected_consensus_timeout = $totemcfg->{consensus} // $expected_token_timeout * 1.2;
>>>> +    return ($expected_token_timeout + $expected_consensus_timeout) / 1000.0;
>>>
>>> we could also ask corosync (via corosync-cmapctl) about most of these,
>>> to avoid duplicating the calculations/defaults. the only thing missing
>>> is the coefficient, though we could probably expose that on the corosync
>>> side as well.
>> 
>> Thanks for having a look at this!
>> 
>> In the original implementation I used the values from cmap directly. The
>> reason I decided to implement it like this later on was that I wanted to
>> be able to calculate the timeout for an arbitrary number of nodes
>> (although n and n+1 would suffice) to be able to display a warning
>> before adding another node if the timeout would increase to a
>> "problematic" level. I suppose using the values from corosync-cmapctl
>> and then adding $node_delta * $token_coefficient to the token timeout
>> would work, but apart from the avoiding duplicating the defaults, I'm
>> not sure this would improve the solution much? Or am I missing
>> something here?
>
> if corosync ever changes its calculation or defaults, the current
> approach is bad ;)
>
> of course, that also still applies if we get the current value and the
> coefficient from corosync, in case it is the formula that changes..

I agree that getting the values from Corosync directly makes more sense
to avoid future divergence between what our implementation looks like
and what Corosync does, at least if we only want to calculate the
current value of the timeout. Given you suggested below that it would
probably make more sense to have warnings only for the current value,
we could do something like:

```perl
sub calculate_membership_recovery_timeout {
    my $cmap = read_cmap();
    return undef if !$cmap;

    my $token = $cmap->{'runtime.config.totem.token'};
    my $consensus = $cmap->{'runtime.config.totem.consensus'};
    return undef if !defined($token) || !defined($consensus);

    return ($token + $consensus) / 1000.0;
}
```

with read_cmap parsing the output of corosync-cmapctl. This could still
be extended to calculate it for an additional node if we wanted to in
the future.

>
>>>> +}
>>>> +
>>>> +sub get_membership_recovery_timeout_warning_level {
>>>> +    my ($total_timeout_secs) = @_;
>>>> +
>> 
>> [snip]
>> 
>>>> +    my $level_msg;
>>>> +    if ($level eq 'change-strongly-recommended') {
>>>> +        $level_msg = "Lowering the token coefficient is strongly recommended";
>>>> +    } elsif ($level eq 'change-recommended') {
>>>> +        $level_msg = "Lowering the token coefficient is recommended";
>>>> +    } elsif ($level eq 'optimize') {
>>>> +        $level_msg = "The token coefficient can be optimized";
>>>> +    }
>>>> +
>>>> +    return
>>>> +        "Sum of Corosync token and consensus timeout is ${total_timeout_secs}s. "
>>>> +        . "$level_msg. "
>>>> +        . "See 'man pvecm' for details.";
>>>
>>> this pretty much duplicates the frontend code - if we leave out the last
>>> line we could just return the warning message, and call the field in the
>>> API return value "totem_warning(s)" or "health_warnings" or just
>>> "warnings" and potentially add more information in the future? we could
>>> still keep the level and return
>>>
>>> warnings = [ 
>>>  level => ...,
>>>  msg => ...,
>>> ]
>>>
>>> but I don't currently see a reason why we'd benefit from returning raw
>>> values and constructing the warning message on both ends?
>> 
>> The messages themselves differ because one warning message is for the
>> current state, whereas the other is for what would happen if another
>> node was added to the cluster, but I agree that it's unnecessarily
>> duplicated. We could instead return the warning message as
>> totem_warnings, as you suggested, but offer different warning messages
>> depending on a $node_delta (+ how many nodes to the current state, which
>> will pretty much be 1 for all cases right now)?
>
> yeah, I also wondered whether we should just have a boolean flag to
> determine whether we want the current value or the one for if one node
> were added to the current setup.. but in the end it doesn't make that
> much of a difference, unless the user for some reason set a very large
> coefficient manually?
>
> for small clusters, we should be below the thresholds anyway, and one
> more node doesn't matter. for big clusters, a single node being added
> with the default settings would add 0.65 * 2.2 = 1.43 seconds to the
> total timeout. the gaps between the warning levels are way bigger than
> that, so maybe just checking the current value is enough anyhow?
>
> if the user for some reason has a huge token timeout or coefficient or
> consensus timeout configured manually, they will most likely already be
> in a warning state anyway.. and with the default settings, joining would
> at most bump them from slightly below a warning level into that warning
> level, it's not like we can jump from "everything fine" to "strongly
> recommended" with a single node addition..

Agreed, would work for me to adapt it such that we only had the single
warning for the current value. Since @Friedrich and I discussed this
off-list initially and this was the primary reason why I implemented it
this way, maybe he has some input here as well, but I'll prepare a v4
using the values from corosync-cmapctl and with a single warning. Then
we could probably omit the warning level as discussed above and simply
return a `totem_warning` string as part of the response and print that,
appending the "See <documentation> for details" part separately for the
web UI and CLI.

>
> it might make more sense to warn about custom values for each of those
> three that are above certain thresholds, in addition to the total
> timeout checks implemented by this series?

[snip]