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 0B70C1FF141 for ; Tue, 19 May 2026 18:12:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 83476DCC4; Tue, 19 May 2026 18:12:40 +0200 (CEST) Message-ID: <1328c54e-8b9b-4002-889f-090e36ac793a@proxmox.com> Date: Tue, 19 May 2026 18:12:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH cluster v3 4/8] add functions to determine warning level for high token timeouts To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pve-devel@lists.proxmox.com References: <20260427170548.307698-1-m.koeppl@proxmox.com> <20260427170548.307698-5-m.koeppl@proxmox.com> <1779112787.r3e8af5igi.astroid@yuna.none> <1779173481.jqlsj6voe5.astroid@yuna.none> Content-Language: en-US From: Friedrich Weber In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1779207142455 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.013 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 Message-ID-Hash: JOM3NFGU2LUDGKPR2WGPAJGRILSF7ASO X-Message-ID-Hash: JOM3NFGU2LUDGKPR2WGPAJGRILSF7ASO X-MailFrom: f.weber@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: 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 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.