[pdm-devel] [PATCH proxmox-datacenter-manager v2 1/1] remotes: remove direct access on underlying sections HashMap

[Gabriel Goller]

Since accessing the underlying SectionConfigData<T> HashMap poses a lot
of risks (missing order insertion, removal) it has been made private and
the DerefMut implementation was removed. Use either Deref for read
access or use the implemented helper functions (insert, get_mut, etc.).
Instead of iterating over the HashMap, use the SectionConfigData
iterator, which will iterate in the correct order as configured in the
`order` vector.

Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
---
 server/src/api/resources.rs            | 2 +-
 server/src/metric_collection/mod.rs    | 8 ++++----
 server/src/remote_tasks/mod.rs         | 6 +++---
 server/src/test_support/fake_remote.rs | 9 +++------
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
index 453d9e8ea6e5..6a8c8ef25b71 100644
--- a/server/src/api/resources.rs
+++ b/server/src/api/resources.rs
@@ -320,7 +320,7 @@ async fn get_top_entities(timeframe: Option<RrdTimeframe>) -> Result<TopEntities
     let (remotes_config, _) = pdm_config::remotes::config()?;
 
     let timeframe = timeframe.unwrap_or(RrdTimeframe::Day);
-    let res = crate::metric_collection::calculate_top(&remotes_config.sections, timeframe, 10);
+    let res = crate::metric_collection::calculate_top(&remotes_config, timeframe, 10);
     Ok(res)
 }
 
diff --git a/server/src/metric_collection/mod.rs b/server/src/metric_collection/mod.rs
index b37d678250a0..726884e5f732 100644
--- a/server/src/metric_collection/mod.rs
+++ b/server/src/metric_collection/mod.rs
@@ -49,14 +49,14 @@ async fn metric_collection_task() -> Result<(), Error> {
             }
         };
 
-        for (remote_name, remote) in &remotes.sections {
-            let start_time = *most_recent_timestamps.get(remote_name).unwrap_or(&0);
+        for (remote_name, remote) in remotes.into_iter() {
+            let start_time = *most_recent_timestamps.get(&remote_name).unwrap_or(&0);
             let remote_name_clone = remote_name.clone();
 
             let res = async {
                 let most_recent_timestamp = match remote.ty {
                     RemoteType::Pve => {
-                        let client = connection::make_pve_client(remote)?;
+                        let client = connection::make_pve_client(&remote)?;
                         let metrics = client
                             .cluster_metrics_export(Some(true), Some(false), Some(start_time))
                             .await?;
@@ -76,7 +76,7 @@ async fn metric_collection_task() -> Result<(), Error> {
                         .await
                     }
                     RemoteType::Pbs => {
-                        let client = connection::make_pbs_client(remote)?;
+                        let client = connection::make_pbs_client(&remote)?;
                         let metrics = client.metrics(Some(true), Some(start_time)).await?;
 
                         // Involves some blocking file IO
diff --git a/server/src/remote_tasks/mod.rs b/server/src/remote_tasks/mod.rs
index 7ded540845f2..234ffa76921b 100644
--- a/server/src/remote_tasks/mod.rs
+++ b/server/src/remote_tasks/mod.rs
@@ -36,10 +36,10 @@ pub async fn get_tasks(max_age: i64, filters: TaskFilters) -> Result<Vec<TaskLis
     // Room for improvements in the future.
     invalidate_cache_for_finished_tasks(&mut cache);
 
-    for (remote_name, remote) in &remotes.sections {
+    for (remote_name, remote) in remotes.iter() {
         let now = proxmox_time::epoch_i64();
 
-        if let Some(tasks) = cache.get_tasks(remote_name.as_str(), now, max_age) {
+        if let Some(tasks) = cache.get_tasks(remote_name, now, max_age) {
             // Data in cache is recent enough and has not been invalidated.
             all_tasks.extend(tasks);
         } else {
@@ -50,7 +50,7 @@ pub async fn get_tasks(max_age: i64, filters: TaskFilters) -> Result<Vec<TaskLis
                     continue;
                 }
             };
-            cache.set_tasks(remote_name.as_str(), tasks.clone(), now);
+            cache.set_tasks(remote_name, tasks.clone(), now);
 
             all_tasks.extend(tasks);
         }
diff --git a/server/src/test_support/fake_remote.rs b/server/src/test_support/fake_remote.rs
index 40f688074da0..0161d8e6e5f7 100644
--- a/server/src/test_support/fake_remote.rs
+++ b/server/src/test_support/fake_remote.rs
@@ -27,13 +27,12 @@ pub struct FakeRemoteConfig {
 
 impl RemoteConfig for FakeRemoteConfig {
     fn config(&self) -> Result<(SectionConfigData<Remote>, ConfigDigest), Error> {
-        let mut order = Vec::new();
-        let mut sections = HashMap::new();
+        let mut section_config = SectionConfigData::default();
 
         for i in 0..self.nr_of_pve_remotes {
             let name = format!("pve-{i}");
 
-            sections.insert(
+            section_config.insert(
                 name.clone(),
                 Remote {
                     ty: pdm_api_types::remotes::RemoteType::Pve,
@@ -44,13 +43,11 @@ impl RemoteConfig for FakeRemoteConfig {
                     web_url: None,
                 },
             );
-
-            order.push(name);
         }
 
         let digest = [0u8; 32].into();
 
-        Ok((SectionConfigData { sections, order }, digest))
+        Ok((section_config, digest))
     }
 
     fn lock_config(&self) -> Result<ApiLockGuard, Error> {
-- 
2.39.5