[pbs-devel] [PATCH proxmox{, -backup} v2 0/7] introduce streaming content api call

[pbs-devel] [PATCH proxmox{, -backup} v2 0/7] introduce streaming content api call

[Dominik Csapak]

This series introduces a new api call `content` on a datastore that
returns the whole content recursively, including namespaces, groups and
snapshots.

It's doing that using the proxmox-router::Stream, so a client can get
the separate records without having to wait for the api call to finish.

This is intended to improve ui responsiveness, since we can show things
even when it's still loading. This can be very useful for large
datastores.

This series contains some refactorings that were necessary, but also
includes one performance improvement for the `groups` api call (stumbled
upon it during testing)

changes from v1:
* remove content type filter from api call (we can decide later how/if
  we want to filter that)
* remove use of exlicit async-stream (can be done via
  proxmox-router::Stream)
* send errors to the stream too
* rename namespace helper
* restructure finding the first finished backup in
  `backup_group_to_group_list_item`
* rename '*List' structs to have more concise names
* return the next level of namespaces in listing

proxmox:

Dominik Csapak (1):
  pbs-api-types: add api types for streaming content api call

 pbs-api-types/Cargo.toml       |  1 +
 pbs-api-types/src/datastore.rs | 89 ++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)


proxmox-backup:

Dominik Csapak (6):
  backup: hierarchy: add new can_access_any_namespace_below helper
  backup: hierarchy: reuse 'NS_PRIVS_OK' for namespace helper
  api: admin: datastore: refactor BackupGroup to GroupListItem
    conversion
  api: admin: datastore: factor out 'get_group_owner'
  api: admin: datastore: optimize `groups` api call
  api: admin: datastore: implement streaming content api call

 src/api2/admin/datastore.rs | 309 +++++++++++++++++++++++++++---------
 src/backup/hierarchy.rs     |  29 ++--
 2 files changed, 251 insertions(+), 87 deletions(-)


Summary over all repositories:
  4 files changed, 341 insertions(+), 87 deletions(-)

-- 
Generated by git-murpp 0.8.1


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

[pbs-devel] [PATCH proxmox v2 1/1] pbs-api-types: add api types for streaming content api call

[Dominik Csapak]

Main point is the new struct
* DatastoreContent, which contains either
 - a namespace
 - a snapshot (including namespace)
 - a backup group (including namespace)

Since the enums variants are newtypes, we need to annotate the 'serde
tag' and given them an 'id-property' and 'id-schema' even though we
don't need it, since it's only relevant for saving these on disk with
section config.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* no contenttype anymore (not necessary currently)
* make properties public (else we can't use them in the ui)
* rename the '*List' types to something better

 pbs-api-types/Cargo.toml       |  1 +
 pbs-api-types/src/datastore.rs | 89 ++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/pbs-api-types/Cargo.toml b/pbs-api-types/Cargo.toml
index 358536d9..b3dee41d 100644
--- a/pbs-api-types/Cargo.toml
+++ b/pbs-api-types/Cargo.toml
@@ -23,5 +23,6 @@ proxmox-lang.workspace=true
 proxmox-s3-client = { workspace = true, features = [ "api-types" ] }
 proxmox-schema = { workspace = true, features = [ "api-macro" ] }
 proxmox-serde.workspace = true
+proxmox-section-config.workspace = true
 proxmox-time.workspace = true
 proxmox-uuid = { workspace = true, features = [ "serde" ] }
diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index fe73cbc4..9b4b9cfa 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1479,6 +1479,86 @@ pub struct NamespaceListItem {
     pub comment: Option<String>,
 }
 
+#[api(
+    properties: {
+        ns: {
+            type: BackupNamespace,
+        },
+        snapshot: {
+            type: SnapshotListItem,
+            flatten: true,
+        },
+    },
+)]
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// Represents a snapshot in a datastore with namespace information
+pub struct SnapshotWithNamespace {
+    /// The namespace of the snapshot
+    pub ns: BackupNamespace,
+    #[serde(flatten)]
+    /// The snapshot information
+    pub snapshot: SnapshotListItem,
+}
+
+#[api(
+    properties: {
+        ns: {
+            type: BackupNamespace,
+        },
+        group: {
+            type: GroupListItem,
+            flatten: true,
+        },
+    },
+)]
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// Represents a snapshot in a datastore with namespace information
+pub struct BackupGroupWithNamespace {
+    /// The namespace of the snapshot
+    pub ns: BackupNamespace,
+    #[serde(flatten)]
+    /// The backup group information
+    pub group: GroupListItem,
+}
+
+#[api(
+    "id-property": "id",
+    "id-schema": {
+        type: String,
+        description: "ID",
+    },
+)]
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+#[serde(tag = "type")]
+pub enum DatastoreContent {
+    NameSpace(NamespaceListItem),
+    Group(BackupGroupWithNamespace),
+    Snapshot(SnapshotWithNamespace),
+}
+
+impl From<NamespaceListItem> for DatastoreContent {
+    fn from(value: NamespaceListItem) -> Self {
+        DatastoreContent::NameSpace(value)
+    }
+}
+
+impl From<(BackupNamespace, GroupListItem)> for DatastoreContent {
+    fn from(value: (BackupNamespace, GroupListItem)) -> Self {
+        let (ns, group) = value;
+        DatastoreContent::Group(BackupGroupWithNamespace { ns, group })
+    }
+}
+
+impl From<(BackupNamespace, SnapshotListItem)> for DatastoreContent {
+    fn from(value: (BackupNamespace, SnapshotListItem)) -> Self {
+        let (ns, snapshot) = value;
+        DatastoreContent::Snapshot(SnapshotWithNamespace { ns, snapshot })
+    }
+}
+
 #[api(
     properties: {
         "backup": { type: BackupDir },
@@ -1759,6 +1839,15 @@ pub const ADMIN_DATASTORE_LIST_NAMESPACE_RETURN_TYPE: ReturnType = ReturnType {
     .schema(),
 };
 
+pub const ADMIN_DATASTORE_LIST_CONTENT_RETURN_TYPE: ReturnType = ReturnType {
+    optional: false,
+    schema: &ArraySchema::new(
+        "Returns the list of namespaces, backup groups and snapshots of a datastore.",
+        &DatastoreContent::API_SCHEMA,
+    )
+    .schema(),
+};
+
 pub const ADMIN_DATASTORE_PRUNE_RETURN_TYPE: ReturnType = ReturnType {
     optional: false,
     schema: &ArraySchema::new(
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 1/6] backup: hierarchy: add new can_access_any_namespace_below helper

[Dominik Csapak]

sometimes we need to check the permissions in a range from a starting
namespace with a certain depth.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* rename helper from 'can_access_any_namespace_in_range'

 src/backup/hierarchy.rs | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
index 8dd71fcf7..febcb9a83 100644
--- a/src/backup/hierarchy.rs
+++ b/src/backup/hierarchy.rs
@@ -68,19 +68,21 @@ pub fn check_ns_privs_full(
     );
 }
 
-pub fn can_access_any_namespace(
+/// Checks if the given user has read/access rights on any namespace on the given datastore,
+/// beginning with `start_ns` up to `max_depth` below.
+pub fn can_access_any_namespace_below(
     store: Arc<DataStore>,
     auth_id: &Authid,
     user_info: &CachedUserInfo,
+    parent_ns: Option<BackupNamespace>,
+    max_depth: Option<usize>,
 ) -> bool {
+    let ns = parent_ns.unwrap_or_default();
     // NOTE: traversing the datastore could be avoided if we had an "ACL tree: is there any priv
     // below /datastore/{store}" helper
-    let mut iter =
-        if let Ok(iter) = store.recursive_iter_backup_ns_ok(BackupNamespace::root(), None) {
-            iter
-        } else {
-            return false;
-        };
+    let Ok(mut iter) = store.recursive_iter_backup_ns_ok(ns, max_depth) else {
+        return false;
+    };
     let wanted =
         PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP;
     let name = store.name();
@@ -90,6 +92,15 @@ pub fn can_access_any_namespace(
     })
 }
 
+/// Checks if the given user has read/access rights on any namespace on given datastore
+pub fn can_access_any_namespace(
+    store: Arc<DataStore>,
+    auth_id: &Authid,
+    user_info: &CachedUserInfo,
+) -> bool {
+    can_access_any_namespace_below(store, auth_id, user_info, None, None)
+}
+
 /// A privilege aware iterator for all backup groups in all Namespaces below an anchor namespace,
 /// most often that will be the `BackupNamespace::root()` one.
 ///
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 2/6] backup: hierarchy: reuse 'NS_PRIVS_OK' for namespace helper

[Dominik Csapak]

Aside from the order of the privileges, it's identical to what we
already used here, so simply reuse it.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v2

 src/backup/hierarchy.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
index febcb9a83..be11ece05 100644
--- a/src/backup/hierarchy.rs
+++ b/src/backup/hierarchy.rs
@@ -83,12 +83,10 @@ pub fn can_access_any_namespace_below(
     let Ok(mut iter) = store.recursive_iter_backup_ns_ok(ns, max_depth) else {
         return false;
     };
-    let wanted =
-        PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP;
     let name = store.name();
     iter.any(|ns| -> bool {
         let user_privs = user_info.lookup_privs(auth_id, &["datastore", name, &ns.to_string()]);
-        user_privs & wanted != 0
+        user_privs & NS_PRIVS_OK != 0
     })
 }
 
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 3/6] api: admin: datastore: refactor BackupGroup to GroupListItem conversion

[Dominik Csapak]

We will reuse this later.

No functionial change intended.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v2

 src/api2/admin/datastore.rs | 107 +++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 45 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a098aca8e..26799de6e 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -142,57 +142,74 @@ pub fn list_groups(
         .try_fold(Vec::new(), |mut group_info, group| {
             let group = group?;
 
-            let owner = match datastore.get_owner(&ns, group.as_ref()) {
-                Ok(auth_id) => auth_id,
-                Err(err) => {
-                    eprintln!(
-                        "Failed to get owner of group '{}' in {} - {}",
-                        group.group(),
-                        print_store_and_ns(&store, &ns),
-                        err
-                    );
-                    return Ok(group_info);
-                }
-            };
-            if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
-                return Ok(group_info);
+            let item =
+                backup_group_to_group_list_item(datastore.clone(), group, &ns, &auth_id, list_all);
+
+            if let Some(item) = item {
+                group_info.push(item);
             }
 
-            let snapshots = match group.list_backups() {
-                Ok(snapshots) => snapshots,
-                Err(_) => return Ok(group_info),
-            };
+            Ok(group_info)
+        })
+}
 
-            let backup_count: u64 = snapshots.len() as u64;
-            if backup_count == 0 {
-                return Ok(group_info);
-            }
+fn backup_group_to_group_list_item(
+    datastore: Arc<DataStore>,
+    group: pbs_datastore::BackupGroup,
+    ns: &BackupNamespace,
+    auth_id: &Authid,
+    list_all: bool,
+) -> Option<GroupListItem> {
+    let owner = match datastore.get_owner(ns, group.as_ref()) {
+        Ok(auth_id) => auth_id,
+        Err(err) => {
+            eprintln!(
+                "Failed to get owner of group '{}' in {} - {}",
+                group.group(),
+                print_store_and_ns(datastore.name(), ns),
+                err
+            );
+            return None;
+        }
+    };
+    if !list_all && check_backup_owner(&owner, auth_id).is_err() {
+        return None;
+    }
 
-            let last_backup = snapshots
-                .iter()
-                .fold(&snapshots[0], |a, b| {
-                    if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
-                        a
-                    } else {
-                        b
-                    }
-                })
-                .to_owned();
-
-            let notes_path = datastore.group_notes_path(&ns, group.as_ref());
-            let comment = file_read_firstline(notes_path).ok();
-
-            group_info.push(GroupListItem {
-                backup: group.into(),
-                last_backup: last_backup.backup_dir.backup_time(),
-                owner: Some(owner),
-                backup_count,
-                files: last_backup.files,
-                comment,
-            });
+    let snapshots = match group.list_backups() {
+        Ok(snapshots) => snapshots,
+        Err(_) => return None,
+    };
 
-            Ok(group_info)
+    let backup_count: u64 = snapshots.len() as u64;
+    if backup_count == 0 {
+        return None;
+    }
+
+    let last_backup = snapshots
+        .iter()
+        .fold(&snapshots[0], |a, b| {
+            if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
+                a
+            } else {
+                b
+            }
         })
+        .to_owned();
+
+    let notes_path = datastore.group_notes_path(ns, group.as_ref());
+    let comment = file_read_firstline(notes_path).ok();
+
+    let item = GroupListItem {
+        backup: group.into(),
+        last_backup: last_backup.backup_dir.backup_time(),
+        owner: Some(owner),
+        backup_count,
+        files: last_backup.files,
+        comment,
+    };
+
+    Some(item)
 }
 
 #[api(
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 4/6] api: admin: datastore: factor out 'get_group_owner'

[Dominik Csapak]

and change the `eprintln` to a `log::warn`

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v2

 src/api2/admin/datastore.rs | 47 +++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 26799de6e..0b133d166 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -160,18 +160,8 @@ fn backup_group_to_group_list_item(
     auth_id: &Authid,
     list_all: bool,
 ) -> Option<GroupListItem> {
-    let owner = match datastore.get_owner(ns, group.as_ref()) {
-        Ok(auth_id) => auth_id,
-        Err(err) => {
-            eprintln!(
-                "Failed to get owner of group '{}' in {} - {}",
-                group.group(),
-                print_store_and_ns(datastore.name(), ns),
-                err
-            );
-            return None;
-        }
-    };
+    let owner = get_group_owner(datastore.name(), ns, &group)?;
+
     if !list_all && check_backup_owner(&owner, auth_id).is_err() {
         return None;
     }
@@ -439,6 +429,25 @@ pub async fn list_snapshots(
     .map_err(|err| format_err!("failed to await blocking task: {err}"))?
 }
 
+fn get_group_owner(
+    store: &str,
+    ns: &BackupNamespace,
+    group: &pbs_datastore::BackupGroup,
+) -> Option<Authid> {
+    match group.get_owner() {
+        Ok(auth_id) => Some(auth_id),
+        Err(err) => {
+            log::warn!(
+                "Failed to get owner of group '{}' in {} - {}",
+                group.group(),
+                print_store_and_ns(store, ns),
+                err
+            );
+            None
+        }
+    }
+}
+
 /// This must not run in a main worker thread as it potentially does tons of I/O.
 unsafe fn list_snapshots_blocking(
     store: String,
@@ -482,17 +491,9 @@ unsafe fn list_snapshots_blocking(
     };
 
     groups.iter().try_fold(Vec::new(), |mut snapshots, group| {
-        let owner = match group.get_owner() {
-            Ok(auth_id) => auth_id,
-            Err(err) => {
-                eprintln!(
-                    "Failed to get owner of group '{}' in {} - {}",
-                    group.group(),
-                    print_store_and_ns(&store, &ns),
-                    err
-                );
-                return Ok(snapshots);
-            }
+        let owner = match get_group_owner(&store, &ns, group) {
+            Some(auth_id) => auth_id,
+            None => return Ok(snapshots),
         };
 
         if !list_all && check_backup_owner(&owner, &auth_id).is_err() {
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 5/6] api: admin: datastore: optimize `groups` api call

[Dominik Csapak]

Currently we always touch all files for each snapshot in a group when
listing them, even though we don't need all that info.

We're only interested in getting either the last finished snapshot
information, or the last unfinished one (which must the only one in
normal use, we can't have multiple unfinished snapshots usually)

Instead of getting all the information upfront, use the snapshot
iterator of the group to get only the id, sort them by time, and
use the first we're interested in, getting the snapshot specific info
only for those we want to check.

In my (admittedly extreme) setup with ~600 groups with ~1000 snapshots
each, this changes the time this api call needs from ~40s to <1s.
(on a relatively fast disk).

While at it, lift the restriction of only returning groups with
snapshots in them, now returning also empty ones.

To keep api compatibility, use a timestamp of 0 for those.
(no valid backup could have been made at that time anyway)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* split out the BackupGroup -> BackupInfo conversion into own
  'filer_map' call, and use 'find' to find the first finished one

 src/api2/admin/datastore.rs | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 0b133d166..643d1694b 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -166,36 +166,38 @@ fn backup_group_to_group_list_item(
         return None;
     }
 
-    let snapshots = match group.list_backups() {
-        Ok(snapshots) => snapshots,
+    let mut snapshots: Vec<_> = match group.iter_snapshots() {
+        Ok(snapshots) => snapshots.collect::<Result<Vec<_>, Error>>().ok()?,
         Err(_) => return None,
     };
 
     let backup_count: u64 = snapshots.len() as u64;
-    if backup_count == 0 {
-        return None;
-    }
+    let last = if backup_count == 1 {
+        // we may have only one unfinished snapshot
+        snapshots.pop().and_then(|dir| BackupInfo::new(dir).ok())
+    } else {
+        // we either have no snapshots, or at least one finished one, since we cannot have
+        // multiple unfinished ones
+        snapshots.sort_by_key(|b| std::cmp::Reverse(b.backup_time()));
+        snapshots
+            .iter()
+            .filter_map(|backup| BackupInfo::new(backup.clone()).ok())
+            .find(|info| info.is_finished())
+    };
 
-    let last_backup = snapshots
-        .iter()
-        .fold(&snapshots[0], |a, b| {
-            if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
-                a
-            } else {
-                b
-            }
-        })
-        .to_owned();
+    let (last_backup, files) = last
+        .map(|info| (info.backup_dir.backup_time(), info.files))
+        .unwrap_or((0, Vec::new()));
 
     let notes_path = datastore.group_notes_path(ns, group.as_ref());
     let comment = file_read_firstline(notes_path).ok();
 
     let item = GroupListItem {
         backup: group.into(),
-        last_backup: last_backup.backup_dir.backup_time(),
+        last_backup,
         owner: Some(owner),
         backup_count,
-        files: last_backup.files,
+        files,
         comment,
     };
 
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup v2 6/6] api: admin: datastore: implement streaming content api call

[Dominik Csapak]

this is a new api call that utilizes `proxmox_router::Stream` to provide
a streaming interface to querying the datastore content.

This can be done when a client requests this api call with the
`application/json-seq` Accept header.

In contrast to the existing api calls, this one
* returns all types of content items (namespaces, groups, snapshots; can
  be filtered with a parameter)
* iterates over them recursively (with the range that is given with the
  parameter)

The api call returns the data in the following order:
* first all visible namespaces
* then for each ns in order
  * each group
  * each snapshot

This is done so that we can have a good way of building a tree view in
the ui.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* don't use a tokio::spawn  around a 'spawn_blocking'
* don't use async-stream, but proxmox-router::Stream
* add new use statements from pbs_api_types in a new line
  (makes the diff more readable..)
* remove content type filter from api call (can be done later still)
* return one level more for namespaces than given, otherwise
  we'll list groups + snapshots for the levels, but not the existing
  namespaces there

 src/api2/admin/datastore.rs | 175 +++++++++++++++++++++++++++++++-----
 1 file changed, 155 insertions(+), 20 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 643d1694b..811ee8713 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -23,7 +23,7 @@ use proxmox_compression::zstd::ZstdEncoder;
 use proxmox_log::LogContext;
 use proxmox_router::{
     http_err, list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission,
-    Router, RpcEnvironment, RpcEnvironmentType, SubdirMap,
+    Record, Router, RpcEnvironment, RpcEnvironmentType, SubdirMap,
 };
 use proxmox_rrd_api_types::{RrdMode, RrdTimeframe};
 use proxmox_schema::*;
@@ -49,6 +49,7 @@ use pbs_api_types::{
     PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
     PRIV_DATASTORE_VERIFY, PRIV_SYS_MODIFY, UPID, UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
 };
+use pbs_api_types::{DatastoreContent, NamespaceListItem};
 use pbs_client::pxar::{create_tar, create_zip};
 use pbs_config::CachedUserInfo;
 use pbs_datastore::backup_info::BackupInfo;
@@ -70,7 +71,10 @@ use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
 
 use crate::api2::backup::optional_ns_param;
 use crate::api2::node::rrd::create_value_from_rrd;
-use crate::backup::{check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK};
+use crate::backup::{
+    can_access_any_namespace_below, check_ns_privs, check_ns_privs_full,
+    ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK,
+};
 use crate::server::jobstate::{compute_schedule_status, Job, JobState};
 use crate::tools::{backup_info_to_snapshot_list_item, get_all_snapshot_files, read_backup_index};
 
@@ -387,7 +391,7 @@ pub async fn delete_snapshot(
 }
 
 #[api(
-    serializing: true,
+    stream: true,
     input: {
         properties: {
             store: { schema: DATASTORE_SCHEMA },
@@ -395,40 +399,125 @@ pub async fn delete_snapshot(
                 type: BackupNamespace,
                 optional: true,
             },
-            "backup-type": {
-                optional: true,
-                type: BackupType,
-            },
-            "backup-id": {
+            "max-depth": {
+                schema: NS_MAX_DEPTH_SCHEMA,
                 optional: true,
-                schema: BACKUP_ID_SCHEMA,
             },
         },
     },
-    returns: pbs_api_types::ADMIN_DATASTORE_LIST_SNAPSHOTS_RETURN_TYPE,
     access: {
         permission: &Permission::Anybody,
         description: "Requires on /datastore/{store}[/{namespace}] either DATASTORE_AUDIT for any \
             or DATASTORE_BACKUP and being the owner of the group",
     },
 )]
-/// List backup snapshots.
-pub async fn list_snapshots(
+/// List datastore content, recursively through all namespaces.
+pub async fn list_content(
     store: String,
     ns: Option<BackupNamespace>,
-    backup_type: Option<BackupType>,
-    backup_id: Option<String>,
+    max_depth: Option<usize>,
     _param: Value,
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
-) -> Result<Vec<SnapshotListItem>, Error> {
+) -> Result<proxmox_router::Stream, Error> {
+    let (sender, receiver) = tokio::sync::mpsc::channel(128);
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
 
-    tokio::task::spawn_blocking(move || unsafe {
-        list_snapshots_blocking(store, ns, backup_type, backup_id, auth_id)
-    })
-    .await
-    .map_err(|err| format_err!("failed to await blocking task: {err}"))?
+    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Read))?;
+
+    // show one level of namespaces more than we use for group/snapshot list
+    let list_max_depth = max_depth.map(|depth| depth + 1);
+    if !can_access_any_namespace_below(
+        datastore.clone(),
+        &auth_id,
+        &user_info,
+        ns.clone(),
+        list_max_depth,
+    ) {
+        proxmox_router::http_bail!(FORBIDDEN, "permission check failed");
+    }
+
+    let ns = ns.unwrap_or_default();
+
+    tokio::task::spawn_blocking(move || {
+        for ns in datastore.recursive_iter_backup_ns_ok(ns.clone(), list_max_depth)? {
+            match check_ns_privs(&store, &ns, &auth_id, NS_PRIVS_OK) {
+                Ok(_) => sender.blocking_send(Record::new(DatastoreContent::from(
+                    NamespaceListItem { ns, comment: None },
+                )))?,
+                // don't disclose errors about ns privileges, since that can be an info leak
+                Err(_) => continue,
+            }
+        }
+
+        for ns in datastore.recursive_iter_backup_ns_ok(ns, max_depth)? {
+            let list_all = match check_ns_privs_full(
+                &store,
+                &ns,
+                &auth_id,
+                PRIV_DATASTORE_AUDIT,
+                PRIV_DATASTORE_BACKUP,
+            ) {
+                Ok(requires_owner) => !requires_owner,
+                // don't disclose errors about ns privileges, since that can be an info leak
+                Err(_) => continue,
+            };
+            for group in datastore.iter_backup_groups(ns.clone())? {
+                let group = match group {
+                    Ok(group) => group,
+                    Err(err) => {
+                        sender.blocking_send(Record::error(err))?;
+                        continue;
+                    }
+                };
+                let group = backup_group_to_group_list_item(
+                    datastore.clone(),
+                    group,
+                    &ns,
+                    &auth_id,
+                    list_all,
+                );
+
+                if let Some(group) = group {
+                    sender
+                        .blocking_send(Record::new(DatastoreContent::from((ns.clone(), group))))?;
+                }
+            }
+
+            for group in datastore.iter_backup_groups(ns.clone())? {
+                let group = match group {
+                    Ok(group) => group,
+                    Err(err) => {
+                        sender.blocking_send(Record::error(err))?;
+                        continue;
+                    }
+                };
+                let owner = match get_group_owner(&store, &ns, &group) {
+                    Some(auth_id) => auth_id,
+                    None => continue,
+                };
+                for backup_dir in group.iter_snapshots()? {
+                    let backup_info = match backup_dir {
+                        Ok(snap) => BackupInfo::new(snap)?,
+                        Err(err) => {
+                            sender.blocking_send(Record::error(err))?;
+                            continue;
+                        }
+                    };
+                    let snapshot = backup_info_to_snapshot_list_item(&backup_info, &owner);
+                    sender.blocking_send(Record::new(DatastoreContent::from((
+                        ns.clone(),
+                        snapshot,
+                    ))))?;
+                }
+            }
+        }
+        Ok::<_, Error>(())
+    });
+
+    Ok(ReceiverStream::new(receiver).into())
 }
 
 fn get_group_owner(
@@ -514,6 +603,51 @@ unsafe fn list_snapshots_blocking(
     })
 }
 
+#[api(
+    serializing: true,
+    input: {
+        properties: {
+            store: { schema: DATASTORE_SCHEMA },
+            ns: {
+                type: BackupNamespace,
+                optional: true,
+            },
+            "backup-type": {
+                optional: true,
+                type: BackupType,
+            },
+            "backup-id": {
+                optional: true,
+                schema: BACKUP_ID_SCHEMA,
+            },
+        },
+    },
+    returns: pbs_api_types::ADMIN_DATASTORE_LIST_SNAPSHOTS_RETURN_TYPE,
+    access: {
+        permission: &Permission::Anybody,
+        description: "Requires on /datastore/{store}[/{namespace}] either DATASTORE_AUDIT for any \
+            or DATASTORE_BACKUP and being the owner of the group",
+    },
+)]
+/// List backup snapshots.
+pub async fn list_snapshots(
+    store: String,
+    ns: Option<BackupNamespace>,
+    backup_type: Option<BackupType>,
+    backup_id: Option<String>,
+    _param: Value,
+    _info: &ApiMethod,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Vec<SnapshotListItem>, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+
+    tokio::task::spawn_blocking(move || unsafe {
+        list_snapshots_blocking(store, ns, backup_type, backup_id, auth_id)
+    })
+    .await
+    .map_err(|err| format_err!("failed to await blocking task: {err}"))?
+}
+
 async fn get_snapshots_count(
     store: &Arc<DataStore>,
     owner: Option<&Authid>,
@@ -2764,6 +2898,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
         "change-owner",
         &Router::new().post(&API_METHOD_SET_BACKUP_OWNER),
     ),
+    ("content", &Router::new().get(&API_METHOD_LIST_CONTENT)),
     (
         "download",
         &Router::new().download(&API_METHOD_DOWNLOAD_FILE),
-- 
2.47.3



_______________________________________________
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 v2 6/6] api: admin: datastore: implement streaming content api call

[Thomas Lamprecht]

Am 08.10.25 um 15:43 schrieb Dominik Csapak:
> this is a new api call that utilizes `proxmox_router::Stream` to provide
> a streaming interface to querying the datastore content.
> 
> This can be done when a client requests this api call with the
> `application/json-seq` Accept header.
> 
> In contrast to the existing api calls, this one
> * returns all types of content items (namespaces, groups, snapshots; can
>   be filtered with a parameter)
> * iterates over them recursively (with the range that is given with the
>   parameter)
> 
> The api call returns the data in the following order:
> * first all visible namespaces
> * then for each ns in order
>   * each group
>   * each snapshot
> 
> This is done so that we can have a good way of building a tree view in
> the ui.

I guess you did not get around to test some more performance / memory
usage here? Might be nice to have whatever stats you did compare encoded
in the commit message here.

I.e. that part of you and my text from patch 6/6 from the v1:

Am 03.10.25 um 13:55 schrieb Thomas Lamprecht:
> Am 03.10.25 um 10:51 schrieb Dominik Csapak:
>> interesting side node, in my rather large setup with ~600 groups and ~1000
>> snapshosts per group, streaming this is faster than using the current
>> `snapshot` api (by a lot):
>> * `snapshot` api -> ~3 min
>> * `content` api with streaming -> ~2:11 min
>> * `content` api without streaming -> ~3 min
>>
>> It seems that either collecting such a 'large' api response (~200MiB)
>> is expensive. My guesses what happens here are either:
>> * frequent (re)allocation of the resulting vec
>> * or serde's serializing code
>
> You could compare peak (RSS) memory usage of the daemon as side-effect,
> and/or also use bpftrace to log bigger allocations. While I did use bpftrace
> lots of times, I did not try this specifically to rust, but I found a
> shorth'ish article that describes doing just that for rust, and looks like
> it would not be _that_ much work (and could be a nice tool to have in the
> belt in the future):
> 
> https://readyset.io/blog/tracing-large-memory-allocations-in-rust-with-bpftrace


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

[pbs-devel] applied: [PATCH proxmox-backup v2 1/6] backup: hierarchy: add new can_access_any_namespace_below helper

[Thomas Lamprecht]

On Wed, 08 Oct 2025 15:43:31 +0200, Dominik Csapak wrote:
> sometimes we need to check the permissions in a range from a starting
> namespace with a certain depth.
> 
> 

Applied, thanks!

[1/6] backup: hierarchy: add new can_access_any_namespace_below helper
      commit: 6a2a7901577373656f00743571c9366ab96ec182


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

[pbs-devel] applied: [PATCH proxmox-backup v2 2/6] backup: hierarchy: reuse 'NS_PRIVS_OK' for namespace helper

[Thomas Lamprecht]

On Wed, 08 Oct 2025 15:43:32 +0200, Dominik Csapak wrote:
> Aside from the order of the privileges, it's identical to what we
> already used here, so simply reuse it.
> 
> 

Applied, thanks!

[2/6] backup: hierarchy: reuse 'NS_PRIVS_OK' for namespace helper
      commit: bca7343c8860b855255376513ff4c7dfcad6e269


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

[pbs-devel] applied: [PATCH proxmox-backup v2 3/6] api: admin: datastore: refactor BackupGroup to GroupListItem conversion

[Thomas Lamprecht]

On Wed, 08 Oct 2025 15:43:33 +0200, Dominik Csapak wrote:
> We will reuse this later.
> 
> No functionial change intended.
> 
> 

Applied, thanks!

[3/6] api: admin: datastore: refactor BackupGroup to GroupListItem conversion
      commit: 45693bfa7a13e780a29e12f4be00f4efc742ebe0


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

[pbs-devel] applied: [PATCH proxmox-backup v2 4/6] api: admin: datastore: factor out 'get_group_owner'

[Thomas Lamprecht]

On Wed, 08 Oct 2025 15:43:34 +0200, Dominik Csapak wrote:
> and change the `eprintln` to a `log::warn`
> 
> 

Applied, thanks!

[4/6] api: admin: datastore: factor out 'get_group_owner'
      commit: 9af3b391b6c465371b895e3d7b81b3fcc640feb6


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

[pbs-devel] applied: [PATCH proxmox-backup v2 5/6] api: admin: datastore: optimize `groups` api call

[Thomas Lamprecht]

On Wed, 08 Oct 2025 15:43:35 +0200, Dominik Csapak wrote:
> Currently we always touch all files for each snapshot in a group when
> listing them, even though we don't need all that info.
> 
> We're only interested in getting either the last finished snapshot
> information, or the last unfinished one (which must the only one in
> normal use, we can't have multiple unfinished snapshots usually)
> 
> [...]

Applied, thanks!

[5/6] api: admin: datastore: optimize `groups` api call
      commit: e59d33f4cede466d8b175bab2f292d6e4279c28c


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