[pve-devel] [PATCH guest-common] replication: avoid passing removed storages to target

[pve-devel] [PATCH guest-common] replication: avoid passing removed storages to target

[Fiona Ebner]

After removing a storage, replication states can still contain
references to it, even if no volume references it anymore.

If a storage does not exist in the storage configuration, the
replication target runs into an error when preparing the job locally.
This error prevents both running and removing the replication job. Fix
it by not passing the invalid storage ID in the first place.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Replication.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 469ca19..bd627e5 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -275,6 +275,9 @@ sub replicate {
     $logfunc->("guest => $guest_name, running => $running");
     $logfunc->("volumes => " . join(',', @$sorted_volids));
 
+    # filter out left-over non-existing/removed storages - avoids error on target
+    $state->{storeid_list} = [ grep { $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];
+
     if (my $remove_job = $jobcfg->{remove_job}) {
 
 	$logfunc->("start job removal - mode '${remove_job}'");
-- 
2.39.2

Re: [pve-devel] [PATCH guest-common] replication: avoid passing removed storages to target

[Fiona Ebner]

Ping

Am 23.06.23 um 12:08 schrieb Fiona Ebner:
> After removing a storage, replication states can still contain
> references to it, even if no volume references it anymore.
> 
> If a storage does not exist in the storage configuration, the
> replication target runs into an error when preparing the job locally.
> This error prevents both running and removing the replication job. Fix
> it by not passing the invalid storage ID in the first place.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Re: [pve-devel] [PATCH guest-common] replication: avoid passing removed storages to target

[Thomas Lamprecht]

Am 23/06/2023 um 12:08 schrieb Fiona Ebner:
> +    # filter out left-over non-existing/removed storages - avoids error on target
> +    $state->{storeid_list} = [ grep { $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];

looks fine in general, just wondering if we'd be better of to make
the grep include-condition a bit more explicit by using `exists` on
the hash:

$state->{storeid_list} = [ grep { exists $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];

albeit, the value should be always truthy, so might be redundant,
depending on how you see this I can apply this patch or a v2.

Re: [pve-devel] [PATCH guest-common] replication: avoid passing removed storages to target

[Fiona Ebner]

Am 30.08.23 um 11:27 schrieb Thomas Lamprecht:
> Am 23/06/2023 um 12:08 schrieb Fiona Ebner:
>> +    # filter out left-over non-existing/removed storages - avoids error on target
>> +    $state->{storeid_list} = [ grep { $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];
> 
> looks fine in general, just wondering if we'd be better of to make
> the grep include-condition a bit more explicit by using `exists` on
> the hash:
> 
> $state->{storeid_list} = [ grep { exists $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];
> 
> albeit, the value should be always truthy, so might be redundant,
> depending on how you see this I can apply this patch or a v2.

I don't like using exists() expect when it's really necessary, because
it's way too easy picking up something that was auto-vivified
accidentally (shouldn't happen in this case, but still). And yes, we can
assume the value is truthy if it's an existing storage, so I didn't
bother with defined() either.

Re: [pve-devel] [PATCH guest-common] replication: avoid passing removed storages to target

[Thomas Lamprecht]

Am 30/08/2023 um 11:42 schrieb Fiona Ebner:
> Am 30.08.23 um 11:27 schrieb Thomas Lamprecht:
>> Am 23/06/2023 um 12:08 schrieb Fiona Ebner:
>>> +    # filter out left-over non-existing/removed storages - avoids error on target
>>> +    $state->{storeid_list} = [ grep { $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];
>>
>> looks fine in general, just wondering if we'd be better of to make
>> the grep include-condition a bit more explicit by using `exists` on
>> the hash:
>>
>> $state->{storeid_list} = [ grep { exists $storecfg->{ids}->{$_} } $state->{storeid_list}->@* ];
>>
>> albeit, the value should be always truthy, so might be redundant,
>> depending on how you see this I can apply this patch or a v2.
> 
> I don't like using exists() expect when it's really necessary, because
> it's way too easy picking up something that was auto-vivified
> accidentally (shouldn't happen in this case, but still).

Unwanted auto-vivification is always a bug though, and can cause problems
elsewhere, so "hiding" them more might make things harder to debug..
But yeah, if, defined would be the better choice here.

> And yes, we can
> assume the value is truthy if it's an existing storage, so I didn't
> bother with defined() either.

Yeah, while refactoring can sometimes change such invariants, I really
doubt it will happen here, so it's fine.

applied, thanks!