Check the number of protected backups early if the protected flag
is set.
Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
else this single call site is subtly different from all the rest, which
could cause problems further down the line if we ever change the prune
logic.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
since they are the ones relevant for pruning and protected backups
have their own separate limit.
Since get_backup_file_list is only used in places where the
unprotected backups are needed, adapt the helper accordingly.
If there is a storage, use PVE::Storage::volume_list to count the
unprotected backups. This avoids a direct invocation of the
proxmox-backup-client for PBS and the limit check can also work for
external storage plugins which might not be dir-based or name the
backups differently.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
which allows users to prepare the backup storage for activation (e.g.
by waking up a remote node).
Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Acked-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Keeps the behavior consistent with what happens for storages. It also
is required to not get into conflict with the check in archive_remove,
i.e. pruning here marks a backup as 'remove' and then archive_remove
complains that it's protected.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
which ended up in the backup task log and can be confusing to users:
> INFO: Backup finished at 2021-10-11 08:40:38
> Result: {
> "data": null
> }
> INFO: Backup job finished successfully
For proxmox-backup-server < 2.0.11-1, there was no output for the
upload-log command.
Reported in the community forum:
https://forum.proxmox.com/threads/backup-finishes-ok-but-data-null-info-on-finish.97740/
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Not doing so leads to an off-by-one error, and pruning in a dumpdir
really should behave the same way as pruning in a storage.
Reported in the community forum:
https://forum.proxmox.com/threads/problem-mit-prune-backups.95737/
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
remove type check for the specific plugins, instead we can deduce it
from the supported content type in config (this can only be set on
storages that declare to support backups). should also work with
external storage plugins.
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
may not be obvious from the task log alone if we're relying on the
retention setting configured on a storage or the ones from local
node's vzdump.conf
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
if passing the hook script command as string, it might get interpreted
as shell command with side-effects. this is pretty harmless, since only
root is allowed to set the script parameter anyway, but making it more
robust and future-proof does not hurt.
tested with a reproducer of "/bin/echo $(touch $(whoami))" as script
parameter, with a file with that name existing, being executable and
having the following contents:
----8<----
echo "hello from hook script"
---->8----
without this change, the hookscript itself is not executed, but
'/bin/sh -c "/bin/echo $(touch $(whoami)) job"' and similar calls are,
which cause the file 'root' to be touched in the current working
directory of the vzdump process (or task worker).
with this change, the file is executed as is without any side-effects of
shell commands in the file name, and the 'hello from hook script' lines
are printed whenever the hook script is called by vzdump.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
so it doesn't get out of scope too early.
Regression introduced by 5620e5761e as pointed
out by Fabian Grünbichler.
Reported in the community forum:
https://forum.proxmox.com/threads/limit-simultaneous-backup-jobs.87489
Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Otherwise storage_info() cannot be used for (at least) PBS storages from an API
call without 'protected => 1', because the password cannot be read from
'/etc/pve/priv'. Note that the function itself does not need the storage to be
active, because it only uses storage_config() and get_backup_dir().
AFAICT new() is the only existing user of this function and can be responsible
for activating the storage itself.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
The initial default from the $confdesc is 1 anyways, and like
this changing the default in /etc/vzdump.conf to 0 actually works.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
reported by perlcritic. The bareword file handles will be depreacated
in the future in perl, so start replacing now..
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
When a job is updated, verify_vzdump_parameters() is called twice. This led to
parse_property_string being called with the already parsed option.
Reported on the pve-user mailing list:
https://lists.proxmox.com/pipermail/pve-user/2021-January/172258.html
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Errors from storage_info() are newline-terminated, because perl would append
the line number otherwise. Chomp those errors, because sendmail() relies
on the presence of a newline to decide if it's multiple problems or only one.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Fixes the case where reading from /etc/vzdump.conf fails.
Also convert the options read from /etc/vzdump.conf before the loop. That
avoids showing a wrong warning when 'prune-backups' is configured in
/etc/vzdump.conf, and maxfiles isn't. Previously, because 'maxfiles' from the
schema defaults was automatically set, the call to parse_prune_backups_maxfiles
after the loop threw the warning that both options are defined.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
and prefer storage, because the storage configuration might contain more
settings. Warning is preferable over dying, because all backups would be
affected (even if they don't use the vzdump.conf parameters) and the settings
could've been compatible (i.e. dumpdir being the storage's dump dir). Previously
one of the two options would randomly be chosen in the loop in new(), because of
perl hash iteration.
Reported here: https://forum.proxmox.com/threads/vzdump-times-out-very-often-on-zfs-storage-pool.80035/post-354277
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
If keep-all is set to 0, adding it doesn't make a difference,
if the key is not in the hash, it won't show up in the 'values'.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Less lines exeeding the character limit, less nesting, less duplicate code,
more readable sprintf arguments.
Co-authored-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
Commit b64dd8c8a5 hard-coded 0 as the default
for maxfiles in the --storage case, but the actual default should be the
value from read_vzdump_defaults(), which obtains the value from
/etc/vzdump.conf or the VZDump schema if the value has not been modified in
that file. The initial default from the schema is 1, not 0.
Tested on PVE 6.1 to verify that behavior.
Move the sanity check for zero-ness to where we have the final value for
maxfiles. Like this, we also have an implicit definedness check and more
importantly, it is more future-proof in case we ever allow maxfiles 0 in the
VZDump schema itself.
Also, force conversion to int to be extra safe.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
The html/text part already has VMID NAME STATUS TIME..., but the text part only
had VMID STATUS TIME... so far. Therefore, add the missing "name" column.
Limit the length of names so that the content of the following columns remains
aligned to the headings. Note that (like before, too) this only works with
monospaced fonts.
Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
If neither the 'remove' option of vzdump nor the 'maxfiles' option in
the storage config are set, assume a value of 0, i.e. do not delete
anything and allow unlimited backups.
Restores previous behaviour that was broken in e6946086e3.
Also fixes a warning about using '== 0' on a non-number type.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
fixes commit f354fe47fc, which changed
the logic to the newer storage prune helpers, but those are designed
in the spirits of PBS, with a keep-option not set meaning to keep
none.
This does not respects the storage special handling of maxfiles.
While in the API/CLI that option must be > 0m in can be zero when set
in a storage configuration entry, and then it means keep all. So, set
the internal remove option to false if that special condition is met.
This would have been a clearer, and less prone to changes,
implementation to begin with.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
The assumption that they already are sorted is no longer valid,
because of the IDs for non-existent guests.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Like this, there will be a backup task (within the big worker task)
for such IDs, which will then visibly (i.e. also visible in the
notification mail) fail with, e.g.:
unable to find VM '123'
In get_included_guests, the key '' was chosen for the orphaned IDs,
because it cannot possibly denote a nodename.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Commit 1a87db9e56 introduced
a usage of plugin before the truthiness check for plugin.
At the moment it might not be possible to trigger this anymore,
because of the guest inclusion rework that happened later on.
But to make tasks for inexistent guest IDs visibly fail again,
this check will be necessary. Also, to get the error message in
the mail, it needs to fail inside the eval block.
Thus, keep the check in the eval block and move the block of code
using the plugin to below the check.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
