[pbs-devel] [PATCH proxmox-backup v2 5/5] sync: conditionally pre-filter source list by verified or encrypted state

[Christian Ebner <c.ebner@proxmox.com>]

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