* [pve-devel] [PATCH cluster 1/1] add 'jobs.cfg' to observed files
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH manager 1/7] replace systemd timer with pvescheduler daemon Dominik Csapak
` (6 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
data/PVE/Cluster.pm | 1 +
data/src/status.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index 4d09c60..ab8f713 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -45,6 +45,7 @@ my $dbbackupdir = "/var/lib/pve-cluster/backup";
# using a computed version and only those can be used by the cfs_*_file methods
my $observed = {
'vzdump.cron' => 1,
+ 'jobs.cfg' => 1,
'storage.cfg' => 1,
'datacenter.cfg' => 1,
'replication.cfg' => 1,
diff --git a/data/src/status.c b/data/src/status.c
index ff0b1e9..9bceaeb 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -89,6 +89,7 @@ static memdb_change_t memdb_change_array[] = {
{ .path = "priv/ipam.db" },
{ .path = "datacenter.cfg" },
{ .path = "vzdump.cron" },
+ { .path = "jobs.cfg" },
{ .path = "ha/crm_commands" },
{ .path = "ha/manager_status" },
{ .path = "ha/resources.cfg" },
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH manager 1/7] replace systemd timer with pvescheduler daemon
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH cluster 1/1] add 'jobs.cfg' to observed files Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-10-29 12:05 ` Fabian Ebner
2021-10-07 8:27 ` [pve-devel] [PATCH manager 2/7] postinst: use reload-or-restart instead of reload-or-try-restart Dominik Csapak
` (5 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel; +Cc: Thomas Lamprecht
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
The whole thing is already prepared for this, the systemd timer was
just a fixed periodic timer with a frequency of one minute. And we
just introduced it as the assumption was made that less memory usage
would be generated with this approach, AFAIK.
But logging 4+ lines just about that the timer was started, even if
it does nothing, and that 24/7 is not to cheap and a bit annoying.
So in a first step add a simple daemon, which forks of a child for
running jobs once a minute.
This could be made still a bit more intelligent, i.e., look if we
have jobs tor run before forking - as forking is not the cheapest
syscall. Further, we could adapt the sleep interval to the next time
we actually need to run a job (and sending a SIGUSR to the daemon if
a job interval changes such, that this interval got narrower)
We try to sync running on minute-change boundaries at start, this
emulates systemd.timer behaviour, we had until now. Also user can
configure jobs on minute precision, so they probably expect that
those also start really close to a minute change event.
Could be adapted to resync during running, to factor in time drift.
But, as long as enough cpu cycles are available we run in correct
monotonic intervalls, so this isn't a must, IMO.
Another improvement could be locking a bit more fine grained, i.e.
not on a per-all-local-job-runs basis, but per-job (per-guest?)
basis, which would improve temporary starvement of small
high-periodic jobs through big, less peridoci jobs.
We argued that it's the user fault if such situations arise, but they
can evolve over time without noticing, especially in compolexer
setups.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/Service/Makefile | 2 +-
PVE/Service/pvescheduler.pm | 102 ++++++++++++++++++++++++++++++++++
bin/Makefile | 6 +-
bin/pvescheduler | 28 ++++++++++
debian/postinst | 3 +-
services/Makefile | 3 +-
services/pvescheduler.service | 16 ++++++
services/pvesr.service | 8 ---
services/pvesr.timer | 12 ----
9 files changed, 155 insertions(+), 25 deletions(-)
create mode 100644 PVE/Service/pvescheduler.pm
create mode 100755 bin/pvescheduler
create mode 100644 services/pvescheduler.service
delete mode 100644 services/pvesr.service
delete mode 100644 services/pvesr.timer
diff --git a/PVE/Service/Makefile b/PVE/Service/Makefile
index fc1cdb14..f66ceb81 100644
--- a/PVE/Service/Makefile
+++ b/PVE/Service/Makefile
@@ -1,6 +1,6 @@
include ../../defines.mk
-SOURCES=pvestatd.pm pveproxy.pm pvedaemon.pm spiceproxy.pm
+SOURCES=pvestatd.pm pveproxy.pm pvedaemon.pm spiceproxy.pm pvescheduler.pm
all:
diff --git a/PVE/Service/pvescheduler.pm b/PVE/Service/pvescheduler.pm
new file mode 100644
index 00000000..ce55c45a
--- /dev/null
+++ b/PVE/Service/pvescheduler.pm
@@ -0,0 +1,102 @@
+package PVE::Service::pvescheduler;
+
+use strict;
+use warnings;
+
+use POSIX qw(WNOHANG);
+use PVE::SafeSyslog;
+use PVE::API2::Replication;
+
+use PVE::Daemon;
+use base qw(PVE::Daemon);
+
+my $cmdline = [$0, @ARGV];
+my %daemon_options = (stop_wait_time => 180, max_workers => 0);
+my $daemon = __PACKAGE__->new('pvescheduler', $cmdline, %daemon_options);
+
+my $finish_jobs = sub {
+ my ($self) = @_;
+ foreach my $cpid (keys %{$self->{jobs}}) {
+ my $waitpid = waitpid($cpid, WNOHANG);
+ if (defined($waitpid) && ($waitpid == $cpid)) {
+ delete ($self->{jobs}->{$cpid});
+ }
+ }
+};
+
+sub run {
+ my ($self) = @_;
+
+ my $jobs= {};
+ $self->{jobs} = $jobs;
+
+ my $old_sig_chld = $SIG{CHLD};
+ local $SIG{CHLD} = sub {
+ local ($@, $!, $?); # do not overwrite error vars
+ $finish_jobs->($self);
+ $old_sig_chld->(@_) if $old_sig_chld;
+ };
+
+ my $logfunc = sub { syslog('info', $_[0]) };
+
+ my $run_jobs = sub {
+ my $child = fork();
+ if (!defined($child)) {
+ die "fork failed: $!\n";
+ } elsif ($child == 0) {
+ $self->after_fork_cleanup();
+ PVE::API2::Replication::run_jobs(undef, $logfunc, 0, 1);
+ POSIX::_exit(0);
+ }
+
+ $jobs->{$child} = 1;
+ };
+
+ # try to run near minute boundaries, makes more sense to the user as he
+ # configures jobs with minute precision
+ my ($current_seconds) = localtime;
+ sleep(60 - $current_seconds) if (60 - $current_seconds >= 5);
+
+ for (;;) {
+ last if $self->{shutdown_request};
+
+ $run_jobs->();
+
+ my $sleep_time = 60;
+ my $slept = 0; # SIGCHLD interrupts sleep, so we need to keep track
+ while ($slept < $sleep_time) {
+ last if $self->{shutdown_request};
+ $slept += sleep($sleep_time - $slept);
+ }
+ }
+
+ # jobs have a lock timeout of 60s, wait a bit more for graceful termination
+ my $timeout = 0;
+ while (keys %$jobs > 0 && $timeout < 75) {
+ kill 'TERM', keys %$jobs;
+ $timeout += sleep(5);
+ }
+ # ensure the rest gets stopped
+ kill 'KILL', keys %$jobs if (keys %$jobs > 0);
+}
+
+sub shutdown {
+ my ($self) = @_;
+
+ syslog('info', 'got shutdown request, signal running jobs to stop');
+
+ kill 'TERM', keys %{$self->{jobs}};
+ $self->{shutdown_request} = 1;
+}
+
+$daemon->register_start_command();
+$daemon->register_stop_command();
+$daemon->register_status_command();
+
+our $cmddef = {
+ start => [ __PACKAGE__, 'start', []],
+ stop => [ __PACKAGE__, 'stop', []],
+ status => [ __PACKAGE__, 'status', [], undef, sub { print shift . "\n";} ],
+};
+
+1;
diff --git a/bin/Makefile b/bin/Makefile
index df30b27c..12cb4671 100644
--- a/bin/Makefile
+++ b/bin/Makefile
@@ -6,7 +6,7 @@ export NOVIEW=1
PERL_DOC_INC_DIRS=..
include /usr/share/pve-doc-generator/pve-doc-generator.mk
-SERVICES = pvestatd pveproxy pvedaemon spiceproxy
+SERVICES = pvestatd pveproxy pvedaemon spiceproxy pvescheduler
CLITOOLS = vzdump pvesubscription pveceph pveam pvesr pvenode pvesh pve6to7
SCRIPTS = \
@@ -52,6 +52,10 @@ pve6to7.1:
printf ".TH PVE6TO7 1\n.SH NAME\npve6to7 \- Proxmox VE upgrade checker script for 6.4 to 7.x\n" > $@
printf ".SH SYNOPSIS\npve6to7 [--full]\n" >> $@
+pvescheduler.8:
+ # FIXME: add to doc-generator
+ echo ".TH pvescheduler 8" > $@
+
pveversion.1.pod: pveversion
pveupgrade.1.pod: pveupgrade
pvereport.1.pod: pvereport
diff --git a/bin/pvescheduler b/bin/pvescheduler
new file mode 100755
index 00000000..5f2bde52
--- /dev/null
+++ b/bin/pvescheduler
@@ -0,0 +1,28 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use PVE::Service::pvescheduler;
+
+use PVE::RPCEnvironment;
+use PVE::SafeSyslog;
+
+$SIG{'__WARN__'} = sub {
+ my $err = $@;
+ my $t = $_[0];
+ chomp $t;
+ print STDERR "$t\n";
+ syslog('warning', "%s", $t);
+ $@ = $err;
+};
+
+my $prepare = sub {
+ my $rpcenv = PVE::RPCEnvironment->init('priv');
+
+ $rpcenv->init_request();
+ $rpcenv->set_language($ENV{LANG});
+ $rpcenv->set_user('root@pam');
+};
+
+PVE::Service::pvescheduler->run_cli_handler(prepare => $prepare);
diff --git a/debian/postinst b/debian/postinst
index aed4da3f..f6407178 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -79,6 +79,7 @@ case "$1" in
deb-systemd-invoke reload-or-try-restart pvestatd.service
deb-systemd-invoke reload-or-try-restart pveproxy.service
deb-systemd-invoke reload-or-try-restart spiceproxy.service
+ deb-systemd-invoke reload-or-try-restart pvescheduler.service
exit 0;;
@@ -102,7 +103,7 @@ case "$1" in
# same as dh_systemd_enable (code copied)
- UNITS="pvedaemon.service pveproxy.service spiceproxy.service pvestatd.service pvebanner.service pvesr.timer pve-daily-update.timer"
+ UNITS="pvedaemon.service pveproxy.service spiceproxy.service pvestatd.service pvebanner.service pvescheduler.service pve-daily-update.timer"
NO_RESTART_UNITS="pvenetcommit.service pve-guests.service"
for unit in ${UNITS} ${NO_RESTART_UNITS}; do
diff --git a/services/Makefile b/services/Makefile
index a3d04a2f..b46c7119 100644
--- a/services/Makefile
+++ b/services/Makefile
@@ -13,8 +13,7 @@ SERVICES= \
pve-storage.target \
pve-daily-update.service\
pve-daily-update.timer \
- pvesr.service \
- pvesr.timer
+ pvescheduler.service
.PHONY: install
install: ${SERVICES}
diff --git a/services/pvescheduler.service b/services/pvescheduler.service
new file mode 100644
index 00000000..11769e80
--- /dev/null
+++ b/services/pvescheduler.service
@@ -0,0 +1,16 @@
+[Unit]
+Description=Proxmox VE scheduler
+ConditionPathExists=/usr/bin/pvescheduler
+Wants=pve-cluster.service
+After=pve-cluster.service
+After=pve-storage.target
+
+[Service]
+ExecStart=/usr/bin/pvescheduler start
+ExecStop=/usr/bin/pvescheduler stop
+PIDFile=/var/run/pvescheduler.pid
+KillMode=process
+Type=forking
+
+[Install]
+WantedBy=multi-user.target
diff --git a/services/pvesr.service b/services/pvesr.service
deleted file mode 100644
index cbaed1ca..00000000
--- a/services/pvesr.service
+++ /dev/null
@@ -1,8 +0,0 @@
-[Unit]
-Description=Proxmox VE replication runner
-ConditionPathExists=/usr/bin/pvesr
-After=pve-cluster.service
-
-[Service]
-Type=oneshot
-ExecStart=/usr/bin/pvesr run --mail 1
diff --git a/services/pvesr.timer b/services/pvesr.timer
deleted file mode 100644
index 01d7b9c7..00000000
--- a/services/pvesr.timer
+++ /dev/null
@@ -1,12 +0,0 @@
-[Unit]
-Description=Proxmox VE replication runner
-
-[Timer]
-AccuracySec=1
-RemainAfterElapse=no
-
-[Timer]
-OnCalendar=minutely
-
-[Install]
-WantedBy=timers.target
\ No newline at end of file
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 1/7] replace systemd timer with pvescheduler daemon
2021-10-07 8:27 ` [pve-devel] [PATCH manager 1/7] replace systemd timer with pvescheduler daemon Dominik Csapak
@ 2021-10-29 12:05 ` Fabian Ebner
2021-11-02 9:26 ` Dominik Csapak
0 siblings, 1 reply; 17+ messages in thread
From: Fabian Ebner @ 2021-10-29 12:05 UTC (permalink / raw)
To: pve-devel, Dominik Csapak, Thomas Lamprecht
Am 07.10.21 um 10:27 schrieb Dominik Csapak:
> From: Thomas Lamprecht <t.lamprecht@proxmox.com>
>
> The whole thing is already prepared for this, the systemd timer was
> just a fixed periodic timer with a frequency of one minute. And we
> just introduced it as the assumption was made that less memory usage
> would be generated with this approach, AFAIK.
>
> But logging 4+ lines just about that the timer was started, even if
> it does nothing, and that 24/7 is not to cheap and a bit annoying.
>
> So in a first step add a simple daemon, which forks of a child for
> running jobs once a minute.
> This could be made still a bit more intelligent, i.e., look if we
> have jobs tor run before forking - as forking is not the cheapest
> syscall. Further, we could adapt the sleep interval to the next time
> we actually need to run a job (and sending a SIGUSR to the daemon if
> a job interval changes such, that this interval got narrower)
>
> We try to sync running on minute-change boundaries at start, this
> emulates systemd.timer behaviour, we had until now. Also user can
> configure jobs on minute precision, so they probably expect that
> those also start really close to a minute change event.
> Could be adapted to resync during running, to factor in time drift.
> But, as long as enough cpu cycles are available we run in correct
> monotonic intervalls, so this isn't a must, IMO.
>
> Another improvement could be locking a bit more fine grained, i.e.
> not on a per-all-local-job-runs basis, but per-job (per-guest?)
> basis, which would improve temporary starvement of small
> high-periodic jobs through big, less peridoci jobs.
> We argued that it's the user fault if such situations arise, but they
> can evolve over time without noticing, especially in compolexer
> setups.
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> PVE/Service/Makefile | 2 +-
> PVE/Service/pvescheduler.pm | 102 ++++++++++++++++++++++++++++++++++
> bin/Makefile | 6 +-
> bin/pvescheduler | 28 ++++++++++
> debian/postinst | 3 +-
> services/Makefile | 3 +-
> services/pvescheduler.service | 16 ++++++
> services/pvesr.service | 8 ---
> services/pvesr.timer | 12 ----
> 9 files changed, 155 insertions(+), 25 deletions(-)
> create mode 100644 PVE/Service/pvescheduler.pm
Just noting: All the other .pm-files in PVE/Service have the executable
flag set.
> create mode 100755 bin/pvescheduler
> create mode 100644 services/pvescheduler.service
> delete mode 100644 services/pvesr.service
> delete mode 100644 services/pvesr.timer
>
> diff --git a/PVE/Service/Makefile b/PVE/Service/Makefile
> index fc1cdb14..f66ceb81 100644
> --- a/PVE/Service/Makefile
> +++ b/PVE/Service/Makefile
> @@ -1,6 +1,6 @@
> include ../../defines.mk
>
> -SOURCES=pvestatd.pm pveproxy.pm pvedaemon.pm spiceproxy.pm
> +SOURCES=pvestatd.pm pveproxy.pm pvedaemon.pm spiceproxy.pm pvescheduler.pm
>
> all:
>
> diff --git a/PVE/Service/pvescheduler.pm b/PVE/Service/pvescheduler.pm
> new file mode 100644
> index 00000000..ce55c45a
> --- /dev/null
> +++ b/PVE/Service/pvescheduler.pm
> @@ -0,0 +1,102 @@
> +package PVE::Service::pvescheduler;
> +
> +use strict;
> +use warnings;
> +
> +use POSIX qw(WNOHANG);
> +use PVE::SafeSyslog;
> +use PVE::API2::Replication;
> +
> +use PVE::Daemon;
> +use base qw(PVE::Daemon);
> +
> +my $cmdline = [$0, @ARGV];
> +my %daemon_options = (stop_wait_time => 180, max_workers => 0);
> +my $daemon = __PACKAGE__->new('pvescheduler', $cmdline, %daemon_options);
> +
> +my $finish_jobs = sub {
> + my ($self) = @_;
> + foreach my $cpid (keys %{$self->{jobs}}) {
> + my $waitpid = waitpid($cpid, WNOHANG);
> + if (defined($waitpid) && ($waitpid == $cpid)) {
> + delete ($self->{jobs}->{$cpid});
> + }
> + }
> +};
> +
> +sub run {
> + my ($self) = @_;
> +
> + my $jobs= {};
> + $self->{jobs} = $jobs;
> +
> + my $old_sig_chld = $SIG{CHLD};
> + local $SIG{CHLD} = sub {
> + local ($@, $!, $?); # do not overwrite error vars
> + $finish_jobs->($self);
> + $old_sig_chld->(@_) if $old_sig_chld;
> + };
> +
> + my $logfunc = sub { syslog('info', $_[0]) };
> +
> + my $run_jobs = sub {
> + my $child = fork();
> + if (!defined($child)) {
> + die "fork failed: $!\n";
> + } elsif ($child == 0) {
> + $self->after_fork_cleanup();
> + PVE::API2::Replication::run_jobs(undef, $logfunc, 0, 1);
Like this, the whole replication log ends up in syslog.
> + POSIX::_exit(0);
> + }
> +
> + $jobs->{$child} = 1;
> + };
> +
> + # try to run near minute boundaries, makes more sense to the user as he
> + # configures jobs with minute precision
> + my ($current_seconds) = localtime;
> + sleep(60 - $current_seconds) if (60 - $current_seconds >= 5);
> +
> + for (;;) {
> + last if $self->{shutdown_request};
> +
> + $run_jobs->();
> +
> + my $sleep_time = 60;
> + my $slept = 0; # SIGCHLD interrupts sleep, so we need to keep track
> + while ($slept < $sleep_time) {
> + last if $self->{shutdown_request};
> + $slept += sleep($sleep_time - $slept);
> + }
> + }
Won't the loop drift away from the minute boundary over time? The rest
of the loop takes time to execute, which pushes in the positive
direction, while the fact that the accuracy when interrupted is only
whole seconds pushes in the negative direction. But we can't really rely
on those to cancel each other out.
> +
> + # jobs have a lock timeout of 60s, wait a bit more for graceful termination
> + my $timeout = 0;
> + while (keys %$jobs > 0 && $timeout < 75) {
> + kill 'TERM', keys %$jobs;
> + $timeout += sleep(5);
> + }
> + # ensure the rest gets stopped
> + kill 'KILL', keys %$jobs if (keys %$jobs > 0);
> +}
> +
> +sub shutdown {
> + my ($self) = @_;
> +
> + syslog('info', 'got shutdown request, signal running jobs to stop');
> +
> + kill 'TERM', keys %{$self->{jobs}};
> + $self->{shutdown_request} = 1;
> +}
> +
> +$daemon->register_start_command();
> +$daemon->register_stop_command();
> +$daemon->register_status_command();
> +
> +our $cmddef = {
> + start => [ __PACKAGE__, 'start', []],
> + stop => [ __PACKAGE__, 'stop', []],
> + status => [ __PACKAGE__, 'status', [], undef, sub { print shift . "\n";} ],
> +};
> +
> +1;
> diff --git a/bin/Makefile b/bin/Makefile
> index df30b27c..12cb4671 100644
> --- a/bin/Makefile
> +++ b/bin/Makefile
> @@ -6,7 +6,7 @@ export NOVIEW=1
> PERL_DOC_INC_DIRS=..
> include /usr/share/pve-doc-generator/pve-doc-generator.mk
>
> -SERVICES = pvestatd pveproxy pvedaemon spiceproxy
> +SERVICES = pvestatd pveproxy pvedaemon spiceproxy pvescheduler
> CLITOOLS = vzdump pvesubscription pveceph pveam pvesr pvenode pvesh pve6to7
>
> SCRIPTS = \
> @@ -52,6 +52,10 @@ pve6to7.1:
> printf ".TH PVE6TO7 1\n.SH NAME\npve6to7 \- Proxmox VE upgrade checker script for 6.4 to 7.x\n" > $@
> printf ".SH SYNOPSIS\npve6to7 [--full]\n" >> $@
>
> +pvescheduler.8:
> + # FIXME: add to doc-generator
> + echo ".TH pvescheduler 8" > $@
> +
> pveversion.1.pod: pveversion
> pveupgrade.1.pod: pveupgrade
> pvereport.1.pod: pvereport
> diff --git a/bin/pvescheduler b/bin/pvescheduler
> new file mode 100755
> index 00000000..5f2bde52
> --- /dev/null
> +++ b/bin/pvescheduler
> @@ -0,0 +1,28 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Service::pvescheduler;
> +
> +use PVE::RPCEnvironment;
> +use PVE::SafeSyslog;
> +
> +$SIG{'__WARN__'} = sub {
> + my $err = $@;
> + my $t = $_[0];
> + chomp $t;
> + print STDERR "$t\n";
> + syslog('warning', "%s", $t);
> + $@ = $err;
> +};
> +
> +my $prepare = sub {
> + my $rpcenv = PVE::RPCEnvironment->init('priv');
> +
> + $rpcenv->init_request();
> + $rpcenv->set_language($ENV{LANG});
> + $rpcenv->set_user('root@pam');
> +};
> +
> +PVE::Service::pvescheduler->run_cli_handler(prepare => $prepare);
> diff --git a/debian/postinst b/debian/postinst
> index aed4da3f..f6407178 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -79,6 +79,7 @@ case "$1" in
> deb-systemd-invoke reload-or-try-restart pvestatd.service
> deb-systemd-invoke reload-or-try-restart pveproxy.service
> deb-systemd-invoke reload-or-try-restart spiceproxy.service
> + deb-systemd-invoke reload-or-try-restart pvescheduler.service
>
> exit 0;;
>
> @@ -102,7 +103,7 @@ case "$1" in
>
> # same as dh_systemd_enable (code copied)
>
> - UNITS="pvedaemon.service pveproxy.service spiceproxy.service pvestatd.service pvebanner.service pvesr.timer pve-daily-update.timer"
> + UNITS="pvedaemon.service pveproxy.service spiceproxy.service pvestatd.service pvebanner.service pvescheduler.service pve-daily-update.timer"
> NO_RESTART_UNITS="pvenetcommit.service pve-guests.service"
>
> for unit in ${UNITS} ${NO_RESTART_UNITS}; do
> diff --git a/services/Makefile b/services/Makefile
> index a3d04a2f..b46c7119 100644
> --- a/services/Makefile
> +++ b/services/Makefile
> @@ -13,8 +13,7 @@ SERVICES= \
> pve-storage.target \
> pve-daily-update.service\
> pve-daily-update.timer \
> - pvesr.service \
> - pvesr.timer
> + pvescheduler.service
>
> .PHONY: install
> install: ${SERVICES}
> diff --git a/services/pvescheduler.service b/services/pvescheduler.service
> new file mode 100644
> index 00000000..11769e80
> --- /dev/null
> +++ b/services/pvescheduler.service
> @@ -0,0 +1,16 @@
> +[Unit]
> +Description=Proxmox VE scheduler
> +ConditionPathExists=/usr/bin/pvescheduler
> +Wants=pve-cluster.service
> +After=pve-cluster.service
> +After=pve-storage.target
> +
> +[Service]
> +ExecStart=/usr/bin/pvescheduler start
> +ExecStop=/usr/bin/pvescheduler stop
> +PIDFile=/var/run/pvescheduler.pid
> +KillMode=process
> +Type=forking
> +
> +[Install]
> +WantedBy=multi-user.target
> diff --git a/services/pvesr.service b/services/pvesr.service
> deleted file mode 100644
> index cbaed1ca..00000000
> --- a/services/pvesr.service
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -[Unit]
> -Description=Proxmox VE replication runner
> -ConditionPathExists=/usr/bin/pvesr
> -After=pve-cluster.service
> -
> -[Service]
> -Type=oneshot
> -ExecStart=/usr/bin/pvesr run --mail 1
> diff --git a/services/pvesr.timer b/services/pvesr.timer
> deleted file mode 100644
> index 01d7b9c7..00000000
> --- a/services/pvesr.timer
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -[Unit]
> -Description=Proxmox VE replication runner
> -
> -[Timer]
> -AccuracySec=1
> -RemainAfterElapse=no
> -
> -[Timer]
> -OnCalendar=minutely
> -
> -[Install]
> -WantedBy=timers.target
> \ No newline at end of file
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 1/7] replace systemd timer with pvescheduler daemon
2021-10-29 12:05 ` Fabian Ebner
@ 2021-11-02 9:26 ` Dominik Csapak
0 siblings, 0 replies; 17+ messages in thread
From: Dominik Csapak @ 2021-11-02 9:26 UTC (permalink / raw)
To: Fabian Ebner, pve-devel, Thomas Lamprecht
On 10/29/21 14:05, Fabian Ebner wrote:
> Am 07.10.21 um 10:27 schrieb Dominik Csapak:
>> From: Thomas Lamprecht <t.lamprecht@proxmox.com>
>>
>> The whole thing is already prepared for this, the systemd timer was
>> just a fixed periodic timer with a frequency of one minute. And we
>> just introduced it as the assumption was made that less memory usage
>> would be generated with this approach, AFAIK.
>>
>> But logging 4+ lines just about that the timer was started, even if
>> it does nothing, and that 24/7 is not to cheap and a bit annoying.
>>
>> So in a first step add a simple daemon, which forks of a child for
>> running jobs once a minute.
>> This could be made still a bit more intelligent, i.e., look if we
>> have jobs tor run before forking - as forking is not the cheapest
>> syscall. Further, we could adapt the sleep interval to the next time
>> we actually need to run a job (and sending a SIGUSR to the daemon if
>> a job interval changes such, that this interval got narrower)
>>
>> We try to sync running on minute-change boundaries at start, this
>> emulates systemd.timer behaviour, we had until now. Also user can
>> configure jobs on minute precision, so they probably expect that
>> those also start really close to a minute change event.
>> Could be adapted to resync during running, to factor in time drift.
>> But, as long as enough cpu cycles are available we run in correct
>> monotonic intervalls, so this isn't a must, IMO.
>>
>> Another improvement could be locking a bit more fine grained, i.e.
>> not on a per-all-local-job-runs basis, but per-job (per-guest?)
>> basis, which would improve temporary starvement of small
>> high-periodic jobs through big, less peridoci jobs.
>> We argued that it's the user fault if such situations arise, but they
>> can evolve over time without noticing, especially in compolexer
>> setups.
>>
>> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> PVE/Service/Makefile | 2 +-
>> PVE/Service/pvescheduler.pm | 102 ++++++++++++++++++++++++++++++++++
>> bin/Makefile | 6 +-
>> bin/pvescheduler | 28 ++++++++++
>> debian/postinst | 3 +-
>> services/Makefile | 3 +-
>> services/pvescheduler.service | 16 ++++++
>> services/pvesr.service | 8 ---
>> services/pvesr.timer | 12 ----
>> 9 files changed, 155 insertions(+), 25 deletions(-)
>> create mode 100644 PVE/Service/pvescheduler.pm
>
> Just noting: All the other .pm-files in PVE/Service have the executable
> flag set.
>
>> create mode 100755 bin/pvescheduler
>> create mode 100644 services/pvescheduler.service
>> delete mode 100644 services/pvesr.service
>> delete mode 100644 services/pvesr.timer
>>
>> diff --git a/PVE/Service/Makefile b/PVE/Service/Makefile
>> index fc1cdb14..f66ceb81 100644
>> --- a/PVE/Service/Makefile
>> +++ b/PVE/Service/Makefile
>> @@ -1,6 +1,6 @@
>> include ../../defines.mk
>> -SOURCES=pvestatd.pm pveproxy.pm pvedaemon.pm spiceproxy.pm
>> +SOURCES=pvestatd.pm pveproxy.pm pvedaemon.pm spiceproxy.pm
>> pvescheduler.pm
>> all:
>> diff --git a/PVE/Service/pvescheduler.pm b/PVE/Service/pvescheduler.pm
>> new file mode 100644
>> index 00000000..ce55c45a
>> --- /dev/null
>> +++ b/PVE/Service/pvescheduler.pm
>> @@ -0,0 +1,102 @@
>> +package PVE::Service::pvescheduler;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use POSIX qw(WNOHANG);
>> +use PVE::SafeSyslog;
>> +use PVE::API2::Replication;
>> +
>> +use PVE::Daemon;
>> +use base qw(PVE::Daemon);
>> +
>> +my $cmdline = [$0, @ARGV];
>> +my %daemon_options = (stop_wait_time => 180, max_workers => 0);
>> +my $daemon = __PACKAGE__->new('pvescheduler', $cmdline,
>> %daemon_options);
>> +
>> +my $finish_jobs = sub {
>> + my ($self) = @_;
>> + foreach my $cpid (keys %{$self->{jobs}}) {
>> + my $waitpid = waitpid($cpid, WNOHANG);
>> + if (defined($waitpid) && ($waitpid == $cpid)) {
>> + delete ($self->{jobs}->{$cpid});
>> + }
>> + }
>> +};
>> +
>> +sub run {
>> + my ($self) = @_;
>> +
>> + my $jobs= {};
>> + $self->{jobs} = $jobs;
>> +
>> + my $old_sig_chld = $SIG{CHLD};
>> + local $SIG{CHLD} = sub {
>> + local ($@, $!, $?); # do not overwrite error vars
>> + $finish_jobs->($self);
>> + $old_sig_chld->(@_) if $old_sig_chld;
>> + };
>> +
>> + my $logfunc = sub { syslog('info', $_[0]) };
>> +
>> + my $run_jobs = sub {
>> + my $child = fork();
>> + if (!defined($child)) {
>> + die "fork failed: $!\n";
>> + } elsif ($child == 0) {
>> + $self->after_fork_cleanup();
>> + PVE::API2::Replication::run_jobs(undef, $logfunc, 0, 1);
>
> Like this, the whole replication log ends up in syslog.
yes, you're right, we probably don't want that in the syslog
(same for vzdump jobs later)
>
>> + POSIX::_exit(0);
>> + }
>> +
>> + $jobs->{$child} = 1;
>> + };
>> +
>> + # try to run near minute boundaries, makes more sense to the user
>> as he
>> + # configures jobs with minute precision
>> + my ($current_seconds) = localtime;
>> + sleep(60 - $current_seconds) if (60 - $current_seconds >= 5);
>> +
>> + for (;;) {
>> + last if $self->{shutdown_request};
>> +
>> + $run_jobs->();
>> +
>> + my $sleep_time = 60;
>> + my $slept = 0; # SIGCHLD interrupts sleep, so we need to keep track
>> + while ($slept < $sleep_time) {
>> + last if $self->{shutdown_request};
>> + $slept += sleep($sleep_time - $slept);
>> + }
>> + }
>
> Won't the loop drift away from the minute boundary over time? The rest
> of the loop takes time to execute, which pushes in the positive
> direction, while the fact that the accuracy when interrupted is only
> whole seconds pushes in the negative direction. But we can't really rely
> on those to cancel each other out.
>
yes, imho the fix would be to pull the correction we have from before
the loop to inside the loop. (maybe only every X loops, since the drift
from the few forks + sleep should not amount to much per iteration)
>> +
>> + # jobs have a lock timeout of 60s, wait a bit more for graceful
>> termination
>> + my $timeout = 0;
>> + while (keys %$jobs > 0 && $timeout < 75) {
>> + kill 'TERM', keys %$jobs;
>> + $timeout += sleep(5);
>> + }
>> + # ensure the rest gets stopped
>> + kill 'KILL', keys %$jobs if (keys %$jobs > 0);
>> +}
>> +
>> +sub shutdown {
>> + my ($self) = @_;
>> +
>> + syslog('info', 'got shutdown request, signal running jobs to stop');
>> +
>> + kill 'TERM', keys %{$self->{jobs}};
>> + $self->{shutdown_request} = 1;
>> +}
>> +
>> +$daemon->register_start_command();
>> +$daemon->register_stop_command();
>> +$daemon->register_status_command();
>> +
>> +our $cmddef = {
>> + start => [ __PACKAGE__, 'start', []],
>> + stop => [ __PACKAGE__, 'stop', []],
>> + status => [ __PACKAGE__, 'status', [], undef, sub { print shift .
>> "\n";} ],
>> +};
>> +
>> +1;
>> diff --git a/bin/Makefile b/bin/Makefile
>> index df30b27c..12cb4671 100644
>> --- a/bin/Makefile
>> +++ b/bin/Makefile
>> @@ -6,7 +6,7 @@ export NOVIEW=1
>> PERL_DOC_INC_DIRS=..
>> include /usr/share/pve-doc-generator/pve-doc-generator.mk
>> -SERVICES = pvestatd pveproxy pvedaemon spiceproxy
>> +SERVICES = pvestatd pveproxy pvedaemon spiceproxy pvescheduler
>> CLITOOLS = vzdump pvesubscription pveceph pveam pvesr pvenode pvesh
>> pve6to7
>> SCRIPTS = \
>> @@ -52,6 +52,10 @@ pve6to7.1:
>> printf ".TH PVE6TO7 1\n.SH NAME\npve6to7 \- Proxmox VE upgrade
>> checker script for 6.4 to 7.x\n" > $@
>> printf ".SH SYNOPSIS\npve6to7 [--full]\n" >> $@
>> +pvescheduler.8:
>> + # FIXME: add to doc-generator
>> + echo ".TH pvescheduler 8" > $@
>> +
>> pveversion.1.pod: pveversion
>> pveupgrade.1.pod: pveupgrade
>> pvereport.1.pod: pvereport
>> diff --git a/bin/pvescheduler b/bin/pvescheduler
>> new file mode 100755
>> index 00000000..5f2bde52
>> --- /dev/null
>> +++ b/bin/pvescheduler
>> @@ -0,0 +1,28 @@
>> +#!/usr/bin/perl
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::Service::pvescheduler;
>> +
>> +use PVE::RPCEnvironment;
>> +use PVE::SafeSyslog;
>> +
>> +$SIG{'__WARN__'} = sub {
>> + my $err = $@;
>> + my $t = $_[0];
>> + chomp $t;
>> + print STDERR "$t\n";
>> + syslog('warning', "%s", $t);
>> + $@ = $err;
>> +};
>> +
>> +my $prepare = sub {
>> + my $rpcenv = PVE::RPCEnvironment->init('priv');
>> +
>> + $rpcenv->init_request();
>> + $rpcenv->set_language($ENV{LANG});
>> + $rpcenv->set_user('root@pam');
>> +};
>> +
>> +PVE::Service::pvescheduler->run_cli_handler(prepare => $prepare);
>> diff --git a/debian/postinst b/debian/postinst
>> index aed4da3f..f6407178 100755
>> --- a/debian/postinst
>> +++ b/debian/postinst
>> @@ -79,6 +79,7 @@ case "$1" in
>> deb-systemd-invoke reload-or-try-restart pvestatd.service
>> deb-systemd-invoke reload-or-try-restart pveproxy.service
>> deb-systemd-invoke reload-or-try-restart spiceproxy.service
>> + deb-systemd-invoke reload-or-try-restart pvescheduler.service
>> exit 0;;
>> @@ -102,7 +103,7 @@ case "$1" in
>> # same as dh_systemd_enable (code copied)
>> - UNITS="pvedaemon.service pveproxy.service spiceproxy.service
>> pvestatd.service pvebanner.service pvesr.timer pve-daily-update.timer"
>> + UNITS="pvedaemon.service pveproxy.service spiceproxy.service
>> pvestatd.service pvebanner.service pvescheduler.service
>> pve-daily-update.timer"
>> NO_RESTART_UNITS="pvenetcommit.service pve-guests.service"
>> for unit in ${UNITS} ${NO_RESTART_UNITS}; do
>> diff --git a/services/Makefile b/services/Makefile
>> index a3d04a2f..b46c7119 100644
>> --- a/services/Makefile
>> +++ b/services/Makefile
>> @@ -13,8 +13,7 @@ SERVICES= \
>> pve-storage.target \
>> pve-daily-update.service\
>> pve-daily-update.timer \
>> - pvesr.service \
>> - pvesr.timer
>> + pvescheduler.service
>> .PHONY: install
>> install: ${SERVICES}
>> diff --git a/services/pvescheduler.service
>> b/services/pvescheduler.service
>> new file mode 100644
>> index 00000000..11769e80
>> --- /dev/null
>> +++ b/services/pvescheduler.service
>> @@ -0,0 +1,16 @@
>> +[Unit]
>> +Description=Proxmox VE scheduler
>> +ConditionPathExists=/usr/bin/pvescheduler
>> +Wants=pve-cluster.service
>> +After=pve-cluster.service
>> +After=pve-storage.target
>> +
>> +[Service]
>> +ExecStart=/usr/bin/pvescheduler start
>> +ExecStop=/usr/bin/pvescheduler stop
>> +PIDFile=/var/run/pvescheduler.pid
>> +KillMode=process
>> +Type=forking
>> +
>> +[Install]
>> +WantedBy=multi-user.target
>> diff --git a/services/pvesr.service b/services/pvesr.service
>> deleted file mode 100644
>> index cbaed1ca..00000000
>> --- a/services/pvesr.service
>> +++ /dev/null
>> @@ -1,8 +0,0 @@
>> -[Unit]
>> -Description=Proxmox VE replication runner
>> -ConditionPathExists=/usr/bin/pvesr
>> -After=pve-cluster.service
>> -
>> -[Service]
>> -Type=oneshot
>> -ExecStart=/usr/bin/pvesr run --mail 1
>> diff --git a/services/pvesr.timer b/services/pvesr.timer
>> deleted file mode 100644
>> index 01d7b9c7..00000000
>> --- a/services/pvesr.timer
>> +++ /dev/null
>> @@ -1,12 +0,0 @@
>> -[Unit]
>> -Description=Proxmox VE replication runner
>> -
>> -[Timer]
>> -AccuracySec=1
>> -RemainAfterElapse=no
>> -
>> -[Timer]
>> -OnCalendar=minutely
>> -
>> -[Install]
>> -WantedBy=timers.target
>> \ No newline at end of file
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pve-devel] [PATCH manager 2/7] postinst: use reload-or-restart instead of reload-or-try-restart
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH cluster 1/1] add 'jobs.cfg' to observed files Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH manager 1/7] replace systemd timer with pvescheduler daemon Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-10-07 8:38 ` [pve-devel] applied: " Thomas Lamprecht
2021-10-07 8:27 ` [pve-devel] [PATCH manager 3/7] api/backup: refactor string for all days Dominik Csapak
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
the only difference is that reload-or-try-restart does not
do anything if the service is not started already.
on upgrade, we explicitely check if the service is enabled, and only
then do this action. so it would now start daemons that were
stopped but not disabled (which is not really valid state anyway)
this starts new daemons (like the pvescheduler) automatically on
a package upgrade
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
debian/postinst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/debian/postinst b/debian/postinst
index f6407178..7be1aa3d 100755
--- a/debian/postinst
+++ b/debian/postinst
@@ -181,7 +181,7 @@ case "$1" in
# modeled after code generated by dh_start
for unit in ${UNITS}; do
if test -n "$2"; then
- dh_action="reload-or-try-restart";
+ dh_action="reload-or-restart";
else
dh_action="start"
fi
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] applied: [PATCH manager 2/7] postinst: use reload-or-restart instead of reload-or-try-restart
2021-10-07 8:27 ` [pve-devel] [PATCH manager 2/7] postinst: use reload-or-restart instead of reload-or-try-restart Dominik Csapak
@ 2021-10-07 8:38 ` Thomas Lamprecht
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Lamprecht @ 2021-10-07 8:38 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
we normally use `d/<foo>` for changes to the debian/ folder, iow, packaging changes.
On 07.10.21 10:27, Dominik Csapak wrote:
> the only difference is that reload-or-try-restart does not
> do anything if the service is not started already.
>
> on upgrade, we explicitely check if the service is enabled, and only
s/explicitely/explicitly/
> then do this action. so it would now start daemons that were
> stopped but not disabled (which is not really valid state anyway)
>
> this starts new daemons (like the pvescheduler) automatically on
> a package upgrade
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> debian/postinst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
with a few brushups to the commit message (and I took the freedom to
add a suggested-by me ;): applied, thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pve-devel] [PATCH manager 3/7] api/backup: refactor string for all days
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
` (2 preceding siblings ...)
2021-10-07 8:27 ` [pve-devel] [PATCH manager 2/7] postinst: use reload-or-restart instead of reload-or-try-restart Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs Dominik Csapak
` (3 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2/Backup.pm | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 3b343636..9dc3b48e 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -17,6 +17,8 @@ use PVE::VZDump::Common;
use base qw(PVE::RESTHandler);
+use constant ALL_DAYS => 'mon,tue,wed,thu,fri,sat,sun';
+
PVE::JSONSchema::register_format('pve-day-of-week', \&verify_day_of_week);
sub verify_day_of_week {
my ($value, $noerr) = @_;
@@ -101,7 +103,7 @@ __PACKAGE__->register_method({
type => 'string', format => 'pve-day-of-week-list',
optional => 1,
description => "Day of week selection.",
- default => 'mon,tue,wed,thu,fri,sat,sun',
+ default => ALL_DAYS,
},
enabled => {
type => 'boolean',
@@ -129,7 +131,7 @@ __PACKAGE__->register_method({
my $create_job = sub {
my $data = cfs_read_file('vzdump.cron');
- $param->{dow} = 'mon,tue,wed,thu,fri,sat,sun' if !defined($param->{dow});
+ $param->{dow} = ALL_DAYS if !defined($param->{dow});
$param->{enabled} = 1 if !defined($param->{enabled});
PVE::VZDump::verify_vzdump_parameters($param, 1);
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
` (3 preceding siblings ...)
2021-10-07 8:27 ` [pve-devel] [PATCH manager 3/7] api/backup: refactor string for all days Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-11-02 13:52 ` Fabian Ebner
2021-10-07 8:27 ` [pve-devel] [PATCH manager 5/7] pvescheduler: run jobs from jobs.cfg Dominik Csapak
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
this adds a SectionConfig handling for jobs (only 'vzdump' for now) that
represents a job that will be handled by pvescheduler and a basic
'job-state' handling for reading/writing state json files
this has some intersections with pvesrs state handling, but does not
use a single state file for all jobs, but seperate ones, like we
do it in the backup-server.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/Jobs.pm | 210 +++++++++++++++++++++++++++++++++++++++++++++
PVE/Jobs/Makefile | 16 ++++
PVE/Jobs/Plugin.pm | 61 +++++++++++++
PVE/Jobs/VZDump.pm | 54 ++++++++++++
PVE/Makefile | 3 +-
5 files changed, 343 insertions(+), 1 deletion(-)
create mode 100644 PVE/Jobs.pm
create mode 100644 PVE/Jobs/Makefile
create mode 100644 PVE/Jobs/Plugin.pm
create mode 100644 PVE/Jobs/VZDump.pm
diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
new file mode 100644
index 00000000..f17676bb
--- /dev/null
+++ b/PVE/Jobs.pm
@@ -0,0 +1,210 @@
+package PVE::Jobs;
+
+use strict;
+use warnings;
+use JSON;
+
+use PVE::Cluster qw(cfs_read_file);
+use PVE::Jobs::Plugin;
+use PVE::Jobs::VZDump;
+use PVE::Tools;
+
+PVE::Jobs::VZDump->register();
+PVE::Jobs::Plugin->init();
+
+my $state_dir = "/var/lib/pve-manager/jobs";
+my $lock_dir = "/var/lock/pve-manager";
+
+my $get_state_file = sub {
+ my ($jobid, $type) = @_;
+ return "$state_dir/$type-$jobid.json";
+};
+
+my $default_state = {
+ state => 'created',
+ time => 0,
+};
+
+# lockless, since we use file_set_contents, which is atomic
+sub read_job_state {
+ my ($jobid, $type) = @_;
+ my $path = $get_state_file->($jobid, $type);
+ return $default_state if ! -e $path;
+
+ my $raw = PVE::Tools::file_get_contents($path);
+
+ return $default_state if $raw eq '';
+
+ # untaint $raw
+ if ($raw =~ m/^(\{.*\})$/) {
+ return decode_json($1);
+ }
+
+ die "invalid json data in '$path'\n";
+}
+
+sub lock_job_state {
+ my ($jobid, $type, $sub) = @_;
+
+ my $filename = "$lock_dir/$type-$jobid.lck";
+
+ my $res = PVE::Tools::lock_file($filename, 10, $sub);
+ die $@ if $@;
+
+ return $res;
+}
+
+# returns the state and checks if the job was 'started' and is now finished
+sub get_job_state {
+ my ($jobid, $type) = @_;
+
+ # first check unlocked to save time,
+ my $state = read_job_state($jobid, $type);
+ if ($state->{state} ne 'started') {
+ return $state; # nothing to check
+ }
+
+ return lock_job_state($jobid, $type, sub {
+ my $state = read_job_state($jobid, $type);
+
+ if ($state->{state} ne 'started') {
+ return $state; # nothing to check
+ }
+
+ my ($task, $filename) = PVE::Tools::upid_decode($state->{upid}, 1);
+ die "unable to parse worker upid\n" if !$task;
+ die "no such task\n" if ! -f $filename;
+
+ my $pstart = PVE::ProcFSTools::read_proc_starttime($task->{pid});
+ if ($pstart && $pstart == $task->{pstart}) {
+ return $state; # still running
+ }
+
+ my $new_state = {
+ state => 'stopped',
+ msg => PVE::Tools::upid_read_status($state->{upid}),
+ upid => $state->{upid}
+ };
+
+ my $path = $get_state_file->($jobid, $type);
+ PVE::Tools::file_set_contents($path, encode_json($new_state));
+ return $new_state;
+ });
+}
+
+# must be called when the job is first created
+sub create_job {
+ my ($jobid, $type) = @_;
+
+ lock_job_state($jobid, $type, sub {
+ my $state = read_job_state($jobid, $type);
+
+ if ($state->{state} ne 'created') {
+ die "job state already exists\n";
+ }
+
+ $state->{time} = time();
+
+ my $path = $get_state_file->($jobid, $type);
+ PVE::Tools::file_set_contents($path, encode_json($state));
+ });
+}
+
+# to be called when the job is removed
+sub remove_job {
+ my ($jobid, $type) = @_;
+ my $path = $get_state_file->($jobid, $type);
+ unlink $path;
+}
+
+# will be called when the job was started according to schedule
+sub started_job {
+ my ($jobid, $type, $upid) = @_;
+ lock_job_state($jobid, $type, sub {
+ my $state = {
+ state => 'started',
+ upid => $upid,
+ };
+
+ my $path = $get_state_file->($jobid, $type);
+ PVE::Tools::file_set_contents($path, encode_json($state));
+ });
+}
+
+# will be called when the job schedule is updated
+sub updated_job_schedule {
+ my ($jobid, $type) = @_;
+ lock_job_state($jobid, $type, sub {
+ my $old_state = read_job_state($jobid, $type);
+
+ if ($old_state->{state} eq 'started') {
+ return; # do not update timestamp on running jobs
+ }
+
+ $old_state->{updated} = time();
+
+ my $path = $get_state_file->($jobid, $type);
+ PVE::Tools::file_set_contents($path, encode_json($old_state));
+ });
+}
+
+sub get_last_runtime {
+ my ($jobid, $type) = @_;
+
+ my $state = read_job_state($jobid, $type);
+
+ if (defined($state->{updated})) {
+ return $state->{updated};
+ }
+
+ if (my $upid = $state->{upid}) {
+ my ($task) = PVE::Tools::upid_decode($upid, 1);
+ die "unable to parse worker upid\n" if !$task;
+ return $task->{starttime};
+ }
+
+ return $state->{time} // 0;
+}
+
+sub run_jobs {
+ my $jobs_cfg = cfs_read_file('jobs.cfg');
+ my $nodename = PVE::INotify::nodename();
+
+ foreach my $id (sort keys %{$jobs_cfg->{ids}}) {
+ my $cfg = $jobs_cfg->{ids}->{$id};
+ my $type = $cfg->{type};
+ my $schedule = delete $cfg->{schedule};
+
+ # only schedule local jobs
+ next if defined($cfg->{node}) && $cfg->{node} eq $nodename;
+
+ # only schedule enabled jobs
+ next if defined($cfg->{enabled}) && !$cfg->{enabled};
+
+ my $last_run = get_last_runtime($id, $type);
+ my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);
+ my $next_sync = PVE::CalendarEvent::compute_next_event($calspec, $last_run) // 0;
+
+ if (time() >= $next_sync) {
+ # only warn on errors, so that all jobs can run
+ my $state = get_job_state($id, $type); # to update the state
+
+ next if $state->{state} eq 'started'; # still running
+
+ my $plugin = PVE::Jobs::Plugin->lookup($type);
+
+ my $upid = eval { $plugin->run($cfg) };
+ warn $@ if $@;
+ if ($upid) {
+ started_job($id, $type, $upid);
+ }
+ }
+ }
+}
+
+sub setup_dirs {
+ mkdir $state_dir;
+ mkdir $lock_dir;
+}
+
+1;
diff --git a/PVE/Jobs/Makefile b/PVE/Jobs/Makefile
new file mode 100644
index 00000000..6023c3ba
--- /dev/null
+++ b/PVE/Jobs/Makefile
@@ -0,0 +1,16 @@
+include ../../defines.mk
+
+PERLSOURCE = \
+ Plugin.pm\
+ VZDump.pm
+
+all:
+
+.PHONY: clean
+clean:
+ rm -rf *~
+
+.PHONY: install
+install: ${PERLSOURCE}
+ install -d ${PERLLIBDIR}/PVE/Jobs
+ install -m 0644 ${PERLSOURCE} ${PERLLIBDIR}/PVE/Jobs
diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
new file mode 100644
index 00000000..69c31cf2
--- /dev/null
+++ b/PVE/Jobs/Plugin.pm
@@ -0,0 +1,61 @@
+package PVE::Jobs::Plugin;
+
+use strict;
+use warnings;
+
+use PVE::Cluster qw(cfs_register_file);
+
+use base qw(PVE::SectionConfig);
+
+cfs_register_file('jobs.cfg',
+ sub { __PACKAGE__->parse_config(@_); },
+ sub { __PACKAGE__->write_config(@_); });
+
+my $defaultData = {
+ propertyList => {
+ type => { description => "Section type." },
+ id => {
+ description => "The ID of the VZDump job.",
+ type => 'string',
+ format => 'pve-configid',
+ },
+ enabled => {
+ description => "Determines if the job is enabled.",
+ type => 'boolean',
+ default => 1,
+ optional => 1,
+ },
+ schedule => {
+ description => "Backup schedule. The format is a subset of `systemd` calendar events.",
+ type => 'string', format => 'pve-calendar-event',
+ maxLength => 128,
+ },
+ },
+};
+
+sub private {
+ return $defaultData;
+}
+
+sub parse_config {
+ my ($class, $filename, $raw) = @_;
+
+ my $cfg = $class->SUPER::parse_config($filename, $raw);
+
+ foreach my $id (sort keys %{$cfg->{ids}}) {
+ my $data = $cfg->{ids}->{$id};
+
+ $data->{id} = $id;
+ $data->{enabled} //= 1;
+ }
+
+ return $cfg;
+}
+
+sub run {
+ my ($class, $cfg) = @_;
+ # implement in subclass
+ die "not implemented";
+}
+
+1;
diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
new file mode 100644
index 00000000..043b7ace
--- /dev/null
+++ b/PVE/Jobs/VZDump.pm
@@ -0,0 +1,54 @@
+package PVE::Jobs::VZDump;
+
+use strict;
+use warnings;
+
+use PVE::INotify;
+use PVE::VZDump::Common;
+use PVE::API2::VZDump;
+use PVE::Cluster;
+
+use base qw(PVE::Jobs::Plugin);
+
+sub type {
+ return 'vzdump';
+}
+
+my $props = PVE::VZDump::Common::json_config_properties();
+
+sub properties {
+ return $props;
+}
+
+sub options {
+ my $options = {
+ enabled => { optional => 1 },
+ schedule => {},
+ };
+ foreach my $opt (keys %$props) {
+ if ($props->{$opt}->{optional}) {
+ $options->{$opt} = { optional => 1 };
+ } else {
+ $options->{$opt} = {};
+ }
+ }
+
+ return $options;
+}
+
+sub run {
+ my ($class, $conf) = @_;
+
+ # remove all non vzdump related options
+ foreach my $opt (keys %$conf) {
+ delete $conf->{$opt} if !defined($props->{$opt});
+ }
+
+ $conf->{quiet} = 1; # do not write to stdout/stderr
+
+ PVE::Cluster::cfs_update(); # refresh vmlist
+
+ return PVE::API2::VZDump->vzdump($conf);
+}
+
+1;
diff --git a/PVE/Makefile b/PVE/Makefile
index 0071fab1..48b85d33 100644
--- a/PVE/Makefile
+++ b/PVE/Makefile
@@ -1,6 +1,6 @@
include ../defines.mk
-SUBDIRS=API2 Status CLI Service Ceph
+SUBDIRS=API2 Status CLI Service Ceph Jobs
PERLSOURCE = \
API2.pm \
@@ -11,6 +11,7 @@ PERLSOURCE = \
CertHelpers.pm \
ExtMetric.pm \
HTTPServer.pm \
+ Jobs.pm \
NodeConfig.pm \
Report.pm \
VZDump.pm
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs
2021-10-07 8:27 ` [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs Dominik Csapak
@ 2021-11-02 13:52 ` Fabian Ebner
2021-11-02 14:33 ` Dominik Csapak
0 siblings, 1 reply; 17+ messages in thread
From: Fabian Ebner @ 2021-11-02 13:52 UTC (permalink / raw)
To: pve-devel, Dominik Csapak
General style nit: for new code, for is preferred over foreach
Am 07.10.21 um 10:27 schrieb Dominik Csapak:
> this adds a SectionConfig handling for jobs (only 'vzdump' for now) that
> represents a job that will be handled by pvescheduler and a basic
> 'job-state' handling for reading/writing state json files
>
> this has some intersections with pvesrs state handling, but does not
> use a single state file for all jobs, but seperate ones, like we
> do it in the backup-server.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> PVE/Jobs.pm | 210 +++++++++++++++++++++++++++++++++++++++++++++
> PVE/Jobs/Makefile | 16 ++++
> PVE/Jobs/Plugin.pm | 61 +++++++++++++
> PVE/Jobs/VZDump.pm | 54 ++++++++++++
> PVE/Makefile | 3 +-
> 5 files changed, 343 insertions(+), 1 deletion(-)
> create mode 100644 PVE/Jobs.pm
> create mode 100644 PVE/Jobs/Makefile
> create mode 100644 PVE/Jobs/Plugin.pm
> create mode 100644 PVE/Jobs/VZDump.pm
>
> diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
> new file mode 100644
> index 00000000..f17676bb
> --- /dev/null
> +++ b/PVE/Jobs.pm
> @@ -0,0 +1,210 @@
> +package PVE::Jobs;
> +
> +use strict;
> +use warnings;
> +use JSON;
> +
> +use PVE::Cluster qw(cfs_read_file);
> +use PVE::Jobs::Plugin;
> +use PVE::Jobs::VZDump;
> +use PVE::Tools;
> +
> +PVE::Jobs::VZDump->register();
> +PVE::Jobs::Plugin->init();
> +
> +my $state_dir = "/var/lib/pve-manager/jobs";
> +my $lock_dir = "/var/lock/pve-manager";
> +
> +my $get_state_file = sub {
> + my ($jobid, $type) = @_;
> + return "$state_dir/$type-$jobid.json";
> +};
> +
> +my $default_state = {
> + state => 'created',
> + time => 0,
> +};
> +
> +# lockless, since we use file_set_contents, which is atomic
s/file_set_contents/file_get_contents/
> +sub read_job_state {
> + my ($jobid, $type) = @_;
> + my $path = $get_state_file->($jobid, $type);
> + return $default_state if ! -e $path;
> +
> + my $raw = PVE::Tools::file_get_contents($path);
> +
> + return $default_state if $raw eq '';
> +
> + # untaint $raw
> + if ($raw =~ m/^(\{.*\})$/) {
> + return decode_json($1);
> + }
> +
> + die "invalid json data in '$path'\n";
> +}
> +
> +sub lock_job_state {
> + my ($jobid, $type, $sub) = @_;
> +
> + my $filename = "$lock_dir/$type-$jobid.lck";
Should new locks use .lock?
> +
> + my $res = PVE::Tools::lock_file($filename, 10, $sub);
> + die $@ if $@;
> +
> + return $res;
> +}
> +
> +# returns the state and checks if the job was 'started' and is now finished
> +sub get_job_state {
It feels a bit weird that this function does two things. Maybe it's
worth separating the "check if finished" part?
> + my ($jobid, $type) = @_;
> +
> + # first check unlocked to save time,
> + my $state = read_job_state($jobid, $type);
> + if ($state->{state} ne 'started') {
> + return $state; # nothing to check
> + }
> +
> + return lock_job_state($jobid, $type, sub {
> + my $state = read_job_state($jobid, $type);
> +
> + if ($state->{state} ne 'started') {
> + return $state; # nothing to check
> + }
> +
> + my ($task, $filename) = PVE::Tools::upid_decode($state->{upid}, 1);
> + die "unable to parse worker upid\n" if !$task;
> + die "no such task\n" if ! -f $filename;
> +
> + my $pstart = PVE::ProcFSTools::read_proc_starttime($task->{pid});
> + if ($pstart && $pstart == $task->{pstart}) {
> + return $state; # still running
> + }
> +
> + my $new_state = {
> + state => 'stopped',
> + msg => PVE::Tools::upid_read_status($state->{upid}),
> + upid => $state->{upid}
> + };
> +
> + my $path = $get_state_file->($jobid, $type);
> + PVE::Tools::file_set_contents($path, encode_json($new_state));
> + return $new_state;
> + });
> +}
> +
> +# must be called when the job is first created
> +sub create_job {
> + my ($jobid, $type) = @_;
> +
> + lock_job_state($jobid, $type, sub {
> + my $state = read_job_state($jobid, $type);
> +
> + if ($state->{state} ne 'created') {
> + die "job state already exists\n";
> + }
> +
> + $state->{time} = time();
> +
> + my $path = $get_state_file->($jobid, $type);
> + PVE::Tools::file_set_contents($path, encode_json($state));
> + });
> +}
> +
> +# to be called when the job is removed
> +sub remove_job {
> + my ($jobid, $type) = @_;
> + my $path = $get_state_file->($jobid, $type);
> + unlink $path;
> +}
From what I can tell, this can be called while the job might be active
and then suddenly the state file is gone. Is that a problem?
> +
> +# will be called when the job was started according to schedule
> +sub started_job {
> + my ($jobid, $type, $upid) = @_;
> + lock_job_state($jobid, $type, sub {
> + my $state = {
> + state => 'started',
> + upid => $upid,
> + };
> +
> + my $path = $get_state_file->($jobid, $type);
> + PVE::Tools::file_set_contents($path, encode_json($state));
> + });
> +}
> +
> +# will be called when the job schedule is updated
> +sub updated_job_schedule {
> + my ($jobid, $type) = @_;
> + lock_job_state($jobid, $type, sub {
> + my $old_state = read_job_state($jobid, $type);
> +
> + if ($old_state->{state} eq 'started') {
> + return; # do not update timestamp on running jobs
> + }
> +
> + $old_state->{updated} = time();
> +
> + my $path = $get_state_file->($jobid, $type);
> + PVE::Tools::file_set_contents($path, encode_json($old_state));
> + });
> +}
> +
> +sub get_last_runtime {
> + my ($jobid, $type) = @_;
> +
> + my $state = read_job_state($jobid, $type);
> +
> + if (defined($state->{updated})) {
> + return $state->{updated};
> + }
I'm confused. Why does 'updated' overwrite the last runtime? Should it
return 0 like for a new job? Is there a problem returning the normal
last runtime for an updated job?
> +
> + if (my $upid = $state->{upid}) {
> + my ($task) = PVE::Tools::upid_decode($upid, 1);
> + die "unable to parse worker upid\n" if !$task;
> + return $task->{starttime};
> + }
> +
> + return $state->{time} // 0;
> +}
> +
> +sub run_jobs {
> + my $jobs_cfg = cfs_read_file('jobs.cfg');
> + my $nodename = PVE::INotify::nodename();
> +
> + foreach my $id (sort keys %{$jobs_cfg->{ids}}) {
> + my $cfg = $jobs_cfg->{ids}->{$id};
> + my $type = $cfg->{type};
> + my $schedule = delete $cfg->{schedule};
Why delete?
> +
> + # only schedule local jobs
> + next if defined($cfg->{node}) && $cfg->{node} eq $nodename;
Shouldn't this be 'ne'?
> +
> + # only schedule enabled jobs
> + next if defined($cfg->{enabled}) && !$cfg->{enabled};
> +
> + my $last_run = get_last_runtime($id, $type);
> + my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);
> + my $next_sync = PVE::CalendarEvent::compute_next_event($calspec, $last_run) // 0;
> +
> + if (time() >= $next_sync) {
> + # only warn on errors, so that all jobs can run
> + my $state = get_job_state($id, $type); # to update the state
> +
> + next if $state->{state} eq 'started'; # still running
> +
> + my $plugin = PVE::Jobs::Plugin->lookup($type);
> +
> + my $upid = eval { $plugin->run($cfg) };
> + warn $@ if $@;
> + if ($upid) {
> + started_job($id, $type, $upid);
> + }
> + }
> + }
> +}
> +
> +sub setup_dirs {
> + mkdir $state_dir;
> + mkdir $lock_dir;
> +}
> +
> +1;
> diff --git a/PVE/Jobs/Makefile b/PVE/Jobs/Makefile
> new file mode 100644
> index 00000000..6023c3ba
> --- /dev/null
> +++ b/PVE/Jobs/Makefile
> @@ -0,0 +1,16 @@
> +include ../../defines.mk
> +
> +PERLSOURCE = \
> + Plugin.pm\
> + VZDump.pm
> +
> +all:
> +
> +.PHONY: clean
> +clean:
> + rm -rf *~
> +
> +.PHONY: install
> +install: ${PERLSOURCE}
> + install -d ${PERLLIBDIR}/PVE/Jobs
> + install -m 0644 ${PERLSOURCE} ${PERLLIBDIR}/PVE/Jobs
> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
> new file mode 100644
> index 00000000..69c31cf2
> --- /dev/null
> +++ b/PVE/Jobs/Plugin.pm
> @@ -0,0 +1,61 @@
> +package PVE::Jobs::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Cluster qw(cfs_register_file);
> +
> +use base qw(PVE::SectionConfig);
> +
> +cfs_register_file('jobs.cfg',
> + sub { __PACKAGE__->parse_config(@_); },
> + sub { __PACKAGE__->write_config(@_); });
> +
> +my $defaultData = {
> + propertyList => {
> + type => { description => "Section type." },
> + id => {
> + description => "The ID of the VZDump job.",
> + type => 'string',
> + format => 'pve-configid',
> + },
> + enabled => {
> + description => "Determines if the job is enabled.",
> + type => 'boolean',
> + default => 1,
> + optional => 1,
> + },
> + schedule => {
> + description => "Backup schedule. The format is a subset of `systemd` calendar events.",
> + type => 'string', format => 'pve-calendar-event',
> + maxLength => 128,
> + },
> + },
> +};
> +
> +sub private {
> + return $defaultData;
> +}
> +
> +sub parse_config {
> + my ($class, $filename, $raw) = @_;
> +
> + my $cfg = $class->SUPER::parse_config($filename, $raw);
> +
> + foreach my $id (sort keys %{$cfg->{ids}}) {
> + my $data = $cfg->{ids}->{$id};
> +
> + $data->{id} = $id;
> + $data->{enabled} //= 1;
> + }
> +
> + return $cfg;
> +}
> +
> +sub run {
> + my ($class, $cfg) = @_;
> + # implement in subclass
> + die "not implemented";
> +}
> +
> +1;
> diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
> new file mode 100644
> index 00000000..043b7ace
> --- /dev/null
> +++ b/PVE/Jobs/VZDump.pm
> @@ -0,0 +1,54 @@
> +package PVE::Jobs::VZDump;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::INotify;
> +use PVE::VZDump::Common;
> +use PVE::API2::VZDump;
> +use PVE::Cluster;
> +
> +use base qw(PVE::Jobs::Plugin);
> +
> +sub type {
> + return 'vzdump';
> +}
> +
> +my $props = PVE::VZDump::Common::json_config_properties();
> +
> +sub properties {
> + return $props;
> +}
> +
> +sub options {
> + my $options = {
> + enabled => { optional => 1 },
> + schedule => {},
> + };
> + foreach my $opt (keys %$props) {
> + if ($props->{$opt}->{optional}) {
> + $options->{$opt} = { optional => 1 };
> + } else {
> + $options->{$opt} = {};
> + }
> + }
> +
> + return $options;
> +}
> +
> +sub run {
> + my ($class, $conf) = @_;
> +
> + # remove all non vzdump related options
> + foreach my $opt (keys %$conf) {
> + delete $conf->{$opt} if !defined($props->{$opt});
> + }
> +
> + $conf->{quiet} = 1; # do not write to stdout/stderr
> +
> + PVE::Cluster::cfs_update(); # refresh vmlist
> +
> + return PVE::API2::VZDump->vzdump($conf);
> +}
> +
> +1;
> diff --git a/PVE/Makefile b/PVE/Makefile
> index 0071fab1..48b85d33 100644
> --- a/PVE/Makefile
> +++ b/PVE/Makefile
> @@ -1,6 +1,6 @@
> include ../defines.mk
>
> -SUBDIRS=API2 Status CLI Service Ceph
> +SUBDIRS=API2 Status CLI Service Ceph Jobs
>
> PERLSOURCE = \
> API2.pm \
> @@ -11,6 +11,7 @@ PERLSOURCE = \
> CertHelpers.pm \
> ExtMetric.pm \
> HTTPServer.pm \
> + Jobs.pm \
> NodeConfig.pm \
> Report.pm \
> VZDump.pm
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs
2021-11-02 13:52 ` Fabian Ebner
@ 2021-11-02 14:33 ` Dominik Csapak
2021-11-03 7:37 ` Fabian Ebner
0 siblings, 1 reply; 17+ messages in thread
From: Dominik Csapak @ 2021-11-02 14:33 UTC (permalink / raw)
To: Fabian Ebner, pve-devel
On 11/2/21 14:52, Fabian Ebner wrote:
[snip]
>> +
>> +sub lock_job_state {
>> + my ($jobid, $type, $sub) = @_;
>> +
>> + my $filename = "$lock_dir/$type-$jobid.lck";
>
> Should new locks use .lock?
yes, thanks, most of such things are copied...
>
>> +
>> + my $res = PVE::Tools::lock_file($filename, 10, $sub);
>> + die $@ if $@;
>> +
>> + return $res;
>> +}
>> +
>> +# returns the state and checks if the job was 'started' and is now
>> finished
>> +sub get_job_state {
>
> It feels a bit weird that this function does two things. Maybe it's
> worth separating the "check if finished" part?
yeah probably, but i'd still want to do them under a single lock
so get_and_check_job_state would be better?
>
>> + my ($jobid, $type) = @_;
>> +
>> + # first check unlocked to save time,
>> + my $state = read_job_state($jobid, $type);
>> + if ($state->{state} ne 'started') {
>> + return $state; # nothing to check
>> + }
>> +
>> + return lock_job_state($jobid, $type, sub {
>> + my $state = read_job_state($jobid, $type);
>> +
>> + if ($state->{state} ne 'started') {
>> + return $state; # nothing to check
>> + }
>> +
>> + my ($task, $filename) = PVE::Tools::upid_decode($state->{upid}, 1);
>> + die "unable to parse worker upid\n" if !$task;
>> + die "no such task\n" if ! -f $filename;
>> +
>> + my $pstart = PVE::ProcFSTools::read_proc_starttime($task->{pid});
>> + if ($pstart && $pstart == $task->{pstart}) {
>> + return $state; # still running
>> + }
>> +
>> + my $new_state = {
>> + state => 'stopped',
>> + msg => PVE::Tools::upid_read_status($state->{upid}),
>> + upid => $state->{upid}
>> + };
>> +
>> + my $path = $get_state_file->($jobid, $type);
>> + PVE::Tools::file_set_contents($path, encode_json($new_state));
>> + return $new_state;
>> + });
>> +}
>> +
>> +# must be called when the job is first created
>> +sub create_job {
>> + my ($jobid, $type) = @_;
>> +
>> + lock_job_state($jobid, $type, sub {
>> + my $state = read_job_state($jobid, $type);
>> +
>> + if ($state->{state} ne 'created') {
>> + die "job state already exists\n";
>> + }
>> +
>> + $state->{time} = time();
>> +
>> + my $path = $get_state_file->($jobid, $type);
>> + PVE::Tools::file_set_contents($path, encode_json($state));
>> + });
>> +}
>> +
>> +# to be called when the job is removed
>> +sub remove_job {
>> + my ($jobid, $type) = @_;
>> + my $path = $get_state_file->($jobid, $type);
>> + unlink $path;
>> +}
>
> From what I can tell, this can be called while the job might be active
> and then suddenly the state file is gone. Is that a problem?
you're right, the finished job would be writing to the file again
the solution would imho be to check if the statefile still exists
on finishing. if not, do not write (because the jobs does not exist anymore)
or we could forbid removing a job while its still running..
>
>> +
>> +# will be called when the job was started according to schedule
>> +sub started_job {
>> + my ($jobid, $type, $upid) = @_;
>> + lock_job_state($jobid, $type, sub {
>> + my $state = {
>> + state => 'started',
>> + upid => $upid,
>> + };
>> +
>> + my $path = $get_state_file->($jobid, $type);
>> + PVE::Tools::file_set_contents($path, encode_json($state));
>> + });
>> +}
>> +
>> +# will be called when the job schedule is updated
>> +sub updated_job_schedule {
>> + my ($jobid, $type) = @_;
>> + lock_job_state($jobid, $type, sub {
>> + my $old_state = read_job_state($jobid, $type);
>> +
>> + if ($old_state->{state} eq 'started') {
>> + return; # do not update timestamp on running jobs
>> + }
>> +
>> + $old_state->{updated} = time();
>> +
>> + my $path = $get_state_file->($jobid, $type);
>> + PVE::Tools::file_set_contents($path, encode_json($old_state));
>> + });
>> +}
>> +
>> +sub get_last_runtime {
>> + my ($jobid, $type) = @_;
>> +
>> + my $state = read_job_state($jobid, $type);
>> +
>> + if (defined($state->{updated})) {
>> + return $state->{updated};
>> + }
>
> I'm confused. Why does 'updated' overwrite the last runtime? Should it
> return 0 like for a new job? Is there a problem returning the normal
> last runtime for an updated job?
for the next run we have to consider the updated time
(new jobs get the current time, not 0)
because if i change from e.g. 10:00 to 8:00 and it is
currently 9:00, it would be unexpected that the
job starts instantly, instead of the next time its 10:00
(we do the same in pbs for all kinds of jobs)
>
>> +
>> + if (my $upid = $state->{upid}) {
>> + my ($task) = PVE::Tools::upid_decode($upid, 1);
>> + die "unable to parse worker upid\n" if !$task;
>> + return $task->{starttime};
>> + }
>> +
>> + return $state->{time} // 0;
>> +}
>> +
>> +sub run_jobs {
>> + my $jobs_cfg = cfs_read_file('jobs.cfg');
>> + my $nodename = PVE::INotify::nodename();
>> +
>> + foreach my $id (sort keys %{$jobs_cfg->{ids}}) {
>> + my $cfg = $jobs_cfg->{ids}->{$id};
>> + my $type = $cfg->{type};
>> + my $schedule = delete $cfg->{schedule};
>
> Why delete?
good question, it's maybe leftover of some intermittent code
change. i'll check and change/comment on it in a v2
it's probably because the vzdump code does not understand the
'schedule' parameter and it's more of a 'meta' information
>
>> +
>> + # only schedule local jobs
>> + next if defined($cfg->{node}) && $cfg->{node} eq $nodename;
>
> Shouldn't this be 'ne'?
you are absolutely right ;)
>
>> +
>> + # only schedule enabled jobs
>> + next if defined($cfg->{enabled}) && !$cfg->{enabled};
>> +
>> + my $last_run = get_last_runtime($id, $type);
>> + my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);
>> + my $next_sync = PVE::CalendarEvent::compute_next_event($calspec,
>> $last_run) // 0;
>> +
>> + if (time() >= $next_sync) {
>> + # only warn on errors, so that all jobs can run
>> + my $state = get_job_state($id, $type); # to update the state
>> +
>> + next if $state->{state} eq 'started'; # still running
>> +
>> + my $plugin = PVE::Jobs::Plugin->lookup($type);
>> +
>> + my $upid = eval { $plugin->run($cfg) };
>> + warn $@ if $@;
>> + if ($upid) {
>> + started_job($id, $type, $upid);
>> + }
>> + }
>> + }
>> +}
>> +
>> +sub setup_dirs {
>> + mkdir $state_dir;
>> + mkdir $lock_dir;
>> +}
>> +
>> +1;
>> diff --git a/PVE/Jobs/Makefile b/PVE/Jobs/Makefile
>> new file mode 100644
>> index 00000000..6023c3ba
>> --- /dev/null
>> +++ b/PVE/Jobs/Makefile
>> @@ -0,0 +1,16 @@
>> +include ../../defines.mk
>> +
>> +PERLSOURCE = \
>> + Plugin.pm\
>> + VZDump.pm
>> +
>> +all:
>> +
>> +.PHONY: clean
>> +clean:
>> + rm -rf *~
>> +
>> +.PHONY: install
>> +install: ${PERLSOURCE}
>> + install -d ${PERLLIBDIR}/PVE/Jobs
>> + install -m 0644 ${PERLSOURCE} ${PERLLIBDIR}/PVE/Jobs
>> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
>> new file mode 100644
>> index 00000000..69c31cf2
>> --- /dev/null
>> +++ b/PVE/Jobs/Plugin.pm
>> @@ -0,0 +1,61 @@
>> +package PVE::Jobs::Plugin;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::Cluster qw(cfs_register_file);
>> +
>> +use base qw(PVE::SectionConfig);
>> +
>> +cfs_register_file('jobs.cfg',
>> + sub { __PACKAGE__->parse_config(@_); },
>> + sub { __PACKAGE__->write_config(@_); });
>> +
>> +my $defaultData = {
>> + propertyList => {
>> + type => { description => "Section type." },
>> + id => {
>> + description => "The ID of the VZDump job.",
>> + type => 'string',
>> + format => 'pve-configid',
>> + },
>> + enabled => {
>> + description => "Determines if the job is enabled.",
>> + type => 'boolean',
>> + default => 1,
>> + optional => 1,
>> + },
>> + schedule => {
>> + description => "Backup schedule. The format is a subset of
>> `systemd` calendar events.",
>> + type => 'string', format => 'pve-calendar-event',
>> + maxLength => 128,
>> + },
>> + },
>> +};
>> +
>> +sub private {
>> + return $defaultData;
>> +}
>> +
>> +sub parse_config {
>> + my ($class, $filename, $raw) = @_;
>> +
>> + my $cfg = $class->SUPER::parse_config($filename, $raw);
>> +
>> + foreach my $id (sort keys %{$cfg->{ids}}) {
>> + my $data = $cfg->{ids}->{$id};
>> +
>> + $data->{id} = $id;
>> + $data->{enabled} //= 1;
>> + }
>> +
>> + return $cfg;
>> +}
>> +
>> +sub run {
>> + my ($class, $cfg) = @_;
>> + # implement in subclass
>> + die "not implemented";
>> +}
>> +
>> +1;
>> diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
>> new file mode 100644
>> index 00000000..043b7ace
>> --- /dev/null
>> +++ b/PVE/Jobs/VZDump.pm
>> @@ -0,0 +1,54 @@
>> +package PVE::Jobs::VZDump;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::INotify;
>> +use PVE::VZDump::Common;
>> +use PVE::API2::VZDump;
>> +use PVE::Cluster;
>> +
>> +use base qw(PVE::Jobs::Plugin);
>> +
>> +sub type {
>> + return 'vzdump';
>> +}
>> +
>> +my $props = PVE::VZDump::Common::json_config_properties();
>> +
>> +sub properties {
>> + return $props;
>> +}
>> +
>> +sub options {
>> + my $options = {
>> + enabled => { optional => 1 },
>> + schedule => {},
>> + };
>> + foreach my $opt (keys %$props) {
>> + if ($props->{$opt}->{optional}) {
>> + $options->{$opt} = { optional => 1 };
>> + } else {
>> + $options->{$opt} = {};
>> + }
>> + }
>> +
>> + return $options;
>> +}
>> +
>> +sub run {
>> + my ($class, $conf) = @_;
>> +
>> + # remove all non vzdump related options
>> + foreach my $opt (keys %$conf) {
>> + delete $conf->{$opt} if !defined($props->{$opt});
>> + }
>> +
>> + $conf->{quiet} = 1; # do not write to stdout/stderr
>> +
>> + PVE::Cluster::cfs_update(); # refresh vmlist
>> +
>> + return PVE::API2::VZDump->vzdump($conf);
>> +}
>> +
>> +1;
>> diff --git a/PVE/Makefile b/PVE/Makefile
>> index 0071fab1..48b85d33 100644
>> --- a/PVE/Makefile
>> +++ b/PVE/Makefile
>> @@ -1,6 +1,6 @@
>> include ../defines.mk
>> -SUBDIRS=API2 Status CLI Service Ceph
>> +SUBDIRS=API2 Status CLI Service Ceph Jobs
>> PERLSOURCE = \
>> API2.pm \
>> @@ -11,6 +11,7 @@ PERLSOURCE = \
>> CertHelpers.pm \
>> ExtMetric.pm \
>> HTTPServer.pm \
>> + Jobs.pm \
>> NodeConfig.pm \
>> Report.pm \
>> VZDump.pm
>>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs
2021-11-02 14:33 ` Dominik Csapak
@ 2021-11-03 7:37 ` Fabian Ebner
0 siblings, 0 replies; 17+ messages in thread
From: Fabian Ebner @ 2021-11-03 7:37 UTC (permalink / raw)
To: Dominik Csapak, pve-devel
Am 02.11.21 um 15:33 schrieb Dominik Csapak:
> On 11/2/21 14:52, Fabian Ebner wrote:
> [snip]
>>> +
>>> +sub lock_job_state {
>>> + my ($jobid, $type, $sub) = @_;
>>> +
>>> + my $filename = "$lock_dir/$type-$jobid.lck";
>>
>> Should new locks use .lock?
>
> yes, thanks, most of such things are copied...
>
>>
>>> +
>>> + my $res = PVE::Tools::lock_file($filename, 10, $sub);
>>> + die $@ if $@;
>>> +
>>> + return $res;
>>> +}
>>> +
>>> +# returns the state and checks if the job was 'started' and is now
>>> finished
>>> +sub get_job_state {
>>
>> It feels a bit weird that this function does two things. Maybe it's
>> worth separating the "check if finished" part?
>
> yeah probably, but i'd still want to do them under a single lock
> so get_and_check_job_state would be better?
>
The lock is only used for the check(+update). For reading, it doesn't
really have an effect. Splitting up would require duplicating the read.
Maybe what's actually bugging me is that the transition started->stopped
is factored out here, as opposed to the transition stopped->started
being inlined in the main loop.
>>
>>> + my ($jobid, $type) = @_;
>>> +
>>> + # first check unlocked to save time,
>>> + my $state = read_job_state($jobid, $type);
>>> + if ($state->{state} ne 'started') {
>>> + return $state; # nothing to check
>>> + }
>>> +
>>> + return lock_job_state($jobid, $type, sub {
>>> + my $state = read_job_state($jobid, $type);
>>> +
>>> + if ($state->{state} ne 'started') {
>>> + return $state; # nothing to check
>>> + }
>>> +
>>> + my ($task, $filename) = PVE::Tools::upid_decode($state->{upid}, 1);
>>> + die "unable to parse worker upid\n" if !$task;
>>> + die "no such task\n" if ! -f $filename;
>>> +
>>> + my $pstart = PVE::ProcFSTools::read_proc_starttime($task->{pid});
>>> + if ($pstart && $pstart == $task->{pstart}) {
>>> + return $state; # still running
>>> + }
>>> +
>>> + my $new_state = {
>>> + state => 'stopped',
>>> + msg => PVE::Tools::upid_read_status($state->{upid}),
>>> + upid => $state->{upid}
>>> + };
>>> +
>>> + my $path = $get_state_file->($jobid, $type);
>>> + PVE::Tools::file_set_contents($path, encode_json($new_state));
>>> + return $new_state;
>>> + });
>>> +}
>>> +
>>> +# must be called when the job is first created
>>> +sub create_job {
>>> + my ($jobid, $type) = @_;
>>> +
>>> + lock_job_state($jobid, $type, sub {
>>> + my $state = read_job_state($jobid, $type);
>>> +
>>> + if ($state->{state} ne 'created') {
>>> + die "job state already exists\n";
>>> + }
>>> +
>>> + $state->{time} = time();
>>> +
>>> + my $path = $get_state_file->($jobid, $type);
>>> + PVE::Tools::file_set_contents($path, encode_json($state));
>>> + });
>>> +}
>>> +
>>> +# to be called when the job is removed
>>> +sub remove_job {
>>> + my ($jobid, $type) = @_;
>>> + my $path = $get_state_file->($jobid, $type);
>>> + unlink $path;
>>> +}
>>
>> From what I can tell, this can be called while the job might be
>> active and then suddenly the state file is gone. Is that a problem?
>
> you're right, the finished job would be writing to the file again
>
> the solution would imho be to check if the statefile still exists
> on finishing. if not, do not write (because the jobs does not exist
> anymore)
Even if we do that, there's another situation that can be problematic:
1. run_jobs decides that a job should be started
2. job is deleted
3. run_jobs starts the job and re-creates the state file
Could be fixed by checking whether the state file still exists in
started_job too or extending the lock for the stopped->started
transition. Maybe the latter part still makes sense anyways (to prevent
races when starting a job, although it really shouldn't matter if
there's only one instance of run_jobs running).
>
> or we could forbid removing a job while its still running..
>
This would also require extending the lock for the stopped->started
transition.
>>
>>> +
>>> +# will be called when the job was started according to schedule
>>> +sub started_job {
>>> + my ($jobid, $type, $upid) = @_;
>>> + lock_job_state($jobid, $type, sub {
>>> + my $state = {
>>> + state => 'started',
>>> + upid => $upid,
>>> + };
>>> +
>>> + my $path = $get_state_file->($jobid, $type);
>>> + PVE::Tools::file_set_contents($path, encode_json($state));
>>> + });
>>> +}
>>> +
>>> +# will be called when the job schedule is updated
>>> +sub updated_job_schedule {
>>> + my ($jobid, $type) = @_;
>>> + lock_job_state($jobid, $type, sub {
>>> + my $old_state = read_job_state($jobid, $type);
>>> +
>>> + if ($old_state->{state} eq 'started') {
>>> + return; # do not update timestamp on running jobs
>>> + }
>>> +
>>> + $old_state->{updated} = time();
>>> +
>>> + my $path = $get_state_file->($jobid, $type);
>>> + PVE::Tools::file_set_contents($path, encode_json($old_state));
>>> + });
>>> +}
>>> +
>>> +sub get_last_runtime {
>>> + my ($jobid, $type) = @_;
>>> +
>>> + my $state = read_job_state($jobid, $type);
>>> +
>>> + if (defined($state->{updated})) {
>>> + return $state->{updated};
>>> + }
>>
>> I'm confused. Why does 'updated' overwrite the last runtime? Should it
>> return 0 like for a new job? Is there a problem returning the normal
>> last runtime for an updated job?
>
> for the next run we have to consider the updated time
> (new jobs get the current time, not 0)
>
> because if i change from e.g. 10:00 to 8:00 and it is
> currently 9:00, it would be unexpected that the
> job starts instantly, instead of the next time its 10:00
> (we do the same in pbs for all kinds of jobs)
>
Ok, makes sense, didn't think about that. There might be a problem
though, because when a currently active job is updated, once it
completes, the 'updated' property is dropped from the state.
>>
>>> +
>>> + if (my $upid = $state->{upid}) {
>>> + my ($task) = PVE::Tools::upid_decode($upid, 1);
>>> + die "unable to parse worker upid\n" if !$task;
>>> + return $task->{starttime};
>>> + }
>>> +
>>> + return $state->{time} // 0;
>>> +}
>>> +
>>> +sub run_jobs {
>>> + my $jobs_cfg = cfs_read_file('jobs.cfg');
>>> + my $nodename = PVE::INotify::nodename();
>>> +
>>> + foreach my $id (sort keys %{$jobs_cfg->{ids}}) {
>>> + my $cfg = $jobs_cfg->{ids}->{$id};
>>> + my $type = $cfg->{type};
>>> + my $schedule = delete $cfg->{schedule};
>>
>> Why delete?
>
> good question, it's maybe leftover of some intermittent code
> change. i'll check and change/comment on it in a v2
>
> it's probably because the vzdump code does not understand the
> 'schedule' parameter and it's more of a 'meta' information
>
>>
>>> +
>>> + # only schedule local jobs
>>> + next if defined($cfg->{node}) && $cfg->{node} eq $nodename;
>>
>> Shouldn't this be 'ne'?
>
> you are absolutely right ;)
>
>>
>>> +
>>> + # only schedule enabled jobs
>>> + next if defined($cfg->{enabled}) && !$cfg->{enabled};
>>> +
>>> + my $last_run = get_last_runtime($id, $type);
>>> + my $calspec = PVE::CalendarEvent::parse_calendar_event($schedule);
>>> + my $next_sync = PVE::CalendarEvent::compute_next_event($calspec,
>>> $last_run) // 0;
>>> +
>>> + if (time() >= $next_sync) {
>>> + # only warn on errors, so that all jobs can run
>>> + my $state = get_job_state($id, $type); # to update the state
>>> +
>>> + next if $state->{state} eq 'started'; # still running
>>> +
>>> + my $plugin = PVE::Jobs::Plugin->lookup($type);
>>> +
>>> + my $upid = eval { $plugin->run($cfg) };
>>> + warn $@ if $@;
>>> + if ($upid) {
>>> + started_job($id, $type, $upid);
>>> + }
>>> + }
>>> + }
>>> +}
>>> +
>>> +sub setup_dirs {
>>> + mkdir $state_dir;
>>> + mkdir $lock_dir;
>>> +}
>>> +
>>> +1;
>>> diff --git a/PVE/Jobs/Makefile b/PVE/Jobs/Makefile
>>> new file mode 100644
>>> index 00000000..6023c3ba
>>> --- /dev/null
>>> +++ b/PVE/Jobs/Makefile
>>> @@ -0,0 +1,16 @@
>>> +include ../../defines.mk
>>> +
>>> +PERLSOURCE = \
>>> + Plugin.pm\
>>> + VZDump.pm
>>> +
>>> +all:
>>> +
>>> +.PHONY: clean
>>> +clean:
>>> + rm -rf *~
>>> +
>>> +.PHONY: install
>>> +install: ${PERLSOURCE}
>>> + install -d ${PERLLIBDIR}/PVE/Jobs
>>> + install -m 0644 ${PERLSOURCE} ${PERLLIBDIR}/PVE/Jobs
>>> diff --git a/PVE/Jobs/Plugin.pm b/PVE/Jobs/Plugin.pm
>>> new file mode 100644
>>> index 00000000..69c31cf2
>>> --- /dev/null
>>> +++ b/PVE/Jobs/Plugin.pm
>>> @@ -0,0 +1,61 @@
>>> +package PVE::Jobs::Plugin;
>>> +
>>> +use strict;
>>> +use warnings;
>>> +
>>> +use PVE::Cluster qw(cfs_register_file);
>>> +
>>> +use base qw(PVE::SectionConfig);
>>> +
>>> +cfs_register_file('jobs.cfg',
>>> + sub { __PACKAGE__->parse_config(@_); },
>>> + sub { __PACKAGE__->write_config(@_); });
>>> +
>>> +my $defaultData = {
>>> + propertyList => {
>>> + type => { description => "Section type." },
>>> + id => {
>>> + description => "The ID of the VZDump job.",
>>> + type => 'string',
>>> + format => 'pve-configid',
>>> + },
>>> + enabled => {
>>> + description => "Determines if the job is enabled.",
>>> + type => 'boolean',
>>> + default => 1,
>>> + optional => 1,
>>> + },
>>> + schedule => {
>>> + description => "Backup schedule. The format is a subset of
>>> `systemd` calendar events.",
>>> + type => 'string', format => 'pve-calendar-event',
>>> + maxLength => 128,
>>> + },
>>> + },
>>> +};
>>> +
>>> +sub private {
>>> + return $defaultData;
>>> +}
>>> +
>>> +sub parse_config {
>>> + my ($class, $filename, $raw) = @_;
>>> +
>>> + my $cfg = $class->SUPER::parse_config($filename, $raw);
>>> +
>>> + foreach my $id (sort keys %{$cfg->{ids}}) {
>>> + my $data = $cfg->{ids}->{$id};
>>> +
>>> + $data->{id} = $id;
>>> + $data->{enabled} //= 1;
>>> + }
>>> +
>>> + return $cfg;
>>> +}
>>> +
>>> +sub run {
>>> + my ($class, $cfg) = @_;
>>> + # implement in subclass
>>> + die "not implemented";
>>> +}
>>> +
>>> +1;
>>> diff --git a/PVE/Jobs/VZDump.pm b/PVE/Jobs/VZDump.pm
>>> new file mode 100644
>>> index 00000000..043b7ace
>>> --- /dev/null
>>> +++ b/PVE/Jobs/VZDump.pm
>>> @@ -0,0 +1,54 @@
>>> +package PVE::Jobs::VZDump;
>>> +
>>> +use strict;
>>> +use warnings;
>>> +
>>> +use PVE::INotify;
>>> +use PVE::VZDump::Common;
>>> +use PVE::API2::VZDump;
>>> +use PVE::Cluster;
>>> +
>>> +use base qw(PVE::Jobs::Plugin);
>>> +
>>> +sub type {
>>> + return 'vzdump';
>>> +}
>>> +
>>> +my $props = PVE::VZDump::Common::json_config_properties();
>>> +
>>> +sub properties {
>>> + return $props;
>>> +}
>>> +
>>> +sub options {
>>> + my $options = {
>>> + enabled => { optional => 1 },
>>> + schedule => {},
>>> + };
>>> + foreach my $opt (keys %$props) {
>>> + if ($props->{$opt}->{optional}) {
>>> + $options->{$opt} = { optional => 1 };
>>> + } else {
>>> + $options->{$opt} = {};
>>> + }
>>> + }
>>> +
>>> + return $options;
>>> +}
>>> +
>>> +sub run {
>>> + my ($class, $conf) = @_;
>>> +
>>> + # remove all non vzdump related options
>>> + foreach my $opt (keys %$conf) {
>>> + delete $conf->{$opt} if !defined($props->{$opt});
>>> + }
>>> +
>>> + $conf->{quiet} = 1; # do not write to stdout/stderr
>>> +
>>> + PVE::Cluster::cfs_update(); # refresh vmlist
>>> +
>>> + return PVE::API2::VZDump->vzdump($conf);
>>> +}
>>> +
>>> +1;
>>> diff --git a/PVE/Makefile b/PVE/Makefile
>>> index 0071fab1..48b85d33 100644
>>> --- a/PVE/Makefile
>>> +++ b/PVE/Makefile
>>> @@ -1,6 +1,6 @@
>>> include ../defines.mk
>>> -SUBDIRS=API2 Status CLI Service Ceph
>>> +SUBDIRS=API2 Status CLI Service Ceph Jobs
>>> PERLSOURCE = \
>>> API2.pm \
>>> @@ -11,6 +11,7 @@ PERLSOURCE = \
>>> CertHelpers.pm \
>>> ExtMetric.pm \
>>> HTTPServer.pm \
>>> + Jobs.pm \
>>> NodeConfig.pm \
>>> Report.pm \
>>> VZDump.pm
>>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pve-devel] [PATCH manager 5/7] pvescheduler: run jobs from jobs.cfg
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
` (4 preceding siblings ...)
2021-10-07 8:27 ` [pve-devel] [PATCH manager 4/7] add PVE/Jobs to handle VZDump jobs Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs Dominik Csapak
2021-10-07 8:27 ` [pve-devel] [PATCH manager 7/7] ui: dc/backup: show id+schedule instead of dow+starttime Dominik Csapak
7 siblings, 0 replies; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
PVE/Jobs is responsible to decide if the job must run (e.g. with a
schedule)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/Service/pvescheduler.pm | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/PVE/Service/pvescheduler.pm b/PVE/Service/pvescheduler.pm
index ce55c45a..81f6b089 100644
--- a/PVE/Service/pvescheduler.pm
+++ b/PVE/Service/pvescheduler.pm
@@ -6,6 +6,7 @@ use warnings;
use POSIX qw(WNOHANG);
use PVE::SafeSyslog;
use PVE::API2::Replication;
+use PVE::Jobs;
use PVE::Daemon;
use base qw(PVE::Daemon);
@@ -39,19 +40,33 @@ sub run {
my $logfunc = sub { syslog('info', $_[0]) };
- my $run_jobs = sub {
+ my $fork = sub {
+ my ($sub) = @_;
my $child = fork();
if (!defined($child)) {
die "fork failed: $!\n";
} elsif ($child == 0) {
$self->after_fork_cleanup();
- PVE::API2::Replication::run_jobs(undef, $logfunc, 0, 1);
+ $sub->();
POSIX::_exit(0);
}
$jobs->{$child} = 1;
};
+ my $run_jobs = sub {
+
+ $fork->(sub {
+ PVE::API2::Replication::run_jobs(undef, $logfunc, 0, 1);
+ });
+
+ $fork->(sub {
+ PVE::Jobs::run_jobs();
+ });
+ };
+
+ PVE::Jobs::setup_dirs();
+
# try to run near minute boundaries, makes more sense to the user as he
# configures jobs with minute precision
my ($current_seconds) = localtime;
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
` (5 preceding siblings ...)
2021-10-07 8:27 ` [pve-devel] [PATCH manager 5/7] pvescheduler: run jobs from jobs.cfg Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-11-03 9:05 ` Fabian Ebner
2021-10-07 8:27 ` [pve-devel] [PATCH manager 7/7] ui: dc/backup: show id+schedule instead of dow+starttime Dominik Csapak
7 siblings, 1 reply; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
in addition to listing the vzdump.cron jobs, also list from the
jobs.cfg file.
updates/creations go into the new jobs.cfg only now
and on update, starttime+dow get converted to a schedule
this transformation is straight forward, since 'dow'
is already in a compatible format (e.g. 'mon,tue') and we simply
append the starttime (if any)
id on creation is optional for now (for api compat), but will
be autogenerated (uuid). on update, we simply take the id from before
(the ids of the other entries in vzdump.cron will change but they would
anyway)
as long as we have the vzdump.cron file, we must lock both
vzdump.cron and jobs.cfg, since we often update both
we also change the backupinfo api call to read the jobs.cfg too
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
PVE/API2/Backup.pm | 243 +++++++++++++++++++++++++++------
PVE/API2/Cluster/BackupInfo.pm | 10 ++
2 files changed, 208 insertions(+), 45 deletions(-)
diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 9dc3b48e..56ed4be8 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -3,6 +3,7 @@ package PVE::API2::Backup;
use strict;
use warnings;
use Digest::SHA;
+use UUID;
use PVE::SafeSyslog;
use PVE::Tools qw(extract_param);
@@ -14,6 +15,7 @@ use PVE::Storage;
use PVE::Exception qw(raise_param_exc);
use PVE::VZDump;
use PVE::VZDump::Common;
+use PVE::Jobs; # for VZDump Jobs
use base qw(PVE::RESTHandler);
@@ -45,6 +47,19 @@ my $assert_param_permission = sub {
}
};
+my $convert_to_schedule = sub {
+ my ($job) = @_;
+
+ my $starttime = $job->{starttime};
+ my $dow = $job->{dow};
+
+ if (!$dow || $dow eq ALL_DAYS) {
+ return "$starttime";
+ }
+
+ return "$dow $starttime";
+};
+
__PACKAGE__->register_method({
name => 'index',
path => '',
@@ -74,8 +89,20 @@ __PACKAGE__->register_method({
my $user = $rpcenv->get_user();
my $data = cfs_read_file('vzdump.cron');
+ my $jobs_data = cfs_read_file('jobs.cfg');
+ my $order = $jobs_data->{order};
+ my $jobs = $jobs_data->{ids};
my $res = $data->{jobs} || [];
+ foreach my $job (@$res) {
+ $job->{schedule} = $convert_to_schedule->($job);
+ }
+
+ foreach my $jobid (sort { $order->{$a} <=> $order->{$b} } keys %$jobs) {
+ my $job = $jobs->{$jobid};
+ next if $job->{type} ne 'vzdump';
+ push @$res, $job;
+ }
return $res;
}});
@@ -93,11 +120,24 @@ __PACKAGE__->register_method({
parameters => {
additionalProperties => 0,
properties => PVE::VZDump::Common::json_config_properties({
+ id => {
+ type => 'string',
+ description => "Job ID (will be autogenerated).",
+ format => 'pve-configid',
+ optional => 1, # FIXME: make required on 8.0
+ },
+ schedule => {
+ description => "Backup schedule. The format is a subset of `systemd` calendar events.",
+ type => 'string', format => 'pve-calendar-event',
+ maxLength => 128,
+ optional => 1,
+ },
starttime => {
type => 'string',
description => "Job Start time.",
pattern => '\d{1,2}:\d{1,2}',
typetext => 'HH:MM',
+ optional => 1,
},
dow => {
type => 'string', format => 'pve-day-of-week-list',
@@ -127,19 +167,40 @@ __PACKAGE__->register_method({
$rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
}
+ if (defined($param->{schedule})) {
+ if (defined($param->{starttime})) {
+ raise_param_exc({ starttime => "'starttime' and 'schedule' cannot both be set" });
+ }
+ } elsif (!defined($param->{starttime})) {
+ raise_param_exc({ schedule => "neither 'starttime' nor 'schedule' were set" });
+ } else {
+ $param->{schedule} = $convert_to_schedule->($param);
+ }
+
+ delete $param->{starttime};
+ delete $param->{dow};
- my $create_job = sub {
- my $data = cfs_read_file('vzdump.cron');
+ $param->{enabled} = 1 if !defined($param->{enabled});
+
+ # autogenerate id for api compatibility FIXME remove with 8.0
+ my $id = extract_param($param, 'id') // uuid();
+
+ cfs_lock_file('jobs.cfg', undef, sub {
+ my $data = cfs_read_file('jobs.cfg');
+
+ die "Job '$id' already exists\n"
+ if $data->{ids}->{$id};
- $param->{dow} = ALL_DAYS if !defined($param->{dow});
- $param->{enabled} = 1 if !defined($param->{enabled});
PVE::VZDump::verify_vzdump_parameters($param, 1);
+ my $plugin = PVE::Jobs::Plugin->lookup('vzdump');
+ my $opts = $plugin->check_config($id, $param, 1, 1);
- push @{$data->{jobs}}, $param;
+ $data->{ids}->{$id} = $opts;
- cfs_write_file('vzdump.cron', $data);
- };
- cfs_lock_file('vzdump.cron', undef, $create_job);
+ PVE::Jobs::create_job($id, 'vzdump');
+
+ cfs_write_file('jobs.cfg', $data);
+ });
die "$@" if ($@);
return undef;
@@ -173,9 +234,16 @@ __PACKAGE__->register_method({
my $jobs = $data->{jobs} || [];
foreach my $job (@$jobs) {
- return $job if $job->{id} eq $param->{id};
+ if ($job->{id} eq $param->{id}) {
+ $job->{schedule} = $convert_to_schedule->($job);
+ return $job;
+ }
}
+ my $jobs_data = cfs_read_file('jobs.cfg');
+ my $job = $jobs_data->{ids}->{$param->{id}};
+ return $job if $job && $job->{type} eq 'vzdump';
+
raise_param_exc({ id => "No such job '$param->{id}'" });
}});
@@ -202,6 +270,8 @@ __PACKAGE__->register_method({
my $rpcenv = PVE::RPCEnvironment::get();
my $user = $rpcenv->get_user();
+ my $id = $param->{id};
+
my $delete_job = sub {
my $data = cfs_read_file('vzdump.cron');
@@ -210,18 +280,32 @@ __PACKAGE__->register_method({
my $found;
foreach my $job (@$jobs) {
- if ($job->{id} eq $param->{id}) {
+ if ($job->{id} eq $id) {
$found = 1;
} else {
push @$newjobs, $job;
}
}
- raise_param_exc({ id => "No such job '$param->{id}'" }) if !$found;
+ if (!$found) {
+ cfs_lock_file('jobs.cfg', undef, sub {
+ my $jobs_data = cfs_read_file('jobs.cfg');
- $data->{jobs} = $newjobs;
+ if (!defined($jobs_data->{ids}->{$id})) {
+ raise_param_exc({ id => "No such job '$id'" });
+ }
+ delete $jobs_data->{ids}->{$id};
+
+ PVE::Jobs::remove_job($id, 'vzudmp');
+
+ cfs_write_file('jobs.cfg', $jobs_data);
+ });
+ die "$@" if $@;
+ } else {
+ $data->{jobs} = $newjobs;
- cfs_write_file('vzdump.cron', $data);
+ cfs_write_file('vzdump.cron', $data);
+ }
};
cfs_lock_file('vzdump.cron', undef, $delete_job);
die "$@" if ($@);
@@ -243,15 +327,23 @@ __PACKAGE__->register_method({
additionalProperties => 0,
properties => PVE::VZDump::Common::json_config_properties({
id => $vzdump_job_id_prop,
+ schedule => {
+ description => "Backup schedule. The format is a subset of `systemd` calendar events.",
+ type => 'string', format => 'pve-calendar-event',
+ maxLength => 128,
+ optional => 1,
+ },
starttime => {
type => 'string',
description => "Job Start time.",
pattern => '\d{1,2}:\d{1,2}',
typetext => 'HH:MM',
+ optional => 1,
},
dow => {
type => 'string', format => 'pve-day-of-week-list',
optional => 1,
+ requires => 'starttime',
description => "Day of week selection.",
},
delete => {
@@ -281,57 +373,111 @@ __PACKAGE__->register_method({
$rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
}
+ if (defined($param->{schedule})) {
+ if (defined($param->{starttime})) {
+ raise_param_exc({ starttime => "'starttime' and 'schedule' cannot both be set" });
+ }
+ } elsif (!defined($param->{starttime})) {
+ raise_param_exc({ schedule => "neither 'starttime' nor 'schedule' were set" });
+ } else {
+ $param->{schedule} = $convert_to_schedule->($param);
+ }
+
+ delete $param->{starttime};
+ delete $param->{dow};
+
+ my $id = extract_param($param, 'id');
+ my $delete = extract_param($param, 'delete');
+ if ($delete) {
+ $delete = [PVE::Tools::split_list($delete)];
+ }
+
my $update_job = sub {
my $data = cfs_read_file('vzdump.cron');
+ my $jobs_data = cfs_read_file('jobs.cfg');
my $jobs = $data->{jobs} || [];
die "no options specified\n" if !scalar(keys %$param);
PVE::VZDump::verify_vzdump_parameters($param);
+ my $plugin = PVE::Jobs::Plugin->lookup('vzdump');
+ my $opts = $plugin->check_config($id, $param, 0, 1);
+
+ # try to find it in old vzdump.cron and convert it to a job
+ my $idx = 0;
+ my $found = 0;
+ foreach my $j (@$jobs) {
+ if ($j->{id} eq $id) {
+ $found = 1;
+ last;
+ }
+ $idx++;
+ }
- my @delete = PVE::Tools::split_list(extract_param($param, 'delete'));
+ my $job;
+ if ($found) {
+ $job = splice @$jobs, $idx, 1;
+ $job->{schedule} = $convert_to_schedule->($job);
+ delete $job->{starttime};
+ delete $job->{dow};
+ delete $job->{id};
+ $job->{type} = 'vzdump';
+ $jobs_data->{ids}->{$id} = $job;
+ } else {
+ $job = $jobs_data->{ids}->{$id};
+ die "no such vzdump job\n" if !$job || $job->{type} ne 'vzdump';
+ }
- foreach my $job (@$jobs) {
- if ($job->{id} eq $param->{id}) {
+ foreach my $k (@$delete) {
+ if (!PVE::VZDump::option_exists($k)) {
+ raise_param_exc({ delete => "unknown option '$k'" });
+ }
- foreach my $k (@delete) {
- if (!PVE::VZDump::option_exists($k)) {
- raise_param_exc({ delete => "unknown option '$k'" });
- }
+ delete $job->{$k};
+ }
- delete $job->{$k};
- }
+ my $schedule_updated = 0;
+ if ($param->{schedule} ne $job->{schedule}) {
+ $schedule_updated = 1;
+ }
- foreach my $k (keys %$param) {
- $job->{$k} = $param->{$k};
- }
+ foreach my $k (keys %$param) {
+ $job->{$k} = $param->{$k};
+ }
- $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool}));
-
- if (defined($param->{vmid})) {
- delete $job->{all};
- delete $job->{exclude};
- delete $job->{pool};
- } elsif ($param->{all}) {
- delete $job->{vmid};
- delete $job->{pool};
- } elsif ($job->{pool}) {
- delete $job->{vmid};
- delete $job->{all};
- delete $job->{exclude};
- }
+ $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool}));
+
+ if (defined($param->{vmid})) {
+ delete $job->{all};
+ delete $job->{exclude};
+ delete $job->{pool};
+ } elsif ($param->{all}) {
+ delete $job->{vmid};
+ delete $job->{pool};
+ } elsif ($job->{pool}) {
+ delete $job->{vmid};
+ delete $job->{all};
+ delete $job->{exclude};
+ }
- PVE::VZDump::verify_vzdump_parameters($job, 1);
+ PVE::VZDump::verify_vzdump_parameters($job, 1);
- cfs_write_file('vzdump.cron', $data);
+ if ($schedule_updated) {
+ PVE::Jobs::updated_job_schedule($id, 'vzdump');
+ }
- return undef;
- }
+ if ($found) {
+ cfs_write_file('vzdump.cron', $data);
}
- raise_param_exc({ id => "No such job '$param->{id}'" });
+ cfs_write_file('jobs.cfg', $jobs_data); # FIXME LOCKING
+
+ return undef;
};
- cfs_lock_file('vzdump.cron', undef, $update_job);
+ cfs_lock_file('vzdump.cron', undef, sub {
+ cfs_lock_file('jobs.cfg', undef, $update_job);
+ die "$@" if ($@);
+ });
die "$@" if ($@);
}});
@@ -422,6 +568,13 @@ __PACKAGE__->register_method({
last;
}
}
+ if (!$job) {
+ my $jobs_data = cfs_read_file('jobs.cfg');
+ my $j = $jobs_data->{ids}->{$param->{id}};
+ if ($j && $j->{type} eq 'vzdump') {
+ $job = $j;
+ }
+ }
raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
my $vmlist = PVE::Cluster::get_vmlist();
diff --git a/PVE/API2/Cluster/BackupInfo.pm b/PVE/API2/Cluster/BackupInfo.pm
index 236e171d..101e45a7 100644
--- a/PVE/API2/Cluster/BackupInfo.pm
+++ b/PVE/API2/Cluster/BackupInfo.pm
@@ -28,6 +28,16 @@ sub get_included_vmids {
push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
}
+ my $vzjobs = cfs_read_file('jobs.cfg');
+
+ for my $id (keys %{$vzjobs->{ids}}) {
+ my $job = $vzjobs->{ids}->{$id};
+ next if $job->{type} ne 'vzdump';
+
+ my $job_included_guests = PVE::VZDump::get_included_guests($job);
+ push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
+ }
+
return { map { $_ => 1 } @all_vmids };
}
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs
2021-10-07 8:27 ` [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs Dominik Csapak
@ 2021-11-03 9:05 ` Fabian Ebner
0 siblings, 0 replies; 17+ messages in thread
From: Fabian Ebner @ 2021-11-03 9:05 UTC (permalink / raw)
To: pve-devel, Dominik Csapak
Am 07.10.21 um 10:27 schrieb Dominik Csapak:
> in addition to listing the vzdump.cron jobs, also list from the
> jobs.cfg file.
>
> updates/creations go into the new jobs.cfg only now
> and on update, starttime+dow get converted to a schedule
> this transformation is straight forward, since 'dow'
> is already in a compatible format (e.g. 'mon,tue') and we simply
> append the starttime (if any)
>
> id on creation is optional for now (for api compat), but will
> be autogenerated (uuid). on update, we simply take the id from before
> (the ids of the other entries in vzdump.cron will change but they would
> anyway)
>
> as long as we have the vzdump.cron file, we must lock both
> vzdump.cron and jobs.cfg, since we often update both
>
> we also change the backupinfo api call to read the jobs.cfg too
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> PVE/API2/Backup.pm | 243 +++++++++++++++++++++++++++------
> PVE/API2/Cluster/BackupInfo.pm | 10 ++
> 2 files changed, 208 insertions(+), 45 deletions(-)
>
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 9dc3b48e..56ed4be8 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -3,6 +3,7 @@ package PVE::API2::Backup;
> use strict;
> use warnings;
> use Digest::SHA;
> +use UUID;
>
> use PVE::SafeSyslog;
> use PVE::Tools qw(extract_param);
> @@ -14,6 +15,7 @@ use PVE::Storage;
> use PVE::Exception qw(raise_param_exc);
> use PVE::VZDump;
> use PVE::VZDump::Common;
> +use PVE::Jobs; # for VZDump Jobs
>
> use base qw(PVE::RESTHandler);
>
> @@ -45,6 +47,19 @@ my $assert_param_permission = sub {
> }
> };
>
> +my $convert_to_schedule = sub {
> + my ($job) = @_;
> +
> + my $starttime = $job->{starttime};
> + my $dow = $job->{dow};
> +
> + if (!$dow || $dow eq ALL_DAYS) {
> + return "$starttime";
> + }
> +
> + return "$dow $starttime";
> +};
> +
> __PACKAGE__->register_method({
> name => 'index',
> path => '',
> @@ -74,8 +89,20 @@ __PACKAGE__->register_method({
> my $user = $rpcenv->get_user();
>
> my $data = cfs_read_file('vzdump.cron');
> + my $jobs_data = cfs_read_file('jobs.cfg');
> + my $order = $jobs_data->{order};
> + my $jobs = $jobs_data->{ids};
>
> my $res = $data->{jobs} || [];
> + foreach my $job (@$res) {
> + $job->{schedule} = $convert_to_schedule->($job);
> + }
> +
> + foreach my $jobid (sort { $order->{$a} <=> $order->{$b} } keys %$jobs) {
> + my $job = $jobs->{$jobid};
> + next if $job->{type} ne 'vzdump';
> + push @$res, $job;
> + }
>
> return $res;
> }});
> @@ -93,11 +120,24 @@ __PACKAGE__->register_method({
> parameters => {
> additionalProperties => 0,
> properties => PVE::VZDump::Common::json_config_properties({
> + id => {
> + type => 'string',
> + description => "Job ID (will be autogenerated).",
> + format => 'pve-configid',
> + optional => 1, # FIXME: make required on 8.0
> + },
> + schedule => {
> + description => "Backup schedule. The format is a subset of `systemd` calendar events.",
> + type => 'string', format => 'pve-calendar-event',
> + maxLength => 128,
> + optional => 1,
> + },
> starttime => {
> type => 'string',
> description => "Job Start time.",
> pattern => '\d{1,2}:\d{1,2}',
> typetext => 'HH:MM',
> + optional => 1,
> },
> dow => {
> type => 'string', format => 'pve-day-of-week-list',
> @@ -127,19 +167,40 @@ __PACKAGE__->register_method({
> $rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
> }
>
> + if (defined($param->{schedule})) {
> + if (defined($param->{starttime})) {
> + raise_param_exc({ starttime => "'starttime' and 'schedule' cannot both be set" });
> + }
Should schedule also clash with dow? Or use requires => 'starttime' like
for the update call?
> + } elsif (!defined($param->{starttime})) {
> + raise_param_exc({ schedule => "neither 'starttime' nor 'schedule' were set" });
> + } else {
> + $param->{schedule} = $convert_to_schedule->($param);
> + }
> +
> + delete $param->{starttime};
> + delete $param->{dow};
>
The schedule conversion/checks could be factored out to avoid
duplication for the update call.
> - my $create_job = sub {
> - my $data = cfs_read_file('vzdump.cron');
> + $param->{enabled} = 1 if !defined($param->{enabled});
> +
> + # autogenerate id for api compatibility FIXME remove with 8.0
> + my $id = extract_param($param, 'id') // uuid();
> +
> + cfs_lock_file('jobs.cfg', undef, sub {
> + my $data = cfs_read_file('jobs.cfg');
> +
> + die "Job '$id' already exists\n"
> + if $data->{ids}->{$id};
>
> - $param->{dow} = ALL_DAYS if !defined($param->{dow});
> - $param->{enabled} = 1 if !defined($param->{enabled});
> PVE::VZDump::verify_vzdump_parameters($param, 1);
> + my $plugin = PVE::Jobs::Plugin->lookup('vzdump');
> + my $opts = $plugin->check_config($id, $param, 1, 1);
>
> - push @{$data->{jobs}}, $param;
> + $data->{ids}->{$id} = $opts;
>
> - cfs_write_file('vzdump.cron', $data);
> - };
> - cfs_lock_file('vzdump.cron', undef, $create_job);
> + PVE::Jobs::create_job($id, 'vzdump');
> +
> + cfs_write_file('jobs.cfg', $data);
> + });
> die "$@" if ($@);
>
> return undef;
> @@ -173,9 +234,16 @@ __PACKAGE__->register_method({
> my $jobs = $data->{jobs} || [];
>
> foreach my $job (@$jobs) {
> - return $job if $job->{id} eq $param->{id};
> + if ($job->{id} eq $param->{id}) {
> + $job->{schedule} = $convert_to_schedule->($job);
> + return $job;
> + }
> }
>
> + my $jobs_data = cfs_read_file('jobs.cfg');
> + my $job = $jobs_data->{ids}->{$param->{id}};
> + return $job if $job && $job->{type} eq 'vzdump';
> +
> raise_param_exc({ id => "No such job '$param->{id}'" });
>
> }});
> @@ -202,6 +270,8 @@ __PACKAGE__->register_method({
> my $rpcenv = PVE::RPCEnvironment::get();
> my $user = $rpcenv->get_user();
>
> + my $id = $param->{id};
> +
> my $delete_job = sub {
> my $data = cfs_read_file('vzdump.cron');
>
> @@ -210,18 +280,32 @@ __PACKAGE__->register_method({
>
> my $found;
> foreach my $job (@$jobs) {
> - if ($job->{id} eq $param->{id}) {
> + if ($job->{id} eq $id) {
> $found = 1;
> } else {
> push @$newjobs, $job;
> }
> }
>
> - raise_param_exc({ id => "No such job '$param->{id}'" }) if !$found;
> + if (!$found) {
> + cfs_lock_file('jobs.cfg', undef, sub {
> + my $jobs_data = cfs_read_file('jobs.cfg');
>
> - $data->{jobs} = $newjobs;
> + if (!defined($jobs_data->{ids}->{$id})) {
> + raise_param_exc({ id => "No such job '$id'" });
> + }
> + delete $jobs_data->{ids}->{$id};
> +
> + PVE::Jobs::remove_job($id, 'vzudmp');
> +
> + cfs_write_file('jobs.cfg', $jobs_data);
> + });
> + die "$@" if $@;
> + } else {
> + $data->{jobs} = $newjobs;
>
> - cfs_write_file('vzdump.cron', $data);
> + cfs_write_file('vzdump.cron', $data);
> + }
> };
> cfs_lock_file('vzdump.cron', undef, $delete_job);
> die "$@" if ($@);
> @@ -243,15 +327,23 @@ __PACKAGE__->register_method({
> additionalProperties => 0,
> properties => PVE::VZDump::Common::json_config_properties({
> id => $vzdump_job_id_prop,
> + schedule => {
> + description => "Backup schedule. The format is a subset of `systemd` calendar events.",
> + type => 'string', format => 'pve-calendar-event',
> + maxLength => 128,
> + optional => 1,
> + },
> starttime => {
> type => 'string',
> description => "Job Start time.",
> pattern => '\d{1,2}:\d{1,2}',
> typetext => 'HH:MM',
> + optional => 1,
> },
> dow => {
> type => 'string', format => 'pve-day-of-week-list',
> optional => 1,
> + requires => 'starttime',
> description => "Day of week selection.",
> },
> delete => {
> @@ -281,57 +373,111 @@ __PACKAGE__->register_method({
> $rpcenv->check($user, "/pool/$pool", ['VM.Backup']);
> }
>
> + if (defined($param->{schedule})) {
> + if (defined($param->{starttime})) {
> + raise_param_exc({ starttime => "'starttime' and 'schedule' cannot both be set" });
> + }
> + } elsif (!defined($param->{starttime})) {
> + raise_param_exc({ schedule => "neither 'starttime' nor 'schedule' were set" });
> + } else {
> + $param->{schedule} = $convert_to_schedule->($param);
> + }
> +
> + delete $param->{starttime};
> + delete $param->{dow};
> +
> + my $id = extract_param($param, 'id');
> + my $delete = extract_param($param, 'delete');
> + if ($delete) {
> + $delete = [PVE::Tools::split_list($delete)];
> + }
> +
> my $update_job = sub {
> my $data = cfs_read_file('vzdump.cron');
> + my $jobs_data = cfs_read_file('jobs.cfg');
>
> my $jobs = $data->{jobs} || [];
>
> die "no options specified\n" if !scalar(keys %$param);
>
> PVE::VZDump::verify_vzdump_parameters($param);
> + my $plugin = PVE::Jobs::Plugin->lookup('vzdump');
> + my $opts = $plugin->check_config($id, $param, 0, 1);
> +
> + # try to find it in old vzdump.cron and convert it to a job
> + my $idx = 0;
> + my $found = 0;
> + foreach my $j (@$jobs) {
> + if ($j->{id} eq $id) {
> + $found = 1;
> + last;
> + }
> + $idx++;
> + }
Haven't tested it, but something like:
my ($idx) = grep { $jobs->[$_]->{id} eq $id } (0 .. scalar(@$jobs)
- 1);
would be a bit shorter, with using defined($idx) instead of $found. Just
an idea though.
>
> - my @delete = PVE::Tools::split_list(extract_param($param, 'delete'));
> + my $job;
> + if ($found) {
> + $job = splice @$jobs, $idx, 1;
> + $job->{schedule} = $convert_to_schedule->($job);
> + delete $job->{starttime};
> + delete $job->{dow};
> + delete $job->{id};
> + $job->{type} = 'vzdump';
> + $jobs_data->{ids}->{$id} = $job;
> + } else {
> + $job = $jobs_data->{ids}->{$id};
> + die "no such vzdump job\n" if !$job || $job->{type} ne 'vzdump';
> + }
>
> - foreach my $job (@$jobs) {
> - if ($job->{id} eq $param->{id}) {
> + foreach my $k (@$delete) {
> + if (!PVE::VZDump::option_exists($k)) {
> + raise_param_exc({ delete => "unknown option '$k'" });
> + }
>
> - foreach my $k (@delete) {
> - if (!PVE::VZDump::option_exists($k)) {
> - raise_param_exc({ delete => "unknown option '$k'" });
> - }
> + delete $job->{$k};
> + }
>
> - delete $job->{$k};
> - }
> + my $schedule_updated = 0;
> + if ($param->{schedule} ne $job->{schedule}) {
> + $schedule_updated = 1;
> + }
>
> - foreach my $k (keys %$param) {
> - $job->{$k} = $param->{$k};
> - }
> + foreach my $k (keys %$param) {
> + $job->{$k} = $param->{$k};
> + }
>
> - $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool}));
> -
> - if (defined($param->{vmid})) {
> - delete $job->{all};
> - delete $job->{exclude};
> - delete $job->{pool};
> - } elsif ($param->{all}) {
> - delete $job->{vmid};
> - delete $job->{pool};
> - } elsif ($job->{pool}) {
> - delete $job->{vmid};
> - delete $job->{all};
> - delete $job->{exclude};
> - }
> + $job->{all} = 1 if (defined($job->{exclude}) && !defined($job->{pool}));
> +
> + if (defined($param->{vmid})) {
> + delete $job->{all};
> + delete $job->{exclude};
> + delete $job->{pool};
> + } elsif ($param->{all}) {
> + delete $job->{vmid};
> + delete $job->{pool};
> + } elsif ($job->{pool}) {
> + delete $job->{vmid};
> + delete $job->{all};
> + delete $job->{exclude};
> + }
>
> - PVE::VZDump::verify_vzdump_parameters($job, 1);
> + PVE::VZDump::verify_vzdump_parameters($job, 1);
>
> - cfs_write_file('vzdump.cron', $data);
> + if ($schedule_updated) {
> + PVE::Jobs::updated_job_schedule($id, 'vzdump');
> + }
>
> - return undef;
> - }
> + if ($found) {
> + cfs_write_file('vzdump.cron', $data);
> }
> - raise_param_exc({ id => "No such job '$param->{id}'" });
> + cfs_write_file('jobs.cfg', $jobs_data); # FIXME LOCKING
Left-over reminder?
> +
> + return undef;
Nit: return is favorable over return undef (matters in list context).
> };
> - cfs_lock_file('vzdump.cron', undef, $update_job);
> + cfs_lock_file('vzdump.cron', undef, sub {
> + cfs_lock_file('jobs.cfg', undef, $update_job);
> + die "$@" if ($@);
> + });
> die "$@" if ($@);
> }});
>
> @@ -422,6 +568,13 @@ __PACKAGE__->register_method({
> last;
> }
> }
> + if (!$job) {
> + my $jobs_data = cfs_read_file('jobs.cfg');
> + my $j = $jobs_data->{ids}->{$param->{id}};
> + if ($j && $j->{type} eq 'vzdump') {
> + $job = $j;
> + }
> + }
> raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
>
> my $vmlist = PVE::Cluster::get_vmlist();
> diff --git a/PVE/API2/Cluster/BackupInfo.pm b/PVE/API2/Cluster/BackupInfo.pm
> index 236e171d..101e45a7 100644
> --- a/PVE/API2/Cluster/BackupInfo.pm
> +++ b/PVE/API2/Cluster/BackupInfo.pm
> @@ -28,6 +28,16 @@ sub get_included_vmids {
> push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
> }
>
> + my $vzjobs = cfs_read_file('jobs.cfg');
> +
> + for my $id (keys %{$vzjobs->{ids}}) {
> + my $job = $vzjobs->{ids}->{$id};
Nit: loop could iterate over values rather than keys.
> + next if $job->{type} ne 'vzdump';
> +
> + my $job_included_guests = PVE::VZDump::get_included_guests($job);
> + push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
> + }
> +
> return { map { $_ => 1 } @all_vmids };
> }
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pve-devel] [PATCH manager 7/7] ui: dc/backup: show id+schedule instead of dow+starttime
2021-10-07 8:27 [pve-devel] [PATCH cluster/manager] add scheduling daemon for pvesr + vzdump (and more) Dominik Csapak
` (6 preceding siblings ...)
2021-10-07 8:27 ` [pve-devel] [PATCH manager 6/7] api/backup: handle new vzdump jobs Dominik Csapak
@ 2021-10-07 8:27 ` Dominik Csapak
2021-11-03 9:21 ` Fabian Ebner
7 siblings, 1 reply; 17+ messages in thread
From: Dominik Csapak @ 2021-10-07 8:27 UTC (permalink / raw)
To: pve-devel
we can now show the id (since its not autogenerated anymore),
and we can always show/edit the schedule instead of the dow+starttime
also add an 'ID' field to the edit/create window and update the
backupinfo window as well
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
www/manager6/dc/Backup.js | 47 ++++++++++++++----------------
www/manager6/dc/BackupJobDetail.js | 10 ++-----
2 files changed, 24 insertions(+), 33 deletions(-)
diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index adefc5f4..87b3d70a 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -176,24 +176,22 @@ Ext.define('PVE.dc.BackupEdit', {
});
let column1 = [
- nodesel,
- storagesel,
{
- xtype: 'pveDayOfWeekSelector',
- name: 'dow',
- fieldLabel: gettext('Day of week'),
- multiSelect: true,
- value: ['sat'],
+ xtype: 'pmxDisplayEditField',
+ name: 'id',
+ fieldLabel: gettext('ID'),
+ renderer: Ext.htmlEncode,
allowBlank: false,
+ minLength: 4,
+ editable: me.isCreate,
},
+ nodesel,
+ storagesel,
{
- xtype: 'timefield',
- fieldLabel: gettext('Start Time'),
- name: 'starttime',
- format: 'H:i',
- formatText: 'HH:MM',
- value: '00:00',
+ xtype: 'pveCalendarEvent',
+ fieldLabel: gettext('Schedule'),
allowBlank: false,
+ name: 'schedule',
},
selModeField,
selPool,
@@ -390,7 +388,7 @@ Ext.define('PVE.dc.BackupEdit', {
success: function(response, options) {
let data = response.result.data;
- data.dow = data.dow.split(',');
+ data.dow = (data.dow || '').split(',');
if (data.all || data.exclude) {
if (data.exclude) {
@@ -532,6 +530,8 @@ Ext.define('PVE.dc.BackupView', {
delete job.starttime;
delete job.dow;
delete job.id;
+ delete job.schedule;
+ delete job.type;
delete job.node;
job.all = job.all === true ? 1 : 0;
@@ -714,6 +714,11 @@ Ext.define('PVE.dc.BackupView', {
disabledCls: 'x-item-enabled',
stopSelection: false,
},
+ {
+ header: gettext('ID'),
+ dataIndex: 'id',
+ hidden: true,
+ },
{
header: gettext('Node'),
width: 100,
@@ -727,17 +732,9 @@ Ext.define('PVE.dc.BackupView', {
},
},
{
- header: gettext('Day of week'),
- width: 200,
- sortable: false,
- dataIndex: 'dow',
- renderer: PVE.Utils.render_backup_days_of_week,
- },
- {
- header: gettext('Start Time'),
- width: 60,
- sortable: true,
- dataIndex: 'starttime',
+ header: gettext('Schedule'),
+ width: 150,
+ dataIndex: 'schedule',
},
{
header: gettext('Storage'),
diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
index b91cb1b7..19b3b1a3 100644
--- a/www/manager6/dc/BackupJobDetail.js
+++ b/www/manager6/dc/BackupJobDetail.js
@@ -184,14 +184,8 @@ Ext.define('PVE.dc.BackupInfo', {
},
{
xtype: 'displayfield',
- name: 'dow',
- fieldLabel: gettext('Day of week'),
- renderer: PVE.Utils.render_backup_days_of_week,
- },
- {
- xtype: 'displayfield',
- name: 'starttime',
- fieldLabel: gettext('Start Time'),
+ name: 'schedule',
+ fieldLabel: gettext('Schedule'),
},
{
xtype: 'displayfield',
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pve-devel] [PATCH manager 7/7] ui: dc/backup: show id+schedule instead of dow+starttime
2021-10-07 8:27 ` [pve-devel] [PATCH manager 7/7] ui: dc/backup: show id+schedule instead of dow+starttime Dominik Csapak
@ 2021-11-03 9:21 ` Fabian Ebner
0 siblings, 0 replies; 17+ messages in thread
From: Fabian Ebner @ 2021-11-03 9:21 UTC (permalink / raw)
To: pve-devel, Dominik Csapak
Am 07.10.21 um 10:27 schrieb Dominik Csapak:
> we can now show the id (since its not autogenerated anymore),
But it's hidden below ;)
I get that the ID is not very telling for existing jobs, but for jobs
with user-chosen IDs it would allow to distinguish them more quickly.
> and we can always show/edit the schedule instead of the dow+starttime
>
> also add an 'ID' field to the edit/create window and update the
> backupinfo window as well
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> www/manager6/dc/Backup.js | 47 ++++++++++++++----------------
> www/manager6/dc/BackupJobDetail.js | 10 ++-----
> 2 files changed, 24 insertions(+), 33 deletions(-)
>
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index adefc5f4..87b3d70a 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -176,24 +176,22 @@ Ext.define('PVE.dc.BackupEdit', {
> });
>
> let column1 = [
> - nodesel,
> - storagesel,
> {
> - xtype: 'pveDayOfWeekSelector',
> - name: 'dow',
> - fieldLabel: gettext('Day of week'),
> - multiSelect: true,
> - value: ['sat'],
> + xtype: 'pmxDisplayEditField',
> + name: 'id',
> + fieldLabel: gettext('ID'),
> + renderer: Ext.htmlEncode,
> allowBlank: false,
> + minLength: 4,
> + editable: me.isCreate,
> },
> + nodesel,
> + storagesel,
> {
> - xtype: 'timefield',
> - fieldLabel: gettext('Start Time'),
> - name: 'starttime',
> - format: 'H:i',
> - formatText: 'HH:MM',
> - value: '00:00',
> + xtype: 'pveCalendarEvent',
> + fieldLabel: gettext('Schedule'),
> allowBlank: false,
> + name: 'schedule',
> },
> selModeField,
> selPool,
> @@ -390,7 +388,7 @@ Ext.define('PVE.dc.BackupEdit', {
> success: function(response, options) {
> let data = response.result.data;
>
> - data.dow = data.dow.split(',');
> + data.dow = (data.dow || '').split(',');
>
> if (data.all || data.exclude) {
> if (data.exclude) {
> @@ -532,6 +530,8 @@ Ext.define('PVE.dc.BackupView', {
> delete job.starttime;
> delete job.dow;
> delete job.id;
> + delete job.schedule;
> + delete job.type;
> delete job.node;
> job.all = job.all === true ? 1 : 0;
>
> @@ -714,6 +714,11 @@ Ext.define('PVE.dc.BackupView', {
> disabledCls: 'x-item-enabled',
> stopSelection: false,
> },
> + {
> + header: gettext('ID'),
> + dataIndex: 'id',
> + hidden: true
Here (also misses trailing comma).
> + },
> {
> header: gettext('Node'),
> width: 100,
> @@ -727,17 +732,9 @@ Ext.define('PVE.dc.BackupView', {
> },
> },
> {
> - header: gettext('Day of week'),
> - width: 200,
> - sortable: false,
> - dataIndex: 'dow',
> - renderer: PVE.Utils.render_backup_days_of_week,
> - },
> - {
> - header: gettext('Start Time'),
> - width: 60,
> - sortable: true,
> - dataIndex: 'starttime',
> + header: gettext('Schedule'),
> + width: 150,
> + dataIndex: 'schedule',
> },
> {
> header: gettext('Storage'),
> diff --git a/www/manager6/dc/BackupJobDetail.js b/www/manager6/dc/BackupJobDetail.js
> index b91cb1b7..19b3b1a3 100644
> --- a/www/manager6/dc/BackupJobDetail.js
> +++ b/www/manager6/dc/BackupJobDetail.js
> @@ -184,14 +184,8 @@ Ext.define('PVE.dc.BackupInfo', {
> },
> {
> xtype: 'displayfield',
> - name: 'dow',
> - fieldLabel: gettext('Day of week'),
> - renderer: PVE.Utils.render_backup_days_of_week,
> - },
> - {
> - xtype: 'displayfield',
> - name: 'starttime',
> - fieldLabel: gettext('Start Time'),
> + name: 'schedule',
> + fieldLabel: gettext('Schedule'),
> },
> {
> xtype: 'displayfield',
>
^ permalink raw reply [flat|nested] 17+ messages in thread