[pbs-devel] [PATCH proxmox-backup 0/4] pull/verify unified progress

[pbs-devel] [PATCH proxmox-backup 0/4] pull/verify unified progress

[Fabian Grünbichler]

this series factors out and reuses the progress calculcation used by
sync/pull for verification tasks, since the progress characteristics are
rather similar:
- we often skip big parts of groups (already synced/recently verified)
- adding more snapshots within a group is usually fast (because of
  deduplication/chunk reuse)

Fabian Grünbichler (4):
  remove BackupGroup::list_groups
  pull: factor out interpolated progress
  progress: add format variants
  verify: use same progress as pull

 src/api2/admin/datastore.rs |  4 +--
 src/backup/backup_info.rs   | 14 --------
 src/backup/datastore.rs     | 65 +++++++++++++++++++++++++++++++++++++
 src/backup/verify.rs        | 61 +++++++++++++++-------------------
 src/client/pull.rs          | 29 +++++++----------
 src/server/prune_job.rs     |  4 +--
 6 files changed, 108 insertions(+), 69 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/4] remove BackupGroup::list_groups

[Fabian Grünbichler]

BackupInfo::list_backup_groups is identical code-wise, and makes more
sense as entry point for listing groups.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    we could of course remove the other one, or move this to datastore.rs altogether?

 src/backup/backup_info.rs | 14 --------------
 src/backup/verify.rs      |  2 +-
 src/client/pull.rs        |  2 +-
 src/server/prune_job.rs   |  4 ++--
 4 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/src/backup/backup_info.rs b/src/backup/backup_info.rs
index 367cb8ee..5ff1a6f8 100644
--- a/src/backup/backup_info.rs
+++ b/src/backup/backup_info.rs
@@ -145,20 +145,6 @@ impl BackupGroup {
 
         Ok(last)
     }
-
-    pub fn list_groups(base_path: &Path) -> Result<Vec<BackupGroup>, Error> {
-        let mut list = Vec::new();
-
-        tools::scandir(libc::AT_FDCWD, base_path, &BACKUP_TYPE_REGEX, |l0_fd, backup_type, file_type| {
-            if file_type != nix::dir::Type::Directory { return Ok(()); }
-            tools::scandir(l0_fd, backup_type, &BACKUP_ID_REGEX, |_l1_fd, backup_id, file_type| {
-                if file_type != nix::dir::Type::Directory { return Ok(()); }
-                list.push(BackupGroup::new(backup_type, backup_id));
-                Ok(())
-            })
-        })?;
-        Ok(list)
-    }
 }
 
 impl std::fmt::Display for BackupGroup {
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 1eccdd67..21a31a3f 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -533,7 +533,7 @@ pub fn verify_all_backups(
         }
     };
 
-    let mut list = match BackupGroup::list_groups(&datastore.base_path()) {
+    let mut list = match BackupInfo::list_backup_groups(&datastore.base_path()) {
         Ok(list) => list
             .into_iter()
             .filter(|group| !(group.backup_type() == "host" && group.backup_id() == "benchmark"))
diff --git a/src/client/pull.rs b/src/client/pull.rs
index 7d55f9fa..0c9afe0a 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -565,7 +565,7 @@ pub async fn pull_store(
 
     if delete {
         let result: Result<(), Error> = proxmox::try_block!({
-            let local_groups = BackupGroup::list_groups(&tgt_store.base_path())?;
+            let local_groups = BackupInfo::list_backup_groups(&tgt_store.base_path())?;
             for local_group in local_groups {
                 if new_groups.contains(&local_group) { continue; }
                 worker.log(format!("delete vanished group '{}/{}'", local_group.backup_type(), local_group.backup_id()));
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index 67d438d9..572f1b04 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -4,7 +4,7 @@ use proxmox::try_block;
 
 use crate::{
     api2::types::*,
-    backup::{compute_prune_info, BackupGroup, DataStore, PruneOptions},
+    backup::{compute_prune_info, BackupInfo, DataStore, PruneOptions},
     server::jobstate::Job,
     server::WorkerTask,
     task_log,
@@ -43,7 +43,7 @@ pub fn do_prune_job(
 
                 let base_path = datastore.base_path();
 
-                let groups = BackupGroup::list_groups(&base_path)?;
+                let groups = BackupInfo::list_backup_groups(&base_path)?;
                 for group in groups {
                     let list = group.list_backups(&base_path)?;
                     let mut prune_info = compute_prune_info(list, &prune_options)?;
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/4] pull: factor out interpolated progress

[Fabian Grünbichler]

and add group/snapshot count info.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Note: not 100% happy with struct and field naming here, very open for
better suggestions..

 src/backup/datastore.rs | 47 +++++++++++++++++++++++++++++++++++++++++
 src/client/pull.rs      | 27 ++++++++++-------------
 2 files changed, 58 insertions(+), 16 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 19efc23f..4f49d03f 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -739,3 +739,50 @@ impl DataStore {
         self.verify_new
     }
 }
+
+#[derive(Debug, Clone, Default)]
+/// Tracker for progress of operations iterating over `Datastore` contents.
+pub struct StoreProgress {
+    /// Completed groups
+    pub done_groups: u64,
+    /// Total groups
+    pub total_groups: u64,
+    /// Completed snapshots within current group
+    pub done_snapshots: u64,
+    /// Total snapshots in current group
+    pub group_snapshots: u64,
+}
+
+impl StoreProgress {
+    pub fn new(total_groups: u64) -> Self {
+        StoreProgress {
+            total_groups,
+            .. Default::default()
+        }
+    }
+
+    /// Calculates an interpolated relative progress based on current counters.
+    pub fn percentage(&self) -> f64 {
+        let per_groups = (self.done_groups as f64) / (self.total_groups as f64);
+        if self.group_snapshots == 0 {
+            per_groups
+        } else {
+            let per_snapshots = (self.done_snapshots as f64) / (self.group_snapshots as f64);
+            per_groups + (1.0 / self.total_groups as f64) * per_snapshots
+        }
+    }
+}
+
+impl std::fmt::Display for StoreProgress {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "{:.2}% ({} of {} groups, {} of {} group snapshots)",
+            self.percentage() * 100.0,
+            self.done_groups,
+            self.total_groups,
+            self.done_snapshots,
+            self.group_snapshots,
+        )
+    }
+}
diff --git a/src/client/pull.rs b/src/client/pull.rs
index 0c9afe0a..2555a14c 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -395,7 +395,7 @@ pub async fn pull_group(
     tgt_store: Arc<DataStore>,
     group: &BackupGroup,
     delete: bool,
-    progress: Option<(usize, usize)>, // (groups_done, group_count)
+    progress: &mut StoreProgress,
 ) -> Result<(), Error> {
 
     let path = format!("api2/json/admin/datastore/{}/snapshots", src_repo.store());
@@ -418,18 +418,10 @@ pub async fn pull_group(
 
     let mut remote_snapshots = std::collections::HashSet::new();
 
-    let (per_start, per_group) = if let Some((groups_done, group_count)) = progress {
-        let per_start = (groups_done as f64)/(group_count as f64);
-        let per_group = 1.0/(group_count as f64);
-        (per_start, per_group)
-    } else {
-        (0.0, 1.0)
-    };
-
     // start with 16384 chunks (up to 65GB)
     let downloaded_chunks = Arc::new(Mutex::new(HashSet::with_capacity(1024*64)));
 
-    let snapshot_count = list.len();
+    progress.group_snapshots = list.len() as u64;
 
     for (pos, item) in list.into_iter().enumerate() {
         let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?;
@@ -469,9 +461,8 @@ pub async fn pull_group(
 
         let result = pull_snapshot_from(worker, reader, tgt_store.clone(), &snapshot, downloaded_chunks.clone()).await;
 
-        let percentage = (pos as f64)/(snapshot_count as f64);
-        let percentage = per_start + percentage*per_group;
-        worker.log(format!("percentage done: {:.2}%", percentage*100.0));
+        progress.done_snapshots = pos as u64 + 1;
+        worker.log(format!("percentage done: {}", progress.clone()));
 
         result?; // stop on error
     }
@@ -523,9 +514,13 @@ pub async fn pull_store(
         new_groups.insert(BackupGroup::new(&item.backup_type, &item.backup_id));
     }
 
-    let group_count = list.len();
+    let mut progress = StoreProgress::new(list.len() as u64);
+
+    for (done, item) in list.into_iter().enumerate() {
+        progress.done_groups = done as u64;
+        progress.done_snapshots = 0;
+        progress.group_snapshots = 0;
 
-    for (groups_done, item) in list.into_iter().enumerate() {
         let group = BackupGroup::new(&item.backup_type, &item.backup_id);
 
         let (owner, _lock_guard) = match tgt_store.create_locked_backup_group(&group, &auth_id) {
@@ -551,7 +546,7 @@ pub async fn pull_store(
             tgt_store.clone(),
             &group,
             delete,
-            Some((groups_done, group_count)),
+            &mut progress,
         ).await {
             worker.log(format!(
                 "sync group {}/{} failed - {}",
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/4] progress: add format variants

[Fabian Grünbichler]

for iterating over a single group, or iterating just on the group level

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/datastore.rs | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 4f49d03f..20617d38 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -775,14 +775,32 @@ impl StoreProgress {
 
 impl std::fmt::Display for StoreProgress {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(
-            f,
-            "{:.2}% ({} of {} groups, {} of {} group snapshots)",
-            self.percentage() * 100.0,
-            self.done_groups,
-            self.total_groups,
-            self.done_snapshots,
-            self.group_snapshots,
-        )
+        if self.group_snapshots == 0 {
+            write!(
+                f,
+                "{:.2}% ({} of {} groups)",
+                self.percentage() * 100.0,
+                self.done_groups,
+                self.total_groups,
+            )
+        } else if self.total_groups == 1 {
+            write!(
+                f,
+                "{:.2}% ({} of {} snapshots)",
+                self.percentage() * 100.0,
+                self.done_snapshots,
+                self.group_snapshots,
+            )
+        } else {
+            write!(
+                f,
+                "{:.2}% ({} of {} groups, {} of {} group snapshots)",
+                self.percentage() * 100.0,
+                self.done_groups,
+                self.total_groups,
+                self.done_snapshots,
+                self.group_snapshots,
+            )
+        }
     }
 }
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/4] verify: use same progress as pull

[Fabian Grünbichler]

percentage of verified groups, interpolating based on snapshot count
within the group. in most cases, this will also be closer to 'real'
progress since added snapshots (those which will be verified) in active
backup groups will be roughly evenly distributed, while number of total
snapshots per group will be heavily skewed towards those groups which
have existed the longest, even though most of those old snapshots will
only be re-verified very infrequently.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs |  4 +--
 src/backup/verify.rs        | 59 ++++++++++++++++---------------------
 2 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index bce58a78..df60dab6 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -687,12 +687,12 @@ pub fn verify(
                 }
                 res
             } else if let Some(backup_group) = backup_group {
-                let (_count, failed_dirs) = verify_backup_group(
+                let failed_dirs = verify_backup_group(
                     datastore,
                     &backup_group,
                     verified_chunks,
                     corrupt_chunks,
-                    None,
+                    &mut StoreProgress::new(1),
                     worker.clone(),
                     worker.upid(),
                     None,
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index 21a31a3f..7ba8d56a 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -10,6 +10,7 @@ use crate::{
     api2::types::*,
     backup::{
         DataStore,
+        StoreProgress,
         DataBlob,
         BackupGroup,
         BackupDir,
@@ -425,11 +426,11 @@ pub fn verify_backup_group(
     group: &BackupGroup,
     verified_chunks: Arc<Mutex<HashSet<[u8;32]>>>,
     corrupt_chunks: Arc<Mutex<HashSet<[u8;32]>>>,
-    progress: Option<(usize, usize)>, // (done, snapshot_count)
+    progress: &mut StoreProgress,
     worker: Arc<dyn TaskState + Send + Sync>,
     upid: &UPID,
     filter: Option<&dyn Fn(&BackupManifest) -> bool>,
-) -> Result<(usize, Vec<String>), Error> {
+) -> Result<Vec<String>, Error> {
 
     let mut errors = Vec::new();
     let mut list = match group.list_backups(&datastore.base_path()) {
@@ -442,19 +443,17 @@ pub fn verify_backup_group(
                 group,
                 err,
             );
-            return Ok((0, errors));
+            return Ok(errors);
         }
     };
 
-    task_log!(worker, "verify group {}:{}", datastore.name(), group);
+    let snapshot_count = list.len();
+    task_log!(worker, "verify group {}:{} ({} snapshots)", datastore.name(), group, snapshot_count);
 
-    let (done, snapshot_count) = progress.unwrap_or((0, list.len()));
+    progress.group_snapshots = snapshot_count as u64;
 
-    let mut count = 0;
     BackupInfo::sort_list(&mut list, false); // newest first
-    for info in list {
-        count += 1;
-
+    for (pos, info) in list.into_iter().enumerate() {
         if !verify_backup_dir(
             datastore.clone(),
             &info.backup_dir,
@@ -466,20 +465,15 @@ pub fn verify_backup_group(
         )? {
             errors.push(info.backup_dir.to_string());
         }
-        if snapshot_count != 0 {
-            let pos = done + count;
-            let percentage = ((pos as f64) * 100.0)/(snapshot_count as f64);
-            task_log!(
-                worker,
-                "percentage done: {:.2}% ({} of {} snapshots)",
-                percentage,
-                pos,
-                snapshot_count,
-            );
-        }
+        progress.done_snapshots = pos as u64 + 1;
+        task_log!(
+            worker,
+            "percentage done: {}",
+            progress
+        );
     }
 
-    Ok((count, errors))
+    Ok(errors)
 }
 
 /// Verify all (owned) backups inside a datastore
@@ -551,34 +545,33 @@ pub fn verify_all_backups(
 
     list.sort_unstable();
 
-    let mut snapshot_count = 0;
-    for group in list.iter() {
-        snapshot_count += group.list_backups(&datastore.base_path())?.len();
-    }
-
     // start with 16384 chunks (up to 65GB)
     let verified_chunks = Arc::new(Mutex::new(HashSet::with_capacity(1024*16)));
 
     // start with 64 chunks since we assume there are few corrupt ones
     let corrupt_chunks = Arc::new(Mutex::new(HashSet::with_capacity(64)));
 
-    task_log!(worker, "found {} snapshots", snapshot_count);
+    let group_count = list.len();
+    task_log!(worker, "found {} groups", group_count);
 
-    let mut done = 0;
-    for group in list {
-        let (count, mut group_errors) = verify_backup_group(
+    let mut progress = StoreProgress::new(group_count as u64);
+
+    for (pos, group) in list.into_iter().enumerate() {
+        progress.done_groups = pos as u64;
+        progress.done_snapshots = 0;
+        progress.group_snapshots = 0;
+
+        let mut group_errors = verify_backup_group(
             datastore.clone(),
             &group,
             verified_chunks.clone(),
             corrupt_chunks.clone(),
-            Some((done, snapshot_count)),
+            &mut progress,
             worker.clone(),
             upid,
             filter,
         )?;
         errors.append(&mut group_errors);
-
-        done += count;
     }
 
     Ok(errors)
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 2/4] pull: factor out interpolated progress

[Dietmar Maurer]

> -        let percentage = (pos as f64)/(snapshot_count as f64);
> -        let percentage = per_start + percentage*per_group;
> -        worker.log(format!("percentage done: {:.2}%", percentage*100.0));
> +        progress.done_snapshots = pos as u64 + 1;
> +        worker.log(format!("percentage done: {}", progress.clone()));

This clone seems unnecessary.

[pbs-devel] applied: [PATCH proxmox-backup 0/4] pull/verify unified progress

[Dietmar Maurer]

applied, with some cleanups on top:

- removed unnecessary clone
- moved code into extra file: src/backup/store_progress.rs

> On 11/30/2020 4:27 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> this series factors out and reuses the progress calculcation used by
> sync/pull for verification tasks, since the progress characteristics are
> rather similar:
> - we often skip big parts of groups (already synced/recently verified)
> - adding more snapshots within a group is usually fast (because of
>   deduplication/chunk reuse)
> 
> Fabian Grünbichler (4):
>   remove BackupGroup::list_groups
>   pull: factor out interpolated progress
>   progress: add format variants
>   verify: use same progress as pull
> 
>  src/api2/admin/datastore.rs |  4 +--
>  src/backup/backup_info.rs   | 14 --------
>  src/backup/datastore.rs     | 65 +++++++++++++++++++++++++++++++++++++
>  src/backup/verify.rs        | 61 +++++++++++++++-------------------
>  src/client/pull.rs          | 29 +++++++----------
>  src/server/prune_job.rs     |  4 +--
>  6 files changed, 108 insertions(+), 69 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel