[pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[Fabian Ebner]

The series consists of two parts:

First part (up to docs 2/2) is for introducing a storage property
to limit the number of protected backups and only count unprotected
backups for the limit check in vzdump.

Second part is a convenience feature allowing 'protected' and 'notes'
to be set directly with vzdump and for manual backups in the UI.


storage:

Fabian Ebner (2):
  list volumes: also return backup type for backups
  plugins: allow limiting the number of protected backups per guest

 PVE/Storage.pm                 | 24 ++++++++++++++++++++++++
 PVE/Storage/BTRFSPlugin.pm     |  3 ++-
 PVE/Storage/CIFSPlugin.pm      |  1 +
 PVE/Storage/CephFSPlugin.pm    |  1 +
 PVE/Storage/DirPlugin.pm       |  1 +
 PVE/Storage/GlusterfsPlugin.pm |  1 +
 PVE/Storage/NFSPlugin.pm       |  1 +
 PVE/Storage/PBSPlugin.pm       |  2 ++
 PVE/Storage/Plugin.pm          |  8 ++++++++
 test/list_volumes_test.pm      | 10 ++++++++++
 10 files changed, 51 insertions(+), 1 deletion(-)


manager:

Fabian Ebner (6):
  vzdump: backup file list: drop unused parameter
  vzdump: backup limit: only count unprotected backups
  ui: storage edit: retention: add max-protected-backups setting
  ui: storage creation: retention: dynamically adapt
    max-protected-backups
  vzdump: support setting protected status and notes
  ui: backup: allow setting protected and notes for manual backup

 PVE/VZDump.pm                        | 69 ++++++++++++++++++---------
 www/manager6/panel/BackupJobPrune.js | 71 ++++++++++++++++++++++++----
 www/manager6/storage/Base.js         |  1 +
 www/manager6/window/Backup.js        | 25 +++++++++-
 4 files changed, 132 insertions(+), 34 deletions(-)


guest-common:

Fabian Ebner (1):
  vzdump: schema: add 'notes' and 'protected' properties

 src/PVE/VZDump/Common.pm | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)


docs:

Fabian Ebner (2):
  storage: switch to prune-backups in examples
  vzdump/storage: mention protected backups limit and give an example

 pve-storage-dir.adoc | 9 +++++----
 pve-storage-pbs.adoc | 2 +-
 vzdump.adoc          | 3 +++
 3 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH storage 1/2] list volumes: also return backup type for backups

[Fabian Ebner]

Otherwise, there is no storage-agnostic way to filter by backup group.

Call it subtype, to not confuse it with content type, and to be able
to re-use it for other content types than backup, if the need ever
arises.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/PBSPlugin.pm  |  1 +
 PVE/Storage/Plugin.pm     |  1 +
 test/list_volumes_test.pm | 10 ++++++++++
 3 files changed, 12 insertions(+)

diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index e186663..4b3f349 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -688,6 +688,7 @@ sub list_volumes {
 	    content => 'backup',
 	    vmid => int($bid),
 	    ctime => $epoch,
+	    subtype => $btype eq 'vm' ? 'qemu' : 'lxc', # convert to PVE backup type
 	};
 
 	$info->{verification} = $item->{verification} if defined($item->{verification});
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 12f1b4b..0a6c619 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1160,6 +1160,7 @@ my $get_subdir_files = sub {
 	    my $archive_info = eval { PVE::Storage::archive_info($fn) } // {};
 
 	    $info->{ctime} = $archive_info->{ctime} if defined($archive_info->{ctime});
+	    $info->{subtype} = $archive_info->{type} // 'unknown';
 
 	    if (defined($vmid) || $fn =~ m!\-([1-9][0-9]{2,8})\-[^/]+\.${format}$!) {
 		$info->{vmid} = $vmid // $1;
diff --git a/test/list_volumes_test.pm b/test/list_volumes_test.pm
index ac75c04..d155cb9 100644
--- a/test/list_volumes_test.pm
+++ b/test/list_volumes_test.pm
@@ -133,6 +133,7 @@ my @tests = (
 		'ctime'   => 1585602700,
 		'format'  => 'vma.gz',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'qemu',
 		'vmid'    => '16110',
 		'volid'   => 'local:backup/vzdump-qemu-16110-2020_03_30-21_11_40.vma.gz',
 	    },
@@ -141,6 +142,7 @@ my @tests = (
 		'ctime'   => 1585602765,
 		'format'  => 'vma.lzo',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'qemu',
 		'vmid'    => '16110',
 		'volid'   => 'local:backup/vzdump-qemu-16110-2020_03_30-21_12_45.vma.lzo',
 	    },
@@ -149,6 +151,7 @@ my @tests = (
 		'ctime'   => 1585602835,
 		'format'  => 'vma',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'qemu',
 		'vmid'    => '16110',
 		'volid'   => 'local:backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma',
 	    },
@@ -157,6 +160,7 @@ my @tests = (
 		'ctime'   => 1585602835,
 		'format'  => 'vma.zst',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'qemu',
 		'vmid'    => '16110',
 		'volid'   => 'local:backup/vzdump-qemu-16110-2020_03_30-21_13_55.vma.zst',
 	    },
@@ -202,6 +206,7 @@ my @tests = (
 		'ctime'   => 1585604370,
 		'format'  => 'tar.lzo',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'lxc',
 		'vmid'    => '16112',
 		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.lzo',
 	    },
@@ -210,6 +215,7 @@ my @tests = (
 		'ctime'   => 1585604970,
 		'format'  => 'tar.gz',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'lxc',
 		'vmid'    => '16112',
 		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz',
 	    },
@@ -218,6 +224,7 @@ my @tests = (
 		'ctime'   => 1585604970,
 		'format'  => 'tar.zst',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'lxc',
 		'vmid'    => '16112',
 		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst',
 	    },
@@ -226,6 +233,7 @@ my @tests = (
 		'ctime'   => 1585605570,
 		'format'  => 'tgz',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'lxc',
 		'vmid'    => '16112',
 		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz',
 	    },
@@ -368,6 +376,7 @@ my @tests = (
 		'ctime'   => 1580759863,
 		'format'  => 'tar.gz',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'lxc',
 		'vmid'    => '19253',
 		'volid'   => 'local:backup/vzdump-lxc-19253-2020_02_03-19_57_43.tar.gz',
 	    },
@@ -376,6 +385,7 @@ my @tests = (
 		'ctime'   => 1548098959,
 		'format'  => 'tar',
 		'size'    => DEFAULT_SIZE,
+		'subtype' => 'lxc',
 		'vmid'    => '19254',
 		'volid'   => 'local:backup/vzdump-lxc-19254-2019_01_21-19_29_19.tar',
 	    },
-- 
2.30.2

[pve-devel] applied: [PATCH storage 1/2] list volumes: also return backup type for backups

[Thomas Lamprecht]

On 16.12.21 13:12, Fabian Ebner wrote:
> Otherwise, there is no storage-agnostic way to filter by backup group.
> 
> Call it subtype, to not confuse it with content type, and to be able
> to re-use it for other content types than backup, if the need ever
> arises.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage/PBSPlugin.pm  |  1 +
>  PVE/Storage/Plugin.pm     |  1 +
>  test/list_volumes_test.pm | 10 ++++++++++
>  3 files changed, 12 insertions(+)
> 
>

applied, thanks!

[pve-devel] [PATCH storage 2/2] plugins: allow limiting the number of protected backups per guest

[Fabian Ebner]

The ability to mark backups as protected broke the implicit assumption
in vzdump that remove=1 and current number of backups being the limit
(i.e. sum of all keep options) will result in a backup being removed.

Introduce a new storage property 'max-protected-backups' to limit the
number of protected backups per guest. Use 5 as a default value, as it
should cover most use cases, while still not having too big of a
potential overhead in many scenarios.

For external plugins that do not return the backup subtype in
list_volumes, all protected backups with the same ID will count
towards the limit.

An alternative would be to count the protected backups when pruning.
While that would avoid the need for a new property, it would break the
current semantics of protected backups being ignored for pruning. It
also would be less flexible, e.g. for PBS, it can make sense to have
both keep-all=1 and a limit for the number of protected snapshots on
the PVE side.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage.pm                 | 24 ++++++++++++++++++++++++
 PVE/Storage/BTRFSPlugin.pm     |  3 ++-
 PVE/Storage/CIFSPlugin.pm      |  1 +
 PVE/Storage/CephFSPlugin.pm    |  1 +
 PVE/Storage/DirPlugin.pm       |  1 +
 PVE/Storage/GlusterfsPlugin.pm |  1 +
 PVE/Storage/NFSPlugin.pm       |  1 +
 PVE/Storage/PBSPlugin.pm       |  1 +
 PVE/Storage/Plugin.pm          |  7 +++++++
 9 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index d64019f..0643fad 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -232,6 +232,30 @@ sub update_volume_attribute {
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
+    my ($vtype, undef, $vmid) = $plugin->parse_volname($volname);
+    my $max_protected_backups = $scfg->{'max-protected-backups'} // 5;
+
+    if (
+	$vtype eq 'backup'
+	&& $vmid
+	&& $attribute eq 'protected'
+	&& $value
+	&& !$plugin->get_volume_attribute($scfg, $storeid, $volname, 'protected')
+	&& $max_protected_backups > -1 # -1 is unlimited
+    ) {
+	my $backups = $plugin->list_volumes($storeid, $scfg, $vmid, ['backup']);
+	my ($backup_type) = map { $_->{subtype} } grep { $_->{volid} eq $volid } $backups->@*;
+
+	my $protected_count = grep {
+	    $_->{protected} && (!$backup_type || ($_->{subtype} && $_->{subtype} eq $backup_type))
+	} $backups->@*;
+
+	if ($max_protected_backups <= $protected_count) {
+	    die "The number of protected backups per guest is limited to $max_protected_backups ".
+		"on storage '$storeid'\n";
+	}
+    }
+
     return $plugin->update_volume_attribute($scfg, $storeid, $volname, $attribute, $value);
 }
 
diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
index c8caa41..7dac34b 100644
--- a/PVE/Storage/BTRFSPlugin.pm
+++ b/PVE/Storage/BTRFSPlugin.pm
@@ -67,7 +67,8 @@ sub options {
 	shared => { optional => 1 },
 	disable => { optional => 1 },
 	maxfiles => { optional => 1 },
-	'prune-backups'=> { optional => 1 },
+	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	content => { optional => 1 },
 	format => { optional => 1 },
 	is_mountpoint => { optional => 1 },
diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index a3f9ebe..ac36d14 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -134,6 +134,7 @@ sub options {
 	disable => { optional => 1 },
 	maxfiles => { optional => 1 },
 	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	content => { optional => 1 },
 	format => { optional => 1 },
 	username => { optional => 1 },
diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
index f75c1b8..4976747 100644
--- a/PVE/Storage/CephFSPlugin.pm
+++ b/PVE/Storage/CephFSPlugin.pm
@@ -155,6 +155,7 @@ sub options {
 	maxfiles => { optional => 1 },
 	keyring => { optional => 1 },
 	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	'fs-name' => { optional => 1 },
     };
 }
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index c60818b..1baad63 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -58,6 +58,7 @@ sub options {
 	disable => { optional => 1 },
 	maxfiles => { optional => 1 },
 	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	content => { optional => 1 },
 	format => { optional => 1 },
 	mkdir => { optional => 1 },
diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
index d8d2b88..ad386d2 100644
--- a/PVE/Storage/GlusterfsPlugin.pm
+++ b/PVE/Storage/GlusterfsPlugin.pm
@@ -133,6 +133,7 @@ sub options {
 	disable => { optional => 1 },
 	maxfiles => { optional => 1 },
 	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	content => { optional => 1 },
 	format => { optional => 1 },
 	mkdir => { optional => 1 },
diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
index 0400c93..5bd7313 100644
--- a/PVE/Storage/NFSPlugin.pm
+++ b/PVE/Storage/NFSPlugin.pm
@@ -85,6 +85,7 @@ sub options {
 	disable => { optional => 1 },
 	maxfiles => { optional => 1 },
 	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	options => { optional => 1 },
 	content => { optional => 1 },
 	format => { optional => 1 },
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index 4b3f349..687901f 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -73,6 +73,7 @@ sub options {
 	'master-pubkey' => { optional => 1 },
 	maxfiles => { optional => 1 },
 	'prune-backups' => { optional => 1 },
+	'max-protected-backups' => { optional => 1 },
 	fingerprint => { optional => 1 },
     };
 }
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 0a6c619..bbe0af4 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -155,6 +155,13 @@ my $defaultData = {
 	    optional => 1,
 	},
 	'prune-backups' => get_standard_option('prune-backups'),
+	'max-protected-backups' => {
+	    description => "Maximal number of protected backups per guest. Use '-1' for unlimited.",
+	    type => 'integer',
+	    minimum => -1,
+	    optional => 1,
+	    default => 5,
+	},
 	shared => {
 	    description => "Mark storage as shared.",
 	    type => 'boolean',
-- 
2.30.2

Re: [pve-devel] [PATCH storage 2/2] plugins: allow limiting the number of protected backups per guest

[Thomas Lamprecht]

On 16.12.21 13:12, Fabian Ebner wrote:
> The ability to mark backups as protected broke the implicit assumption
> in vzdump that remove=1 and current number of backups being the limit
> (i.e. sum of all keep options) will result in a backup being removed.
> 
> Introduce a new storage property 'max-protected-backups' to limit the
> number of protected backups per guest. Use 5 as a default value, as it
> should cover most use cases, while still not having too big of a
> potential overhead in many scenarios.
> 
> For external plugins that do not return the backup subtype in
> list_volumes, all protected backups with the same ID will count
> towards the limit.
> 
> An alternative would be to count the protected backups when pruning.
> While that would avoid the need for a new property, it would break the
> current semantics of protected backups being ignored for pruning. It
> also would be less flexible, e.g. for PBS, it can make sense to have
> both keep-all=1 and a limit for the number of protected snapshots on
> the PVE side.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage.pm                 | 24 ++++++++++++++++++++++++
>  PVE/Storage/BTRFSPlugin.pm     |  3 ++-
>  PVE/Storage/CIFSPlugin.pm      |  1 +
>  PVE/Storage/CephFSPlugin.pm    |  1 +
>  PVE/Storage/DirPlugin.pm       |  1 +
>  PVE/Storage/GlusterfsPlugin.pm |  1 +
>  PVE/Storage/NFSPlugin.pm       |  1 +
>  PVE/Storage/PBSPlugin.pm       |  1 +
>  PVE/Storage/Plugin.pm          |  7 +++++++
>  9 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index d64019f..0643fad 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -232,6 +232,30 @@ sub update_volume_attribute {
>      my $scfg = storage_config($cfg, $storeid);
>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
> +    my ($vtype, undef, $vmid) = $plugin->parse_volname($volname);
> +    my $max_protected_backups = $scfg->{'max-protected-backups'} // 5;

maybe the default limit should be user privilege dependent? E.g., for root and users
with .Allocate on the storage it wouldn't be a problem to have unlimited (or a higher
count) as default? I mean, it's naturally a bit odd to differ, but one can argue a lot
with auto-magic-convenience ;P

> +
> +    if (
> +	$vtype eq 'backup'
> +	&& $vmid
> +	&& $attribute eq 'protected'
> +	&& $value
> +	&& !$plugin->get_volume_attribute($scfg, $storeid, $volname, 'protected')
> +	&& $max_protected_backups > -1 # -1 is unlimited
> +    ) {
> +	my $backups = $plugin->list_volumes($storeid, $scfg, $vmid, ['backup']);
> +	my ($backup_type) = map { $_->{subtype} } grep { $_->{volid} eq $volid } $backups->@*;
> +
> +	my $protected_count = grep {
> +	    $_->{protected} && (!$backup_type || ($_->{subtype} && $_->{subtype} eq $backup_type))
> +	} $backups->@*;
> +
> +	if ($max_protected_backups <= $protected_count) {
> +	    die "The number of protected backups per guest is limited to $max_protected_backups ".
> +		"on storage '$storeid'\n";
> +	}
> +    }
> +
>      return $plugin->update_volume_attribute($scfg, $storeid, $volname, $attribute, $value);
>  }

Re: [pve-devel] [PATCH storage 2/2] plugins: allow limiting the number of protected backups per guest

[Fabian Ebner]

Am 16.03.22 um 17:42 schrieb Thomas Lamprecht:
> On 16.12.21 13:12, Fabian Ebner wrote:
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index d64019f..0643fad 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -232,6 +232,30 @@ sub update_volume_attribute {
>>      my $scfg = storage_config($cfg, $storeid);
>>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>  
>> +    my ($vtype, undef, $vmid) = $plugin->parse_volname($volname);
>> +    my $max_protected_backups = $scfg->{'max-protected-backups'} // 5;
> 
> maybe the default limit should be user privilege dependent? E.g., for root and users
> with .Allocate on the storage it wouldn't be a problem to have unlimited (or a higher
> count) as default? I mean, it's naturally a bit odd to differ, but one can argue a lot
> with auto-magic-convenience ;P
> 

Would add another dimension to the complexity ;) Also feels a bit
awkward code-wise at a first glance, as we'd need to get the rpcenv/user
in such a sub (or pass the user or pre-computed default in somehow). But
if you really want to, I can give it a go for v2.

Re: [pve-devel] [PATCH storage 2/2] plugins: allow limiting the number of protected backups per guest

[Thomas Lamprecht]

On 17.03.22 09:03, Fabian Ebner wrote:
> Am 16.03.22 um 17:42 schrieb Thomas Lamprecht:
>> On 16.12.21 13:12, Fabian Ebner wrote:
>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>> index d64019f..0643fad 100755
>>> --- a/PVE/Storage.pm
>>> +++ b/PVE/Storage.pm
>>> @@ -232,6 +232,30 @@ sub update_volume_attribute {
>>>      my $scfg = storage_config($cfg, $storeid);
>>>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>  
>>> +    my ($vtype, undef, $vmid) = $plugin->parse_volname($volname);
>>> +    my $max_protected_backups = $scfg->{'max-protected-backups'} // 5;
>>
>> maybe the default limit should be user privilege dependent? E.g., for root and users
>> with .Allocate on the storage it wouldn't be a problem to have unlimited (or a higher
>> count) as default? I mean, it's naturally a bit odd to differ, but one can argue a lot
>> with auto-magic-convenience ;P
>>
> 
> Would add another dimension to the complexity ;) Also feels a bit

sure, but actually avoids bothering with that setting in the first place for most
simple setups, so could be a net benefit.

> awkward code-wise at a first glance, as we'd need to get the rpcenv/user
> in such a sub (or pass the user or pre-computed default in somehow). But
> if you really want to, I can give it a go for v2.

hmm, yeah.. iff, it should probably happen in a separate get_protected_limit($scfg)
helper and we get the rpcenv singleton in other, rather deep, code paths too, so
not a big issue IMO, but not to hard feelings here.

[pve-devel] [PATCH manager 1/6] vzdump: backup file list: drop unused parameter

[Fabian Ebner]

It's not used anymore since b5a3ab3d20f3b62f615ea09341a2c83fc3d243a2
aligned pruning in a dumpdir with pruning on a storage.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index b5a5fadd..daaaf0e3 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -685,12 +685,10 @@ sub compressor_info {
 }
 
 sub get_backup_file_list {
-    my ($dir, $bkname, $exclude_fn) = @_;
+    my ($dir, $bkname) = @_;
 
     my $bklist = [];
     foreach my $fn (<$dir/${bkname}-*>) {
-	next if $exclude_fn && $fn eq $exclude_fn;
-
 	my $archive_info = eval { PVE::Storage::archive_info($fn) } // {};
 	if ($archive_info->{is_std_name}) {
 	    my $filename = $archive_info->{filename};
-- 
2.30.2

[pve-devel] [PATCH manager 2/6] vzdump: backup limit: only count unprotected backups

[Fabian Ebner]

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 run_client_cmd
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>
---

Dependency bump for storage is needed for it to filter by backup type
and not just ID, and also since we'd like to have the limiting
behavior for protected there, before making the limit weaker here.

 PVE/VZDump.pm | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index daaaf0e3..8e20c320 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -684,16 +684,17 @@ sub compressor_info {
     }
 }
 
-sub get_backup_file_list {
+sub get_unprotected_backup_file_list {
     my ($dir, $bkname) = @_;
 
     my $bklist = [];
     foreach my $fn (<$dir/${bkname}-*>) {
 	my $archive_info = eval { PVE::Storage::archive_info($fn) } // {};
 	if ($archive_info->{is_std_name}) {
-	    my $filename = $archive_info->{filename};
+	    my $path = "$dir/$archive_info->{filename}";
+	    next if -e PVE::Storage::protection_file_path($path);
 	    my $backup = {
-		'path' => "$dir/$filename",
+		'path' => $path,
 		'ctime' => $archive_info->{ctime},
 	    };
 	    push @{$bklist}, $backup;
@@ -773,13 +774,15 @@ sub exec_backup_task {
 
 	if ($backup_limit && !$opts->{remove}) {
 	    my $count;
-	    if ($self->{opts}->{pbs}) {
-		my $res = PVE::Storage::PBSPlugin::run_client_cmd($opts->{scfg}, $opts->{storage}, 'snapshots', $pbs_group_name);
-		$count = scalar(@$res);
+	    if (my $storeid = $opts->{storage}) {
+		my $backups = PVE::Storage::volume_list($cfg, $storeid, $vmid, 'backup');
+		$count = grep {
+		    !$_->{protected} && (!$_->{subtype} || $_->{subtype} eq $vmtype)
+		} $backups->@*;
 	    } else {
-		my $bklist = get_backup_file_list($opts->{dumpdir}, $bkname);
-		$count = scalar(@$bklist);
+		$count = scalar(get_unprotected_backup_file_list($opts->{dumpdir}, $bkname)->@*);
 	    }
+
 	    die "There is a max backup limit of $backup_limit enforced by the".
 		" target storage or the vzdump parameters.".
 		" Either increase the limit or delete old backup(s).\n"
@@ -989,13 +992,7 @@ sub exec_backup_task {
 	    debugmsg ('info', "prune older backups with retention: $keepstr", $logfd);
 	    my $pruned = 0;
 	    if (!defined($opts->{storage})) {
-		my $bklist = get_backup_file_list($opts->{dumpdir}, $bkname);
-
-		for my $prune_entry ($bklist->@*) {
-		    if (-e PVE::Storage::protection_file_path($prune_entry->{path})) {
-			$prune_entry->{mark} = 'protected';
-		    }
-		}
+		my $bklist = get_unprotected_backup_file_list($opts->{dumpdir}, $bkname);
 
 		PVE::Storage::prune_mark_backup_group($bklist, $prune_options);
 
-- 
2.30.2

[pve-devel] [RFC manager 3/6] ui: storage edit: retention: add max-protected-backups setting

[Fabian Ebner]

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

Not sure if it's worth exposing this in the UI at all.

 www/manager6/panel/BackupJobPrune.js | 47 ++++++++++++++++++++++------
 www/manager6/storage/Base.js         |  1 +
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/www/manager6/panel/BackupJobPrune.js b/www/manager6/panel/BackupJobPrune.js
index a58fe1fe..555bccf2 100644
--- a/www/manager6/panel/BackupJobPrune.js
+++ b/www/manager6/panel/BackupJobPrune.js
@@ -15,21 +15,32 @@ Ext.define('PVE.panel.BackupJobPrune', {
 	} else if (this.isCreate && !this.rendered) {
 	    return this.keepAllDefaultForCreate ? { 'prune-backups': 'keep-all=1' } : {};
 	}
+
+	let options = { 'delete': [] };
+
+	if ('max-protected-backups' in formValues) {
+	    options['max-protected-backups'] = formValues['max-protected-backups'];
+	} else if (this.hasMaxProtected) {
+	    options.delete.push('max-protected-backups');
+	}
+
+	delete formValues['max-protected-backups'];
 	delete formValues.delete;
+
 	let retention = PVE.Parser.printPropertyString(formValues);
 	if (retention === '') {
-	    if (this.isCreate) {
-		return {};
-	    }
-	    // always delete old 'maxfiles' on edit, we map it to keep-last on window load
-	    return {
-		'delete': ['prune-backups', 'maxfiles'],
-	    };
+	    options.delete.push('prune-backups');
+	} else {
+	    options['prune-backups'] = retention;
 	}
-	let options = { 'prune-backups': retention };
+
 	if (!this.isCreate) {
-	    options.delete = 'maxfiles';
+	    // always delete old 'maxfiles' on edit, we map it to keep-last on window load
+	    options.delete.push('maxfiles');
+	} else {
+	    delete options.delete;
 	}
+
 	return options;
     },
 
@@ -57,6 +68,10 @@ Ext.define('PVE.panel.BackupJobPrune', {
 	    }
 	    panel.down('component[name=pbs-hint]').setHidden(!panel.showPBSHint);
 
+	    let maxProtected = panel.down('proxmoxintegerfield[name=max-protected-backups]');
+	    maxProtected.setDisabled(!panel.hasMaxProtected);
+	    maxProtected.setHidden(!panel.hasMaxProtected);
+
 	    panel.query('pmxPruneKeepField').forEach(field => {
 		field.on('change', panel.updateComponents, panel);
 	    });
@@ -95,5 +110,19 @@ Ext.define('PVE.panel.BackupJobPrune', {
 	    padding: '5 1',
 	    html: gettext("It's preferred to configure backup retention directly on the Proxmox Backup Server."),
 	},
+	{
+	    xtype: 'proxmoxintegerfield',
+	    name: 'max-protected-backups',
+	    fieldLabel: gettext('Maximum Protected'),
+	    minValue: -1,
+	    hidden: true,
+	    disabled: true,
+	    emptyText: '5',
+	    deleteEmpty: true,
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': Ext.String.format(gettext('Use {0} for unlimited'), -1),
+	    },
+	},
     ],
 });
diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js
index 89dc2eff..7f6d7a09 100644
--- a/www/manager6/storage/Base.js
+++ b/www/manager6/storage/Base.js
@@ -120,6 +120,7 @@ Ext.define('PVE.storage.BaseEdit', {
 		    {
 			xtype: 'pveBackupJobPrunePanel',
 			title: gettext('Backup Retention'),
+			hasMaxProtected: true,
 			isCreate: me.isCreate,
 			keepAllDefaultForCreate: true,
 			showPBSHint: me.ipanel.isPBS,
-- 
2.30.2

[pve-devel] [RFC manager 4/6] ui: storage creation: retention: dynamically adapt max-protected-backups

[Fabian Ebner]

depending on the other retention settings and as long as it isn't
dirty (other than from the updating itself). Allow an unlimited number
when keep-all=1 and use the default fall-back to 5 when pruning or
fall-back for retention is configured.

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

Might be too much automagic, maybe just setting -1 in afterrender and
the never-rendered scenario is also enough? That is, if we even want
the GUI default for creation to be unlimited. Just thought that it
would better fit the keep-all default.

 www/manager6/panel/BackupJobPrune.js | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/www/manager6/panel/BackupJobPrune.js b/www/manager6/panel/BackupJobPrune.js
index 555bccf2..880f0234 100644
--- a/www/manager6/panel/BackupJobPrune.js
+++ b/www/manager6/panel/BackupJobPrune.js
@@ -13,7 +13,14 @@ Ext.define('PVE.panel.BackupJobPrune', {
 	if (this.needMask) { // isMasked() may not yet be true if not rendered once
 	    return {};
 	} else if (this.isCreate && !this.rendered) {
-	    return this.keepAllDefaultForCreate ? { 'prune-backups': 'keep-all=1' } : {};
+	    let options = {};
+	    if (this.keepAllDefaultForCreate) {
+		options['prune-backups'] = 'keep-all=1';
+		if (this.hasMaxProtected) {
+		    options['max-protected-backups'] = -1;
+		}
+	    }
+	    return options;
 	}
 
 	let options = { 'delete': [] };
@@ -54,6 +61,17 @@ Ext.define('PVE.panel.BackupJobPrune', {
 	    field.setDisabled(keepAll);
 	});
 	me.down('component[name=no-keeps-hint]').setHidden(anyValue || keepAll);
+
+	let maxProtected = me.down('proxmoxintegerfield[name=max-protected-backups]');
+	if (me.isCreate && me.hasMaxProtected && !maxProtected.isDirty()) {
+	    if (keepAll) {
+		maxProtected.setValue(-1);
+		maxProtected.resetOriginalValue();
+	    } else {
+		maxProtected.setValue('');
+		maxProtected.resetOriginalValue();
+	    }
+	}
     },
 
     listeners: {
@@ -71,6 +89,10 @@ Ext.define('PVE.panel.BackupJobPrune', {
 	    let maxProtected = panel.down('proxmoxintegerfield[name=max-protected-backups]');
 	    maxProtected.setDisabled(!panel.hasMaxProtected);
 	    maxProtected.setHidden(!panel.hasMaxProtected);
+	    if (panel.isCreate && panel.hasMaxProtected) {
+		maxProtected.setValue(-1);
+		maxProtected.resetOriginalValue();
+	    }
 
 	    panel.query('pmxPruneKeepField').forEach(field => {
 		field.on('change', panel.updateComponents, panel);
-- 
2.30.2

[pve-devel] [PATCH docs 1/2] storage: switch to prune-backups in examples

[Fabian Ebner]

because maxfiles is deprecated.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-storage-dir.adoc | 8 ++++----
 pve-storage-pbs.adoc | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/pve-storage-dir.adoc b/pve-storage-dir.adoc
index 090a44b..d1ca3eb 100644
--- a/pve-storage-dir.adoc
+++ b/pve-storage-dir.adoc
@@ -55,12 +55,12 @@ needs to be an absolute file system path.
 dir: backup
         path /mnt/backup
         content backup
-        maxfiles 7
+        prune-backups keep-last=7
 ----
 
-Above configuration defines a storage pool called `backup`. That pool
-can be used to store up to 7 backups (`maxfiles 7`) per VM. The real
-path for the backup files is `/mnt/backup/dump/...`.
+The above configuration defines a storage pool called `backup`. That pool can be
+used to store up to 7 backups (`keep-last=7`) per VM. The real path for the
+backup files is `/mnt/backup/dump/...`.
 
 
 File naming conventions
diff --git a/pve-storage-pbs.adoc b/pve-storage-pbs.adoc
index a3d7da1..0ec93de 100644
--- a/pve-storage-pbs.adoc
+++ b/pve-storage-pbs.adoc
@@ -71,7 +71,7 @@ pbs: backup
         server enya.proxmox.com
         content backup
         fingerprint 09:54:ef:..snip..:88:af:47:fe:4c:3b:cf:8b:26:88:0b:4e:3c:b2
-        maxfiles 0
+        prune-backups keep-all=1
         username archiver@pbs
 ----
 
-- 
2.30.2

[pve-devel] [PATCH docs 2/2] vzdump/storage: mention protected backups limit and give an example

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-storage-dir.adoc | 5 +++--
 vzdump.adoc          | 3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/pve-storage-dir.adoc b/pve-storage-dir.adoc
index d1ca3eb..4116fe5 100644
--- a/pve-storage-dir.adoc
+++ b/pve-storage-dir.adoc
@@ -56,11 +56,12 @@ dir: backup
         path /mnt/backup
         content backup
         prune-backups keep-last=7
+        max-protected-backups 3
 ----
 
 The above configuration defines a storage pool called `backup`. That pool can be
-used to store up to 7 backups (`keep-last=7`) per VM. The real path for the
-backup files is `/mnt/backup/dump/...`.
+used to store up to 7 regular backups (`keep-last=7`) and 3 protected backups
+per VM. The real path for the backup files is `/mnt/backup/dump/...`.
 
 
 File naming conventions
diff --git a/vzdump.adoc b/vzdump.adoc
index b9dca2a..71221e7 100644
--- a/vzdump.adoc
+++ b/vzdump.adoc
@@ -301,6 +301,9 @@ For filesystem-based storages, the protection is implemented via a sentinel file
 `<backup-name>.protected`. For Proxmox Backup Server, it is handled on the
 server side (available since Proxmox Backup Server version 2.1).
 
+Use the storage option `max-protected-backups` to control how many protected
+backups per guest are allowed on the storage. Use `-1` for unlimited.
+
 [[vzdump_restore]]
 Restore
 -------
-- 
2.30.2

[pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Fabian Ebner]

In command_line(), notes are printed, quoted, but otherwise as is,
which is a bit ugly for multi-line notes. But it is part of the
commandline.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/VZDump/Common.pm | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index 83d7413..5bb35ec 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -233,7 +233,19 @@ my $confdesc = {
 	type => 'string',
 	description => 'Backup all known guest systems included in the specified pool.',
 	optional => 1,
-    }
+    },
+    notes => {
+	type => 'string',
+	description => "Notes to add to the backup(s).",
+	requires => 'storage',
+	optional => 1,
+    },
+    protected => {
+	type => 'boolean',
+	description => "If true, mark backup(s) as protected.",
+	requires => 'storage',
+	optional => 1,
+    },
 };
 
 sub get_confdesc {
-- 
2.30.2

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Fabian Ebner]

Am 16.12.21 um 13:12 schrieb Fabian Ebner:
> In command_line(), notes are printed, quoted, but otherwise as is,
> which is a bit ugly for multi-line notes. But it is part of the
> commandline.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/VZDump/Common.pm | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
> index 83d7413..5bb35ec 100644
> --- a/src/PVE/VZDump/Common.pm
> +++ b/src/PVE/VZDump/Common.pm
> @@ -233,7 +233,19 @@ my $confdesc = {
>  	type => 'string',
>  	description => 'Backup all known guest systems included in the specified pool.',
>  	optional => 1,
> -    }
> +    },
> +    notes => {
> +	type => 'string',
> +	description => "Notes to add to the backup(s).",
> +	requires => 'storage',
> +	optional => 1,
> +    },

Quoting off-list discussion here, where it belongs and so we don't
forget about it ;)

With regard to fixing https://bugzilla.proxmox.com/show_bug.cgi?id=438
after all those years.

Fabian G.:
we could offer something like a simple template system that allows
substitution of certain variables (like name, or source node
hostname/clustername, ..). or just a boolean switch for setting VM/CT
$HOSTNAME from $CLUSTER/$NODENAME (or an enum, with
[job-comment,hostname,long,none] where long is that, and hostname is
just the guest hostname, and job-comment is the comment of the vzdump
job if one is set)

Me:
The template variant would be the most flexible one and would avoid the
need for a second vzdump option besides --notes. Ideally, support for it
would be there from the beginning though, as otherwise it will stop
working for a user wanting to literally set $HOSTNAME when we add it ;)
The downside is that it doesn't match the volume-level --notes option,
but I don't think that should be a big deal.

Fabian G.:
well it could just be called notes-template for vzdump to disambiguate?

> +    protected => {
> +	type => 'boolean',
> +	description => "If true, mark backup(s) as protected.",
> +	requires => 'storage',
> +	optional => 1,
> +    },
>  };
>  
>  sub get_confdesc {

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Thomas Lamprecht]

On 16.03.22 12:04, Fabian Ebner wrote:
> Am 16.12.21 um 13:12 schrieb Fabian Ebner:
>> In command_line(), notes are printed, quoted, but otherwise as is,
>> which is a bit ugly for multi-line notes. But it is part of the
>> commandline.
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/VZDump/Common.pm | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
>> index 83d7413..5bb35ec 100644
>> --- a/src/PVE/VZDump/Common.pm
>> +++ b/src/PVE/VZDump/Common.pm
>> @@ -233,7 +233,19 @@ my $confdesc = {
>>  	type => 'string',
>>  	description => 'Backup all known guest systems included in the specified pool.',
>>  	optional => 1,
>> -    }
>> +    },
>> +    notes => {
>> +	type => 'string',
>> +	description => "Notes to add to the backup(s).",
>> +	requires => 'storage',
>> +	optional => 1,
>> +    },
> 
> Quoting off-list discussion here, where it belongs and so we don't
> forget about it ;)
> 
> With regard to fixing https://bugzilla.proxmox.com/show_bug.cgi?id=438
> after all those years.
> 
> Fabian G.:
> we could offer something like a simple template system that allows
> substitution of certain variables (like name, or source node
> hostname/clustername, ..). or just a boolean switch for setting VM/CT
> $HOSTNAME from $CLUSTER/$NODENAME (or an enum, with
> [job-comment,hostname,long,none] where long is that, and hostname is
> just the guest hostname, and job-comment is the comment of the vzdump
> job if one is set)
> 
> Me:
> The template variant would be the most flexible one and would avoid the
> need for a second vzdump option besides --notes. Ideally, support for it
> would be there from the beginning though, as otherwise it will stop
> working for a user wanting to literally set $HOSTNAME when we add it ;)
> The downside is that it doesn't match the volume-level --notes option,
> but I don't think that should be a big deal.
> 
> Fabian G.:
> well it could just be called notes-template for vzdump to disambiguate?


fwiw, I believe I commented that approach in the internal chat a while ago,
but as its search functions are abysmal I don't find it anymore.

IIRC, just extend what we have now and allow a fixed set of {VARS} (vmid,
guest name, host name, job-id, ..?).

While extending one has a slight chance of changing an existing setup I find
this very unlikely in this specific case, as we had no such feature whatsoever
and it makes not sense in any practical example to use such special strings
for a backup comment.

That said, if one can whip up another reason besides backward compat for
having a separate flag to turn this on/off then feel free to comment.

I mean, for the backup jobs itself it could have some value to differ
between the comment about the job itself and a comment template for the
resulting backups.

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Fabian Ebner]

Am 16.03.22 um 19:25 schrieb Thomas Lamprecht:
> On 16.03.22 12:04, Fabian Ebner wrote:
>> Am 16.12.21 um 13:12 schrieb Fabian Ebner:
>>
>> Fabian G.:
>> we could offer something like a simple template system that allows
>> substitution of certain variables (like name, or source node
>> hostname/clustername, ..). or just a boolean switch for setting VM/CT
>> $HOSTNAME from $CLUSTER/$NODENAME (or an enum, with
>> [job-comment,hostname,long,none] where long is that, and hostname is
>> just the guest hostname, and job-comment is the comment of the vzdump
>> job if one is set)
>>
>> Me:
>> The template variant would be the most flexible one and would avoid the
>> need for a second vzdump option besides --notes. Ideally, support for it
>> would be there from the beginning though, as otherwise it will stop
>> working for a user wanting to literally set $HOSTNAME when we add it ;)
>> The downside is that it doesn't match the volume-level --notes option,
>> but I don't think that should be a big deal.
>>
>> Fabian G.:
>> well it could just be called notes-template for vzdump to disambiguate?
> 
> 
> fwiw, I believe I commented that approach in the internal chat a while ago,
> but as its search functions are abysmal I don't find it anymore.
> 
> IIRC, just extend what we have now and allow a fixed set of {VARS} (vmid,
> guest name, host name, job-id, ..?).

I might be misunderstanding, but we don't have anything right now,
because this patch would be the one introducing the option?

> 
> While extending one has a slight chance of changing an existing setup I find
> this very unlikely in this specific case, as we had no such feature whatsoever
> and it makes not sense in any practical example to use such special strings
> for a backup comment.

Yes, I'd simply document the list of currently valid variables, and that
it might be extended in the future.

> 
> That said, if one can whip up another reason besides backward compat for
> having a separate flag to turn this on/off then feel free to comment.
> 
> I mean, for the backup jobs itself it could have some value to differ
> between the comment about the job itself and a comment template for the
> resulting backups.

Yes, I think it'd be better to not mix the job's comment (which is part
of the generic job properties) and the vzdump-specific notes{-template}
which this patch (or rather a future version of it) will introduce.

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Thomas Lamprecht]

On 17.03.22 08:57, Fabian Ebner wrote:
> Am 16.03.22 um 19:25 schrieb Thomas Lamprecht:
>> On 16.03.22 12:04, Fabian Ebner wrote:
>>> Am 16.12.21 um 13:12 schrieb Fabian Ebner:
>>>
>>> Fabian G.:
>>> we could offer something like a simple template system that allows
>>> substitution of certain variables (like name, or source node
>>> hostname/clustername, ..). or just a boolean switch for setting VM/CT
>>> $HOSTNAME from $CLUSTER/$NODENAME (or an enum, with
>>> [job-comment,hostname,long,none] where long is that, and hostname is
>>> just the guest hostname, and job-comment is the comment of the vzdump
>>> job if one is set)
>>>
>>> Me:
>>> The template variant would be the most flexible one and would avoid the
>>> need for a second vzdump option besides --notes. Ideally, support for it
>>> would be there from the beginning though, as otherwise it will stop
>>> working for a user wanting to literally set $HOSTNAME when we add it ;)
>>> The downside is that it doesn't match the volume-level --notes option,
>>> but I don't think that should be a big deal.
>>>
>>> Fabian G.:
>>> well it could just be called notes-template for vzdump to disambiguate?
>>
>>
>> fwiw, I believe I commented that approach in the internal chat a while ago,
>> but as its search functions are abysmal I don't find it anymore.
>>
>> IIRC, just extend what we have now and allow a fixed set of {VARS} (vmid,
>> guest name, host name, job-id, ..?).
> 
> I might be misunderstanding, but we don't have anything right now,
> because this patch would be the one introducing the option?
> 

yeah in this vzdump CLI context I was confusing, I talked more from the
job POV.

>>
>> While extending one has a slight chance of changing an existing setup I find
>> this very unlikely in this specific case, as we had no such feature whatsoever
>> and it makes not sense in any practical example to use such special strings
>> for a backup comment.
> 
> Yes, I'd simply document the list of currently valid variables, and that
> it might be extended in the future.
> 

k.

a subset of the VM config could be possibly interesting too (e.g., ostype, first
line of description) but as the config is already in the backup it may not be worth
it - so maybe better to wait for the non-obvious variables for users to actually
requesting them.

>>
>> That said, if one can whip up another reason besides backward compat for
>> having a separate flag to turn this on/off then feel free to comment.
>>
>> I mean, for the backup jobs itself it could have some value to differ
>> between the comment about the job itself and a comment template for the
>> resulting backups.
> 
> Yes, I think it'd be better to not mix the job's comment (which is part
> of the generic job properties) and the vzdump-specific notes{-template}
> which this patch (or rather a future version of it) will introduce.

Agree. So, to summarize, vzdump does the interpreting for a plain, new `--notes`
CLI which it also prints (with variables already resolved) in the task log and
sets that also as note for the (created) backup.

The job config would get a new notes-template config that allows to add such
a dynamically interpreted string to each backup created by this job by bassing
it as --notes to vzdump. That way we avoid vzdump having two different, clashing,
CLI options but keep it cleanly separated for the job definitions.

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Fabian Ebner]

Am 17.03.22 um 09:07 schrieb Thomas Lamprecht:
> On 17.03.22 08:57, Fabian Ebner wrote:
>> Am 16.03.22 um 19:25 schrieb Thomas Lamprecht:
>>>
>>> While extending one has a slight chance of changing an existing setup I find
>>> this very unlikely in this specific case, as we had no such feature whatsoever
>>> and it makes not sense in any practical example to use such special strings
>>> for a backup comment.
>>
>> Yes, I'd simply document the list of currently valid variables, and that
>> it might be extended in the future.
>>
> 
> k.
> 
> a subset of the VM config could be possibly interesting too (e.g., ostype, first
> line of description) but as the config is already in the backup it may not be worth
> it - so maybe better to wait for the non-obvious variables for users to actually
> requesting them.
> 

Ok

>>>
>>> That said, if one can whip up another reason besides backward compat for
>>> having a separate flag to turn this on/off then feel free to comment.
>>>
>>> I mean, for the backup jobs itself it could have some value to differ
>>> between the comment about the job itself and a comment template for the
>>> resulting backups.
>>
>> Yes, I think it'd be better to not mix the job's comment (which is part
>> of the generic job properties) and the vzdump-specific notes{-template}
>> which this patch (or rather a future version of it) will introduce.
> 
> Agree. So, to summarize, vzdump does the interpreting for a plain, new `--notes`
> CLI which it also prints (with variables already resolved) in the task log and
> sets that also as note for the (created) backup.
> 
> The job config would get a new notes-template config that allows to add such
> a dynamically interpreted string to each backup created by this job by bassing
> it as --notes to vzdump. That way we avoid vzdump having two different, clashing,
> CLI options but keep it cleanly separated for the job definitions.

I think it'd be better or even necessary for vzdump to do the variable
expansion, because some things are not known when we start the job, for
example guest name. It'd then be enough to add a notes-template option
for vzdump, and have the job pass it along as-is with the rest of the
vzdump-config.

Re: [pve-devel] [PATCH guest-common 1/1] vzdump: schema: add 'notes' and 'protected' properties

[Thomas Lamprecht]

On 17.03.22 09:18, Fabian Ebner wrote:
>> Agree. So, to summarize, vzdump does the interpreting for a plain, new `--notes`
>> CLI which it also prints (with variables already resolved) in the task log and
>> sets that also as note for the (created) backup.
>>
>> The job config would get a new notes-template config that allows to add such
>> a dynamically interpreted string to each backup created by this job by bassing
>> it as --notes to vzdump. That way we avoid vzdump having two different, clashing,
>> CLI options but keep it cleanly separated for the job definitions.
>
> I think it'd be better or even necessary for vzdump to do the variable
> expansion, because some things are not known when we start the job, for
> example guest name. It'd then be enough to add a notes-template option
> for vzdump, and have the job pass it along as-is with the rest of the
> vzdump-config.

I meant to convey that, the "with variables already resolved" was meant for when
printed to the task log, not when passed to vzdump (worded a bit confusing though) ;-)

[pve-devel] [PATCH manager 5/6] vzdump: support setting protected status and notes

[Fabian Ebner]

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>
---

Dependency bump for guest-common needed.

Arguably, a warning from failing to set notes is not very visible, but
I didn't want to make it a full-blown error.

 PVE/VZDump.pm | 46 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 8e20c320..1aa3822e 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use Fcntl ':flock';
+use File::Basename;
 use File::Path;
 use IO::File;
 use IO::Select;
@@ -772,21 +773,30 @@ sub exec_backup_task {
 	    }
 	}
 
-	if ($backup_limit && !$opts->{remove}) {
+	if (($backup_limit && !$opts->{remove}) || $opts->{protected}) {
 	    my $count;
+	    my $protected_count;
 	    if (my $storeid = $opts->{storage}) {
-		my $backups = PVE::Storage::volume_list($cfg, $storeid, $vmid, 'backup');
-		$count = grep {
-		    !$_->{protected} && (!$_->{subtype} || $_->{subtype} eq $vmtype)
-		} $backups->@*;
+		my @backups = grep {
+		    !$_->{subtype} || $_->{subtype} eq $vmtype
+		} PVE::Storage::volume_list($cfg, $storeid, $vmid, 'backup')->@*;
+
+		$count = grep { !$_->{protected} } @backups;
+		$protected_count = scalar(@backups) - $count;
 	    } else {
 		$count = scalar(get_unprotected_backup_file_list($opts->{dumpdir}, $bkname)->@*);
 	    }
 
-	    die "There is a max backup limit of $backup_limit enforced by the".
-		" target storage or the vzdump parameters.".
-		" Either increase the limit or delete old backup(s).\n"
-		if $count >= $backup_limit;
+	    if ($opts->{protected}) {
+		my $max_protected = $opts->{scfg}->{'max-protected-backups'} // 5;
+		if ($max_protected > -1 && $protected_count >= $max_protected) {
+		    die "The number of protected backups per guest is limited to $max_protected ".
+			"on storage '$opts->{storage}'\n";
+		}
+	    } elsif ($count >= $backup_limit) {
+		die "There is a max backup limit of $backup_limit enforced by the target storage ".
+		    "or the vzdump parameters. Either increase the limit or delete old backups.\n";
+	    }
 	}
 
 	if (!$self->{opts}->{pbs}) {
@@ -987,6 +997,24 @@ sub exec_backup_task {
 	    debugmsg ('info', "archive file size: $cs", $logfd);
 	}
 
+	# Mark as protected before pruning.
+	if (my $storeid = $opts->{storage}) {
+	    my $volname = $opts->{pbs} ? $task->{target} : basename($task->{target});
+	    my $volid = "${storeid}:backup/${volname}";
+
+	    if ((my $notes = $opts->{notes}) && $opts->{notes} ne '') {
+		debugmsg('info', "adding notes to backup", $logfd);
+		eval { PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', $notes) };
+		debugmsg('warn', "unable to add notes - $@", $logfd) if $@;
+	    }
+
+	    if ($opts->{protected}) {
+		debugmsg('info', "marking backup as protected", $logfd);
+		eval { PVE::Storage::update_volume_attribute($cfg, $volid, 'protected', 1) };
+		die "unable to set protected flag - $@\n" if $@;
+	    }
+	}
+
 	if ($opts->{remove}) {
 	    my $keepstr = join(', ', map { "$_=$prune_options->{$_}" } sort keys %$prune_options);
 	    debugmsg ('info', "prune older backups with retention: $keepstr", $logfd);
-- 
2.30.2

[pve-devel] [PATCH manager 6/6] ui: backup: allow setting protected and notes for manual backup

[Fabian Ebner]

Setting a width, so the text area can fill the horizontal space.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 www/manager6/window/Backup.js | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/www/manager6/window/Backup.js b/www/manager6/window/Backup.js
index 726122c8..4c4bac3f 100644
--- a/www/manager6/window/Backup.js
+++ b/www/manager6/window/Backup.js
@@ -154,19 +154,33 @@ Ext.define('PVE.window.Backup', {
 	    },
 	});
 
+	let protectedCheckbox = Ext.create('Proxmox.form.Checkbox', {
+	    name: 'protected',
+	    checked: false,
+	    uncheckedValue: 0,
+	    fieldLabel: gettext('Protected'),
+	});
+
 	me.formPanel = Ext.create('Proxmox.panel.InputPanel', {
 	    bodyPadding: 10,
 	    border: false,
 	    column1: [
 		storagesel,
 		modeSelector,
-		removeCheckbox,
+		protectedCheckbox,
 	    ],
 	    column2: [
 		compressionSelector,
 		mailtoField,
+		removeCheckbox,
 	    ],
 	    columnB: [
+		{
+		    xtype: 'textareafield',
+		    name: 'notes',
+		    fieldLabel: gettext('Notes'),
+		    anchor: '100%',
+		},
 		{
 		    xtype: 'label',
 		    name: 'pruneLabel',
@@ -229,6 +243,14 @@ Ext.define('PVE.window.Backup', {
 		    params.compress = values.compress;
 		}
 
+		if (values.protected) {
+		    params.protected = values.protected;
+		}
+
+		if (values.notes) {
+		    params.notes = values.notes;
+		}
+
 		Proxmox.Utils.API2Request({
 		    url: '/nodes/' + me.nodename + '/vzdump',
 		    params: params,
@@ -272,6 +294,7 @@ Ext.define('PVE.window.Backup', {
 	    modal: true,
 	    layout: 'auto',
 	    border: false,
+	    width: 600,
 	    items: [me.formPanel],
 	    buttons: [helpBtn, '->', submitBtn],
 	    listeners: {
-- 
2.30.2

Re: [pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[Dominik Csapak]

one question:

either it's not explained, or i missed it:
what do we gain by having a limit on the number of protected backups?

storage 2/2 mentions that protection broke some assumption of vzdump
which is (somehow? not really explained imho) fixing it?

if it's not fixing it, what is the relation between those things?

also, why have a 'magic' -1 value that means indefinite, we could
simply always have that behavior?

in my opinion, it makes no sense to limit the number of protected
backups..

if it is necessary for some reason, it would be good to include
that reason either in the commit message, or at least in the cover
letter...

On 12/16/21 13:12, Fabian Ebner wrote:
> The series consists of two parts:
> 
> First part (up to docs 2/2) is for introducing a storage property
> to limit the number of protected backups and only count unprotected
> backups for the limit check in vzdump.
> 
> Second part is a convenience feature allowing 'protected' and 'notes'
> to be set directly with vzdump and for manual backups in the UI.
> 
> 
> storage:
> 
> Fabian Ebner (2):
>    list volumes: also return backup type for backups
>    plugins: allow limiting the number of protected backups per guest
> 
>   PVE/Storage.pm                 | 24 ++++++++++++++++++++++++
>   PVE/Storage/BTRFSPlugin.pm     |  3 ++-
>   PVE/Storage/CIFSPlugin.pm      |  1 +
>   PVE/Storage/CephFSPlugin.pm    |  1 +
>   PVE/Storage/DirPlugin.pm       |  1 +
>   PVE/Storage/GlusterfsPlugin.pm |  1 +
>   PVE/Storage/NFSPlugin.pm       |  1 +
>   PVE/Storage/PBSPlugin.pm       |  2 ++
>   PVE/Storage/Plugin.pm          |  8 ++++++++
>   test/list_volumes_test.pm      | 10 ++++++++++
>   10 files changed, 51 insertions(+), 1 deletion(-)
> 
> 
> manager:
> 
> Fabian Ebner (6):
>    vzdump: backup file list: drop unused parameter
>    vzdump: backup limit: only count unprotected backups
>    ui: storage edit: retention: add max-protected-backups setting
>    ui: storage creation: retention: dynamically adapt
>      max-protected-backups
>    vzdump: support setting protected status and notes
>    ui: backup: allow setting protected and notes for manual backup
> 
>   PVE/VZDump.pm                        | 69 ++++++++++++++++++---------
>   www/manager6/panel/BackupJobPrune.js | 71 ++++++++++++++++++++++++----
>   www/manager6/storage/Base.js         |  1 +
>   www/manager6/window/Backup.js        | 25 +++++++++-
>   4 files changed, 132 insertions(+), 34 deletions(-)
> 
> 
> guest-common:
> 
> Fabian Ebner (1):
>    vzdump: schema: add 'notes' and 'protected' properties
> 
>   src/PVE/VZDump/Common.pm | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> 
> docs:
> 
> Fabian Ebner (2):
>    storage: switch to prune-backups in examples
>    vzdump/storage: mention protected backups limit and give an example
> 
>   pve-storage-dir.adoc | 9 +++++----
>   pve-storage-pbs.adoc | 2 +-
>   vzdump.adoc          | 3 +++
>   3 files changed, 9 insertions(+), 5 deletions(-)
> 

Re: [pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[Thomas Lamprecht]

On 20/12/2021 11:31, Dominik Csapak wrote:
> what do we gain by having a limit on the number of protected backups?

We avoid allowing users to create an infinite number of backups.

Remember that unprotected backups do not count towards the keep-X retention
parameters as they are considered a specially marked snapshot outside the
regular schedule, and doing so could lead to situations where no new backup
can be made (if sum of keep-X == sum of protected backups), which can be
pretty bad.

Now, if a admin wants to limit the amount of backups a user can make of the
VMs those users own, the admin sets now keep-X (which superseded max-backups)
The sum of all keep-X is always the maximal, total amount of backups that can
be made, but if the user marks every new backup immediately as protected they
can overstep that limit arbitrarily, this series addresses that while not
breaking backward comparability.

> 
> storage 2/2 mentions that protection broke some assumption of vzdump
> which is (somehow? not really explained imho) fixing it?
> 
> if it's not fixing it, what is the relation between those things?
> 
> also, why have a 'magic' -1 value that means indefinite, we could
> simply always have that behavior?
> 
> in my opinion, it makes no sense to limit the number of protected
> backups..

see above, having the whole picture should bring sense to this..

> 
> if it is necessary for some reason, it would be good to include
> that reason either in the commit message, or at least in the cover
> letter...
> 

I mean while the cover letter only hints it, commit message from the storage
2/2 patch is pretty clear to me.. FWIW, this was discussed quite extensively
between Fabian E. and myself, and that result was further discussed with Fabian G.
off-list.

Re: [pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[Dominik Csapak]

On 12/21/21 16:11, Thomas Lamprecht wrote:
> On 20/12/2021 11:31, Dominik Csapak wrote:
>> what do we gain by having a limit on the number of protected backups?
> 
> We avoid allowing users to create an infinite number of backups.
> 
> Remember that unprotected backups do not count towards the keep-X retention
> parameters as they are considered a specially marked snapshot outside the
> regular schedule, and doing so could lead to situations where no new backup
> can be made (if sum of keep-X == sum of protected backups), which can be
> pretty bad.
> 
> Now, if a admin wants to limit the amount of backups a user can make of the
> VMs those users own, the admin sets now keep-X (which superseded max-backups)
> The sum of all keep-X is always the maximal, total amount of backups that can
> be made, but if the user marks every new backup immediately as protected they
> can overstep that limit arbitrarily, this series addresses that while not
> breaking backward comparability.
> 
>>
>> storage 2/2 mentions that protection broke some assumption of vzdump
>> which is (somehow? not really explained imho) fixing it?
>>
>> if it's not fixing it, what is the relation between those things?
>>
>> also, why have a 'magic' -1 value that means indefinite, we could
>> simply always have that behavior?
>>
>> in my opinion, it makes no sense to limit the number of protected
>> backups..
> 
> see above, having the whole picture should bring sense to this..
> 
>>
>> if it is necessary for some reason, it would be good to include
>> that reason either in the commit message, or at least in the cover
>> letter...
>>
> 
> I mean while the cover letter only hints it, commit message from the storage
> 2/2 patch is pretty clear to me.. FWIW, this was discussed quite extensively
> between Fabian E. and myself, and that result was further discussed with Fabian G.
> off-list.

OK, i get it now (also talked with fabian g. off-list).
i did not conclude from the storage 2/2 patch that
it implements an upper limit of backups, so i was confused.

Thanks :)

Re: [pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[Fabian Ebner]

Am 12/22/21 um 08:02 schrieb Dominik Csapak:
> On 12/21/21 16:11, Thomas Lamprecht wrote:
>> On 20/12/2021 11:31, Dominik Csapak wrote:
>>> what do we gain by having a limit on the number of protected backups?
>>
>> We avoid allowing users to create an infinite number of backups.
>>
>> Remember that unprotected backups do not count towards the keep-X 
>> retention
>> parameters as they are considered a specially marked snapshot outside the
>> regular schedule, and doing so could lead to situations where no new 
>> backup
>> can be made (if sum of keep-X == sum of protected backups), which can be
>> pretty bad.
>>
>> Now, if a admin wants to limit the amount of backups a user can make 
>> of the
>> VMs those users own, the admin sets now keep-X (which superseded 
>> max-backups)
>> The sum of all keep-X is always the maximal, total amount of backups 
>> that can
>> be made, but if the user marks every new backup immediately as 
>> protected they
>> can overstep that limit arbitrarily, this series addresses that while not
>> breaking backward comparability.
>>
>>>
>>> storage 2/2 mentions that protection broke some assumption of vzdump
>>> which is (somehow? not really explained imho) fixing it?
>>>
>>> if it's not fixing it, what is the relation between those things?
>>>
>>> also, why have a 'magic' -1 value that means indefinite, we could
>>> simply always have that behavior?
>>>
>>> in my opinion, it makes no sense to limit the number of protected
>>> backups..
>>
>> see above, having the whole picture should bring sense to this..
>>
>>>
>>> if it is necessary for some reason, it would be good to include
>>> that reason either in the commit message, or at least in the cover
>>> letter...
>>>

Sorry about the lack of information. Of course the "why?" should be 
covered in the cover letter, and in storage 2/2, I should've made 
explicit what the implication of the broken assumption is/why the new 
property helps.

>>
>> I mean while the cover letter only hints it, commit message from the 
>> storage
>> 2/2 patch is pretty clear to me.. FWIW, this was discussed quite 
>> extensively
>> between Fabian E. and myself, and that result was further discussed 
>> with Fabian G.
>> off-list.
> 
> OK, i get it now (also talked with fabian g. off-list).
> i did not conclude from the storage 2/2 patch that
> it implements an upper limit of backups, so i was confused.
> 
> Thanks :)
> 

Re: [pve-devel] [PATCH-SERIES storage/manager/guest-common/docs] improvements for protected backups

[Fabian Ebner]

Ping for the series. Or do we not want the limit after all (as far as
I'm aware, nobody complained yet)?

Am 16.12.21 um 13:12 schrieb Fabian Ebner:
> The series consists of two parts:
> 
> First part (up to docs 2/2) is for introducing a storage property
> to limit the number of protected backups and only count unprotected
> backups for the limit check in vzdump.
> 
> Second part is a convenience feature allowing 'protected' and 'notes'
> to be set directly with vzdump and for manual backups in the UI.
> 
> 
> storage:
> 
> Fabian Ebner (2):
>   list volumes: also return backup type for backups
>   plugins: allow limiting the number of protected backups per guest
> 
>  PVE/Storage.pm                 | 24 ++++++++++++++++++++++++
>  PVE/Storage/BTRFSPlugin.pm     |  3 ++-
>  PVE/Storage/CIFSPlugin.pm      |  1 +
>  PVE/Storage/CephFSPlugin.pm    |  1 +
>  PVE/Storage/DirPlugin.pm       |  1 +
>  PVE/Storage/GlusterfsPlugin.pm |  1 +
>  PVE/Storage/NFSPlugin.pm       |  1 +
>  PVE/Storage/PBSPlugin.pm       |  2 ++
>  PVE/Storage/Plugin.pm          |  8 ++++++++
>  test/list_volumes_test.pm      | 10 ++++++++++
>  10 files changed, 51 insertions(+), 1 deletion(-)
> 
> 
> manager:
> 
> Fabian Ebner (6):
>   vzdump: backup file list: drop unused parameter
>   vzdump: backup limit: only count unprotected backups
>   ui: storage edit: retention: add max-protected-backups setting
>   ui: storage creation: retention: dynamically adapt
>     max-protected-backups
>   vzdump: support setting protected status and notes
>   ui: backup: allow setting protected and notes for manual backup
> 
>  PVE/VZDump.pm                        | 69 ++++++++++++++++++---------
>  www/manager6/panel/BackupJobPrune.js | 71 ++++++++++++++++++++++++----
>  www/manager6/storage/Base.js         |  1 +
>  www/manager6/window/Backup.js        | 25 +++++++++-
>  4 files changed, 132 insertions(+), 34 deletions(-)
> 
> 
> guest-common:
> 
> Fabian Ebner (1):
>   vzdump: schema: add 'notes' and 'protected' properties
> 
>  src/PVE/VZDump/Common.pm | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> 
> docs:
> 
> Fabian Ebner (2):
>   storage: switch to prune-backups in examples
>   vzdump/storage: mention protected backups limit and give an example
> 
>  pve-storage-dir.adoc | 9 +++++----
>  pve-storage-pbs.adoc | 2 +-
>  vzdump.adoc          | 3 +++
>  3 files changed, 9 insertions(+), 5 deletions(-)
>