Re: [pve-devel] [PATCH cluster 3/3] pmxcfs: status.c: always use 9.0 rrd files

["Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>]

LGTM, and I especially like that this patch reduces the complexity of 
pmxcfs a lot.


Reviewed-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
Tested-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>



On 05.09.25 15:55, Aaron Lauterer wrote:
> Assuming that we can keep the old RRD files around and fetch data
> directly there if needed, we can skip the migration step and just write
> to new 9.0 RRD files.
> 
> This simplifies the whole approach introduced in
> 9b00ac9 status: introduce new pve-{type}- rrd and metric format
> 
> We will always create a new 9.0 file if not present and then need to
> handle the following cases:
> 
> * old pve2-{type} format -> pad missing columns
> * pve-{type}-9.0 matches -> no changes needed
> * pve-{type}- received, but version doesn't match -> new format, cut at
>    expected columns
> 
> Since we don't need the old RRD definitions anymore, they are removed.
> Otherwise the compile will throw an unused variable warning.
> The `rrd_def_{type}_columns` variables get a `_pve2` suffix to make it
> clear what they are for.
> 
> The `checked_mkdir` pattern for VMs is aligned with how we do it for
> nodes and storages, to call it once more, to be safe.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> Notes:
>      if I misunderstood the reason for the doulbe checked_mkdir, then it
>      might only be needed for the storage due to the additional subdirs?
> 
>   src/pmxcfs/status.c | 238 ++++++--------------------------------------
>   1 file changed, 30 insertions(+), 208 deletions(-)
> 
> diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
> index 130f08f..cb03e4e 100644
> --- a/src/pmxcfs/status.c
> +++ b/src/pmxcfs/status.c
> @@ -1097,36 +1097,9 @@ kventry_hash_set(GHashTable *kvhash, const char *key, gconstpointer data, size_t
>   // We create the RRD files with a 60 second stepsize, therefore, RRA timesteps
>   // are alwys per 60 seconds. These 60 seconds are usually showing up in other
>   // code paths where we interact with RRD data!
> -static const char *rrd_def_node[] = {
> -    "DS:loadavg:GAUGE:120:0:U",
> -    "DS:maxcpu:GAUGE:120:0:U",
> -    "DS:cpu:GAUGE:120:0:U",
> -    "DS:iowait:GAUGE:120:0:U",
> -    "DS:memtotal:GAUGE:120:0:U",
> -    "DS:memused:GAUGE:120:0:U",
> -    "DS:swaptotal:GAUGE:120:0:U",
> -    "DS:swapused:GAUGE:120:0:U",
> -    "DS:roottotal:GAUGE:120:0:U",
> -    "DS:rootused:GAUGE:120:0:U",
> -    "DS:netin:DERIVE:120:0:U",
> -    "DS:netout:DERIVE:120:0:U",
> -
> -    "RRA:AVERAGE:0.5:1:70",     // 1 min avg - one hour
> -    "RRA:AVERAGE:0.5:30:70",    // 30 min avg - one day
> -    "RRA:AVERAGE:0.5:180:70",   // 3 hour avg - one week
> -    "RRA:AVERAGE:0.5:720:70",   // 12 hour avg - one month
> -    "RRA:AVERAGE:0.5:10080:70", // 7 day avg - ony year
> -
> -    "RRA:MAX:0.5:1:70",     // 1 min max - one hour
> -    "RRA:MAX:0.5:30:70",    // 30 min max - one day
> -    "RRA:MAX:0.5:180:70",   // 3 hour max - one week
> -    "RRA:MAX:0.5:720:70",   // 12 hour max - one month
> -    "RRA:MAX:0.5:10080:70", // 7 day max - ony year
> -    NULL,
> -};
>   
>   // This *must* be the number of columns as defined above.
> -static const int rrd_def_node_columns = 12;
> +static const int rrd_def_node_columns_pve2 = 12;
>   
>   static const char *rrd_def_node_pve9_0[] = {
>       "DS:loadavg:GAUGE:120:0:U",
> @@ -1165,34 +1138,8 @@ static const char *rrd_def_node_pve9_0[] = {
>   // This *must* be the number of columns as defined above.
>   static const int rrd_def_node_pve9_0_columns = 19;
>   
> -static const char *rrd_def_vm[] = {
> -    "DS:maxcpu:GAUGE:120:0:U",
> -    "DS:cpu:GAUGE:120:0:U",
> -    "DS:maxmem:GAUGE:120:0:U",
> -    "DS:mem:GAUGE:120:0:U",
> -    "DS:maxdisk:GAUGE:120:0:U",
> -    "DS:disk:GAUGE:120:0:U",
> -    "DS:netin:DERIVE:120:0:U",
> -    "DS:netout:DERIVE:120:0:U",
> -    "DS:diskread:DERIVE:120:0:U",
> -    "DS:diskwrite:DERIVE:120:0:U",
> -
> -    "RRA:AVERAGE:0.5:1:70",     // 1 min avg - one hour
> -    "RRA:AVERAGE:0.5:30:70",    // 30 min avg - one day
> -    "RRA:AVERAGE:0.5:180:70",   // 3 hour avg - one week
> -    "RRA:AVERAGE:0.5:720:70",   // 12 hour avg - one month
> -    "RRA:AVERAGE:0.5:10080:70", // 7 day avg - ony year
> -
> -    "RRA:MAX:0.5:1:70",     // 1 min max - one hour
> -    "RRA:MAX:0.5:30:70",    // 30 min max - one day
> -    "RRA:MAX:0.5:180:70",   // 3 hour max - one week
> -    "RRA:MAX:0.5:720:70",   // 12 hour max - one month
> -    "RRA:MAX:0.5:10080:70", // 7 day max - ony year
> -    NULL,
> -};
> -
>   // This *must* be the number of columns as defined above.
> -static const int rrd_def_vm_columns = 10;
> +static const int rrd_def_vm_columns_pve2 = 10;
>   
>   static const char *rrd_def_vm_pve9_0[] = {
>       "DS:maxcpu:GAUGE:120:0:U",
> @@ -1229,24 +1176,6 @@ static const char *rrd_def_vm_pve9_0[] = {
>   // This *must* be the number of columns as defined above.
>   static const int rrd_def_vm_pve9_0_columns = 17;
>   
> -static const char *rrd_def_storage[] = {
> -    "DS:total:GAUGE:120:0:U",
> -    "DS:used:GAUGE:120:0:U",
> -
> -    "RRA:AVERAGE:0.5:1:70",     // 1 min avg - one hour
> -    "RRA:AVERAGE:0.5:30:70",    // 30 min avg - one day
> -    "RRA:AVERAGE:0.5:180:70",   // 3 hour avg - one week
> -    "RRA:AVERAGE:0.5:720:70",   // 12 hour avg - one month
> -    "RRA:AVERAGE:0.5:10080:70", // 7 day avg - ony year
> -
> -    "RRA:MAX:0.5:1:70",     // 1 min max - one hour
> -    "RRA:MAX:0.5:30:70",    // 30 min max - one day
> -    "RRA:MAX:0.5:180:70",   // 3 hour max - one week
> -    "RRA:MAX:0.5:720:70",   // 12 hour max - one month
> -    "RRA:MAX:0.5:10080:70", // 7 day max - ony year
> -    NULL,
> -};
> -
>   static const char *rrd_def_storage_pve9_0[] = {
>       "DS:total:GAUGE:120:0:U",
>       "DS:used:GAUGE:120:0:U",
> @@ -1356,68 +1285,27 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
>           }
>   
>           filename = g_strdup_printf(RRDDIR "/pve-node-9.0/%s", node);
> -        char *filename_pve2 = g_strdup_printf(RRDDIR "/pve2-node/%s", node);
> -
> -        int use_pve2_file = 0;
>   
> -        // check existing rrd files and directories
> -        if (g_file_test(filename, G_FILE_TEST_EXISTS)) {
> -            // pve-node-9.0 file exists, we use that
> -            // TODO: get conditions so, that we do not have this empty branch
> -        } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
> -            // old file exists, use it
> -            use_pve2_file = 1;
> -
> -            g_free(filename);
> -            filename = filename_pve2;
> -            filename_pve2 = NULL;
> -        } else {
> -            // neither file exists, check for directories to decide and create file
> +        if (!g_file_test(filename, G_FILE_TEST_EXISTS)) {
> +            checked_mkdir(RRDDIR "/pve-node-9.0", 0755);
>   
> -            if (g_file_test(RRDDIR "/pve-node-9.0", G_FILE_TEST_IS_DIR)) {
> +            char *dir = g_path_get_dirname(filename);
> +            checked_mkdir(dir, 0755);
> +            g_free(dir);
>   
> -                int argcount = sizeof(rrd_def_node_pve9_0) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_node_pve9_0);
> -            } else if (g_file_test(RRDDIR "/pve2-node", G_FILE_TEST_IS_DIR)) {
> -                use_pve2_file = 1;
> -
> -                g_free(filename);
> -                filename = filename_pve2;
> -                filename_pve2 = NULL;
> -
> -                char *dir = g_path_get_dirname(filename);
> -                checked_mkdir(dir, 0755);
> -                g_free(dir);
> -
> -                int argcount = sizeof(rrd_def_node) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_node);
> -            } else {
> -                // no dir exists yet, use new pve-node-9.0
> -                checked_mkdir(RRDDIR "/pve-node-9.0", 0755);
> -
> -                char *dir = g_path_get_dirname(filename);
> -                checked_mkdir(dir, 0755);
> -                g_free(dir);
> -
> -                int argcount = sizeof(rrd_def_node_pve9_0) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_node_pve9_0);
> -            }
> +            int argcount = sizeof(rrd_def_node_pve9_0) / sizeof(void *) - 1;
> +            create_rrd_file(filename, argcount, rrd_def_node_pve9_0);
>           }
>   
>           skip = 2; // first two columns are live data that isn't archived
>   
> -        if (strncmp(key, "pve2-node/", 10) == 0 && !use_pve2_file) {
> -            padding = rrd_def_node_pve9_0_columns - rrd_def_node_columns;
> -        } else if (strncmp(key, "pve-node-", 9) == 0 && use_pve2_file) {
> -            keep_columns = rrd_def_node_columns;
> +        if (strncmp(key, "pve2-node/", 10) == 0) {
> +            padding = rrd_def_node_pve9_0_columns - rrd_def_node_columns_pve2;
>           } else if (strncmp(key, "pve-node-9.0/", 13) != 0) {
>               // we received an unknown format, expectation is it is newer and has more columns
>               // than we can currently handle
>               keep_columns = rrd_def_node_pve9_0_columns;
>           }
> -
> -        g_free(filename_pve2);
> -
>       } else if (strncmp(key, "pve2.3-vm/", 10) == 0 || strncmp(key, "pve-vm-", 7) == 0) {
>   
>           const char *vmid = rrd_skip_data(key, 1, '/');
> @@ -1431,59 +1319,28 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
>           }
>   
>           filename = g_strdup_printf(RRDDIR "/pve-vm-9.0/%s", vmid);
> -        char *filename_pve2 = g_strdup_printf(RRDDIR "/pve2-vm/%s", vmid);
> -
> -        int use_pve2_file = 0;
> -
> -        // check existing rrd files and directories
> -        if (g_file_test(filename, G_FILE_TEST_EXISTS)) {
> -            // pve-vm-9.0 file exists, we use that
> -            // TODO: get conditions so, that we do not have this empty branch
> -        } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
> -            // old file exists, use it
> -            use_pve2_file = 1;
> -            g_free(filename);
> -            filename = filename_pve2;
> -            filename_pve2 = NULL;
> -        } else {
> -            // neither file exists, check for directories to decide and create file
> -
> -            if (g_file_test(RRDDIR "/pve-vm-9.0", G_FILE_TEST_IS_DIR)) {
>   
> -                int argcount = sizeof(rrd_def_vm_pve9_0) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_vm_pve9_0);
> -            } else if (g_file_test(RRDDIR "/pve2-vm", G_FILE_TEST_IS_DIR)) {
> -                use_pve2_file = 1;
> +        if (!g_file_test(filename, G_FILE_TEST_EXISTS)) {
> +            // no dir exists yet, use new pve-vm-9.0
> +            checked_mkdir(RRDDIR "/pve-vm-9.0", 0755);
>   
> -                g_free(filename);
> -                filename = filename_pve2;
> -                filename_pve2 = NULL;
> +            char *dir = g_path_get_dirname(filename);
> +            checked_mkdir(dir, 0755);
> +            g_free(dir);
>   
> -                int argcount = sizeof(rrd_def_vm) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_vm);
> -            } else {
> -                // no dir exists yet, use new pve-vm-9.0
> -                checked_mkdir(RRDDIR "/pve-vm-9.0", 0755);
> -
> -                int argcount = sizeof(rrd_def_vm_pve9_0) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_vm_pve9_0);
> -            }
> +            int argcount = sizeof(rrd_def_vm_pve9_0) / sizeof(void *) - 1;
> +            create_rrd_file(filename, argcount, rrd_def_vm_pve9_0);
>           }
>   
>           skip = 4; // first 4 columns are live data that isn't archived
>   
> -        if (strncmp(key, "pve2.3-vm/", 10) == 0 && !use_pve2_file) {
> -            padding = rrd_def_vm_pve9_0_columns - rrd_def_vm_columns;
> -        } else if (strncmp(key, "pve-vm-", 7) == 0 && use_pve2_file) {
> -            keep_columns = rrd_def_vm_columns;
> +        if (strncmp(key, "pve2.3-vm/", 10) == 0) {
> +            padding = rrd_def_vm_pve9_0_columns - rrd_def_vm_columns_pve2;
>           } else if (strncmp(key, "pve-vm-9.0/", 11) != 0) {
>               // we received an unknown format, expectation is it is newer and has more columns
>               // than we can currently handle
>               keep_columns = rrd_def_vm_pve9_0_columns;
>           }
> -
> -        g_free(filename_pve2);
> -
>       } else if (strncmp(key, "pve2-storage/", 13) == 0 || strncmp(key, "pve-storage-", 12) == 0) {
>           const char *node = rrd_skip_data(key, 1, '/'); // will contain {node}/{storage}
>   
> @@ -1502,49 +1359,17 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
>           }
>   
>           filename = g_strdup_printf(RRDDIR "/pve-storage-9.0/%s", node);
> -        char *filename_pve2 = g_strdup_printf(RRDDIR "/pve2-storage/%s", node);
> -
> -        // check existing rrd files and directories
> -        if (g_file_test(filename, G_FILE_TEST_EXISTS)) {
> -            // pve-storage-9.0 file exists, we use that
> -            // TODO: get conditions so, that we do not have this empty branch
> -        } else if (g_file_test(filename_pve2, G_FILE_TEST_EXISTS)) {
> -            // old file exists, use it
> -            g_free(filename);
> -            filename = filename_pve2;
> -            filename_pve2 = NULL;
> -        } else {
> -            // neither file exists, check for directories to decide and create file
> -
> -            if (g_file_test(RRDDIR "/pve-storage-9.0", G_FILE_TEST_IS_DIR)) {
> -                char *dir = g_path_get_dirname(filename);
> -                checked_mkdir(dir, 0755);
> -                g_free(dir);
> -
> -                int argcount = sizeof(rrd_def_storage_pve9_0) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
> -            } else if (g_file_test(RRDDIR "/pve2-storage", G_FILE_TEST_IS_DIR)) {
> -                g_free(filename);
> -                filename = filename_pve2;
> -                filename_pve2 = NULL;
> -
> -                char *dir = g_path_get_dirname(filename);
> -                checked_mkdir(dir, 0755);
> -                g_free(dir);
> -
> -                int argcount = sizeof(rrd_def_storage) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_storage);
> -            } else {
> -                // no dir exists yet, use new pve-storage-9.0
> -                checked_mkdir(RRDDIR "/pve-storage-9.0", 0755);
>   
> -                char *dir = g_path_get_dirname(filename);
> -                checked_mkdir(dir, 0755);
> -                g_free(dir);
> +        if (!g_file_test(filename, G_FILE_TEST_EXISTS)) {
> +            // no dir exists yet, use new pve-storage-9.0
> +            checked_mkdir(RRDDIR "/pve-storage-9.0", 0755);
>   
> -                int argcount = sizeof(rrd_def_storage_pve9_0) / sizeof(void *) - 1;
> -                create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
> -            }
> +            char *dir = g_path_get_dirname(filename);
> +            checked_mkdir(dir, 0755);
> +            g_free(dir);
> +
> +            int argcount = sizeof(rrd_def_storage_pve9_0) / sizeof(void *) - 1;
> +            create_rrd_file(filename, argcount, rrd_def_storage_pve9_0);
>           }
>   
>           // actual data columns didn't change between pve2-storage and pve-storage-9.0
> @@ -1553,9 +1378,6 @@ static void update_rrd_data(const char *key, gconstpointer data, size_t len) {
>               // than we can currently handle
>               keep_columns = rrd_def_storage_pve9_0_columns;
>           }
> -
> -        g_free(filename_pve2);
> -
>       } else {
>           goto keyerror;
>       }



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel