* [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots
@ 2024-11-27 8:26 Fabian Grünbichler
2024-11-27 9:05 ` Dominik Csapak
2024-11-27 9:05 ` [pbs-devel] applied: " Thomas Lamprecht
0 siblings, 2 replies; 5+ messages in thread
From: Fabian Grünbichler @ 2024-11-27 8:26 UTC (permalink / raw)
To: pbs-devel
when loading the verification state for a local snapshot, it must first be
ensured that it actually exists, else the lack of manifest will be interpreted
as corrupt snapshot triggering a "resync" that is actually a sync of all
missing snapshots.
Fixes: 0974ddfa17be018f777d6ece90a71bfa8fc130d8 "fix #3786: api: add resync-corrupt option to sync jobs"
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
src/server/pull.rs | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 9abb673ae..361ed0687 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -551,21 +551,23 @@ async fn pull_group(
.store
.backup_dir(target_ns.clone(), dir.clone());
if let Ok(local_dir) = local_dir {
- match local_dir.verify_state() {
- Ok(Some(state)) => {
- if state == VerifyState::Failed {
+ if local_dir.full_path().exists() {
+ match local_dir.verify_state() {
+ Ok(Some(state)) => {
+ if state == VerifyState::Failed {
+ return Some((dir, true));
+ }
+ }
+ Ok(None) => {
+ // The verify_state item was not found in the manifest, this means the
+ // snapshot is new.
+ }
+ Err(_) => {
+ // There was an error loading the manifest, probably better if we
+ // resync.
return Some((dir, true));
}
}
- Ok(None) => {
- // The verify_state item was not found in the manifest, this means the
- // snapshot is new.
- }
- Err(_) => {
- // There was an error loading the manifest, probably better if we
- // resync.
- return Some((dir, true));
- }
}
}
}
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots
2024-11-27 8:26 [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots Fabian Grünbichler
@ 2024-11-27 9:05 ` Dominik Csapak
2024-11-27 9:17 ` Fabian Grünbichler
2024-11-27 9:05 ` [pbs-devel] applied: " Thomas Lamprecht
1 sibling, 1 reply; 5+ messages in thread
From: Dominik Csapak @ 2024-11-27 9:05 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Fabian Grünbichler
Code wise it looks good to me, and tested fine.
I am not sure though if there isn't a slight TOCTOU issue?
if the snapshot is deleted between the exist and verify_state check?
(not sure if there's a lock anyway here, couldn't tell from the surrounding code)
in that case we could maybe check err for ENOENT (if that's returned?) or returning
a custom Error type that includes that information
aside from that, consider this
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>
On 11/27/24 09:26, Fabian Grünbichler wrote:
> when loading the verification state for a local snapshot, it must first be
> ensured that it actually exists, else the lack of manifest will be interpreted
> as corrupt snapshot triggering a "resync" that is actually a sync of all
> missing snapshots.
>
> Fixes: 0974ddfa17be018f777d6ece90a71bfa8fc130d8 "fix #3786: api: add resync-corrupt option to sync jobs"
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> src/server/pull.rs | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 9abb673ae..361ed0687 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -551,21 +551,23 @@ async fn pull_group(
> .store
> .backup_dir(target_ns.clone(), dir.clone());
> if let Ok(local_dir) = local_dir {
> - match local_dir.verify_state() {
> - Ok(Some(state)) => {
> - if state == VerifyState::Failed {
> + if local_dir.full_path().exists() {
> + match local_dir.verify_state() {
> + Ok(Some(state)) => {
> + if state == VerifyState::Failed {
> + return Some((dir, true));
> + }
> + }
> + Ok(None) => {
> + // The verify_state item was not found in the manifest, this means the
> + // snapshot is new.
> + }
> + Err(_) => {
> + // There was an error loading the manifest, probably better if we
> + // resync.
> return Some((dir, true));
> }
> }
> - Ok(None) => {
> - // The verify_state item was not found in the manifest, this means the
> - // snapshot is new.
> - }
> - Err(_) => {
> - // There was an error loading the manifest, probably better if we
> - // resync.
> - return Some((dir, true));
> - }
> }
> }
> }
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots
2024-11-27 9:05 ` Dominik Csapak
@ 2024-11-27 9:17 ` Fabian Grünbichler
2024-11-27 10:46 ` Thomas Lamprecht
0 siblings, 1 reply; 5+ messages in thread
From: Fabian Grünbichler @ 2024-11-27 9:17 UTC (permalink / raw)
To: Dominik Csapak, Proxmox Backup Server development discussion
On November 27, 2024 10:05 am, Dominik Csapak wrote:
> Code wise it looks good to me, and tested fine.
> I am not sure though if there isn't a slight TOCTOU issue?
> if the snapshot is deleted between the exist and verify_state check?
> (not sure if there's a lock anyway here, couldn't tell from the surrounding code)
there is no lock at that point, that would also make this much more
expensive (we need to do this for every existing snapshot after all).
in general, sync should handle snapshots disappearing gracefully, and
the race window here is tiny.
handling ENOENT might be nice as additional safeguard, not sure if we
properly bubble that up atm though..
> in that case we could maybe check err for ENOENT (if that's returned?) or returning
> a custom Error type that includes that information
>
> aside from that, consider this
>
> Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
> Tested-by: Dominik Csapak <d.csapak@proxmox.com>
>
>
> On 11/27/24 09:26, Fabian Grünbichler wrote:
>> when loading the verification state for a local snapshot, it must first be
>> ensured that it actually exists, else the lack of manifest will be interpreted
>> as corrupt snapshot triggering a "resync" that is actually a sync of all
>> missing snapshots.
>>
>> Fixes: 0974ddfa17be018f777d6ece90a71bfa8fc130d8 "fix #3786: api: add resync-corrupt option to sync jobs"
>>
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> src/server/pull.rs | 26 ++++++++++++++------------
>> 1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>> index 9abb673ae..361ed0687 100644
>> --- a/src/server/pull.rs
>> +++ b/src/server/pull.rs
>> @@ -551,21 +551,23 @@ async fn pull_group(
>> .store
>> .backup_dir(target_ns.clone(), dir.clone());
>> if let Ok(local_dir) = local_dir {
>> - match local_dir.verify_state() {
>> - Ok(Some(state)) => {
>> - if state == VerifyState::Failed {
>> + if local_dir.full_path().exists() {
>> + match local_dir.verify_state() {
>> + Ok(Some(state)) => {
>> + if state == VerifyState::Failed {
>> + return Some((dir, true));
>> + }
>> + }
>> + Ok(None) => {
>> + // The verify_state item was not found in the manifest, this means the
>> + // snapshot is new.
>> + }
>> + Err(_) => {
>> + // There was an error loading the manifest, probably better if we
>> + // resync.
>> return Some((dir, true));
>> }
>> }
>> - Ok(None) => {
>> - // The verify_state item was not found in the manifest, this means the
>> - // snapshot is new.
>> - }
>> - Err(_) => {
>> - // There was an error loading the manifest, probably better if we
>> - // resync.
>> - return Some((dir, true));
>> - }
>> }
>> }
>> }
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots
2024-11-27 9:17 ` Fabian Grünbichler
@ 2024-11-27 10:46 ` Thomas Lamprecht
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2024-11-27 10:46 UTC (permalink / raw)
To: Proxmox Backup Server development discussion,
Fabian Grünbichler, Dominik Csapak
Am 27.11.24 um 10:17 schrieb Fabian Grünbichler:
> handling ENOENT might be nice as additional safeguard, not sure if we
> properly bubble that up atm though..
Yes, would be nice(r), but it indeed seems like the underlying
BackupDir::load_blob from pbs_datastore hides the actual error.
We should switch that over to use anyhow's context and probably drop the
try_block; not really useful here.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [pbs-devel] applied: [PATCH proxmox-backup] pull: properly skip missing snapshots
2024-11-27 8:26 [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots Fabian Grünbichler
2024-11-27 9:05 ` Dominik Csapak
@ 2024-11-27 9:05 ` Thomas Lamprecht
1 sibling, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2024-11-27 9:05 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Fabian Grünbichler
Am 27.11.24 um 09:26 schrieb Fabian Grünbichler:
> when loading the verification state for a local snapshot, it must first be
> ensured that it actually exists, else the lack of manifest will be interpreted
> as corrupt snapshot triggering a "resync" that is actually a sync of all
> missing snapshots.
>
> Fixes: 0974ddfa17be018f777d6ece90a71bfa8fc130d8 "fix #3786: api: add resync-corrupt option to sync jobs"
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> src/server/pull.rs | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
>
applied, with some subjective rewording/addition to commit message, thanks!
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-11-27 10:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-27 8:26 [pbs-devel] [PATCH proxmox-backup] pull: properly skip missing snapshots Fabian Grünbichler
2024-11-27 9:05 ` Dominik Csapak
2024-11-27 9:17 ` Fabian Grünbichler
2024-11-27 10:46 ` Thomas Lamprecht
2024-11-27 9:05 ` [pbs-devel] applied: " Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox