[pbs-devel] [PATCH proxmox-backup 0/3] improve and extend admin/datastore/status api

[pbs-devel] [PATCH proxmox-backup 0/3] improve and extend admin/datastore/status api

[Dominik Csapak]

i'll need that for the pbs datastore summary, sending it now since the ui is
not completely done yet

Dominik Csapak (3):
  backup/datastore: count still bad chunks for the status
  backup/datastore: save garbage collection status to disk
  admin/datastore: add more info to status call

 src/api2/admin/datastore.rs | 61 +++++++++++++++++++++++++++++++++++--
 src/api2/types/mod.rs       |  3 ++
 src/backup/chunk_store.rs   |  2 ++
 src/backup/datastore.rs     | 34 +++++++++++++++++++--
 4 files changed, 95 insertions(+), 5 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/3] backup/datastore: count still bad chunks for the status

[Dominik Csapak]

we want to show the user that there are still bad chunks after a garbage
collection

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/types/mod.rs     | 3 +++
 src/backup/chunk_store.rs | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index f97db557..1b9a305f 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -587,6 +587,8 @@ pub struct GarbageCollectionStatus {
     pub pending_chunks: usize,
     /// Number of chunks marked as .bad by verify that have been removed by GC.
     pub removed_bad: usize,
+    /// Number of chunks still marked as .bad after garbage collection.
+    pub still_bad: usize,
 }
 
 impl Default for GarbageCollectionStatus {
@@ -602,6 +604,7 @@ impl Default for GarbageCollectionStatus {
             pending_bytes: 0,
             pending_chunks: 0,
             removed_bad: 0,
+            still_bad: 0,
         }
     }
 }
diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
index 96c46efb..b7556f8a 100644
--- a/src/backup/chunk_store.rs
+++ b/src/backup/chunk_store.rs
@@ -354,9 +354,11 @@ impl ChunkStore {
                         },
                         Err(nix::Error::Sys(nix::errno::Errno::ENOENT)) => {
                             // chunk hasn't been rewritten yet, keep .bad file
+                            status.still_bad += 1;
                         },
                         Err(err) => {
                             // some other error, warn user and keep .bad file around too
+                            status.still_bad += 1;
                             crate::task_warn!(
                                 worker,
                                 "error during stat on '{:?}' - {}",
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/3] backup/datastore: save garbage collection status to disk

[Dominik Csapak]

and load it again when opening it

this way we can persist the status of the last garbage collect across
daemon reloads and reboots

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/backup/datastore.rs | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 2389cc6c..485a10ff 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -9,7 +9,7 @@ use std::fs::File;
 use anyhow::{bail, format_err, Error};
 use lazy_static::lazy_static;
 
-use proxmox::tools::fs::{replace_file, CreateOptions, open_file_locked};
+use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions, open_file_locked};
 
 use super::backup_info::{BackupGroup, BackupDir};
 use super::chunk_store::ChunkStore;
@@ -71,7 +71,20 @@ impl DataStore {
     fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
         let chunk_store = ChunkStore::open(store_name, path)?;
 
-        let gc_status = GarbageCollectionStatus::default();
+        let mut gc_status_path = chunk_store.base_path();
+        gc_status_path.push(".gc-status");
+
+        let gc_status = if let Some(state) = file_read_optional_string(gc_status_path)? {
+            match serde_json::from_str(&state) {
+                Ok(state) => state,
+                Err(err) => {
+                    eprintln!("error reading gc-status: {}", err);
+                    GarbageCollectionStatus::default()
+                }
+            }
+        } else {
+            GarbageCollectionStatus::default()
+        };
 
         Ok(Self {
             chunk_store: Arc::new(chunk_store),
@@ -572,6 +585,23 @@ impl DataStore {
                 crate::task_log!(worker, "Average chunk size: {}", HumanByte::from(avg_chunk));
             }
 
+            if let Ok(serialized) = serde_json::to_string(&gc_status) {
+                let mut path = self.base_path();
+                path.push(".gc-status");
+
+                let backup_user = crate::backup::backup_user()?;
+                let mode = nix::sys::stat::Mode::from_bits_truncate(0o0644);
+                // set the correct owner/group/permissions while saving file
+                // owner(rw) = backup, group(r)= backup
+                let options = CreateOptions::new()
+                    .perm(mode)
+                    .owner(backup_user.uid)
+                    .group(backup_user.gid);
+
+                // ignore errors
+                let _ = replace_file(path, serialized.as_bytes(), options);
+            }
+
             *self.last_gc_status.lock().unwrap() = gc_status;
 
         } else {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/3] admin/datastore: add more info to status call

[Dominik Csapak]

add also the snapshot counts as well as the status of the last garbage
collection

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/admin/datastore.rs | 61 +++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index feeff8af..e1a0aacc 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -423,6 +423,37 @@ pub fn list_snapshots (
     Ok(snapshots)
 }
 
+// returns a map from type to (group_count, snapshot_count)
+fn get_snaphots_count(store: &DataStore) -> Result<HashMap<String, (usize, usize)>, Error> {
+    let base_path = store.base_path();
+    let backup_list = BackupInfo::list_backups(&base_path)?;
+    let mut groups = HashSet::new();
+    let mut result: HashMap<String, (usize, usize)> = HashMap::new();
+    for info in backup_list {
+        let group = info.backup_dir.group();
+
+        let id = group.backup_id();
+        let backup_type = group.backup_type();
+
+        let mut new_id = false;
+
+        if groups.insert(format!("{}-{}", &backup_type, &id)) {
+            new_id = true;
+        }
+
+        if let Some(mut counts) = result.get_mut(backup_type) {
+            counts.1 += 1;
+            if new_id {
+                counts.0 +=1;
+            }
+        } else {
+            result.insert(backup_type.to_string(), (1, 1));
+        }
+    }
+
+    Ok(result)
+}
+
 #[api(
     input: {
         properties: {
@@ -432,7 +463,21 @@ pub fn list_snapshots (
         },
     },
     returns: {
-        type: StorageStatus,
+        description: "The overall Datastore status and information.",
+        type: Object,
+        properties: {
+            storage: {
+                type: StorageStatus,
+            },
+            counts: {
+                description: "Group and Snapshot counts per Type",
+                type: Object,
+                properties: { },
+            },
+            "gc-status": {
+                type: GarbageCollectionStatus,
+            },
+        },
     },
     access: {
         permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
@@ -443,9 +488,19 @@ pub fn status(
     store: String,
     _info: &ApiMethod,
     _rpcenv: &mut dyn RpcEnvironment,
-) -> Result<StorageStatus, Error> {
+) -> Result<Value, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
-    crate::tools::disks::disk_usage(&datastore.base_path())
+    let storage_status = crate::tools::disks::disk_usage(&datastore.base_path())?;
+    let counts = get_snaphots_count(&datastore)?;
+    let gc_status = datastore.last_gc_status();
+
+    let res = json!({
+        "storage": storage_status,
+        "counts": counts,
+        "gc-status": gc_status,
+    });
+
+    Ok(res)
 }
 
 #[api(
-- 
2.20.1

[pbs-devel] applied-series: [PATCH proxmox-backup 0/3] improve and extend admin/datastore/status api

[Thomas Lamprecht]

On 23.10.20 16:32, Dominik Csapak wrote:
> i'll need that for the pbs datastore summary, sending it now since the ui is
> not completely done yet
> 
> Dominik Csapak (3):
>   backup/datastore: count still bad chunks for the status
>   backup/datastore: save garbage collection status to disk
>   admin/datastore: add more info to status call
> 
>  src/api2/admin/datastore.rs | 61 +++++++++++++++++++++++++++++++++++--
>  src/api2/types/mod.rs       |  3 ++
>  src/backup/chunk_store.rs   |  2 ++
>  src/backup/datastore.rs     | 34 +++++++++++++++++++--
>  4 files changed, 95 insertions(+), 5 deletions(-)
> 



applied series, thanks!

Re: [pbs-devel] [PATCH proxmox-backup 3/3] admin/datastore: add more info to status call

[Dietmar Maurer]

This change makes the API call very slow. because it scans over the whole directory tree.
Is this really necessary?


> On 10/23/2020 4:32 PM Dominik Csapak <d.csapak@proxmox.com> wrote:
> 
>  
> add also the snapshot counts as well as the status of the last garbage
> collection
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 61 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index feeff8af..e1a0aacc 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -423,6 +423,37 @@ pub fn list_snapshots (
>      Ok(snapshots)
>  }
>  
> +// returns a map from type to (group_count, snapshot_count)
> +fn get_snaphots_count(store: &DataStore) -> Result<HashMap<String, (usize, usize)>, Error> {
> +    let base_path = store.base_path();
> +    let backup_list = BackupInfo::list_backups(&base_path)?;
> +    let mut groups = HashSet::new();
> +    let mut result: HashMap<String, (usize, usize)> = HashMap::new();
> +    for info in backup_list {
> +        let group = info.backup_dir.group();
> +
> +        let id = group.backup_id();
> +        let backup_type = group.backup_type();
> +
> +        let mut new_id = false;
> +
> +        if groups.insert(format!("{}-{}", &backup_type, &id)) {
> +            new_id = true;
> +        }
> +
> +        if let Some(mut counts) = result.get_mut(backup_type) {
> +            counts.1 += 1;
> +            if new_id {
> +                counts.0 +=1;
> +            }
> +        } else {
> +            result.insert(backup_type.to_string(), (1, 1));
> +        }
> +    }
> +
> +    Ok(result)
> +}
> +
>  #[api(
>      input: {
>          properties: {
> @@ -432,7 +463,21 @@ pub fn list_snapshots (
>          },
>      },
>      returns: {
> -        type: StorageStatus,
> +        description: "The overall Datastore status and information.",
> +        type: Object,
> +        properties: {
> +            storage: {
> +                type: StorageStatus,
> +            },
> +            counts: {
> +                description: "Group and Snapshot counts per Type",
> +                type: Object,
> +                properties: { },
> +            },
> +            "gc-status": {
> +                type: GarbageCollectionStatus,
> +            },
> +        },
>      },
>      access: {
>          permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
> @@ -443,9 +488,19 @@ pub fn status(
>      store: String,
>      _info: &ApiMethod,
>      _rpcenv: &mut dyn RpcEnvironment,
> -) -> Result<StorageStatus, Error> {
> +) -> Result<Value, Error> {
>      let datastore = DataStore::lookup_datastore(&store)?;
> -    crate::tools::disks::disk_usage(&datastore.base_path())
> +    let storage_status = crate::tools::disks::disk_usage(&datastore.base_path())?;
> +    let counts = get_snaphots_count(&datastore)?;
> +    let gc_status = datastore.last_gc_status();
> +
> +    let res = json!({
> +        "storage": storage_status,
> +        "counts": counts,
> +        "gc-status": gc_status,
> +    });
> +
> +    Ok(res)
>  }
>  
>  #[api(
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 3/3] admin/datastore: add more info to status call

[Dominik Csapak]

On 11/3/20 6:52 AM, Dietmar Maurer wrote:
> This change makes the API call very slow. because it scans over the whole directory tree.
> Is this really necessary?
> 


well, not necessary, but it provides some good information to show to 
the user, maybe we can cache that information somehow?

(previously we showed the list of snapshots by default, which is even 
more work & slower)