Hi,

The patch set works as advertised and is very clean in terms of the switch from vzdump.cron to jobs.cfg.

One very minor inconvenience that i seen was the fact that for vzdump.cron 'Pool based' backups, you can't simply select 'Edit->Ok' to convert it to the new format, but rather you need to change some detail to allow the edit. However, this could barely be considered an issue, as it just turns conversion into a two-step process of 'make minor edit. revert minor edit'.

The IDs that are automatically generated for the old cron jobs are a bit ugly and all get shuffled each time a job is moved from vzdump.cron to jobs.cfg (as mentioned in the commit message), although they are at least easy to change from the job config file, so I don't imagine anyone using these default IDs for very long. On this note however, is having an API call to change the ID something that we want to avoid, or can it be added? I can see users being annoyed by not being able to change it from the GUI, even if only for the initial conversion.

When entering the ID of new jobs, it might also be helpful to show the allowed characters in a tool-tip box and to prevent the user trying to validate bad IDs by disabling the "Create" button while invalid, as is done with the 'add storage' window. The "invalid ID" message is a bit vague in any case, so I would at least mention what is allowed there.

Other than this, I didn't find any problems.

Tested-By: Dylan Whyte <d.wh...@proxmox.com>


On 11/8/21 2:07 PM, Dominik Csapak wrote:
with this series, we implement a new daemon (pvescheduler) that takes
over from pvesrs' systemd timer (original patch from thomas[0]) and
extends it with a generic job handling mechanism

then i convert the vzdump cron jobs to these jobs, the immediate
gain is that users can use calendarevent schedules instead of
dow + starttime

for now, this 'jobs.cfg' only handles vzdump jobs, but should be easily
extendable for other type of recurring jobs (like auth realm sync, etc.)

also, i did not yet convert the replication jobs to this job system,
but that could probably be done without too much effort (though
i did not look too deeply into it)

if some version of this gets applied, the further plan would be
to remove the vzdump.cron part completely with 8.0, but until then
we must at least list/parse that

whats currently missing but not too hard to add is a calculated
'next-run' column in the gui

changes from v1:
* do not log replication into the syslog
* readjust the loop to start at the full minute every 1000 loops
* rework locking state locking/handling:
   - i introduces a new 'starting' state that is set before we start
     and we set it to started after the start.
     we sadly cannot start the job while we hold the lock, since the open
     file descriptor will be still open in the worker, and then we cannot
     get the flock again. now it's more modeled after how we do qm/ct
     long running locks (by writing 'starting' locked into the state)
   - the stop check is now its own call at the beginning of the job handling
   - handle created/removed jobs properly:
     i did not think of state handling on other nodes in my previous
     iteration. now on every loop, i sync the statefiles with the config
     (create/remvoe) so that the file gets created/removed on all nodes
* incorporated fabians feedback for the api (thanks!)

0: https://lists.proxmox.com/pipermail/pve-devel/2018-April/031357.html

pve-cluster:

Dominik Csapak (1):
   add 'jobs.cfg' to observed files

  data/PVE/Cluster.pm | 1 +
  data/src/status.c   | 1 +
  2 files changed, 2 insertions(+)

pve-manager:

Dominik Csapak (5):
   add PVE/Jobs to handle VZDump jobs
   pvescheduler: run jobs from jobs.cfg
   api/backup: refactor string for all days
   api/backup: handle new vzdump jobs
   ui: dc/backup: show id+schedule instead of dow+starttime

Thomas Lamprecht (1):
   replace systemd timer with pvescheduler daemon

  PVE/API2/Backup.pm                 | 235 +++++++++++++++++++-----
  PVE/API2/Cluster/BackupInfo.pm     |   9 +
  PVE/Jobs.pm                        | 286 +++++++++++++++++++++++++++++
  PVE/Jobs/Makefile                  |  16 ++
  PVE/Jobs/Plugin.pm                 |  61 ++++++
  PVE/Jobs/VZDump.pm                 |  54 ++++++
  PVE/Makefile                       |   3 +-
  PVE/Service/Makefile               |   2 +-
  PVE/Service/pvescheduler.pm        | 131 +++++++++++++
  bin/Makefile                       |   6 +-
  bin/pvescheduler                   |  28 +++
  debian/postinst                    |   3 +-
  services/Makefile                  |   3 +-
  services/pvescheduler.service      |  16 ++
  services/pvesr.service             |   8 -
  services/pvesr.timer               |  12 --
  www/manager6/dc/Backup.js          |  46 +++--
  www/manager6/dc/BackupJobDetail.js |  10 +-
  18 files changed, 823 insertions(+), 106 deletions(-)
  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
  create mode 100755 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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to