[pve-devel] [PATCH-SERIES] remove replicated volumes on guest purge

[pve-devel] [PATCH-SERIES] remove replicated volumes on guest purge

[Fabian Ebner]

Introduces two helper functions in Replication.pm and ReplicationConfig.pm
so that the guests can do the removal easily.

destroy_vm contains a check whether the guest is still in use by a
linked clone (in the LXC case triggered by the storage backend at vdisk_free),
so that needs to happen first.
That check could be factored out and removing replicated volumes moved
to before destroy_vm, but I feel like it's cleaner to first destroy the
VM and do all related cleanups later (as it is now).

The problem is that the guest config does not contain any volumes
after destroy_vm, and run_full_removal would do nothing, because
on removal, run_replication currently only considers storages that
show up in the config and not those from the replication job state.

Therefore, this depends on the following patch to be applied first:
https://lists.proxmox.com/pipermail/pve-devel/2020-October/045386.html

Dependency bumps: qemu-server,pve-container -> pve-guest-common
are needed for patches #2 and #3
and I think the reverse bumps are needed for patch #4

-- 
2.20.1

[pve-devel] [PATCH guest-common 1/5] add list_local_jobs and run_full_removal functions

[Fabian Ebner]

helpful for removing replicated volumes when purging a guest.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Replication.pm       | 11 +++++++++++
 PVE/ReplicationConfig.pm |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/PVE/Replication.pm b/PVE/Replication.pm
index 132e8bb..a2138fc 100644
--- a/PVE/Replication.pm
+++ b/PVE/Replication.pm
@@ -425,4 +425,15 @@ sub run_replication {
     return $volumes;
 }
 
+sub run_full_removal {
+    my ($guest_class, $jobcfg) = @_;
+
+    my $start_time = time();
+    my $logfunc = sub { print shift . "\n"; };
+
+    $jobcfg->{remove_job} = 'full';
+
+    run_replication($guest_class, $jobcfg, $start_time, $start_time, $logfunc);
+}
+
 1;
diff --git a/PVE/ReplicationConfig.pm b/PVE/ReplicationConfig.pm
index 66ef842..77905a0 100644
--- a/PVE/ReplicationConfig.pm
+++ b/PVE/ReplicationConfig.pm
@@ -228,6 +228,13 @@ sub find_local_replication_job {
     return undef;
 }
 
+sub list_local_jobs {
+    my ($cfg, $vmid) = @_;
+
+    my @jobs = grep { $_->{type} eq 'local' && $_->{guest} == $vmid } values %{$cfg->{ids}};
+    return \@jobs;
+}
+
 # switch local replication job target
 sub switch_replication_job_target {
     my ($vmid, $old_target, $new_target) = @_;
-- 
2.20.1

[pve-devel] [PATCH qemu-server 2/5] remove replicated volumes on purge

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 8da616a..c6b1088 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -17,6 +17,7 @@ use PVE::Exception qw(raise raise_param_exc raise_perm_exc);
 use PVE::Storage;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
+use PVE::Replication;
 use PVE::ReplicationConfig;
 use PVE::GuestHelpers;
 use PVE::QemuConfig;
@@ -1539,7 +1540,14 @@ __PACKAGE__->register_method({
 		PVE::Firewall::remove_vmfw_conf($vmid);
 		if ($param->{purge}) {
 		    print "purging VM $vmid from related configurations..\n";
-		    PVE::ReplicationConfig::remove_vmid_jobs($vmid);
+
+		    my $repl_conf = PVE::ReplicationConfig->new();
+		    my $jobs = $repl_conf->list_local_jobs($vmid);
+		    foreach my $job (@{$jobs}) {
+			eval { PVE::Replication::run_full_removal('PVE::QemuConfig', $job); };
+			warn $@ if $@;
+		    }
+
 		    PVE::VZDump::Plugin::remove_vmid_from_backup_jobs($vmid);
 
 		    if ($ha_managed) {
-- 
2.20.1

[pve-devel] [PATCH container 3/5] remove replicated volumes on purge

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/API2/LXC.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 9ecfb12..9fc7d98 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -15,6 +15,7 @@ use PVE::Firewall;
 use PVE::Storage;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
+use PVE::Replication;
 use PVE::ReplicationConfig;
 use PVE::LXC;
 use PVE::LXC::Create;
@@ -694,7 +695,14 @@ __PACKAGE__->register_method({
 	    PVE::Firewall::remove_vmfw_conf($vmid);
 	    if ($param->{purge}) {
 	        print "purging CT $vmid from related configurations..\n";
-		PVE::ReplicationConfig::remove_vmid_jobs($vmid);
+
+		my $repl_conf = PVE::ReplicationConfig->new();
+		my $jobs = $repl_conf->list_local_jobs($vmid);
+		foreach my $job (@{$jobs}) {
+		    eval { PVE::Replication::run_full_removal('PVE::LXC::Config', $job); };
+		    warn $@ if $@;
+		}
+
 		PVE::VZDump::Plugin::remove_vmid_from_backup_jobs($vmid);
 
 		if ($ha_managed) {
-- 
2.20.1

[pve-devel] [PATCH guest-common 4/5] remove the now unused remove_vmid_jobs

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

I *think* that I have all the repositiories where this would
potentially be used, but please double check that there are
no users of this anywhere.

 PVE/ReplicationConfig.pm | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/PVE/ReplicationConfig.pm b/PVE/ReplicationConfig.pm
index 77905a0..a220594 100644
--- a/PVE/ReplicationConfig.pm
+++ b/PVE/ReplicationConfig.pm
@@ -265,20 +265,6 @@ sub delete_job {
     lock($code);
 }
 
-sub remove_vmid_jobs {
-    my ($vmid) = @_;
-
-    my $code = sub {
-	my $cfg = __PACKAGE__->new();
-	foreach my $id (keys %{$cfg->{ids}}) {
-	    delete $cfg->{ids}->{$id} if ($cfg->{ids}->{$id}->{guest} == $vmid);
-	}
-	$cfg->write();
-    };
-
-    lock($code);
-}
-
 sub swap_source_target_nolock {
     my ($jobid) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH guest-common 5/5] cleanup: iterate over values in find_local_replication_job

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/ReplicationConfig.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/PVE/ReplicationConfig.pm b/PVE/ReplicationConfig.pm
index a220594..343ad2a 100644
--- a/PVE/ReplicationConfig.pm
+++ b/PVE/ReplicationConfig.pm
@@ -218,9 +218,7 @@ sub check_for_existing_jobs {
 sub find_local_replication_job {
     my ($cfg, $vmid, $target) = @_;
 
-    foreach my $id (keys %{$cfg->{ids}}) {
-	my $data = $cfg->{ids}->{$id};
-
+    foreach my $data (values %{$cfg->{ids}}) {
 	return $data if $data->{type} eq 'local' &&
 	    $data->{guest} == $vmid && $data->{target} eq $target;
     }
-- 
2.20.1

Re: [pve-devel] [PATCH-SERIES] remove replicated volumes on guest purge

[Thomas Lamprecht]

On 14.10.20 13:36, Fabian Ebner wrote:
> Introduces two helper functions in Replication.pm and ReplicationConfig.pm
> so that the guests can do the removal easily.
> 
> destroy_vm contains a check whether the guest is still in use by a
> linked clone (in the LXC case triggered by the storage backend at vdisk_free),
> so that needs to happen first.
> That check could be factored out and removing replicated volumes moved
> to before destroy_vm, but I feel like it's cleaner to first destroy the
> VM and do all related cleanups later (as it is now).
> 
> The problem is that the guest config does not contain any volumes
> after destroy_vm, and run_full_removal would do nothing, because
> on removal, run_replication currently only considers storages that
> show up in the config and not those from the replication job state.
> 
> Therefore, this depends on the following patch to be applied first:
> https://lists.proxmox.com/pipermail/pve-devel/2020-October/045386.html
> 
> Dependency bumps: qemu-server,pve-container -> pve-guest-common
> are needed for patches #2 and #3
> and I think the reverse bumps are needed for patch #4
> 

Besides that, is this still relevant? If so, it may need some rebasing,
at least guest-common does.

Re: [pve-devel] [PATCH-SERIES] remove replicated volumes on guest purge

[Fabian Ebner]

Am 28.01.21 um 17:20 schrieb Thomas Lamprecht:
> On 14.10.20 13:36, Fabian Ebner wrote:
>> Introduces two helper functions in Replication.pm and ReplicationConfig.pm
>> so that the guests can do the removal easily.
>>
>> destroy_vm contains a check whether the guest is still in use by a
>> linked clone (in the LXC case triggered by the storage backend at vdisk_free),
>> so that needs to happen first.
>> That check could be factored out and removing replicated volumes moved
>> to before destroy_vm, but I feel like it's cleaner to first destroy the
>> VM and do all related cleanups later (as it is now).
>>
>> The problem is that the guest config does not contain any volumes
>> after destroy_vm, and run_full_removal would do nothing, because
>> on removal, run_replication currently only considers storages that
>> show up in the config and not those from the replication job state.
>>
>> Therefore, this depends on the following patch to be applied first:
>> https://lists.proxmox.com/pipermail/pve-devel/2020-October/045386.html
>>
>> Dependency bumps: qemu-server,pve-container -> pve-guest-common
>> are needed for patches #2 and #3
>> and I think the reverse bumps are needed for patch #4
>>
> 
> Besides that, is this still relevant? If so, it may need some rebasing,
> at least guest-common does.
> 

If the decision that we want to remove replicated volumes on purge 
hasn't changed, then yes, it's still relevant.

But it might be a good idea to introduce this change close(r) to the 
next release, and mention it in the "known issues", so fewer people are 
surprised by it.

In any case, this depends on another patch (as mentioned above), so you 
probably want to take a look at that series first (it still applies for 
me): https://lists.proxmox.com/pipermail/pve-devel/2020-October/045388.html

The second patch is the important one.