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

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

[Hannes Laimer]

... if 'purge'.


v2, based on Fabian Ebner <f.ebner@proxmox.com>'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 | 18 +++++++++++++++++-
 1 file changed, 17 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 v2 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 | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
index ba3685ec..a2a84095 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,22 @@ 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 (sort keys %{$jobs_data->{ids}}) {
+	    my $vmids = \$jobs_data->{ids}->{$id}->{vmid};
+	    next if !defined($$vmids);
+
+	    $$vmids = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($$vmids));
+	    delete $jobs_data->{ids}->{$id} if $$vmids eq '';
+        }
+        cfs_write_file('jobs.cfg', $jobs_data);
+    });
+}
+
 sub setup_dirs {
     mkdir $state_dir;
     mkdir $lock_dir;
-- 
2.30.2

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

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
FIXUP: sort was not needed and should not have been there

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

diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
index ba3685ec..ea41523b 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,22 @@ 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 $vmids = \$jobs_data->{ids}->{$id}->{vmid};
+	    next if !defined($$vmids);
+
+	    $$vmids = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($$vmids));
+	    delete $jobs_data->{ids}->{$id} if $$vmids eq '';
+        }
+        cfs_write_file('jobs.cfg', $jobs_data);
+    });
+}
+
 sub setup_dirs {
     mkdir $state_dir;
     mkdir $lock_dir;
-- 
2.30.2

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

[Fabian Ebner]

Am 03.03.22 um 18:10 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> FIXUP: sort was not needed and should not have been there
> 
>  PVE/Jobs.pm | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
> index ba3685ec..ea41523b 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,22 @@ 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 $vmids = \$jobs_data->{ids}->{$id}->{vmid};

This style with manipulating a scalar hash value via reference is not
commonly used in our code base. Usually, a helper variable for the hash
reference is created and then that one is manipulated. You had that in
v1. My comment there was just about the last assignment in
  my $job = $jobs_data->{ids}->{$id}; # was part of each in v1
  ...
  # The hash that $job references is manipulated, but not the reference
  # itself
  ...
  $jobs_data->{ids}->{$id} = $job;
being unnecessary.

> +	    next if !defined($$vmids);
> +
> +	    $$vmids = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($$vmids));
> +	    delete $jobs_data->{ids}->{$id} if $$vmids eq '';
> +        }
> +        cfs_write_file('jobs.cfg', $jobs_data);
> +    });
> +}
> +
>  sub setup_dirs {
>      mkdir $state_dir;
>      mkdir $lock_dir;

[pve-devel] [PATCH pve-manager v2] fix #3903: jobs: add remove vmid from jobs helper

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
changed back to v1, but without the unnecessary stuff. Thanks for the
feedback @Fabian Ebner

 PVE/Jobs.pm | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
index ba3685ec..db6fa97d 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,21 @@ 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};
+	    next if !defined($job->{vmid});
+	    $job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
+	    delete $jobs_data->{ids}->{$id} if $job->{vmid} eq '';
+	}
+        cfs_write_file('jobs.cfg', $jobs_data);
+    });
+}
+
 sub setup_dirs {
     mkdir $state_dir;
     mkdir $lock_dir;
-- 
2.30.2

Re: [pve-devel] [PATCH pve-manager v2] fix #3903: jobs: add remove vmid from jobs helper

[Fabian Ebner]

Am 07.03.22 um 07:43 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> changed back to v1, but without the unnecessary stuff. Thanks for the
> feedback @Fabian Ebner
> 
>  PVE/Jobs.pm | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
> index ba3685ec..db6fa97d 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,21 @@ 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};
> +	    next if !defined($job->{vmid});
> +	    $job->{vmid} = join(',', grep { $_ ne $vmid } PVE::Tools::split_list($job->{vmid}));
> +	    delete $jobs_data->{ids}->{$id} if $job->{vmid} eq '';

There is a remove_job() function that's supposed to be called when a job
is removed. It'll be called by synchronize_job_states_with_config too,
but it'd be cleaner to call it directly.

Also, the old behavior is to remove a VM ID upon purge from 'exclude'
too. For consistency, we need do that here too. See
remove_vmid_from_jobs in guest-common's PVE/VZDump/Plugin.pm for comparison.

'exclude' is specific to backups, so there should be a plugin method for
removing a VMID from a job, which the VZDump plugin overrides, and the
iterator here should just call the method from the job's plugin. Well,
technically, 'vmid' is also specific to backups, because it's not part
of the defaultData properties of the generic plugin.

> +	}
> +        cfs_write_file('jobs.cfg', $jobs_data);
> +    });
> +}
> +
>  sub setup_dirs {
>      mkdir $state_dir;
>      mkdir $lock_dir;

[pve-devel] [PATCH pve-container v2 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 | 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 v2 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 | 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