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

[pbs-devel] [PATCH proxmox-backup 0/5] prefilter source list by 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.

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(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 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 392494488..55cb3a0b3 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -62,8 +62,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, LocalChunkReader, StoreProgress,
+    check_backup_owner, ensure_datastore_is_mounted, task_tracking, BackupDir, DataStore,
+    LocalChunkReader, StoreProgress,
 };
 use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{formatter, WorkerTask};
@@ -529,11 +529,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) {
@@ -622,7 +619,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.39.5



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

[pbs-devel] [PATCH proxmox-backup 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 55cb3a0b3..9b3ffb8e0 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;
@@ -41,13 +40,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,
-    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, 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;
@@ -74,8 +72,8 @@ use crate::backup::{
     check_ns_privs_full, verify_all_backups, verify_backup_dir, verify_backup_group, verify_filter,
     ListAccessibleBackupGroups, 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};
 
 const GROUP_NOTES_FILE_NAME: &str = "notes";
 
@@ -114,56 +112,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: {
@@ -529,73 +477,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,
@@ -619,7 +500,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.39.5



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

[pbs-devel] [PATCH proxmox-backup 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 b1724c142..832ee1b85 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;
@@ -541,11 +541,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();
 
@@ -568,7 +568,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 e71012ed8..fdfed1b5a 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -672,8 +672,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");
@@ -698,19 +701,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 09814ef0c..155a0cb4b 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 {
@@ -244,11 +245,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;
 
@@ -357,11 +358,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!({
@@ -380,16 +381,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 {
@@ -451,18 +451,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.39.5



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

[pbs-devel] [PATCH proxmox-backup 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 832ee1b85..53de74f19 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -547,13 +547,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();
@@ -567,8 +560,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);
@@ -600,6 +592,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
@@ -611,7 +617,7 @@ async fn pull_group(
                 transfer_last_skip_info.update(dir.time);
                 return None;
             }
-            Some((dir, false))
+            Some((dir, verified))
         })
         .collect();
 
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup 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 53de74f19..49f1d7a66 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -28,8 +28,9 @@ use pbs_datastore::{check_backup_owner, DataStore, StoreProgress};
 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;
@@ -561,8 +562,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 fdfed1b5a..d9a7aea8a 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -26,8 +26,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;
 
@@ -682,12 +683,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);
@@ -698,11 +693,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 155a0cb4b..8c0569672 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;
@@ -761,10 +761,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.39.5



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