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

[Friedrich Weber <f.weber@proxmox.com>]

Thanks for taking a look at this @Fabian! Some comments inline.

On 19/05/2026 13:39, Michael Köppl wrote:
> 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:
>>>>> [...]
>>>>> +    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?

If we directly return the warning text via the API and display them in
the frontend here, can we still translate them?

>>>
>>> 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.

Yeah, IIRC I was in favor of re-implementing the formula on our side
(instead of reading the values from cmapctl) because this allowed us to
easily compute the new timeouts for N+1 before joining a new node to the
cluster. But Fabian has a good point that this doesn't give us much of a
benefit, so I'd agree that we can probably get away with only computing
the current timeout, and if we do that, taking the values from cmapctl
would certainly be nicer.