[pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy

[pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy

[Hannes Laimer]

... if 'purge'.


v3, based on Fabian Ebner's feedback:
 - correctly handle 'exclude'
 - move plugin specific stuff into the corresponding plugin(currently
   just VZDump)
 - add missing call to remove_job, when removing job

v2, based on Fabian Ebner's feedback:
 - rewrote remove_vmid_from_jobs sub
 - add missing 'use' statements


pve-manager:

Hannes Laimer (1):
  fix #3903: jobs: add remove vmid from jobs helper

 PVE/Jobs.pm        | 21 ++++++++++++++++++++-
 PVE/Jobs/Plugin.pm |  7 +++++++
 PVE/Jobs/VZDump.pm | 13 +++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)


pve-container:

Hannes Laimer (1):
  fix #3903: api2: remove vmid from jobs.cfg

 src/PVE/API2/LXC.pm | 2 ++
 1 file changed, 2 insertions(+)


qemu-server:

Hannes Laimer (1):
  fix #3903: api2: remove vmid from jobs.cfg

 PVE/API2/Qemu.pm | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 PVE/Jobs.pm        | 21 ++++++++++++++++++++-
 PVE/Jobs/Plugin.pm |  7 +++++++
 PVE/Jobs/VZDump.pm | 13 +++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
index ba3685ec..5b2c4c7b 100644
--- a/PVE/Jobs.pm
+++ b/PVE/Jobs.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 use JSON;
 
-use PVE::Cluster qw(cfs_read_file cfs_lock_file);
+use PVE::Cluster qw(cfs_read_file cfs_lock_file cfs_write_file);
 use PVE::Jobs::Plugin;
 use PVE::Jobs::VZDump;
 use PVE::Tools;
@@ -274,6 +274,25 @@ sub synchronize_job_states_with_config {
     die $@ if $@;
 }
 
+sub remove_vmid_from_jobs {
+    my ($vmid) = @_;
+
+    cfs_lock_file('jobs.cfg', undef, sub {
+	my $jobs_data = cfs_read_file('jobs.cfg');
+	for my $id (keys %{$jobs_data->{ids}}) {
+	    my $job = $jobs_data->{ids}->{$id};
+	    my $type = $job->{type};
+	    my $plugin = PVE::Jobs::Plugin->lookup($type);
+	    $jobs_data->{ids}->{$id} = $plugin->remove_vmid_from_job($job, $vmid);
+	    if (!defined($jobs_data->{ids}->{$id})) {
+		delete $jobs_data->{ids}->{$id};
+		remove_job($id, $type);
+	    }
+	}
+        cfs_write_file('jobs.cfg', $jobs_data);
+    });
+}
+
 sub setup_dirs {
     mkdir $state_dir;
     mkdir $lock_dir;
diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
index 6098360b..645c460f 100644
--- a/PVE/Jobs/Plugin.pm
+++ b/PVE/Jobs/Plugin.pm
@@ -80,6 +80,13 @@ sub encode_value {
     return $plugin->encode_value($type, $key, $value);
 }
 
+sub remove_vmid_from_job {
+    my ($class, $type, $job, $vmid) = @_;
+
+    my $plugin = __PACKAGE__->lookup($type);
+    $plugin->remove_vmid_from_job($job, $vmid);
+}
+
 sub write_config {
     my ($class, $filename, $cfg) = @_;
 
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
index 44fe33dc..3aaa66b8 100644
--- a/PVE/Jobs/VZDump.pm
+++ b/PVE/Jobs/VZDump.pm
@@ -64,6 +64,19 @@ sub encode_value {
     return $value;
 }
 
+sub remove_vmid_from_job {
+    my ($class, $job, $vmid) = @_;
+
+    if (defined($job->{vmid})) {
+	$job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
+	undef $job if $job->{vmid} eq '';
+    } elsif (defined($job->{exclude})) {
+	$job->{exclude} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{exclude}));
+	delete $job->{exclude} if $job->{exclude} eq '';
+    }
+    return $job;
+}
+
 sub run {
     my ($class, $conf) = @_;
 
-- 
2.30.2

Re: [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper

[Fabian Ebner]

Am 14.03.22 um 10:26 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  PVE/Jobs.pm        | 21 ++++++++++++++++++++-
>  PVE/Jobs/Plugin.pm |  7 +++++++
>  PVE/Jobs/VZDump.pm | 13 +++++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
> index ba3685ec..5b2c4c7b 100644
> --- a/PVE/Jobs.pm
> +++ b/PVE/Jobs.pm
> @@ -4,7 +4,7 @@ use strict;
>  use warnings;
>  use JSON;
>  
> -use PVE::Cluster qw(cfs_read_file cfs_lock_file);
> +use PVE::Cluster qw(cfs_read_file cfs_lock_file cfs_write_file);
>  use PVE::Jobs::Plugin;
>  use PVE::Jobs::VZDump;
>  use PVE::Tools;
> @@ -274,6 +274,25 @@ sub synchronize_job_states_with_config {
>      die $@ if $@;
>  }
>  
> +sub remove_vmid_from_jobs {
> +    my ($vmid) = @_;
> +
> +    cfs_lock_file('jobs.cfg', undef, sub {
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	for my $id (keys %{$jobs_data->{ids}}) {
> +	    my $job = $jobs_data->{ids}->{$id};
> +	    my $type = $job->{type};
> +	    my $plugin = PVE::Jobs::Plugin->lookup($type);
> +	    $jobs_data->{ids}->{$id} = $plugin->remove_vmid_from_job($job, $vmid);
> +	    if (!defined($jobs_data->{ids}->{$id})) {
> +		delete $jobs_data->{ids}->{$id};

If it's not defined, delete shouldn't be necessary anymore.

> +		remove_job($id, $type);
> +	    }
> +	}
> +        cfs_write_file('jobs.cfg', $jobs_data);
> +    });
> +}
> +
>  sub setup_dirs {
>      mkdir $state_dir;
>      mkdir $lock_dir;
> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
> index 6098360b..645c460f 100644
> --- a/PVE/Jobs/Plugin.pm
> +++ b/PVE/Jobs/Plugin.pm
> @@ -80,6 +80,13 @@ sub encode_value {
>      return $plugin->encode_value($type, $key, $value);
>  }
>  
> +sub remove_vmid_from_job {
> +    my ($class, $type, $job, $vmid) = @_;

The signature here is different from the one in the VZDump plugin.

> +
> +    my $plugin = __PACKAGE__->lookup($type);
> +    $plugin->remove_vmid_from_job($job, $vmid);

The default implementation should either do nothing or, IMHO better, die
with something like "remove_vmid_from_job: implement in subclass". With
this implementation, calling remove_vmid_from_job on a plugin which
doesn't override it, will result in an infinite recursion (at least it
would if the signature would match ;)).

> +}
> +
>  sub write_config {
>      my ($class, $filename, $cfg) = @_;
>  
> diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
> index 44fe33dc..3aaa66b8 100644
> --- a/PVE/Jobs/VZDump.pm
> +++ b/PVE/Jobs/VZDump.pm
> @@ -64,6 +64,19 @@ sub encode_value {
>      return $value;
>  }
>  
> +sub remove_vmid_from_job {
> +    my ($class, $job, $vmid) = @_;
> +
> +    if (defined($job->{vmid})) {
> +	$job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
> +	undef $job if $job->{vmid} eq '';
> +    } elsif (defined($job->{exclude})) {
> +	$job->{exclude} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{exclude}));
> +	delete $job->{exclude} if $job->{exclude} eq '';
> +    }
> +    return $job;
> +}
> +
>  sub run {
>      my ($class, $conf) = @_;
>  

Re: [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper

[Fabian Ebner]

Am 14.03.22 um 11:00 schrieb Fabian Ebner:
> Am 14.03.22 um 10:26 schrieb Hannes Laimer:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> @@ -274,6 +274,25 @@ sub synchronize_job_states_with_config {
>>      die $@ if $@;
>>  }
>>  
>> +sub remove_vmid_from_jobs {
>> +    my ($vmid) = @_;
>> +
>> +    cfs_lock_file('jobs.cfg', undef, sub {
>> +	my $jobs_data = cfs_read_file('jobs.cfg');
>> +	for my $id (keys %{$jobs_data->{ids}}) {
>> +	    my $job = $jobs_data->{ids}->{$id};
>> +	    my $type = $job->{type};
>> +	    my $plugin = PVE::Jobs::Plugin->lookup($type);
>> +	    $jobs_data->{ids}->{$id} = $plugin->remove_vmid_from_job($job, $vmid);
>> +	    if (!defined($jobs_data->{ids}->{$id})) {
>> +		delete $jobs_data->{ids}->{$id};
> 
> If it's not defined, delete shouldn't be necessary anymore.

Well, assuming the writer handles that correctly. Actually, I don't mind
having the delete here after all.

[pve-devel] [PATCH pve-manager v4 1/3] fix #3903: jobs: add remove vmid from jobs helper

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
v3->v4:
 - fix signature in base plugin
 - die in base plugin

 PVE/Jobs.pm        | 21 ++++++++++++++++++++-
 PVE/Jobs/Plugin.pm |  6 ++++++
 PVE/Jobs/VZDump.pm | 13 +++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
index ba3685ec..5b2c4c7b 100644
--- a/PVE/Jobs.pm
+++ b/PVE/Jobs.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 use JSON;
 
-use PVE::Cluster qw(cfs_read_file cfs_lock_file);
+use PVE::Cluster qw(cfs_read_file cfs_lock_file cfs_write_file);
 use PVE::Jobs::Plugin;
 use PVE::Jobs::VZDump;
 use PVE::Tools;
@@ -274,6 +274,25 @@ sub synchronize_job_states_with_config {
     die $@ if $@;
 }
 
+sub remove_vmid_from_jobs {
+    my ($vmid) = @_;
+
+    cfs_lock_file('jobs.cfg', undef, sub {
+	my $jobs_data = cfs_read_file('jobs.cfg');
+	for my $id (keys %{$jobs_data->{ids}}) {
+	    my $job = $jobs_data->{ids}->{$id};
+	    my $type = $job->{type};
+	    my $plugin = PVE::Jobs::Plugin->lookup($type);
+	    $jobs_data->{ids}->{$id} = $plugin->remove_vmid_from_job($job, $vmid);
+	    if (!defined($jobs_data->{ids}->{$id})) {
+		delete $jobs_data->{ids}->{$id};
+		remove_job($id, $type);
+	    }
+	}
+        cfs_write_file('jobs.cfg', $jobs_data);
+    });
+}
+
 sub setup_dirs {
     mkdir $state_dir;
     mkdir $lock_dir;
diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
index 6098360b..1900fe43 100644
--- a/PVE/Jobs/Plugin.pm
+++ b/PVE/Jobs/Plugin.pm
@@ -80,6 +80,12 @@ sub encode_value {
     return $plugin->encode_value($type, $key, $value);
 }
 
+sub remove_vmid_from_job {
+    my ($class, $job, $vmid) = @_;
+
+    die "remove_vmid_from_job: implemented in subclass";
+}
+
 sub write_config {
     my ($class, $filename, $cfg) = @_;
 
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
index 44fe33dc..3aaa66b8 100644
--- a/PVE/Jobs/VZDump.pm
+++ b/PVE/Jobs/VZDump.pm
@@ -64,6 +64,19 @@ sub encode_value {
     return $value;
 }
 
+sub remove_vmid_from_job {
+    my ($class, $job, $vmid) = @_;
+
+    if (defined($job->{vmid})) {
+	$job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
+	undef $job if $job->{vmid} eq '';
+    } elsif (defined($job->{exclude})) {
+	$job->{exclude} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{exclude}));
+	delete $job->{exclude} if $job->{exclude} eq '';
+    }
+    return $job;
+}
+
 sub run {
     my ($class, $conf) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH pve-container v3 2/3] fix #3903: api2: remove vmid from jobs.cfg

[Hannes Laimer]

... on destroy if 'purge' is selected

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
v2->v3: no changes

 src/PVE/API2/LXC.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 84712f7..095f421 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -16,6 +16,7 @@ use PVE::Storage;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
 use PVE::ReplicationConfig;
+use PVE::Jobs;
 use PVE::LXC;
 use PVE::LXC::Create;
 use PVE::LXC::Migrate;
@@ -758,6 +759,7 @@ __PACKAGE__->register_method({
 	        print "purging CT $vmid from related configurations..\n";
 		PVE::ReplicationConfig::remove_vmid_jobs($vmid);
 		PVE::VZDump::Plugin::remove_vmid_from_backup_jobs($vmid);
+		PVE::Jobs::remove_vmid_from_jobs($vmid);
 
 		if ($ha_managed) {
 		    PVE::HA::Config::delete_service_from_config("ct:$vmid");
-- 
2.30.2

[pve-devel] [PATCH qemu-server v3 3/3] fix #3903: api2: remove vmid from jobs.cfg

[Hannes Laimer]

... on destroy if 'purge' is selected

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
v2->v3: no changes

 PVE/API2/Qemu.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9be1caf..465abc2 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -31,6 +31,7 @@ use PVE::AccessControl;
 use PVE::INotify;
 use PVE::Network;
 use PVE::Firewall;
+use PVE::Jobs;
 use PVE::API2::Firewall::VM;
 use PVE::API2::Qemu::Agent;
 use PVE::VZDump::Plugin;
@@ -1696,6 +1697,7 @@ __PACKAGE__->register_method({
 		    print "purging VM $vmid from related configurations..\n";
 		    PVE::ReplicationConfig::remove_vmid_jobs($vmid);
 		    PVE::VZDump::Plugin::remove_vmid_from_backup_jobs($vmid);
+		    PVE::Jobs::remove_vmid_from_jobs($vmid);
 
 		    if ($ha_managed) {
 			PVE::HA::Config::delete_service_from_config("vm:$vmid");
-- 
2.30.2

Re: [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy

[Thomas Lamprecht]

higher level comment: We cannot do it this way as it adds a cyclic dependency
between pve-container <-> pve-manager and qemu-server <-> pve-manager, which
is not allowed (such things, besides being ugly, make bootstrapping *a lot*
harder, and as I'm most often doing the bootstrap effort I NAK this).
pve-manager must only be a perl-module consumer for other repos, never a
provider.

I mean, it's not like Dominik and I did not predict such things when adding
the Jobs stuff in manager, but neither guest-common (there are guets unrelated
jobs we want to add like (finally) LDAP sync) nor pve-common (to high up the chain
means less access to other modules, e.g., pve-cluster) weren't a really good fitting
place then, especially as we had quite the time pressure, so we added all in
pve-manager as there we have the least coupling for moving it in the future.

I'd actually move the base jobs stuff to pve-cluster (possibly a new binary
package) as that's the highest point we can move it and it's not completely
wrong.