From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id CD6A51FF17C for ; Wed, 11 Jun 2025 16:18:32 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DEFA3AFEA; Wed, 11 Jun 2025 16:18:54 +0200 (CEST) Message-ID: <94178d5d-51f5-4657-a6cb-72dbfbf98487@proxmox.com> Date: Wed, 11 Jun 2025 16:18:20 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Thomas Lamprecht , Proxmox VE development discussion References: <20250523160029.404400-1-a.lauterer@proxmox.com> <20250523160029.404400-3-a.lauterer@proxmox.com> From: Aaron Lauterer In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.031 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 2025-06-02 15:31, Thomas Lamprecht wrote: > 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. good point, I'll elaborate more in the commit msg on the next patch version. in short, the full key, eg. "pve2-node/foo" is usually the final location within the RRD directory and in some instances it has been used as passed. Given that we now need to handle newer (pve9- in this RFC) data, we need to make sure that we set the final path explicitly to the "pve2-" location, even if we get a newer "pve9-" key. > > Did not checked out the whole series closely, so just a few things I noticed > from a quick look. > >> >> Signed-off-by: Aaron Lauterer >> --- >> 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". Okay, sounds good. > >> + } >> + >> 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 > ) { > .... > will do >> 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. I'll add a comment at the end of that line, mentioning where it comes from > >> + } >> + >> + 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 true, but I am also contemplating changing the key format overall to be a bit more flexible in the future. See my reply to this patch: https://lore.proxmox.com/pve-devel/91fe7ca4-a07b-4326-8587-f4e08f1ecd5e@proxmox.com/ > >> + >> + 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. thanks for the hints. I did try to play it safe first as my C knowledge isn't that great, therefore I rather played it safe. But I'll integrate this and the following recommendations. > > 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