[pve-devel] [PATCH cluster-pve8 2/2] status: handle new pve9- metrics update data
Aaron Lauterer
a.lauterer at proxmox.com
Wed Jun 11 16:18:20 CEST 2025
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 at 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;
>>
>
More information about the pve-devel
mailing list