Re: [pve-devel] [PATCH pve-guest-common 2/3] jobs: move VZDump plugin from pve-manager

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On March 22, 2022 8:34 am, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/Makefile           |   4 +
>  src/PVE/Jobs.pm        | 282 +++++++++++++++++++++++++++++++++++++++++
>  src/PVE/Jobs/VZDump.pm |  87 +++++++++++++
>  3 files changed, 373 insertions(+)
>  create mode 100644 src/PVE/Jobs.pm
>  create mode 100644 src/PVE/Jobs/VZDump.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index baa2688..853b562 100644
> --- a/src/Makefile
> +++ b/src/Makefile

[..]

> --- /dev/null
> +++ b/src/PVE/Jobs.pm
> @@ -0,0 +1,282 @@
> +package PVE::Jobs;
> +
> +use strict;
> +use warnings;
> +use JSON;
> +
> +use PVE::Cluster qw(cfs_read_file cfs_lock_file);
> +use PVE::Jobs::Plugin;
> +use PVE::Jobs::VZDump;
> +use PVE::Tools;
> +
> +PVE::Jobs::VZDump->register();
> +PVE::Jobs::Plugin->init();

so replication would also need to move here when it gets converted to a 
regular job instead of being called directly by pvescheduler

currently those get called via PVE::API2::Replication->run_jobs(), which 
lives in pve-manager..

> +
> +my $state_dir = "/var/lib/pve-manager/jobs";
> +my $lock_dir = "/var/lock/pve-manager";

any thoughts about migrating these? if not possible, some rationale for 
why they have to stay until XXX would be good :)

> +
> +my $get_state_file = sub {
> +    my ($jobid, $type) = @_;
> +    return "$state_dir/$type-$jobid.json";
> +};
> +

[..]

> diff --git a/src/PVE/Jobs/VZDump.pm b/src/PVE/Jobs/VZDump.pm
> new file mode 100644
> index 0000000..44fe33d
> --- /dev/null
> +++ b/src/PVE/Jobs/VZDump.pm
> @@ -0,0 +1,87 @@
> +package PVE::Jobs::VZDump;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::INotify;
> +use PVE::VZDump::Common;
> +use PVE::API2::VZDump;

and this (like PVE::API2::Replication mentioned above) lives in 
pve-manager as well and in turn calls other code that also lives in 
pve-manager, mainly

PVE::VZDump
-> PVE::VZDump::QemuServer (qemu-server, which is above pve-guest-common 
in the dep tree)
-> PVE::VZDump::LXC (pve-container, same problem)
-> PVE::HA::Config & PVE::HA::Env::PVE2 (already involved in a cycle 
with pve-container / qemu-server, that would just need some look whether 
it makes stuff worse or not)
-> PVE::VZDump::Common & PVE::VZDump::Plugin (pve-guest-common, so okay)
-> PVE::API2Tools (pve-manager)

so that whole chain can't move to pve-guest-common, and the 'use' 
statement and call to the API handler can't be there either.

a few approaches that might help:
- handwave the issue away by calling the vzdump binary instead of the 
  API handler (extra process that needs to be tracked, no way to 
  preserve authuser if we do manual job execution, .., kinda 'meh')
- split the plugin definition (which we want to move as low in the 
  dependency tree as possible so that more other packages can use it for 
  job modification) from the 'run' sub for the job type (e.g., by 
  allowing to specify it in the scheduler/pve-manager if the plugin 
  doesn't have one, a simple map of plugin type -> run sub could work)
- other great ideas? ;)

like I said, the same issue also applies to replication once we convert 
that to jobs, so any solution would need to take that into account 
(e.g., the two I mentioned above would work for that as well - via 
'pvesr' call or leaving the run part in pve-manager and only moving the 
rest to pve-guest-common).

> +use PVE::Cluster;
> +use PVE::JSONSchema;
> +
> +use base qw(PVE::Jobs::Plugin);
> +
> +sub type {
> +    return 'vzdump';
> +}
> +
> +my $props = PVE::VZDump::Common::json_config_properties();
> +

[..]

> +
> +sub run {
> +    my ($class, $conf) = @_;
> +
> +    # remove all non vzdump related options
> +    foreach my $opt (keys %$conf) {
> +	delete $conf->{$opt} if !defined($props->{$opt});
> +    }
> +
> +    my $retention = $conf->{'prune-backups'};
> +    if ($retention && ref($retention) eq 'HASH') { # fixup, its required as string parameter
> +	$conf->{'prune-backups'} = PVE::JSONSchema::print_property_string($retention, 'prune-backups');
> +    }
> +
> +    $conf->{quiet} = 1; # do not write to stdout/stderr
> +
> +    PVE::Cluster::cfs_update(); # refresh vmlist
> +
> +    return PVE::API2::VZDump->vzdump($conf);

this call here is the problematic one!

> +}
> +
> +1;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>