[pbs-devel] [PATCH proxmox-backup v2 0/5] prefilter source list by encryption/verify state for sync jobs

[pbs-devel] [PATCH proxmox-backup v2 0/5] prefilter source list by encryption/verify state for sync jobs

[Christian Ebner]

Since the introduction of the `verified-only` and `encrypted-only`
optional parameters for sync jobs, it is possible to exclude snapshot
if their are not encrypted or verified from the sync.

The current implementation, filtering the snapshots based on the
manifest information only directly before syncing said snapshot, leads
to the somewhat unexpected behaviour that in combination with the
transfer-last parameter set, the transfer-last filtering is performed
before the encryption/verificaton state filtering.

This patch series improves the current behaviour by pre-filtering the
snapshots already when generating the list of to be synced items.
This allows to apply the transfer-last filtering only after the
verified-only and encrypted-only filters, resulting in a more natural
behaviour.

Since this breaks with the current filter order, the breaking changes
should be considered for version 4 only.

Changes since version 1:
- rebased onto current master

proxmox-backup:

Christian Ebner (5):
  api: admin: refactor generation of backup dir for snapshot listing
  api: factor out helper converting backup info to snapshot list item
  sync: source: list snapshot items instead of backup directories
  pull: refactor source snapshot list filtering logic
  sync: conditionally pre-filter source list by verified or encrypted
    state

 src/api2/admin/datastore.rs | 142 +++---------------------------------
 src/server/pull.rs          |  49 ++++++++-----
 src/server/push.rs          |  44 +++++++----
 src/server/sync.rs          |  73 +++++++++++++-----
 src/tools/mod.rs            | 129 ++++++++++++++++++++++++++++++++
 5 files changed, 252 insertions(+), 185 deletions(-)


Summary over all repositories:
  5 files changed, 252 insertions(+), 185 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-backup v2 1/5] api: admin: refactor generation of backup dir for snapshot listing

[Christian Ebner]

Instead of recreating the `pbs_api_types::BackupDir` from information
via the backup group, use the `BackupDir` as stored in `BackupInfo`.

As a side effect this allows to drop the backup group as parameter to
the closure generating the snapshot list items from the backup info
objects.

No functional changes intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 8f1d0e07b..d73e6d64f 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -64,8 +64,8 @@ use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::BackupManifest;
 use pbs_datastore::prune::compute_prune_info;
 use pbs_datastore::{
-    check_backup_owner, ensure_datastore_is_mounted, task_tracking, BackupDir, BackupGroup,
-    DataStore, DatastoreBackend, LocalChunkReader, StoreProgress,
+    check_backup_owner, ensure_datastore_is_mounted, task_tracking, BackupDir, DataStore,
+    DatastoreBackend, LocalChunkReader, StoreProgress,
 };
 use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
@@ -516,11 +516,8 @@ unsafe fn list_snapshots_blocking(
         (None, None) => datastore.list_backup_groups(ns.clone())?,
     };
 
-    let info_to_snapshot_list_item = |group: &BackupGroup, owner, info: BackupInfo| {
-        let backup = pbs_api_types::BackupDir {
-            group: group.into(),
-            time: info.backup_dir.backup_time(),
-        };
+    let info_to_snapshot_list_item = |owner, info: BackupInfo| {
+        let backup = info.backup_dir.dir().to_owned();
         let protected = info.protected;
 
         match get_all_snapshot_files(&info) {
@@ -609,7 +606,7 @@ unsafe fn list_snapshots_blocking(
         snapshots.extend(
             group_backups
                 .into_iter()
-                .map(|info| info_to_snapshot_list_item(group, Some(owner.clone()), info)),
+                .map(|info| info_to_snapshot_list_item(Some(owner.clone()), info)),
         );
 
         Ok(snapshots)
-- 
2.47.2



_______________________________________________
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/5] api: factor out helper converting backup info to snapshot list item

[Christian Ebner]

Move the logic currently provided by a closure to it's dedicated
named helper function.

Further, in order to reuse the same code for the generation of the
snapshot list items to be used by the sync job source readers, move
the helper and related helper functions to the tools module.

No functional changes intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 135 +++---------------------------------
 src/tools/mod.rs            | 129 ++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 127 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index d73e6d64f..07e2f928b 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1,6 +1,5 @@
 //! Datastore Management
 
-use std::collections::HashSet;
 use std::ffi::OsStr;
 use std::ops::Deref;
 use std::os::unix::ffi::OsStrExt;
@@ -43,13 +42,12 @@ use pbs_api_types::{
     BackupContent, BackupGroupDeleteStats, BackupNamespace, BackupType, Counts, CryptMode,
     DataStoreConfig, DataStoreListItem, DataStoreMountStatus, DataStoreStatus,
     GarbageCollectionJobStatus, GroupListItem, JobScheduleStatus, KeepOptions, MaintenanceMode,
-    MaintenanceType, Operation, PruneJobOptions, SnapshotListItem, SnapshotVerifyState,
-    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, MANIFEST_BLOB_NAME, 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,
+    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,
 };
 use pbs_client::pxar::{create_tar, create_zip};
 use pbs_config::CachedUserInfo;
@@ -73,8 +71,8 @@ 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::server::jobstate::{compute_schedule_status, Job, JobState};
+use crate::tools::{backup_info_to_snapshot_list_item, get_all_snapshot_files, read_backup_index};
 
 // helper to unify common sequence of checks:
 // 1. check privs on NS (full or limited access)
@@ -101,56 +99,6 @@ fn check_privs_and_load_store(
     Ok(datastore)
 }
 
-fn read_backup_index(
-    backup_dir: &BackupDir,
-) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
-    let (manifest, index_size) = backup_dir.load_manifest()?;
-
-    let mut result = Vec::new();
-    for item in manifest.files() {
-        result.push(BackupContent {
-            filename: item.filename.clone(),
-            crypt_mode: Some(item.crypt_mode),
-            size: Some(item.size),
-        });
-    }
-
-    result.push(BackupContent {
-        filename: MANIFEST_BLOB_NAME.to_string(),
-        crypt_mode: match manifest.signature {
-            Some(_) => Some(CryptMode::SignOnly),
-            None => Some(CryptMode::None),
-        },
-        size: Some(index_size),
-    });
-
-    Ok((manifest, result))
-}
-
-fn get_all_snapshot_files(
-    info: &BackupInfo,
-) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
-    let (manifest, mut files) = read_backup_index(&info.backup_dir)?;
-
-    let file_set = files.iter().fold(HashSet::new(), |mut acc, item| {
-        acc.insert(item.filename.clone());
-        acc
-    });
-
-    for file in &info.files {
-        if file_set.contains(file) {
-            continue;
-        }
-        files.push(BackupContent {
-            filename: file.to_string(),
-            size: None,
-            crypt_mode: None,
-        });
-    }
-
-    Ok((manifest, files))
-}
-
 #[api(
     input: {
         properties: {
@@ -516,73 +464,6 @@ unsafe fn list_snapshots_blocking(
         (None, None) => datastore.list_backup_groups(ns.clone())?,
     };
 
-    let info_to_snapshot_list_item = |owner, info: BackupInfo| {
-        let backup = info.backup_dir.dir().to_owned();
-        let protected = info.protected;
-
-        match get_all_snapshot_files(&info) {
-            Ok((manifest, files)) => {
-                // extract the first line from notes
-                let comment: Option<String> = manifest.unprotected["notes"]
-                    .as_str()
-                    .and_then(|notes| notes.lines().next())
-                    .map(String::from);
-
-                let fingerprint = match manifest.fingerprint() {
-                    Ok(fp) => fp,
-                    Err(err) => {
-                        eprintln!("error parsing fingerprint: '{}'", err);
-                        None
-                    }
-                };
-
-                let verification: Option<SnapshotVerifyState> = match manifest.verify_state() {
-                    Ok(verify) => verify,
-                    Err(err) => {
-                        eprintln!("error parsing verification state : '{}'", err);
-                        None
-                    }
-                };
-
-                let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
-
-                SnapshotListItem {
-                    backup,
-                    comment,
-                    verification,
-                    fingerprint,
-                    files,
-                    size,
-                    owner,
-                    protected,
-                }
-            }
-            Err(err) => {
-                eprintln!("error during snapshot file listing: '{}'", err);
-                let files = info
-                    .files
-                    .into_iter()
-                    .map(|filename| BackupContent {
-                        filename,
-                        size: None,
-                        crypt_mode: None,
-                    })
-                    .collect();
-
-                SnapshotListItem {
-                    backup,
-                    comment: None,
-                    verification: None,
-                    fingerprint: None,
-                    files,
-                    size: None,
-                    owner,
-                    protected,
-                }
-            }
-        }
-    };
-
     groups.iter().try_fold(Vec::new(), |mut snapshots, group| {
         let owner = match group.get_owner() {
             Ok(auth_id) => auth_id,
@@ -606,7 +487,7 @@ unsafe fn list_snapshots_blocking(
         snapshots.extend(
             group_backups
                 .into_iter()
-                .map(|info| info_to_snapshot_list_item(Some(owner.clone()), info)),
+                .map(|info| backup_info_to_snapshot_list_item(&info, &owner)),
         );
 
         Ok(snapshots)
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
index 322894dd7..6556effe3 100644
--- a/src/tools/mod.rs
+++ b/src/tools/mod.rs
@@ -3,9 +3,16 @@
 //! This is a collection of small and useful tools.
 
 use anyhow::{bail, Error};
+use std::collections::HashSet;
 
+use pbs_api_types::{
+    Authid, BackupContent, CryptMode, SnapshotListItem, SnapshotVerifyState, MANIFEST_BLOB_NAME,
+};
 use proxmox_http::{client::Client, HttpOptions, ProxyConfig};
 
+use pbs_datastore::backup_info::{BackupDir, BackupInfo};
+use pbs_datastore::manifest::BackupManifest;
+
 pub mod config;
 pub mod disks;
 pub mod fs;
@@ -61,3 +68,125 @@ pub fn setup_safe_path_env() {
         std::env::remove_var(name);
     }
 }
+
+pub(crate) fn read_backup_index(
+    backup_dir: &BackupDir,
+) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
+    let (manifest, index_size) = backup_dir.load_manifest()?;
+
+    let mut result = Vec::new();
+    for item in manifest.files() {
+        result.push(BackupContent {
+            filename: item.filename.clone(),
+            crypt_mode: Some(item.crypt_mode),
+            size: Some(item.size),
+        });
+    }
+
+    result.push(BackupContent {
+        filename: MANIFEST_BLOB_NAME.to_string(),
+        crypt_mode: match manifest.signature {
+            Some(_) => Some(CryptMode::SignOnly),
+            None => Some(CryptMode::None),
+        },
+        size: Some(index_size),
+    });
+
+    Ok((manifest, result))
+}
+
+pub(crate) fn get_all_snapshot_files(
+    info: &BackupInfo,
+) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
+    let (manifest, mut files) = read_backup_index(&info.backup_dir)?;
+
+    let file_set = files.iter().fold(HashSet::new(), |mut acc, item| {
+        acc.insert(item.filename.clone());
+        acc
+    });
+
+    for file in &info.files {
+        if file_set.contains(file) {
+            continue;
+        }
+        files.push(BackupContent {
+            filename: file.to_string(),
+            size: None,
+            crypt_mode: None,
+        });
+    }
+
+    Ok((manifest, files))
+}
+
+/// Helper to transform `BackupInfo` to `SnapshotListItem` with given owner.
+pub(crate) fn backup_info_to_snapshot_list_item(
+    info: &BackupInfo,
+    owner: &Authid,
+) -> SnapshotListItem {
+    let backup = info.backup_dir.dir().to_owned();
+    let protected = info.protected;
+    let owner = Some(owner.to_owned());
+
+    match get_all_snapshot_files(info) {
+        Ok((manifest, files)) => {
+            // extract the first line from notes
+            let comment: Option<String> = manifest.unprotected["notes"]
+                .as_str()
+                .and_then(|notes| notes.lines().next())
+                .map(String::from);
+
+            let fingerprint = match manifest.fingerprint() {
+                Ok(fp) => fp,
+                Err(err) => {
+                    eprintln!("error parsing fingerprint: '{}'", err);
+                    None
+                }
+            };
+
+            let verification: Option<SnapshotVerifyState> = match manifest.verify_state() {
+                Ok(verify) => verify,
+                Err(err) => {
+                    eprintln!("error parsing verification state : '{err}'");
+                    None
+                }
+            };
+
+            let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
+
+            SnapshotListItem {
+                backup,
+                comment,
+                verification,
+                fingerprint,
+                files,
+                size,
+                owner,
+                protected,
+            }
+        }
+        Err(err) => {
+            eprintln!("error during snapshot file listing: '{err}'");
+            let files = info
+                .files
+                .iter()
+                .map(|filename| BackupContent {
+                    filename: filename.to_owned(),
+                    size: None,
+                    crypt_mode: None,
+                })
+                .collect();
+
+            SnapshotListItem {
+                backup,
+                comment: None,
+                verification: None,
+                fingerprint: None,
+                files,
+                size: None,
+                owner,
+                protected,
+            }
+        }
+    }
+}
-- 
2.47.2



_______________________________________________
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/5] sync: source: list snapshot items instead of backup directories

[Christian Ebner]

The snapshot list items contain further information such as the
source verify and encryption state, so that allows filtering based on
these information after list generation already. Since this
information is already returned by the api calls in case of remote
sources and obtained by switching to `BackupGroup::list_backups`
instead of `BackupGroup::iter_snapshots` for local sources, return
the whole snapshot list item information instead of reducing it to
the subset containing the backup directory only when reading from
source.

Adapts the trait accordingly and renames `list_backup_dirs` to the
now better fitting `list_backup_snapshots`.

No functional changes intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/pull.rs | 16 +++++++++-------
 src/server/push.rs | 17 ++++++++++-------
 src/server/sync.rs | 39 ++++++++++++++++++++++-----------------
 3 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index a4cc1d52b..51c8aa2ea 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -12,9 +12,9 @@ use tracing::info;
 
 use pbs_api_types::{
     print_store_and_ns, ArchiveType, Authid, BackupArchiveName, BackupDir, BackupGroup,
-    BackupNamespace, GroupFilter, Operation, RateLimitConfig, Remote, VerifyState,
-    CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, PRIV_DATASTORE_AUDIT,
-    PRIV_DATASTORE_BACKUP,
+    BackupNamespace, GroupFilter, Operation, RateLimitConfig, Remote, SnapshotListItem,
+    VerifyState, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH,
+    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
 };
 use pbs_client::BackupRepository;
 use pbs_config::CachedUserInfo;
@@ -607,11 +607,11 @@ async fn pull_group(
     let mut already_synced_skip_info = SkipInfo::new(SkipReason::AlreadySynced);
     let mut transfer_last_skip_info = SkipInfo::new(SkipReason::TransferLast);
 
-    let mut raw_list: Vec<BackupDir> = params
+    let mut raw_list: Vec<SnapshotListItem> = params
         .source
-        .list_backup_dirs(source_namespace, group)
+        .list_backup_snapshots(source_namespace, group)
         .await?;
-    raw_list.sort_unstable_by(|a, b| a.time.cmp(&b.time));
+    raw_list.sort_unstable_by(|a, b| a.backup.time.cmp(&b.backup.time));
 
     let total_amount = raw_list.len();
 
@@ -634,7 +634,9 @@ async fn pull_group(
     let list: Vec<(BackupDir, bool)> = raw_list
         .into_iter()
         .enumerate()
-        .filter_map(|(pos, dir)| {
+        .filter_map(|(pos, item)| {
+            let dir = item.backup;
+
             source_snapshots.insert(dir.time);
             // If resync_corrupt is set, check if the corresponding local snapshot failed to
             // verification
diff --git a/src/server/push.rs b/src/server/push.rs
index 4a25d51cf..879fe3288 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -674,8 +674,11 @@ pub(crate) async fn push_group(
     let mut already_synced_skip_info = SkipInfo::new(SkipReason::AlreadySynced);
     let mut transfer_last_skip_info = SkipInfo::new(SkipReason::TransferLast);
 
-    let mut snapshots: Vec<BackupDir> = params.source.list_backup_dirs(namespace, group).await?;
-    snapshots.sort_unstable_by(|a, b| a.time.cmp(&b.time));
+    let mut snapshots: Vec<SnapshotListItem> = params
+        .source
+        .list_backup_snapshots(namespace, group)
+        .await?;
+    snapshots.sort_unstable_by(|a, b| a.backup.time.cmp(&b.backup.time));
 
     if snapshots.is_empty() {
         info!("Group '{group}' contains no snapshots to sync to remote");
@@ -700,19 +703,19 @@ pub(crate) async fn push_group(
     let snapshots: Vec<BackupDir> = snapshots
         .into_iter()
         .enumerate()
-        .filter(|&(pos, ref snapshot)| {
+        .filter_map(|(pos, item)| {
+            let snapshot = item.backup;
             source_snapshots.insert(snapshot.time);
             if last_snapshot_time >= snapshot.time {
                 already_synced_skip_info.update(snapshot.time);
-                return false;
+                return None;
             }
             if pos < cutoff {
                 transfer_last_skip_info.update(snapshot.time);
-                return false;
+                return None;
             }
-            true
+            Some(snapshot)
         })
-        .map(|(_, dir)| dir)
         .collect();
 
     if already_synced_skip_info.count > 0 {
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 9238a8626..86ff64c60 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -32,6 +32,7 @@ use crate::backup::ListAccessibleBackupGroups;
 use crate::server::jobstate::Job;
 use crate::server::pull::{pull_store, PullParameters};
 use crate::server::push::{push_store, PushParameters};
+use crate::tools::backup_info_to_snapshot_list_item;
 
 #[derive(Default)]
 pub(crate) struct RemovedVanishedStats {
@@ -238,11 +239,11 @@ pub(crate) trait SyncSource: Send + Sync {
     ) -> Result<Vec<BackupGroup>, Error>;
 
     /// Lists backup directories for a specific group within a specific namespace from the source.
-    async fn list_backup_dirs(
+    async fn list_backup_snapshots(
         &self,
         namespace: &BackupNamespace,
         group: &BackupGroup,
-    ) -> Result<Vec<BackupDir>, Error>;
+    ) -> Result<Vec<SnapshotListItem>, Error>;
     fn get_ns(&self) -> BackupNamespace;
     fn get_store(&self) -> &str;
 
@@ -351,11 +352,11 @@ impl SyncSource for RemoteSource {
         )
     }
 
-    async fn list_backup_dirs(
+    async fn list_backup_snapshots(
         &self,
         namespace: &BackupNamespace,
         group: &BackupGroup,
-    ) -> Result<Vec<BackupDir>, Error> {
+    ) -> Result<Vec<SnapshotListItem>, Error> {
         let path = format!("api2/json/admin/datastore/{}/snapshots", self.repo.store());
 
         let mut args = json!({
@@ -374,16 +375,15 @@ impl SyncSource for RemoteSource {
         Ok(snapshot_list
             .into_iter()
             .filter_map(|item: SnapshotListItem| {
-                let snapshot = item.backup;
                 // in-progress backups can't be synced
                 if item.size.is_none() {
-                    info!("skipping snapshot {snapshot} - in-progress backup");
+                    info!("skipping snapshot {} - in-progress backup", item.backup);
                     return None;
                 }
 
-                Some(snapshot)
+                Some(item)
             })
-            .collect::<Vec<BackupDir>>())
+            .collect::<Vec<SnapshotListItem>>())
     }
 
     fn get_ns(&self) -> BackupNamespace {
@@ -445,18 +445,23 @@ impl SyncSource for LocalSource {
         .collect::<Vec<pbs_api_types::BackupGroup>>())
     }
 
-    async fn list_backup_dirs(
+    async fn list_backup_snapshots(
         &self,
         namespace: &BackupNamespace,
         group: &BackupGroup,
-    ) -> Result<Vec<BackupDir>, Error> {
-        Ok(self
-            .store
-            .backup_group(namespace.clone(), group.clone())
-            .iter_snapshots()?
-            .filter_map(Result::ok)
-            .map(|snapshot| snapshot.dir().to_owned())
-            .collect::<Vec<BackupDir>>())
+    ) -> Result<Vec<SnapshotListItem>, Error> {
+        let backup_group = self.store.backup_group(namespace.clone(), group.clone());
+        Ok(backup_group
+            .list_backups()?
+            .iter()
+            .filter_map(|info| {
+                let owner = match backup_group.get_owner() {
+                    Ok(owner) => owner,
+                    Err(_) => return None,
+                };
+                Some(backup_info_to_snapshot_list_item(info, &owner))
+            })
+            .collect::<Vec<SnapshotListItem>>())
     }
 
     fn get_ns(&self) -> BackupNamespace {
-- 
2.47.2



_______________________________________________
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/5] pull: refactor source snapshot list filtering logic

[Christian Ebner]

In preparation for filter based on the snapshot's verify and
encryption state before applying the transfer last cutoff.

This will allow to correctly use the transfer last filter on the
correspondingly prefiltered items only.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/pull.rs | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 51c8aa2ea..ee9daf4cf 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -613,13 +613,6 @@ async fn pull_group(
         .await?;
     raw_list.sort_unstable_by(|a, b| a.backup.time.cmp(&b.backup.time));
 
-    let total_amount = raw_list.len();
-
-    let cutoff = params
-        .transfer_last
-        .map(|count| total_amount.saturating_sub(count))
-        .unwrap_or_default();
-
     let target_ns = source_namespace.map_prefix(&params.source.get_ns(), &params.target.ns)?;
 
     let mut source_snapshots = HashSet::new();
@@ -633,8 +626,7 @@ async fn pull_group(
     // Also stores if the snapshot is corrupt (verification job failed)
     let list: Vec<(BackupDir, bool)> = raw_list
         .into_iter()
-        .enumerate()
-        .filter_map(|(pos, item)| {
+        .filter_map(|item| {
             let dir = item.backup;
 
             source_snapshots.insert(dir.time);
@@ -666,6 +658,20 @@ async fn pull_group(
                     }
                 }
             }
+            Some((dir, false))
+        })
+        .collect();
+
+    let total_amount = list.len();
+    let cutoff = params
+        .transfer_last
+        .map(|count| total_amount.saturating_sub(count))
+        .unwrap_or_default();
+
+    let list: Vec<(BackupDir, bool)> = list
+        .into_iter()
+        .enumerate()
+        .filter_map(|(pos, (dir, verified))| {
             // Note: the snapshot represented by `last_sync_time` might be missing its backup log
             // or post-backup verification state if those were not yet available during the last
             // sync run, always resync it
@@ -677,7 +683,7 @@ async fn pull_group(
                 transfer_last_skip_info.update(dir.time);
                 return None;
             }
-            Some((dir, false))
+            Some((dir, verified))
         })
         .collect();
 
-- 
2.47.2



_______________________________________________
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/5] sync: conditionally pre-filter source list by verified or encrypted state

[Christian Ebner]

Commit 40ccd1ac ("fix #6072: server: sync encrypted or verified
snapshots only") introduced flags for sync jobs to include encrypted
or verified snapshots only. The chosen approach to check the snapshot
state right before sync only does however lead to possibly unexpected
results if the jobs parameters also include a transfer last setting,
as it is applied beforehand.

In order to improve this behaviour, already pre-filter the snapshot
source list if either the `verified-only` or the `encrypted-only`
flags are set.

The state has to be re-checked once again for consistency reasons,
when the snapshot is read locked and about to be synced. Snapshots
are not included if the state would match after the list generation
and will be excluded if the state no longer matches.

This is a breaking change in the filter logic.

Reported-by: Stefan Hanreich <s.hanreich@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/pull.rs | 11 ++++++++---
 src/server/push.rs | 31 +++++++++++++++++++++----------
 src/server/sync.rs | 34 +++++++++++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index ee9daf4cf..e1c8a7acd 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -28,8 +28,9 @@ use pbs_datastore::{check_backup_owner, DataStore, DatastoreBackend, StoreProgre
 use pbs_tools::sha::sha256;
 
 use super::sync::{
-    check_namespace_depth_limit, ignore_not_verified_or_encrypted, LocalSource, RemoteSource,
-    RemovedVanishedStats, SkipInfo, SkipReason, SyncSource, SyncSourceReader, SyncStats,
+    check_namespace_depth_limit, exclude_not_verified_or_encrypted,
+    ignore_not_verified_or_encrypted, LocalSource, RemoteSource, RemovedVanishedStats, SkipInfo,
+    SkipReason, SyncSource, SyncSourceReader, SyncStats,
 };
 use crate::backup::{check_ns_modification_privs, check_ns_privs};
 use crate::tools::parallel_handler::ParallelHandler;
@@ -627,8 +628,12 @@ async fn pull_group(
     let list: Vec<(BackupDir, bool)> = raw_list
         .into_iter()
         .filter_map(|item| {
-            let dir = item.backup;
+            if exclude_not_verified_or_encrypted(&item, params.verified_only, params.encrypted_only)
+            {
+                return None;
+            }
 
+            let dir = item.backup;
             source_snapshots.insert(dir.time);
             // If resync_corrupt is set, check if the corresponding local snapshot failed to
             // verification
diff --git a/src/server/push.rs b/src/server/push.rs
index 879fe3288..d7884fce2 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -28,8 +28,9 @@ use pbs_datastore::read_chunk::AsyncReadChunk;
 use pbs_datastore::{DataStore, StoreProgress};
 
 use super::sync::{
-    check_namespace_depth_limit, ignore_not_verified_or_encrypted, LocalSource,
-    RemovedVanishedStats, SkipInfo, SkipReason, SyncSource, SyncStats,
+    check_namespace_depth_limit, exclude_not_verified_or_encrypted,
+    ignore_not_verified_or_encrypted, LocalSource, RemovedVanishedStats, SkipInfo, SkipReason,
+    SyncSource, SyncStats,
 };
 use crate::api2::config::remote;
 
@@ -684,12 +685,6 @@ pub(crate) async fn push_group(
         info!("Group '{group}' contains no snapshots to sync to remote");
     }
 
-    let total_snapshots = snapshots.len();
-    let cutoff = params
-        .transfer_last
-        .map(|count| total_snapshots.saturating_sub(count))
-        .unwrap_or_default();
-
     let target_namespace = params.map_to_target(namespace)?;
     let mut target_snapshots = fetch_target_snapshots(params, &target_namespace, group).await?;
     target_snapshots.sort_unstable_by_key(|a| a.backup.time);
@@ -700,11 +695,27 @@ pub(crate) async fn push_group(
         .unwrap_or(i64::MIN);
 
     let mut source_snapshots = HashSet::new();
+    let snapshots: Vec<BackupDir> = snapshots
+        .into_iter()
+        .filter_map(|item| {
+            if exclude_not_verified_or_encrypted(&item, params.verified_only, params.encrypted_only)
+            {
+                return None;
+            }
+            Some(item.backup)
+        })
+        .collect();
+
+    let total_snapshots = snapshots.len();
+    let cutoff = params
+        .transfer_last
+        .map(|count| total_snapshots.saturating_sub(count))
+        .unwrap_or_default();
+
     let snapshots: Vec<BackupDir> = snapshots
         .into_iter()
         .enumerate()
-        .filter_map(|(pos, item)| {
-            let snapshot = item.backup;
+        .filter_map(|(pos, snapshot)| {
             source_snapshots.insert(snapshot.time);
             if last_snapshot_time >= snapshot.time {
                 already_synced_skip_info.update(snapshot.time);
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 86ff64c60..50e41ae5c 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -20,8 +20,8 @@ use proxmox_router::HttpError;
 
 use pbs_api_types::{
     Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupListItem, SnapshotListItem,
-    SyncDirection, SyncJobConfig, VerifyState, CLIENT_LOG_BLOB_NAME, MAX_NAMESPACE_DEPTH,
-    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
+    SyncDirection, SyncJobConfig, VerifyState, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME,
+    MAX_NAMESPACE_DEPTH, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
 };
 use pbs_client::{BackupReader, BackupRepository, HttpClient, RemoteChunkReader};
 use pbs_datastore::data_blob::DataBlob;
@@ -755,10 +755,38 @@ pub(super) fn ignore_not_verified_or_encrypted(
             .iter()
             .all(|file| file.chunk_crypt_mode() == CryptMode::Encrypt)
         {
-            info!("Snapshot {snapshot} not encrypted but encrypted-only set, snapshot skipped");
             return true;
         }
     }
 
     false
 }
+
+pub(super) fn exclude_not_verified_or_encrypted(
+    item: &SnapshotListItem,
+    verified_only: bool,
+    encrypted_only: bool,
+) -> bool {
+    if verified_only {
+        match &item.verification {
+            Some(state) if state.state == VerifyState::Ok => (),
+            _ => return true,
+        }
+    }
+
+    if encrypted_only
+        && !item.files.iter().all(|content| {
+            if content.filename == MANIFEST_BLOB_NAME.as_ref() {
+                content.crypt_mode == Some(CryptMode::SignOnly)
+            } else if content.filename == CLIENT_LOG_BLOB_NAME.as_ref() {
+                true
+            } else {
+                content.crypt_mode == Some(CryptMode::Encrypt)
+            }
+        })
+    {
+        return true;
+    }
+
+    false
+}
-- 
2.47.2



_______________________________________________
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 0/5] prefilter source list by encryption/verify state for sync jobs

[Thomas Lamprecht]

On Tue, 29 Jul 2025 11:31:34 +0200, Christian Ebner wrote:
> Since the introduction of the `verified-only` and `encrypted-only`
> optional parameters for sync jobs, it is possible to exclude snapshot
> if their are not encrypted or verified from the sync.
> 
> The current implementation, filtering the snapshots based on the
> manifest information only directly before syncing said snapshot, leads
> to the somewhat unexpected behaviour that in combination with the
> transfer-last parameter set, the transfer-last filtering is performed
> before the encryption/verificaton state filtering.
> 
> [...]

Applied, thanks!

[1/5] api: admin: refactor generation of backup dir for snapshot listing
      commit: 6225f6d4f1e13b5653b4edb86fc8c85d5787ee2f
[2/5] api: factor out helper converting backup info to snapshot list item
      commit: d18ff90417939ad655dc93b34f34f9abacc79b1f
[3/5] sync: source: list snapshot items instead of backup directories
      commit: a86638b5d39a92e2ce26d5d0c243aa65947bca26
[4/5] pull: refactor source snapshot list filtering logic
      commit: eea1176f8271f0a57576fe8f8fba363a6d24ed3a
[5/5] sync: conditionally pre-filter source list by verified or encrypted state
      commit: 793c4368f66547803d0ceb3c0d4d1ee8dca12533


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