From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 872931FF15C for ; Fri, 3 Oct 2025 13:56:15 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5122927D5; Fri, 3 Oct 2025 13:56:15 +0200 (CEST) Message-ID: Date: Fri, 3 Oct 2025 13:55:41 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox Backup Server development discussion , Dominik Csapak References: <20251003085045.1346864-1-d.csapak@proxmox.com> <20251003085045.1346864-8-d.csapak@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20251003085045.1346864-8-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759492516854 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.176 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 6/6] api: admin: datastore: implement streaming content api call X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" Am 03.10.25 um 10:51 schrieb Dominik Csapak: > this is a new api call that utilizes `async-stream` together with > `proxmox_router::Stream` to provide a streaming interface to querying > the datastore content. > > This can be done when a client reuqests 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 > --- > This should be thouroughly checked for permission checks. I did it to > the best of my ability, but of course some bug/issue could have crept in. > > 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 > but the cost seems still pretty high for that. > LMK if i should further investigate this. tbh. if this holds up in an in-depth review, especially at priv checking like you mentioned, I'm fine with taking it as is; mostly mentioned above as it would be interesting for a deeper understanding, and in my experience especially bpftrace is often quite widely applicable, so it can be worth spending some time playing around with it during "calmer times" ^^ Am 03.10.25 um 10:51 schrieb Dominik Csapak: > diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs > index 2252dcfa4..bf94f6400 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::*; > @@ -39,15 +39,16 @@ use pxar::EntryKind; > > use pbs_api_types::{ > print_ns_and_snapshot, print_store_and_ns, ArchiveType, Authid, BackupArchiveName, > - BackupContent, BackupGroupDeleteStats, BackupNamespace, BackupType, Counts, CryptMode, > - DataStoreConfig, DataStoreListItem, DataStoreMountStatus, DataStoreStatus, > - GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, MaintenanceMode, > - MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, SyncJobConfig, > - BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, > - BACKUP_TYPE_SCHEMA, CATALOG_NAME, CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA, > - IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, > - 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, > + BackupContent, BackupGroupDeleteStats, BackupNamespace, BackupType, ContentListItem, > + ContentType, Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreMountStatus, > + DataStoreStatus, GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, > + MaintenanceMode, MaintenanceType, NamespaceListItem, Operation, PruneJobOptions, > + SnapshotListItem, SyncJobConfig, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, > + BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, CATALOG_NAME, > + CLIENT_LOG_BLOB_NAME, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, > + NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, 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, oof. Would be probably good to split this use statement into multiple ones, e.g. one for PRIV_* one for other const UPPERCASE thingies, and then maybe one for Backup* types and one for DataStore* and the rest. While one can diff per word to see what's going on, this still causes lot's of churn for applying/merging if anything happened in between and history (blame). But doesn't have to be the job of you and this patch series, I'm just venting it. > }; > use pbs_client::pxar::{create_tar, create_zip}; > use pbs_config::CachedUserInfo; > @@ -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_in_range, 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}; > > @@ -396,7 +400,7 @@ pub async fn delete_snapshot( > } > > #[api( > - serializing: true, > + stream: true, > input: { > properties: { > store: { schema: DATASTORE_SCHEMA }, > @@ -404,40 +408,137 @@ pub async fn delete_snapshot( > type: BackupNamespace, > optional: true, > }, > - "backup-type": { > + "max-depth": { > + schema: NS_MAX_DEPTH_SCHEMA, > optional: true, > - type: BackupType, > }, > - "backup-id": { > + "content-type": { > optional: true, > - schema: BACKUP_ID_SCHEMA, > + type: ContentType, > }, > }, > }, > - 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, > - backup_type: Option, > - backup_id: Option, > + max_depth: Option, > + content_type: Option, > _param: Value, > _info: &ApiMethod, > rpcenv: &mut dyn RpcEnvironment, > -) -> Result, Error> { > +) -> Result { > + let (sender, mut 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))?; > + if !can_access_any_namespace_in_range( > + datastore.clone(), > + &auth_id, > + &user_info, > + ns.clone(), > + max_depth, > + ) { > + proxmox_router::http_bail!(FORBIDDEN, "permission check failed"); > + } > + > + let ns = ns.unwrap_or_default(); > + > + let (list_ns, list_group, list_snapshots) = match content_type { > + Some(ContentType::Namespace) => (true, false, false), > + Some(ContentType::Group) => (false, true, false), > + Some(ContentType::Snapshot) => (false, false, true), > + None => (true, true, true), > + }; Hmm, might it make sense to have a filter param with a flag per type? So that one can choose to include groups and snapshots, but not namespaces? Albeit, here it's not really a filter in the classical sense, as besides for skipping snapshot it basically only affects namespaces or groups that are empty, otherwise the info is there indirectly anyway. OTOH. most use cases might just use max-depth to return everything from a single level and load the rest on demand/select. So might be an option to skip this param for now, but maybe someone else has better input or arguments here. > + > + tokio::spawn(async move { Is this really needed? The spawn blocking below already moves the function to a thread dedicated for stuff that can block, so this seems like a needless indirection, or am I overlooking something? > + tokio::task::spawn_blocking(move || { Looked at the rest below more shallowly, nothing stuck out, but would indeed warrant a more in-depth review. _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel