[pve-devel] [PATCH-SERIES v2 manager/storage] Some breaking API changes/cleanups

[pve-devel] [PATCH-SERIES v2 manager/storage] Some breaking API changes/cleanups

[Fabian Ebner]

Improved version of the not-yet applied patches and follow-ups, regarding backup
retention and CIFS credentials, and path+RFC for CephFS at the end.


Changes from v1:
    * improve CIFS credential renaming
    * mention that maxfiles is deprecated
    * add checks for pve6to7
    * add patch+RFC for CephFS


manager stable(+master):

Fabian Ebner (2):
  pve6to7: add checks for backup retention options
  pve6to7: add check for CIFS credentials

 PVE/CLI/pve6to7.pm | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)


manager master:

Fabian Ebner (1):
  ui: storage: update hint about prune-backups default

 www/manager6/storage/Base.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)


storage master:

Fabian Ebner (5):
  postinst: move cifs credential files into subdirectory upon update
  config: mention that maxfiles is deprecated
  config: add backup content type to default local storage
  cephfs: revert safe-guard check for Luminous
  cephfs: update reminder for removal

 PVE/Storage.pm              |  2 +-
 PVE/Storage/CIFSPlugin.pm   |  3 ---
 PVE/Storage/CephFSPlugin.pm |  8 ++------
 PVE/Storage/Plugin.pm       | 14 +++++++++++---
 debian/postinst             | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 47 insertions(+), 13 deletions(-)
 create mode 100644 debian/postinst

-- 
2.30.2

[pve-devel] [PATCH v2 manager 1/3] pve6to7: add checks for backup retention options

[Fabian Ebner]

Note that it's not possible to use read_vzdump_defaults() and storage_config(),
because they auto-converts maxfiles already.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/CLI/pve6to7.pm | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index 0b6267d5..b8263dab 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -18,6 +18,7 @@ use PVE::RPCEnvironment;
 use PVE::Storage;
 use PVE::Tools qw(run_command);
 use PVE::QemuServer;
+use PVE::VZDump::Common;
 
 use Term::ANSIColor;
 
@@ -513,6 +514,66 @@ sub check_ceph {
     }
 }
 
+sub check_backup_retention_settings {
+    log_info("Checking backup retention settings..");
+
+    my $pass = 1;
+
+    my $node_has_retention;
+
+    my $maxfiles_msg = "parameter 'maxfiles' is deprecated with PVE 7.x and will be removed in a " .
+	"future version, use 'prune-backups' instead.";
+
+    eval {
+	my $confdesc = PVE::VZDump::Common::get_confdesc();
+
+	my $fn = "/etc/vzdump.conf";
+	my $raw = PVE::Tools::file_get_contents($fn);
+
+	my $conf_schema = { type => 'object', properties => $confdesc, };
+	my $param = PVE::JSONSchema::parse_config($conf_schema, $fn, $raw);
+
+	if (defined($param->{maxfiles})) {
+	    $pass = 0;
+	    log_warn("$fn - $maxfiles_msg");
+	}
+
+	$node_has_retention = defined($param->{maxfiles}) || defined($param->{'prune-backups'});
+    };
+    if (my $err = $@) {
+	$pass = 0;
+	log_warn("unable to parse node's VZDump configuration - $err");
+    }
+
+    my $storage_cfg = PVE::Storage::config();
+
+    for my $storeid (keys $storage_cfg->{ids}->%*) {
+	my $scfg = $storage_cfg->{ids}->{$storeid};
+
+	if (defined($scfg->{maxfiles})) {
+	    $pass = 0;
+	    log_warn("storage '$storeid' - $maxfiles_msg");
+	}
+
+	next if !$scfg->{content}->{backup};
+	next if defined($scfg->{maxfiles}) || defined($scfg->{'prune-backups'});
+	next if $node_has_retention;
+
+	log_info("storage '$storeid' - no backup retention settings defined - by default, PVE " .
+	    "7.x will no longer keep only the last backup, but all backups");
+    }
+
+    my $vzdump_cron = PVE::Cluster::cfs_read_file('vzdump.cron');
+
+    # only warn once, there might be many jobs...
+    if (scalar(grep { defined($_->{maxfiles}) } $vzdump_cron->{jobs}->@*)) {
+	$pass = 0;
+	log_warn("/etc/pve/vzdump_cron - $maxfiles_msg");
+    }
+
+    log_pass("no problems found.") if $pass;
+}
+
 sub check_misc {
     print_header("MISCELLANEOUS CHECKS");
     my $ssh_config = eval { PVE::Tools::file_get_contents('/root/.ssh/config') };
@@ -602,6 +663,8 @@ sub check_misc {
 	    log_pass("Certificate '$fn' passed Debian Busters security level for TLS connections ($size >= 2048)");
 	}
     }
+
+    check_backup_retention_settings();
 }
 
 __PACKAGE__->register_method ({
-- 
2.20.1

[pve-devel] [PATCH v2 manager 2/3] pve6to7: add check for CIFS credentials

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/CLI/pve6to7.pm | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/PVE/CLI/pve6to7.pm b/PVE/CLI/pve6to7.pm
index b8263dab..f486ac2d 100644
--- a/PVE/CLI/pve6to7.pm
+++ b/PVE/CLI/pve6to7.pm
@@ -574,6 +574,27 @@ sub check_backup_retention_settings {
     log_pass("no problems found.") if $pass;
 }
 
+sub check_cifs_credential_location {
+    log_info("checking CIFS credential location..");
+
+    my $regex = qr/^(.*)\.cred$/;
+
+    my $found;
+
+    PVE::Tools::dir_glob_foreach('/etc/pve/priv/', $regex, sub {
+	my ($filename) = @_;
+
+	my ($basename) = $filename =~ $regex;
+
+	log_warn("CIFS credentials '/etc/pve/priv/$filename' will be moved to " .
+	    "'/etc/pve/priv/storage/$basename.pw' during the update");
+
+	$found = 1;
+    });
+
+    log_pass("no CIFS credentials at outdated location found.") if !$found;
+}
+
 sub check_misc {
     print_header("MISCELLANEOUS CHECKS");
     my $ssh_config = eval { PVE::Tools::file_get_contents('/root/.ssh/config') };
@@ -665,6 +686,7 @@ sub check_misc {
     }
 
     check_backup_retention_settings();
+    check_cifs_credential_location();
 }
 
 __PACKAGE__->register_method ({
-- 
2.20.1

[pve-devel] [PATCH v2 manager 3/3] ui: storage: update hint about prune-backups default

[Fabian Ebner]

and also remove the keepLastEmptyText, which too suggested the old default.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 www/manager6/storage/Base.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js
index 20ec3464..ee8b54e8 100644
--- a/www/manager6/storage/Base.js
+++ b/www/manager6/storage/Base.js
@@ -62,8 +62,6 @@ Ext.define('PVE.panel.StoragePruneInputPanel', {
 
     onlineHelp: 'vzdump_retention',
 
-    keepLastEmptyText: gettext('1'),
-
     onGetValues: function(formValues) {
 	if (this.needMask) { // isMasked() may not yet be true if not rendered once
 	    return {};
@@ -138,7 +136,7 @@ Ext.define('PVE.panel.StoragePruneInputPanel', {
 	    name: 'no-keeps-hint',
 	    hidden: true,
 	    padding: '5 1',
-	    html: gettext('Without any keep option, the nodes vzdump.conf or `keep-last 1` is used as fallback for backup jobs'),
+	    html: gettext('Without any keep option, the node\'s vzdump.conf or `keep-all` is used as fallback for backup jobs'),
 	},
 	{
 	    xtype: 'component',
-- 
2.30.2

[pve-devel] [PATCH v2 storage 1/5] postinst: move cifs credential files into subdirectory upon update

[Fabian Ebner]

and drop the compat code.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * make sure to create the /etc/pve/priv/storage directory which might not
      exist yet
    * always print info when a file was found
    * use variable for target path
    * use mv
    * removing the helper needs to wait until 8.0

 PVE/Storage/CIFSPlugin.pm |  3 ---
 debian/postinst           | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)
 create mode 100644 debian/postinst

diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index be06cc7..9d69b01 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -59,9 +59,6 @@ sub get_cred_file {
 
     if (-e $cred_file) {
 	return $cred_file;
-    } elsif (-e "/etc/pve/priv/${storeid}.cred") {
-	# FIXME: remove fallback with 7.0 by doing a rename on upgrade from 6.x
-	return "/etc/pve/priv/${storeid}.cred";
     }
     return undef;
 }
diff --git a/debian/postinst b/debian/postinst
new file mode 100644
index 0000000..508075b
--- /dev/null
+++ b/debian/postinst
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+set -e
+
+#DEBHELPER#
+
+case "$1" in
+  configure)
+    if test -n "$2"; then
+
+        # TODO: remove once PVE 8.0 is released
+        if dpkg --compare-versions "$2" 'lt' '7.0-3'; then
+            for file in /etc/pve/priv/*.cred; do
+                if [ -f "$file" ]; then
+                    mkdir -p "/etc/pve/priv/storage"
+                    echo "Info: found CIFS credentials using old path: $file" >&2
+                    base=$(basename --suffix=".cred" "$file")
+                    target="/etc/pve/priv/storage/$base.pw"
+                    if [ -f "$target" ]; then
+                        echo "Warning: not renaming $file, because $target already exists!" >&2
+                    else
+                        echo "Info: renaming $file to $target" >&2
+                        mv "$file" "$target"
+                    fi
+                fi
+            done
+        fi
+    fi
+    ;;
+
+esac
+
+exit 0
-- 
2.30.2

[pve-devel] [PATCH v2 storage 2/5] config: mention that maxfiles is deprecated

[Fabian Ebner]

Don't add an explicit deprecation warning on parsing (yet), this already done in
the pve6to7 script. Also, automatic conversion to 'prune-backups' happens when
the section config is read, so over time fewer users should be affected.
Postpone explicit warning/dropping the parameter to a future major release.

Also switch the setting for the default 'local' storage to 'prune-backups'.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * also update the description
    * also switch the default 'local' setting

 PVE/Storage.pm        | 2 +-
 PVE/Storage/Plugin.pm | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 3aa2100..e109c02 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -125,7 +125,7 @@ sub lock_storage_config {
     }
 }
 
-# FIXME remove maxfiles for PVE 7.0
+# FIXME remove maxfiles for PVE 8.0 or PVE 9.0
 my $convert_maxfiles_to_prune_backups = sub {
     my ($scfg) = @_;
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 318d13a..f0c15d5 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -133,7 +133,8 @@ my $defaultData = {
 	    optional => 1,
 	},
 	maxfiles => {
-	    description => "Maximal number of backup files per VM. Use '0' for unlimted.",
+	    description => "Deprecated: use 'prune-backups' instead. " .
+		"Maximal number of backup files per VM. Use '0' for unlimted.",
 	    type => 'integer',
 	    minimum => 0,
 	    optional => 1,
@@ -402,7 +403,7 @@ sub parse_config {
 	    type => 'dir',
 	    priority => 0, # force first entry
 	    path => '/var/lib/vz',
-	    maxfiles => 0,
+	    'prune-backups' => 'keep-all=1',
 	    content => { images => 1, rootdir => 1, vztmpl => 1, iso => 1, snippets => 1},
 	};
     }
-- 
2.30.2

[pve-devel] [PATCH v2 storage 3/5] config: add backup content type to default local storage

[Fabian Ebner]

which is used if there is no ('dir'-type) 'local' entry. Storage configurations
made by the installer also support backups for the 'local' storage, and the
'prune-backups' parameter is not really useful otherwise.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/Storage/Plugin.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index f0c15d5..948002e 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -404,7 +404,14 @@ sub parse_config {
 	    priority => 0, # force first entry
 	    path => '/var/lib/vz',
 	    'prune-backups' => 'keep-all=1',
-	    content => { images => 1, rootdir => 1, vztmpl => 1, iso => 1, snippets => 1},
+	    content => {
+		backup => 1,
+		images => 1,
+		iso => 1,
+		rootdir => 1,
+		snippets => 1,
+		vztmpl => 1,
+	    },
 	};
     }
 
-- 
2.30.2

[pve-devel] [PATCH v2 storage 4/5] cephfs: revert safe-guard check for Luminous

[Fabian Ebner]

It's necessary to be on Nautilus before upgrading to 7.x, so the check is no
longer needed. See commit e54c3e334760491954bc42f3585a8b5b136d4b1d. It didn't
cleanly revert, because there were cleanups made afterwards.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 PVE/Storage/CephFSPlugin.pm | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
index 480dc57..da64080 100644
--- a/PVE/Storage/CephFSPlugin.pm
+++ b/PVE/Storage/CephFSPlugin.pm
@@ -98,10 +98,7 @@ sub cephfs_mount {
     } else {
 	push @opts, "name=$cmd_option->{userid}";
 	push @opts, "secretfile=$secretfile" if defined($secretfile);
-
-	# FIXME: remove version check in PVE 7.0, only needed for Luminous -> Nautilus
-	my ($subversions) = PVE::CephConfig::local_ceph_version();
-	push @opts, "conf=$configfile" if defined($configfile) && @$subversions[0] > 12;
+	push @opts, "conf=$configfile" if defined($configfile);
     }
 
     push @opts, $scfg->{options} if $scfg->{options};
-- 
2.30.2

[pve-devel] [RFC v2 storage 5/5] cephfs: update reminder for systemd_netmount removal

[Fabian Ebner]

Commit d9ece228fbbbf0eb575c8a067a70ed7023078f84 introduced the workaround with
using systemd units and 25e222ca0d96a0da0d491e749b437d2822215e9e re-used the
functionality for fuse-mounts too.

The latter commit suggests to switch to using mount.fuse.ceph for the '_netdev'
option, but it doesn't seem to work:

 root@pve701 / # mount -t fuse.ceph 10.10.10.11,10.10.10.12,10.10.10.13:/ /mnttest/fuse -o 'ceph.id=admin,ceph.keyfile=/etc/pve/priv/ceph/cephfs.secret,ceph.conf=/etc/pve/ceph.conf,_netdev'
 ceph-fuse[20729]: starting ceph client
 2021-06-15T14:22:00.631+0200 7f995f878080 -1 init, newargv = 0x55e09fc11a40 newargc=11
 ceph-fuse[20729]: starting fuse
 root@pve701 / # mount -t ceph 10.10.10.11,10.10.10.12,10.10.10.13:/ /mnttest/normal -o 'name=admin,secretfile=/etc/pve/priv/ceph/cephfs.secret,conf=/etc/pve/ceph.conf,_netdev'
 root@pve701 / # mount | grep mnttest
 ceph-fuse on /mnttest/fuse type fuse.ceph-fuse (rw,nosuid,nodev,relatime,user_id=0,group_id=0,allow_other)
 10.10.10.11,10.10.10.12,10.10.10.13:/ on /mnttest/normal type ceph (rw,relatime,name=admin,secret=<hidden>,acl,_netdev)

Also, the return value is not propagated by mount.fuse.ceph, meaning the output
would need to be parsed...

 root@pve701 ~ # mount -t fuse.ceph 10.10.10.11,10.10.10.12,10.10.10.13:/ /mnttest/fuse -o 'ceph.id=admin,ceph.keyfile=/etc/pve/priv/ceph/cephfs.secret,ceph.conf=/etc/pve/ceph.conf,_netdev'
 2021-06-15T14:42:56.326+0200 7f634edae080 -1 init, newargv = 0x560cdb5e0a40 newargc=11
 ceph-fuse[34480]: starting ceph client
 fuse: mountpoint is not empty
 fuse: if you are sure this is safe, use the 'nonempty' mount option
 ceph-fuse[34480]: fuse failed to start
 2021-06-15T14:42:56.338+0200 7f634edae080 -1
 fuse_mount(mountpoint=/mnttest/fuse) failed.
 Mount failed with status code: 5
 root@pve701 ~ # echo $?
 0

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

Hope I'm missing something and we can actually switch.

 PVE/Storage/CephFSPlugin.pm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
index da64080..f919ab8 100644
--- a/PVE/Storage/CephFSPlugin.pm
+++ b/PVE/Storage/CephFSPlugin.pm
@@ -38,8 +38,7 @@ sub cephfs_is_mounted {
     return undef;
 }
 
-# FIXME: remove in PVE 7.0 where systemd is recent enough to not have those
-#        local-fs/remote-fs dependency cycles generated for _netdev mounts...
+# FIXME: remove once it's possible to specify _netdev for fuse.ceph mounts
 sub systemd_netmount {
     my ($where, $type, $what, $opts) = @_;
 
-- 
2.30.2

[pve-devel] applied-series: [PATCH-SERIES v2 manager/storage] Some breaking API changes/cleanups

[Fabian Grünbichler]

with two follow-ups:
- pve-storage postinst: handle read-only postinst (warn, but don't fail 
  the upgrade)
- pve-manager: wrap vzdump.cron check in eval {} and warn on error

On June 16, 2021 9:26 am, Fabian Ebner wrote:
> Improved version of the not-yet applied patches and follow-ups, regarding backup
> retention and CIFS credentials, and path+RFC for CephFS at the end.
> 
> 
> Changes from v1:
>     * improve CIFS credential renaming
>     * mention that maxfiles is deprecated
>     * add checks for pve6to7
>     * add patch+RFC for CephFS
> 
> 
> manager stable(+master):
> 
> Fabian Ebner (2):
>   pve6to7: add checks for backup retention options
>   pve6to7: add check for CIFS credentials
> 
>  PVE/CLI/pve6to7.pm | 85 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> 
> manager master:
> 
> Fabian Ebner (1):
>   ui: storage: update hint about prune-backups default
> 
>  www/manager6/storage/Base.js | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> 
> storage master:
> 
> Fabian Ebner (5):
>   postinst: move cifs credential files into subdirectory upon update
>   config: mention that maxfiles is deprecated
>   config: add backup content type to default local storage
>   cephfs: revert safe-guard check for Luminous
>   cephfs: update reminder for removal
> 
>  PVE/Storage.pm              |  2 +-
>  PVE/Storage/CIFSPlugin.pm   |  3 ---
>  PVE/Storage/CephFSPlugin.pm |  8 ++------
>  PVE/Storage/Plugin.pm       | 14 +++++++++++---
>  debian/postinst             | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 47 insertions(+), 13 deletions(-)
>  create mode 100644 debian/postinst
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH v2 storage 1/5] postinst: move cifs credential files into subdirectory upon update

[Thomas Lamprecht]

On 16.06.21 09:26, Fabian Ebner wrote:
> +case "$1" in
> +  configure)
> +    if test -n "$2"; then
> +
> +        # TODO: remove once PVE 8.0 is released
> +        if dpkg --compare-versions "$2" 'lt' '7.0-3'; then
> +            for file in /etc/pve/priv/*.cred; do
> +                if [ -f "$file" ]; then
> +                    mkdir -p "/etc/pve/priv/storage"
> +                    echo "Info: found CIFS credentials using old path: $file" >&2
> +                    base=$(basename --suffix=".cred" "$file")
> +                    target="/etc/pve/priv/storage/$base.pw"
> +                    if [ -f "$target" ]; then
> +                        echo "Warning: not renaming $file, because $target already exists!" >&2

can we diff in this case and remove the old one if the files are identical?

> +                    else
> +                        echo "Info: renaming $file to $target" >&2
> +                        mv "$file" "$target"
> +                    fi
> +                fi
> +            done
> +        fi
> +    fi
> +    ;;
> +
> +esac
> +
> +exit 0
> 

Re: [pve-devel] [PATCH v2 storage 1/5] postinst: move cifs credential files into subdirectory upon update

[Fabian Ebner]

Am 16.06.21 um 15:11 schrieb Thomas Lamprecht:
> On 16.06.21 09:26, Fabian Ebner wrote:
>> +case "$1" in
>> +  configure)
>> +    if test -n "$2"; then
>> +
>> +        # TODO: remove once PVE 8.0 is released
>> +        if dpkg --compare-versions "$2" 'lt' '7.0-3'; then
>> +            for file in /etc/pve/priv/*.cred; do
>> +                if [ -f "$file" ]; then
>> +                    mkdir -p "/etc/pve/priv/storage"
>> +                    echo "Info: found CIFS credentials using old path: $file" >&2
>> +                    base=$(basename --suffix=".cred" "$file")
>> +                    target="/etc/pve/priv/storage/$base.pw"
>> +                    if [ -f "$target" ]; then
>> +                        echo "Warning: not renaming $file, because $target already exists!" >&2
> 
> can we diff in this case and remove the old one if the files are identical?
> 

Sure, I'll send a follow-up.

>> +                    else
>> +                        echo "Info: renaming $file to $target" >&2
>> +                        mv "$file" "$target"
>> +                    fi
>> +                fi
>> +            done
>> +        fi
>> +    fi
>> +    ;;
>> +
>> +esac
>> +
>> +exit 0
>>
>