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.laute...@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-f4e08f1ec...@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
