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

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

[Hannes Laimer]

... if 'purge'.

pve-manager:

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

 PVE/Jobs/Plugin.pm | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)


pve-conatiner:

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

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


qemu-server:

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

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

-- 
2.30.2

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

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 PVE/Jobs/Plugin.pm | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
index 6098360b..4883a193 100644
--- a/PVE/Jobs/Plugin.pm
+++ b/PVE/Jobs/Plugin.pm
@@ -3,7 +3,7 @@ package PVE::Jobs::Plugin;
 use strict;
 use warnings;
 
-use PVE::Cluster qw(cfs_register_file);
+use PVE::Cluster qw(cfs_register_file cfs_lock_file cfs_read_file cfs_write_file);
 
 use base qw(PVE::SectionConfig);
 
@@ -92,6 +92,23 @@ sub write_config {
     $class->SUPER::write_config($filename, $cfg);
 }
 
+sub remove_vmid_from_jobs {
+    my ($vmid) = @_;
+    cfs_lock_file('jobs.cfg', undef, sub {
+	my $jobs_data = cfs_read_file('jobs.cfg');
+	while((my $id, my $job) = each (%{$jobs_data->{ids}})){
+	    next if !defined($job->{vmid});
+	    $job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
+	    if ($job->{vmid} eq '') {
+		delete $jobs_data->{ids}->{$id};
+	    } else {
+		$jobs_data->{ids}->{$id} = $job;
+	    }
+	}
+	cfs_write_file('jobs.cfg', $jobs_data);
+    });
+}
+
 sub run {
     my ($class, $cfg) = @_;
     # implement in subclass
-- 
2.30.2

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

[Fabian Ebner]

Am 01.03.22 um 09:51 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  PVE/Jobs/Plugin.pm | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
> index 6098360b..4883a193 100644
> --- a/PVE/Jobs/Plugin.pm
> +++ b/PVE/Jobs/Plugin.pm
> @@ -3,7 +3,7 @@ package PVE::Jobs::Plugin;
>  use strict;
>  use warnings;
>  
> -use PVE::Cluster qw(cfs_register_file);
> +use PVE::Cluster qw(cfs_register_file cfs_lock_file cfs_read_file cfs_write_file);
>  
>  use base qw(PVE::SectionConfig);
>  
> @@ -92,6 +92,23 @@ sub write_config {
>      $class->SUPER::write_config($filename, $cfg);
>  }
>  
> +sub remove_vmid_from_jobs {

Since this is iterating over jobs of all kinds and not something that
should be overridden by derived plugins, it should rather be part of
Jobs.pm.

If, in the future, plugins ever need more fine-grained control, we can
always add a helper method to remove a vmid from a single job and adapt
this function to call the helper method from the appropriate plugin for
each job.

> +    my ($vmid) = @_;

Style nit: we usually put a blank line after the parameter assignment.

> +    cfs_lock_file('jobs.cfg', undef, sub {
> +	my $jobs_data = cfs_read_file('jobs.cfg');
> +	while((my $id, my $job) = each (%{$jobs_data->{ids}})){


Style nits:
* We don't really use 'each'. One reason is that the state of the
iterator is not reset when returning early. Not relevant here, but it's
just not nice behavior.
* Missing spaces before/after outermost parentheses

> +	    next if !defined($job->{vmid});
> +	    $job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
> +	    if ($job->{vmid} eq '') {
> +		delete $jobs_data->{ids}->{$id};
> +	    } else {
> +		$jobs_data->{ids}->{$id} = $job;

Isn't this just assigning the same reference again?

> +	    }
> +	}
> +	cfs_write_file('jobs.cfg', $jobs_data);
> +    });
> +}
> +
>  sub run {
>      my ($class, $cfg) = @_;
>      # implement in subclass

[pve-devel] [PATCH pve-container 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>
---
 src/PVE/API2/LXC.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 84712f7..2e4146e 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -758,6 +758,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::Plugin::remove_vmid_from_jobs($vmid);
 
 		if ($ha_managed) {
 		    PVE::HA::Config::delete_service_from_config("ct:$vmid");
-- 
2.30.2

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

[Fabian Ebner]

Am 01.03.22 um 09:51 schrieb Hannes Laimer:
> ... on destroy if 'purge' is selected
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/PVE/API2/LXC.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 84712f7..2e4146e 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -758,6 +758,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::Plugin::remove_vmid_from_jobs($vmid);

Should add a
    use PVE::Jobs::Plugin;
(or PVE::Jobs if the function is moved there) to the imports.

Same for the next patch.

>  
>  		if ($ha_managed) {
>  		    PVE::HA::Config::delete_service_from_config("ct:$vmid");

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

[Hannes Laimer]

Am 02.03.22 um 11:16 schrieb Fabian Ebner:
> Am 01.03.22 um 09:51 schrieb Hannes Laimer:
>> ... on destroy if 'purge' is selected
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/PVE/API2/LXC.pm | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>> index 84712f7..2e4146e 100644
>> --- a/src/PVE/API2/LXC.pm
>> +++ b/src/PVE/API2/LXC.pm
>> @@ -758,6 +758,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::Plugin::remove_vmid_from_jobs($vmid);
> 
> Should add a
>      use PVE::Jobs::Plugin;
> (or PVE::Jobs if the function is moved there) to the imports.
The reason I did not do that in the first place is that it is only used 
once in the whole file and I felt like I would make an already quite 
large import section even bigger. Should the previous two lines also use 
use? Do we have some kind of policy for when and when not to use use?
> 
> Same for the next patch.
> 
>>   
>>   		if ($ha_managed) {
>>   		    PVE::HA::Config::delete_service_from_config("ct:$vmid");

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

[Fabian Ebner]

Am 02.03.22 um 15:28 schrieb Hannes Laimer:
> Am 02.03.22 um 11:16 schrieb Fabian Ebner:
>> Am 01.03.22 um 09:51 schrieb Hannes Laimer:
>>> ... on destroy if 'purge' is selected
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>>   src/PVE/API2/LXC.pm | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>>> index 84712f7..2e4146e 100644
>>> --- a/src/PVE/API2/LXC.pm
>>> +++ b/src/PVE/API2/LXC.pm
>>> @@ -758,6 +758,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::Plugin::remove_vmid_from_jobs($vmid);
>>
>> Should add a
>>      use PVE::Jobs::Plugin;
>> (or PVE::Jobs if the function is moved there) to the imports.
> The reason I did not do that in the first place is that it is only used
> once in the whole file and I felt like I would make an already quite
> large import section even bigger. Should the previous two lines also use
> use? Do we have some kind of policy for when and when not to use use?

If the module is not imported explicitly, it will only work if you're
lucky and it's already imported (e.g. if C imports B and B imports A,
then C can call functions from A). But then things are brittle: Imagine
that at some point, B might not use anything from A anymore and so the
import is dropped there. Then Perl still will compile fine, but suddenly
the call in C is broken!

Ideally, each module lists all the modules it uses explicitly.
Unfortunately, Perl is very lax here (and everywhere :P).

>>
>> Same for the next patch.
>>
>>>             if ($ha_managed) {
>>>               PVE::HA::Config::delete_service_from_config("ct:$vmid");

[pve-devel] [PATCH qemu-server 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>
---
 PVE/API2/Qemu.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9be1caf..f100d2c 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1696,6 +1696,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::Plugin::remove_vmid_from_jobs($vmid);
 
 		    if ($ha_managed) {
 			PVE::HA::Config::delete_service_from_config("vm:$vmid");
-- 
2.30.2