[pve-devel] [PATCH manager cluster docs] Introduce cluster-wide vzdump configuration

[pve-devel] [PATCH manager cluster docs] Introduce cluster-wide vzdump configuration

[Leo Nunner]

This patch introduces a cluster-wide vzdump configuration file. This
isn't an override per se, but rather a fallback if there is no local
configuration (or the specific parameter is not configured in the local
configuration).

The settings get merged like this:

/etc/vzdump.conf <- /etc/pve/vzdump.conf <- default values

so the local configuration takes the highest priority.

manager:

Leo Nunner (1):
  fix #4235: vzdump: add cluster-wide configuration

 PVE/VZDump.pm | 50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

cluster:

Leo Nunner (1):
  fix #4234: vzdump: add cluster-wide configuration

 data/PVE/Cluster.pm       |  1 +
 data/PVE/Cluster/Setup.pm | 32 +++++++++++++++++++++++++++++---
 data/src/status.c         |  1 +
 3 files changed, 31 insertions(+), 3 deletions(-)

docs:

Leo Nunner (1):
  vzdump: document the new cluster-wide config file

 vzdump.adoc | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH manager] fix #4235: vzdump: add cluster-wide configuration

[Leo Nunner]

The different config files get merged in the following order:

/etc/vzdump.conf <- /etc/pve/vzdump.conf <- default values

So the local configuration takes the highest precedence, then the
cluster-wide config, and finally the default values.

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
RFC: I'm not really sure where the cfs_register_file call should go. I
saw that there is one for vzdump.cron in pve-guest-common/src/PVE/VZDump/Common.pm,
but I felt like it was more fitting here, since I was passing the parsing function.

 PVE/VZDump.pm | 50 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index a04837e7..d0ab03ac 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -13,7 +13,7 @@ use IPC::Open3;
 use POSIX qw(strftime);
 use Time::Local;
 
-use PVE::Cluster qw(cfs_read_file);
+use PVE::Cluster qw(cfs_register_file cfs_read_file);
 use PVE::DataCenterConfig;
 use PVE::Exception qw(raise_param_exc);
 use PVE::HA::Config;
@@ -258,6 +258,32 @@ sub check_vmids {
     return $res;
 }
 
+sub parse_vzdump_conf {
+    my ($fn, $content) = @_;
+
+    return {} if !defined($content);
+
+    my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
+    my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $content);
+    if (my $excludes = $res->{'exclude-path'}) {
+        $res->{'exclude-path'} = PVE::Tools::split_args($excludes);
+    }
+    if (defined($res->{mailto})) {
+        my @mailto = split_list($res->{mailto});
+        $res->{mailto} = [ @mailto ];
+    }
+    $parse_prune_backups_maxfiles->($res, "options in '$fn'");
+    parse_performance($res);
+
+    if (defined($res->{storage} && defined($res->{dumpdir}))) {
+        debugmsg('warn', "both 'storage' and 'dumpdir' defined in $fn - ignoring 'dumpdir'");
+        delete $res->{dumpdir};
+    }
+
+    return $res;
+}
+
+cfs_register_file("vzdump.conf", \&parse_vzdump_conf);
 
 sub read_vzdump_defaults {
 
@@ -266,7 +292,7 @@ sub read_vzdump_defaults {
     my $defaults = {
 	map {
 	    my $default = $confdesc->{$_}->{default};
-	     defined($default) ? ($_ => $default) : ()
+	    defined($default) ? ($_ => $default) : ()
 	} keys %$confdesc_for_defaults
     };
     $parse_prune_backups_maxfiles->($defaults, "defaults in VZDump schema");
@@ -274,26 +300,22 @@ sub read_vzdump_defaults {
 
     my $raw;
     eval { $raw = PVE::Tools::file_get_contents($fn); };
-    return $defaults if $@;
 
-    my $conf_schema = { type => 'object', properties => $confdesc_for_defaults };
-    my $res = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
-    if (my $excludes = $res->{'exclude-path'}) {
-	$res->{'exclude-path'} = PVE::Tools::split_args($excludes);
-    }
-    if (defined($res->{mailto})) {
-	my @mailto = split_list($res->{mailto});
-	$res->{mailto} = [ @mailto ];
+    my $res = parse_vzdump_conf($fn, $raw);
+
+    # Merge in cluster-wide config
+    my $cluster_conf = cfs_read_file("vzdump.conf");
+    foreach my $key (keys %$cluster_conf) {
+	$res->{$key} = $cluster_conf->{$key} if !defined($res->{$key});
     }
-    $parse_prune_backups_maxfiles->($res, "options in '$fn'");
-    parse_performance($res);
 
+    # Merge in defaults
     foreach my $key (keys %$defaults) {
 	$res->{$key} = $defaults->{$key} if !defined($res->{$key});
     }
 
     if (defined($res->{storage}) && defined($res->{dumpdir})) {
-	debugmsg('warn', "both 'storage' and 'dumpdir' defined in '$fn' - ignoring 'dumpdir'");
+	debugmsg('warn', "both 'storage' and 'dumpdir' defined in config - ignoring 'dumpdir'");
 	delete $res->{dumpdir};
     }
 
-- 
2.30.2

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

[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;
 
-    PVE::Tools::file_set_contents($filename, $vzdump_cron_dummy)
-	if ! -f $filename;
+    PVE::Tools::file_set_contents($conf, $vzdump_conf_dummy)
+	if ! -f $conf;
 
     gen_pve_vzdump_symlink();
 };
diff --git a/data/src/status.c b/data/src/status.c
index 5e1e841..9e520a5 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 = "vzdump.conf" },
 	{ .path = "jobs.cfg" },
 	{ .path = "ha/crm_commands" },
 	{ .path = "ha/manager_status" },
-- 
2.30.2

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

[Thomas Lamprecht]

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.

[pve-devel] [PATCH docs] vzdump: document the new cluster-wide config file

[Leo Nunner]

+ change the wording from "Global configuration" to "Node-wide
configuration"

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
The wording was previously already changed by Fiona but hasn't been
merged yet:

https://lists.proxmox.com/pipermail/pve-devel/2022-December/055123.html

 vzdump.adoc | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/vzdump.adoc b/vzdump.adoc
index ce46529..253f264 100644
--- a/vzdump.adoc
+++ b/vzdump.adoc
@@ -473,13 +473,17 @@ entire process happens transparently from a user's point of view.
 Configuration
 -------------
 
-Global configuration is stored in `/etc/vzdump.conf`. The file uses a
-simple colon separated key/value format. Each line has the following
-format:
+Node-wide configuration is stored in `/etc/vzdump.conf`. A cluster-wide
+configuration can be found at `/etc/pve/vzdump.conf`. Note that this is merely
+a fallback, and that the node-wide configuration always has higher precedence.
+Options that are not set in the node-wide configuration will be taken from
+the cluster configuration, and, if not set there either, will take the default
+value. The files use a simple colon separated key/value format. Each line
+has the following format:
 
  OPTION: value
 
-Blank lines in the file are ignored, and lines starting with a `#`
+Blank lines in the files are ignored, and lines starting with a `#`
 character are treated as comments and are also ignored. Values from
 this file are used as default, and can be overwritten on the command
 line.
-- 
2.30.2

Re: [pve-devel] [PATCH docs] vzdump: document the new cluster-wide config file

[Thomas Lamprecht]

On 09/02/2023 10:27, Leo Nunner wrote:
> + change the wording from "Global configuration" to "Node-wide
> configuration"
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
> The wording was previously already changed by Fiona but hasn't been
> merged yet:
> 
> https://lists.proxmox.com/pipermail/pve-devel/2022-December/055123.html
> 
>  vzdump.adoc | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/vzdump.adoc b/vzdump.adoc
> index ce46529..253f264 100644
> --- a/vzdump.adoc
> +++ b/vzdump.adoc
> @@ -473,13 +473,17 @@ entire process happens transparently from a user's point of view.
>  Configuration
>  -------------
>  
> -Global configuration is stored in `/etc/vzdump.conf`. The file uses a
> -simple colon separated key/value format. Each line has the following
> -format:
> +Node-wide configuration is stored in `/etc/vzdump.conf`. A cluster-wide
> +configuration can be found at `/etc/pve/vzdump.conf`. Note that this is merely
> +a fallback, and that the node-wide configuration always has higher precedence.

but node-specific configs do not take precedence over CLI? This is worded a bit
confusing, maybe go for something more explicit as, i.e., something along the
lines of:

For each option the order of precendence is, Job or CLI option, if not defined then
local node-specific config, if not defined there then datacenter wide config and
if not defined there the default documented in the API schema.

> +Options that are not set in the node-wide configuration will be taken from
> +the cluster configuration, and, if not set there either, will take the default
> +value. The files use a simple colon separated key/value format. Each line
> +has the following format:
>  
>   OPTION: value
>  
> -Blank lines in the file are ignored, and lines starting with a `#`
> +Blank lines in the files are ignored, and lines starting with a `#`
>  character are treated as comments and are also ignored. Values from
>  this file are used as default, and can be overwritten on the command
>  line.