* [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy @ 2022-03-14 9:26 Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper Hannes Laimer ` (3 more replies) 0 siblings, 4 replies; 8+ messages in thread From: Hannes Laimer @ 2022-03-14 9:26 UTC (permalink / raw) To: pve-devel ... 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper 2022-03-14 9:26 [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Hannes Laimer @ 2022-03-14 9:26 ` Hannes Laimer 2022-03-14 10:00 ` Fabian Ebner 2022-03-15 10:52 ` [pve-devel] [PATCH pve-manager v4 " Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-container v3 2/3] fix #3903: api2: remove vmid from jobs.cfg Hannes Laimer ` (2 subsequent siblings) 3 siblings, 2 replies; 8+ messages in thread From: Hannes Laimer @ 2022-03-14 9:26 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper 2022-03-14 9:26 ` [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper Hannes Laimer @ 2022-03-14 10:00 ` Fabian Ebner 2022-03-14 10:18 ` Fabian Ebner 2022-03-15 10:52 ` [pve-devel] [PATCH pve-manager v4 " Hannes Laimer 1 sibling, 1 reply; 8+ messages in thread From: Fabian Ebner @ 2022-03-14 10:00 UTC (permalink / raw) To: pve-devel, Hannes Laimer 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) = @_; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper 2022-03-14 10:00 ` Fabian Ebner @ 2022-03-14 10:18 ` Fabian Ebner 0 siblings, 0 replies; 8+ messages in thread From: Fabian Ebner @ 2022-03-14 10:18 UTC (permalink / raw) To: pve-devel, Hannes Laimer 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH pve-manager v4 1/3] fix #3903: jobs: add remove vmid from jobs helper 2022-03-14 9:26 ` [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper Hannes Laimer 2022-03-14 10:00 ` Fabian Ebner @ 2022-03-15 10:52 ` Hannes Laimer 1 sibling, 0 replies; 8+ messages in thread From: Hannes Laimer @ 2022-03-15 10:52 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH pve-container v3 2/3] fix #3903: api2: remove vmid from jobs.cfg 2022-03-14 9:26 [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper Hannes Laimer @ 2022-03-14 9:26 ` Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH qemu-server v3 3/3] " Hannes Laimer 2022-03-16 7:15 ` [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Thomas Lamprecht 3 siblings, 0 replies; 8+ messages in thread From: Hannes Laimer @ 2022-03-14 9:26 UTC (permalink / raw) To: pve-devel ... 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH qemu-server v3 3/3] fix #3903: api2: remove vmid from jobs.cfg 2022-03-14 9:26 [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-container v3 2/3] fix #3903: api2: remove vmid from jobs.cfg Hannes Laimer @ 2022-03-14 9:26 ` Hannes Laimer 2022-03-16 7:15 ` [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Thomas Lamprecht 3 siblings, 0 replies; 8+ messages in thread From: Hannes Laimer @ 2022-03-14 9:26 UTC (permalink / raw) To: pve-devel ... 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy 2022-03-14 9:26 [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Hannes Laimer ` (2 preceding siblings ...) 2022-03-14 9:26 ` [pve-devel] [PATCH qemu-server v3 3/3] " Hannes Laimer @ 2022-03-16 7:15 ` Thomas Lamprecht 3 siblings, 0 replies; 8+ messages in thread From: Thomas Lamprecht @ 2022-03-16 7:15 UTC (permalink / raw) To: Proxmox VE development discussion, Hannes Laimer 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-16 7:15 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-14 9:26 [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-manager v3 1/3] fix #3903: jobs: add remove vmid from jobs helper Hannes Laimer 2022-03-14 10:00 ` Fabian Ebner 2022-03-14 10:18 ` Fabian Ebner 2022-03-15 10:52 ` [pve-devel] [PATCH pve-manager v4 " Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH pve-container v3 2/3] fix #3903: api2: remove vmid from jobs.cfg Hannes Laimer 2022-03-14 9:26 ` [pve-devel] [PATCH qemu-server v3 3/3] " Hannes Laimer 2022-03-16 7:15 ` [pve-devel] [PATCH-SERIES v3] fix #3903: remove vmid from jobs.cfg on destroy Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox