From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id B37B91FF191
for <inbox@lore.proxmox.com>; Mon, 2 Jun 2025 15:31:36 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 5321A34259;
Mon, 2 Jun 2025 15:31:53 +0200 (CEST)
Message-ID: <e4e828bb-6d20-465d-a17a-0b2c44e2660b@proxmox.com>
Date: Mon, 2 Jun 2025 15:31:18 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Aaron Lauterer <a.lauterer@proxmox.com>
References: <20250523160029.404400-1-a.lauterer@proxmox.com>
<20250523160029.404400-3-a.lauterer@proxmox.com>
Content-Language: de-AT, en-US
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
Autocrypt: addr=t.lamprecht@proxmox.com; keydata=
xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J
uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w
OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD
90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7
UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+
wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6
YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY
virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt
dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ
jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt
cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO
R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK
CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/
bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R
G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF
sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB
FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB
UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk
tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1
feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI
tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu
cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR
D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95
QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy
HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD
txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6
NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY
ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM
8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X
s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l
gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9
GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk
IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j
/Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk
IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0
F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS
RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj
C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA
kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF
BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg
8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja
0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx
In-Reply-To: <20250523160029.404400-3-a.lauterer@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.036 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
RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
Validity was blocked. See
https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
information.
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH cluster-pve8 2/2] status: handle new pve9-
metrics update data
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Am 23.05.25 um 18:00 schrieb Aaron Lauterer:
> For PVE9 there will be additional fields in the metrics that are
> collected. The new columns/fields are added at the end of the current
> ones. Therefore, if we get the new format, we need to cut it.
>
> Paths to rrd filenames needed to be set manually to 'pve2-...' and will
> use the 'node' part instead of the full key, as that could also be
> 'pve9-...' which does not exists.
any implications on using the node part, or is it fine here and resulting to
what is used now anyway? More rationale would definitively be helpful.
Did not checked out the whole series closely, so just a few things I noticed
from a quick look.
>
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> src/pmxcfs/status.c | 51 ++++++++++++++++++++++++++++++++++++++-------
> src/pmxcfs/status.h | 2 ++
> 2 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
> index 77a18d8..3fdb179 100644
> --- a/src/pmxcfs/status.c
> +++ b/src/pmxcfs/status.c
> @@ -1236,6 +1236,8 @@ rrd_skip_data(
> return data;
> }
>
> +static char* rrd_format_update_buffer = NULL;
> +
> static void
> update_rrd_data(
> const char *key,
> @@ -1255,9 +1257,15 @@ update_rrd_data(
>
> char *filename = NULL;
>
> + if (!rrd_format_update_buffer) {
> + rrd_format_update_buffer = (char*)malloc(RRD_FORMAT_BUFFER_SIZE);
pmxcfs uses the glib (which is not to be confused with glibc, the GNU std lib), and
while I'm not a big fan of that, it still makes sense to keep it consistency until it
gets ripped out (or rewritten to rust, ...), so please use "g_malloc0" here.
If this is a fixed buffer that is frequently used it could also be allocated statically
there as array.
And btw. this pointer is only safe to share as the function is called inside the local
rrdentry_hash_set which in turn is called inside the public cfs_status_set, which takes
the global mutex; otherwise this would be rather dangerous; so noting that it is and must
be protected by that mutex would be always good for such things.
But actually, I do not think you need that buffer at all, see further below, where you
"cut it off".
> + }
> +
> int skip = 0;
> + int data_cutoff = 0; // how many columns after initial skip should be a cut-off
>
> - if (strncmp(key, "pve2-node/", 10) == 0) {
> + if (strncmp(key, "pve2-node/", 10) == 0 ||
> + strncmp(key, "pve9-node/", 10) == 0) {
please move the `) {` to a new line to make the code in the block stand more
out from the one in the if condition, and thus more readable (I know style in
pmxcfs is not great as is, but new code can do slightly better)
I.e. something like:
if (
strncmp(key, "pve2-node/", 10) == 0
|| strncmp(key, "pve9-node/", 10) == 0
) {
....
> const char *node = key + 10;
>
> skip = 2;
> @@ -1268,7 +1276,11 @@ update_rrd_data(
> if (strlen(node) < 1)
> goto keyerror;
>
> - filename = g_strdup_printf(RRDDIR "/%s", key);
> + if (strncmp(key, "pve9-node/", 10) == 0) {
> + data_cutoff = 13;
Do not just use seemingly random integers without a comment with a short
rationale.
> + }
> +
> + filename = g_strdup_printf(RRDDIR "/pve2-node/%s", node);
>
> if (!g_file_test(filename, G_FILE_TEST_EXISTS)) {
>
> @@ -1277,8 +1289,15 @@ update_rrd_data(
> create_rrd_file(filename, argcount, rrd_def_node);
> }
>
> - } else if (strncmp(key, "pve2.3-vm/", 10) == 0) {
> - const char *vmid = key + 10;
> + } else if (strncmp(key, "pve2.3-vm/", 10) == 0 ||
> + strncmp(key, "pve9-vm/", 8) == 0) {
use 9.0 and you avoid the need for below differentiation
> +
> + const char *vmid;
> + if (strncmp(key, "pve2.3-vm/", 10) == 0) {
> + vmid = key + 10;
> + } else {
> + vmid = key + 8;
> + }
>
> skip = 4;
>
> @@ -1288,6 +1307,10 @@ update_rrd_data(
> if (strlen(vmid) < 1)
> goto keyerror;
>
> + if (strncmp(key, "pve9-vm/", 8) == 0) {
> + data_cutoff = 11;
> + }
> +
> filename = g_strdup_printf(RRDDIR "/%s/%s", "pve2-vm", vmid);
>
> if (!g_file_test(filename, G_FILE_TEST_EXISTS)) {
> @@ -1297,7 +1320,8 @@ update_rrd_data(
> create_rrd_file(filename, argcount, rrd_def_vm);
> }
>
> - } else if (strncmp(key, "pve2-storage/", 13) == 0) {
> + } else if (strncmp(key, "pve2-storage/", 13) == 0 ||
> + strncmp(key, "pve9-storage/", 13) == 0) {
> const char *node = key + 13;
>
> const char *storage = node;
> @@ -1315,7 +1339,7 @@ update_rrd_data(
> if (strlen(storage) < 1)
> goto keyerror;
>
> - filename = g_strdup_printf(RRDDIR "/%s", key);
> + filename = g_strdup_printf(RRDDIR "/pve2-storage/%s", node);
>
> if (!g_file_test(filename, G_FILE_TEST_EXISTS)) {
>
> @@ -1335,7 +1359,20 @@ update_rrd_data(
>
> const char *dp = skip ? rrd_skip_data(data, skip) : data;
>
> - const char *update_args[] = { dp, NULL };
> + if (data_cutoff) {
> + const char *cut = rrd_skip_data(dp, data_cutoff);
> + const int data_len = cut - dp - 1; // -1 to remove last colon
> + snprintf(rrd_format_update_buffer, RRD_FORMAT_BUFFER_SIZE, "%.*s", data_len, dp);
This is inefficient in multiple ways, you already get the cut point nicely in
the cut string pointer, just write a zero there and your string is terminated;
That's how wonderful C is ;) And dangerous, but it really only gets less dangerous
by stopping to use it, so no point in bending backwards like your code does here,
as it still relies on the same underlying facts, just copies data around much
more.
In other words, replace most of this here with:
*(cut - 1) = 0; // terminate string by replacing colon from field separator with zero.
> + } else {
> + snprintf(rrd_format_update_buffer, RRD_FORMAT_BUFFER_SIZE, "%s", dp);
this branch can then be dropped completely
> + }
> +
> + const char *update_args[] = { rrd_format_update_buffer, NULL };
here just keep the original that passes the dp string pointer, and do not be thrown
off by dp being defined as const, that means the pointer is const, not the value it
points too.
> +
> + // TODO: remove in non RFC, but useful for debug logging to see if data is handled correctly
> + // cfs_message("KEY: %s", key);
> + // cfs_message("DATA: %s", dp);
> + // cfs_message("BUFFER: %s", rrd_format_update_buffer);
you could add a single cfs_debug statement and keep it then, but I think using gdb
and a setting a breakpoint here to inspect these would be actually enough.
>
> if (use_daemon) {
> int status;
> diff --git a/src/pmxcfs/status.h b/src/pmxcfs/status.h
> index 041cb34..1a38f43 100644
> --- a/src/pmxcfs/status.h
> +++ b/src/pmxcfs/status.h
> @@ -34,6 +34,8 @@
>
> #define CFS_MAX_STATUS_SIZE (32*1024)
>
> +#define RRD_FORMAT_BUFFER_SIZE (1024 * 1024) // 1 MiB
> +
> typedef struct cfs_clnode cfs_clnode_t;
> typedef struct cfs_clinfo cfs_clinfo_t;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel