[pbs-devel] [PATCH proxmox-backup 1/3] sync: fix premature return in snapshot skip filter logic

[pbs-devel] [PATCH proxmox-backup 1/3] sync: fix premature return in snapshot skip filter logic

[Christian Ebner]

While checking which snapshots to sync, the filter logic incorrectly
included the first snapshot newer that the last synced one
unconditionally, bypassing the transfer last check for that one
snapshot. Following snapshots are correctly handled again.

E.g. of an incorrect sync by excerpt of a task log provided by a user
in the community forum [0], with transfer last set to 1:

```
skipped: 2 snapshot(s) (2024-09-29T18:00:28Z .. 2024-10-20T18:00:29Z) - older than the newest local snapshot
skipped: 5 snapshot(s) (2024-10-28T19:00:28Z .. 2024-11-01T19:00:32Z) - due to transfer-last
sync snapshot vm/110/2024-10-27T19:00:25Z
...
sync snapshot vm/110/2024-11-02T19:00:23Z
```

Not only the last, but the first newer than newest and last were
incorrectly synced.

By dropping the early return, leading to incorrect inclusion of the
snapshot, the transfer last condition is now correctly checked as
well.

Link to the issue reported in the community forum:
[0] https://forum.proxmox.com/threads/156873/

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/pull.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 3117f7d2c..cc1427196 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -534,7 +534,6 @@ async fn pull_group(
             } else if already_synced_skip_info.count > 0 {
                 info!("{already_synced_skip_info}");
                 already_synced_skip_info.reset();
-                return true;
             }
 
             if pos < cutoff && last_sync_time != dir.time {
-- 
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/3] sync: pull: do not resync currently newest snapshot on target

[Christian Ebner]

The currently newest snapshot of a group on the sync target is not
excluded from the list of already synced snapshots, leading to a
re-sync.

Filter out the snapshot as well.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Might be ignored if the re-sync is intetional.

Implementation already present since commit:
de8ec041 ("src/api2/sync.rs: implement remote sync")

 src/server/pull.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index cc1427196..7aa191d96 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -528,7 +528,7 @@ async fn pull_group(
         .enumerate()
         .filter(|&(pos, ref dir)| {
             source_snapshots.insert(dir.time);
-            if last_sync_time > dir.time {
+            if last_sync_time >= dir.time {
                 already_synced_skip_info.update(dir.time);
                 return false;
             } else if already_synced_skip_info.count > 0 {
@@ -536,7 +536,7 @@ async fn pull_group(
                 already_synced_skip_info.reset();
             }
 
-            if pos < cutoff && last_sync_time != dir.time {
+            if pos < cutoff {
                 transfer_last_skip_info.update(dir.time);
                 return false;
             } else if transfer_last_skip_info.count > 0 {
-- 
2.39.5



_______________________________________________
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 2/3] sync: pull: do not resync currently newest snapshot on target

[Fabian Grünbichler]

On November 4, 2024 11:58 am, Christian Ebner wrote:
> The currently newest snapshot of a group on the sync target is not
> excluded from the list of already synced snapshots, leading to a
> re-sync.

this is intentional, the last snapshot might not have been completely
done on the source side when we last synced it (e.g., backup log still
missing, post-backup verification not done yet, ..).

> 
> Filter out the snapshot as well.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Might be ignored if the re-sync is intetional.
> 
> Implementation already present since commit:
> de8ec041 ("src/api2/sync.rs: implement remote sync")
> 
>  src/server/pull.rs | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index cc1427196..7aa191d96 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -528,7 +528,7 @@ async fn pull_group(
>          .enumerate()
>          .filter(|&(pos, ref dir)| {
>              source_snapshots.insert(dir.time);
> -            if last_sync_time > dir.time {
> +            if last_sync_time >= dir.time {
>                  already_synced_skip_info.update(dir.time);
>                  return false;
>              } else if already_synced_skip_info.count > 0 {
> @@ -536,7 +536,7 @@ async fn pull_group(
>                  already_synced_skip_info.reset();
>              }
>  
> -            if pos < cutoff && last_sync_time != dir.time {
> +            if pos < cutoff {
>                  transfer_last_skip_info.update(dir.time);
>                  return false;
>              } else if transfer_last_skip_info.count > 0 {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
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 2/3] sync: pull: do not resync currently newest snapshot on target

[Christian Ebner]

On 11/4/24 13:15, Fabian Grünbichler wrote:
> On November 4, 2024 11:58 am, Christian Ebner wrote:
>> The currently newest snapshot of a group on the sync target is not
>> excluded from the list of already synced snapshots, leading to a
>> re-sync.
> 
> this is intentional, the last snapshot might not have been completely
> done on the source side when we last synced it (e.g., backup log still
> missing, post-backup verification not done yet, ..).

Ah I see, thanks for clarification: was already expecting this to be 
more subtle.

Will amend the patch to explicitly mention this as a comment.


_______________________________________________
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/3] sync: pull: simplify logic for source snapshot filtering

[Christian Ebner]

Decouple the actual filter logic from the skip reason output logic by
pulling the latter out of the filter closue.

Makes the filtering logic more intuitive.

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

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 7aa191d96..8f00ae0af 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -531,23 +531,25 @@ async fn pull_group(
             if last_sync_time >= dir.time {
                 already_synced_skip_info.update(dir.time);
                 return false;
-            } else if already_synced_skip_info.count > 0 {
-                info!("{already_synced_skip_info}");
-                already_synced_skip_info.reset();
             }
-
             if pos < cutoff {
                 transfer_last_skip_info.update(dir.time);
                 return false;
-            } else if transfer_last_skip_info.count > 0 {
-                info!("{transfer_last_skip_info}");
-                transfer_last_skip_info.reset();
             }
             true
         })
         .map(|(_, dir)| dir)
         .collect();
 
+    if already_synced_skip_info.count > 0 {
+        info!("{already_synced_skip_info}");
+        already_synced_skip_info.reset();
+    }
+    if transfer_last_skip_info.count > 0 {
+        info!("{transfer_last_skip_info}");
+        transfer_last_skip_info.reset();
+    }
+
     // start with 65536 chunks (up to 256 GiB)
     let downloaded_chunks = Arc::new(Mutex::new(HashSet::with_capacity(1024 * 64)));
 
-- 
2.39.5



_______________________________________________
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 3/3] sync: pull: simplify logic for source snapshot filtering

[Fabian Grünbichler]

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

but needs a rebase cause of patch#2 ;)

On November 4, 2024 11:58 am, Christian Ebner wrote:
> Decouple the actual filter logic from the skip reason output logic by
> pulling the latter out of the filter closue.
> 
> Makes the filtering logic more intuitive.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/server/pull.rs | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 7aa191d96..8f00ae0af 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -531,23 +531,25 @@ async fn pull_group(
>              if last_sync_time >= dir.time {
>                  already_synced_skip_info.update(dir.time);
>                  return false;
> -            } else if already_synced_skip_info.count > 0 {
> -                info!("{already_synced_skip_info}");
> -                already_synced_skip_info.reset();
>              }
> -
>              if pos < cutoff {
>                  transfer_last_skip_info.update(dir.time);
>                  return false;
> -            } else if transfer_last_skip_info.count > 0 {
> -                info!("{transfer_last_skip_info}");
> -                transfer_last_skip_info.reset();
>              }
>              true
>          })
>          .map(|(_, dir)| dir)
>          .collect();
>  
> +    if already_synced_skip_info.count > 0 {
> +        info!("{already_synced_skip_info}");
> +        already_synced_skip_info.reset();
> +    }
> +    if transfer_last_skip_info.count > 0 {
> +        info!("{transfer_last_skip_info}");
> +        transfer_last_skip_info.reset();
> +    }
> +
>      // start with 65536 chunks (up to 256 GiB)
>      let downloaded_chunks = Arc::new(Mutex::new(HashSet::with_capacity(1024 * 64)));
>  
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

[pbs-devel] partially-applied: [PATCH proxmox-backup 1/3] sync: fix premature return in snapshot skip filter logic

[Fabian Grünbichler]

applied this one!

On November 4, 2024 11:58 am, Christian Ebner wrote:
> While checking which snapshots to sync, the filter logic incorrectly
> included the first snapshot newer that the last synced one
> unconditionally, bypassing the transfer last check for that one
> snapshot. Following snapshots are correctly handled again.
> 
> E.g. of an incorrect sync by excerpt of a task log provided by a user
> in the community forum [0], with transfer last set to 1:
> 
> ```
> skipped: 2 snapshot(s) (2024-09-29T18:00:28Z .. 2024-10-20T18:00:29Z) - older than the newest local snapshot
> skipped: 5 snapshot(s) (2024-10-28T19:00:28Z .. 2024-11-01T19:00:32Z) - due to transfer-last
> sync snapshot vm/110/2024-10-27T19:00:25Z
> ...
> sync snapshot vm/110/2024-11-02T19:00:23Z
> ```
> 
> Not only the last, but the first newer than newest and last were
> incorrectly synced.
> 
> By dropping the early return, leading to incorrect inclusion of the
> snapshot, the transfer last condition is now correctly checked as
> well.
> 
> Link to the issue reported in the community forum:
> [0] https://forum.proxmox.com/threads/156873/
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/server/pull.rs | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 3117f7d2c..cc1427196 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -534,7 +534,6 @@ async fn pull_group(
>              } else if already_synced_skip_info.count > 0 {
>                  info!("{already_synced_skip_info}");
>                  already_synced_skip_info.reset();
> -                return true;
>              }
>  
>              if pos < cutoff && last_sync_time != dir.time {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

Re: [pbs-devel] partially-applied: [PATCH proxmox-backup 1/3] sync: fix premature return in snapshot skip filter logic

[Christian Ebner]

superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20241104125614.162491-1-c.ebner@proxmox.com/T/


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