Re: [pve-devel] [PATCH cluster] fix #4234: vzdump: add cluster-wide configuration

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 09/02/2023 um 10:27 schrieb Leo Nunner:
> Introduce a cluster-wide vzdump.conf file which gets filled with the
> default vzdump configuration.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> 
>  data/PVE/Cluster.pm       |  1 +
>  data/PVE/Cluster/Setup.pm | 32 +++++++++++++++++++++++++++++---
>  data/src/status.c         |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index 0154aae..efca58f 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,
> +    'vzdump.conf' => 1,
>      'jobs.cfg' => 1,
>      'storage.cfg' => 1,
>      'datacenter.cfg' => 1,
> diff --git a/data/PVE/Cluster/Setup.pm b/data/PVE/Cluster/Setup.pm
> index 108817e..061fe08 100644
> --- a/data/PVE/Cluster/Setup.pm
> +++ b/data/PVE/Cluster/Setup.pm
> @@ -579,6 +579,28 @@ PATH="/usr/sbin:/usr/bin:/sbin:/bin"
>  
>  __EOD
>  
> +my $vzdump_conf_dummy = <<__EOD;
> +# vzdump default settings
> +# these are overruled by the node-specific configuration in /etc/vzdump.conf
> +
> +#tmpdir: DIR
> +#dumpdir: DIR
> +#storage: STORAGE_ID
> +#mode: snapshot|suspend|stop
> +#bwlimit: KBPS
> +#performance: max-workers=N
> +#ionice: PRI
> +#lockwait: MINUTES
> +#stopwait: MINUTES
> +#stdexcludes: BOOLEAN
> +#mailto: ADDRESSLIST
> +#prune-backups: keep-INTERVAL=N[,...]
> +#script: FILENAME
> +#exclude-path: PATHLIST
> +#pigz: N
> +#notes-template: {{guestname}}
> +__EOD
> +
>  sub gen_pve_vzdump_symlink {
>  
>      my $filename = "/etc/pve/vzdump.cron";
> @@ -593,10 +615,14 @@ sub gen_pve_vzdump_symlink {
>  
>  sub gen_pve_vzdump_files {
>  
> -    my $filename = "/etc/pve/vzdump.cron";
> +    my $cron = "/etc/pve/vzdump.cron";
> +    my $conf = "/etc/pve/vzdump.conf";
> +
> +    PVE::Tools::file_set_contents($cron, $vzdump_cron_dummy)
> +	if ! -f $cron;

not directly related but shouldn't we drop writing out a vzdump.cron now that
all vzdump backups are handled through the jobs.cfg?

>  
> -    PVE::Tools::file_set_contents($filename, $vzdump_cron_dummy)
> -	if ! -f $filename;
> +    PVE::Tools::file_set_contents($conf, $vzdump_conf_dummy)
> +	if ! -f $conf;

I'm not a fan of setting this for the cluster too just because we have it on
node-level, users should either read the docs. Besides, the implementation as
is is actually open for TOCTOU race as the file could have been written since
checking for existence by another process possibly on another node.

So if, we'd need to use a cfs domain lock + possibly a local flock (and split
it out in its own patch, not related to registering the file to CFS), but I'd
rather just omit it completely.