[pve-devel] [PATCH rrd-migration-tool] migrate storage: properly handle storage IDs with a dot

[Fiona Ebner]

Storage IDs in Proxmox VE may contain dots, which makes a simple
rename adding a '.old' extension impossible without potential
breakage. The storage RRD is grouped by nodes, so to fix it, create
a '.old' directory for each node and move migrated RRD files there.

If a previous migration with the logic before this change is detected,
the old logic will be kept to avoid picking up '.old' RRD files
created by that previous migration.

Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
---
 src/main.rs | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/src/main.rs b/src/main.rs
index fb58d3a..03a84b8 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -317,7 +317,10 @@ fn mv_old(file: &str) -> Result<()> {
 }
 
 /// Colllect all RRD files in the provided directory
-fn collect_rrd_files(location: &PathBuf) -> Result<Vec<(CString, OsString)>> {
+fn collect_rrd_files(
+    location: &PathBuf,
+    include_files_with_extension: bool,
+) -> Result<Vec<(CString, OsString)>> {
     let mut files: Vec<(CString, OsString)> = Vec::new();
 
     let contents = match fs::read_dir(location) {
@@ -331,7 +334,7 @@ fn collect_rrd_files(location: &PathBuf) -> Result<Vec<(CString, OsString)>> {
     contents
         .filter(|f| f.is_ok())
         .map(|f| f.unwrap().path())
-        .filter(|f| f.is_file() && f.extension().is_none())
+        .filter(|f| f.is_file() && (include_files_with_extension || f.extension().is_none()))
         .for_each(|file| {
             let path = CString::new(file.as_path().as_os_str().as_bytes())
                 .expect("Could not convert path to CString.");
@@ -416,7 +419,7 @@ fn migrate_guests(
     println!("Migrating RRD metrics data for virtual guests…");
     println!("Using {threads} thread(s)");
 
-    let guest_source_files = collect_rrd_files(&source_dir_guests)?;
+    let guest_source_files = collect_rrd_files(&source_dir_guests, false)?;
 
     if guest_source_files.is_empty() {
         println!("No guest metrics to migrate");
@@ -518,7 +521,7 @@ fn migrate_nodes(
         std::fs::create_dir(&target_dir_nodes)?;
     }
 
-    let node_source_files = collect_rrd_files(&source_dir_nodes)?;
+    let node_source_files = collect_rrd_files(&source_dir_nodes, false)?;
 
     let mut no_migration_err = true;
     for file in node_source_files {
@@ -579,17 +582,25 @@ fn migrate_storage(
 
     let mut no_migration_err = true;
     // storage has another layer of directories per node over which we need to iterate
+    // Storage IDs may contain dots, so the old RRD files are moved to a .old directory per node
+    // rather than renamed themselves.
     fs::read_dir(&source_dir_storage)?
         .filter(|f| f.is_ok())
         .map(|f| f.unwrap().path())
-        .filter(|f| f.is_dir())
+        .filter(|f| f.is_dir() && f.extension().is_none())
         .try_for_each(|node| {
             let mut source_storage_subdir = source_dir_storage.clone();
             source_storage_subdir.push(node.file_name().unwrap());
 
+            let source_storage_subdir_old = source_storage_subdir.as_path().with_extension("old");
+
             let mut target_storage_subdir = target_dir_storage.clone();
             target_storage_subdir.push(node.file_name().unwrap());
 
+            // If already migrated using the old rename logic, don't try to migrate with new logic.
+            let migrated_using_old_rename =
+                target_storage_subdir.exists() && !source_storage_subdir_old.exists();
+
             if !target_storage_subdir.exists() && migrate {
                 fs::create_dir(target_storage_subdir.as_path())?;
                 let metadata = target_storage_subdir.metadata()?;
@@ -598,7 +609,18 @@ fn migrate_storage(
                 fs::set_permissions(&target_storage_subdir, permissions)?;
             }
 
-            let storage_source_files = collect_rrd_files(&source_storage_subdir)?;
+            if !source_storage_subdir_old.exists() && migrate && !migrated_using_old_rename {
+                fs::create_dir(source_storage_subdir_old.as_path())?;
+                let metadata = source_storage_subdir_old.metadata()?;
+                let mut permissions = metadata.permissions();
+                permissions.set_mode(0o755);
+                fs::set_permissions(&source_storage_subdir_old, permissions)?;
+            }
+
+            // Storage IDs may contain dots, so need to consider all extensions.
+            // If old logic was used already before, don't use new logic.
+            let storage_source_files =
+                collect_rrd_files(&source_storage_subdir, !migrated_using_old_rename)?;
             for file in storage_source_files {
                 println!(
                     "Migrating metrics for storage '{}/{}'",
@@ -609,6 +631,7 @@ fn migrate_storage(
                 );
 
                 let full_path = file.0.clone().into_string().unwrap();
+                let target_path = source_storage_subdir_old.join(file.1.clone());
                 match do_rrd_migration(
                     file,
                     &target_storage_subdir,
@@ -617,7 +640,12 @@ fn migrate_storage(
                     force,
                 ) {
                     Ok(()) => {
-                        mv_old(full_path.as_str())?;
+                        // Keep using old logic if already migrated with old logic.
+                        if migrated_using_old_rename {
+                            mv_old(full_path.as_str())?;
+                        } else {
+                            fs::rename(full_path, target_path)?;
+                        }
                     }
                     Err(err) => {
                         eprintln!("{err}"); // includes information messages, so just print.
@@ -625,6 +653,11 @@ fn migrate_storage(
                     }
                 }
             }
+
+            if source_storage_subdir.read_dir()?.next().is_none() {
+                fs::remove_dir(source_storage_subdir)?;
+            }
+
             Ok::<(), Error>(())
         })?;
 
-- 
2.47.2