public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Aaron Lauterer <a.lauterer@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH cluster-pve8 2/2] status: handle new pve9- metrics update data
Date: Wed, 11 Jun 2025 16:18:20 +0200	[thread overview]
Message-ID: <94178d5d-51f5-4657-a6cb-72dbfbf98487@proxmox.com> (raw)
In-Reply-To: <e4e828bb-6d20-465d-a17a-0b2c44e2660b@proxmox.com>



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

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


  reply	other threads:[~2025-06-11 14:18 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-23 16:00 [pve-devel] [RFC cluster/common/container/manager/pve9-rrd-migration-tool/qemu-server/storage 00/19] Expand and migrate RRD data Aaron Lauterer
2025-05-23 16:00 ` [pve-devel] [PATCH cluster-pve8 1/2] cfs status.c: drop old pve2-vm rrd schema support Aaron Lauterer
2025-05-23 16:00 ` [pve-devel] [PATCH cluster-pve8 2/2] status: handle new pve9- metrics update data Aaron Lauterer
2025-05-23 16:35   ` Aaron Lauterer
2025-06-02 13:31   ` Thomas Lamprecht
2025-06-11 14:18     ` Aaron Lauterer [this message]
2025-05-23 16:00 ` [pve-devel] [PATCH pve9-rrd-migration-tool 1/1] introduce rrd migration tool for pve8 -> pve9 Aaron Lauterer
2025-05-23 16:00 ` [pve-devel] [PATCH cluster 1/1] status: introduce new pve9- rrd and metric format Aaron Lauterer
2025-05-23 16:37 ` [pve-devel] [PATCH common 1/4] fix error in pressure parsing Aaron Lauterer
2025-05-23 16:37 ` [pve-devel] [PATCH common 2/4] add functions to retrieve pressures for vm/ct Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH common 3/4] add helper to fetch value from smaps_rollup for pid Aaron Lauterer
2025-06-02 14:11     ` Thomas Lamprecht
2025-05-23 16:37   ` [pve-devel] [PATCH common 4/4] metrics: add buffer and cache to meminfo Aaron Lauterer
2025-06-02 14:07     ` Thomas Lamprecht
2025-06-11 15:17       ` Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH manager 1/5] api2tools: drop old VM rrd schema Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH manager 2/5] pvestatd: collect and distribute new pve9- metrics Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH manager 3/5] api: nodes: rrd and rrddata fetch from new pve9-node rrd files if present Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH manager 4/5] api2tools: extract stats: handle existence of new pve9- data Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH manager 5/5] ui: rrdmodels: add new columns Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH storage 1/1] status: rrddata: use new pve9 rrd location if file is present Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH qemu-server 1/4] metrics: add pressure to metrics Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH qemu-server 2/4] vmstatus: add memhost for host view of vm mem consumption Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH qemu-server 3/4] vmstatus: switch mem stat to PSS of VM cgroup Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH qemu-server 4/4] rrddata: use new pve9 rrd location if file is present Aaron Lauterer
2025-05-23 16:37   ` [pve-devel] [PATCH container 1/1] " Aaron Lauterer
2025-06-02 14:39   ` [pve-devel] [PATCH common 2/4] add functions to retrieve pressures for vm/ct Thomas Lamprecht
2025-05-26 11:52 ` [pve-devel] [RFC cluster/common/container/manager/pve9-rrd-migration-tool/qemu-server/storage 00/19] Expand and migrate RRD data DERUMIER, Alexandre via pve-devel
2025-07-09 11:26 ` Aaron Lauterer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=94178d5d-51f5-4657-a6cb-72dbfbf98487@proxmox.com \
    --to=a.lauterer@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal