public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected
@ 2021-09-30 11:42 Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 1/7] dir plugin: update notes: don't fail if file is already removed Fabian Ebner
                   ` (12 more replies)
  0 siblings, 13 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

Protected bacukps cannot be removed accidentally and will be ignored
for pruning. A <backup>.protected file serves as a protection marker
for file-based storages.


Changes from v1:
    * Avoid races when using unlink.
    * Also add fall-back for {get, update}_volume_notes to other
      plugin implementations, because external plugins might be
      derived from those too.
    * Add UI integration patches.


For the storage part, an APIAGE+APIVER bump is needed.
Dependency bump from pve-manager to pve-storage is needed.


To work, the PBS integration needs Dominik's patches for PBS (seems
like a rebase is needed for those, I tested on top of v2.0.10):
https://lists.proxmox.com/pipermail/pbs-devel/2021-September/004099.html


pve-storage:

Fabian Ebner (7):
  dir plugin: update notes: don't fail if file is already removed
  dir plugin: get notes: return undef if notes are not supported
  add generalized functions to manage volume attributes
  prune mark: preserve additional information for the keep-all case
  fix #3307: make it possible to set protection for backups
  prune: mark renamed and protected backups differently
  pbs: integrate support for protected

 PVE/API2/Storage/Content.pm      | 36 ++++++++++---
 PVE/API2/Storage/PruneBackups.pm |  5 +-
 PVE/Storage.pm                   | 23 +++++---
 PVE/Storage/BTRFSPlugin.pm       |  4 +-
 PVE/Storage/CIFSPlugin.pm        | 13 +++++
 PVE/Storage/CephFSPlugin.pm      | 12 +++++
 PVE/Storage/DirPlugin.pm         | 62 +++++++++++++++++++++-
 PVE/Storage/NFSPlugin.pm         | 13 +++++
 PVE/Storage/PBSPlugin.pm         | 90 +++++++++++++++++++++++++++++++-
 PVE/Storage/Plugin.pm            | 45 +++++++++++++++-
 test/prune_backups_test.pm       | 17 +++++-
 11 files changed, 296 insertions(+), 24 deletions(-)


pve-manager:

Fabian Ebner (5):
  vzdump: skip protected backups for dumpdir pruning
  ui: storage content: avoid redundant options hasNotesColumn and
    hideColumns
  ui: backup views: add protected column
  ui: backup views: add button to change protection status
  ui: prune: also handle new 'renamed' keep reason

 PVE/VZDump.pm                       |  7 +++++++
 www/manager6/grid/BackupView.js     | 25 +++++++++++++++++++++++++
 www/manager6/storage/BackupView.js  | 18 ++++++++++++++++++
 www/manager6/storage/Browser.js     |  1 -
 www/manager6/storage/ContentView.js | 20 ++++++++++++++------
 www/manager6/window/Prune.js        |  2 ++
 6 files changed, 66 insertions(+), 7 deletions(-)

-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 1/7] dir plugin: update notes: don't fail if file is already removed
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 2/7] dir plugin: get notes: return undef if notes are not supported Fabian Ebner
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

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

Changes from v1:
    * Avoid race by always attempting to remove.

 PVE/Storage/DirPlugin.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 2267f11..30ed7ca 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -2,8 +2,11 @@ package PVE::Storage::DirPlugin;
 
 use strict;
 use warnings;
+
 use Cwd;
 use File::Path;
+use POSIX;
+
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
 
@@ -110,7 +113,7 @@ sub update_volume_notes {
     if (defined($notes) && $notes ne '') {
 	PVE::Tools::file_set_contents($path, $notes);
     } else {
-	unlink $path or die "could not delete notes - $!\n";
+	unlink $path or $! == ENOENT or die "could not delete notes - $!\n";
     }
     return;
 }
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 2/7] dir plugin: get notes: return undef if notes are not supported
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 1/7] dir plugin: update notes: don't fail if file is already removed Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 3/7] add generalized functions to manage volume attributes Fabian Ebner
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

This avoids showing empty notes in the result of the content/{volid}
API call for volumes that do not even support notes. It's also in
preparation for the proposed get_volume_attribute generalization,
which expects undef to be returned when an attribute is not supported.

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

New in v2.

 PVE/Storage/DirPlugin.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 30ed7ca..0ac9b06 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -93,6 +93,9 @@ sub parse_is_mountpoint {
 sub get_volume_notes {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
+    my ($vtype) = $class->parse_volname($volname);
+    return if $vtype ne 'backup';
+
     my $path = $class->filesystem_path($scfg, $volname);
     $path .= $class->SUPER::NOTES_EXT;
 
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 3/7] add generalized functions to manage volume attributes
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 1/7] dir plugin: update notes: don't fail if file is already removed Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 2/7] dir plugin: get notes: return undef if notes are not supported Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 4/7] prune mark: preserve additional information for the keep-all case Fabian Ebner
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

replacing the ones for handling notes. To ensure backwards
compatibility with external plugins, all plugins that do not just call
another implementation need to call $class->{get, update}_volume_notes
when the attribute is 'notes' to catch any derived implementations.

This is mainly done to avoid the need to add new methods every time a
new attribute is added.

Not adding a timeout parameter like the notes functions have, because
it was not used and can still be added if it ever is needed in the
future.

For get_volume_attribute, undef will indicate that the attribute is
not supported. This makes it possible to distinguish "not supported"
from "error getting the attribute", which is useful when the attribute
is important for an operation. For example, free_image checking for
protection (introduced in a later patch) can abort if getting the
'protected' attribute fails.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * Use fall-back in DirPlugin and PBSPlugin too, and keep exisitng
      {get, update}_volume_notes implementations around for now.

Requires an APIAGE+APIVER bump, but so does the whole series.

The alternative to undef indicating "not supported" would be to add a
separate method like supports_volume_attribute. Or adapt
volume_has_feature, but it doesn't really feel like the right fit as
it's currently intended for features for guest images only.

 PVE/API2/Storage/Content.pm |  7 ++++---
 PVE/Storage.pm              | 12 ++++++------
 PVE/Storage/BTRFSPlugin.pm  |  4 ++--
 PVE/Storage/CIFSPlugin.pm   | 13 +++++++++++++
 PVE/Storage/CephFSPlugin.pm | 12 ++++++++++++
 PVE/Storage/DirPlugin.pm    | 24 ++++++++++++++++++++++++
 PVE/Storage/NFSPlugin.pm    | 13 +++++++++++++
 PVE/Storage/PBSPlugin.pm    | 24 ++++++++++++++++++++++++
 PVE/Storage/Plugin.pm       | 36 ++++++++++++++++++++++++++++++++++++
 9 files changed, 134 insertions(+), 11 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 4d0ceb6..b3dc593 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -321,11 +321,12 @@ __PACKAGE__->register_method ({
 	    format => $format,
 	};
 
-	# not all storages/types support notes, so ignore errors here
+	# keep going if fetching an optional attribute fails
 	eval {
-	    my $notes = PVE::Storage::get_volume_notes($cfg, $volid);
+	    my $notes = PVE::Storage::get_volume_attribute($cfg, $volid, 'notes');
 	    $entry->{notes} = $notes if defined($notes);
 	};
+	warn $@ if $@;
 
 	return $entry;
     }});
@@ -371,7 +372,7 @@ __PACKAGE__->register_method ({
 	PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid);
 
 	if (exists $param->{notes}) {
-	    PVE::Storage::update_volume_notes($cfg, $volid, $param->{notes});
+	    PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', $param->{notes});
 	}
 
 	return undef;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 7b319fe..729c90e 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -215,24 +215,24 @@ sub file_size_info {
     return PVE::Storage::Plugin::file_size_info($filename, $timeout);
 }
 
-sub get_volume_notes {
-    my ($cfg, $volid, $timeout) = @_;
+sub get_volume_attribute {
+    my ($cfg, $volid, $attribute) = @_;
 
     my ($storeid, $volname) = parse_volume_id($volid);
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-    return $plugin->get_volume_notes($scfg, $storeid, $volname, $timeout);
+    return $plugin->get_volume_attribute($scfg, $storeid, $volname, $attribute);
 }
 
-sub update_volume_notes {
-    my ($cfg, $volid, $notes, $timeout) = @_;
+sub update_volume_attribute {
+    my ($cfg, $volid, $attribute, $value) = @_;
 
     my ($storeid, $volname) = parse_volume_id($volid);
     my $scfg = storage_config($cfg, $storeid);
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
-    $plugin->update_volume_notes($scfg, $storeid, $volname, $notes, $timeout);
+    return $plugin->update_volume_attribute($scfg, $storeid, $volname, $attribute, $value);
 }
 
 sub volume_size_info {
diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
index b30000b..c3e9b32 100644
--- a/PVE/Storage/BTRFSPlugin.pm
+++ b/PVE/Storage/BTRFSPlugin.pm
@@ -136,9 +136,9 @@ sub status {
     return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache);
 }
 
-# TODO: sub get_volume_notes {}
+# TODO: sub get_volume_attribute {}
 
-# TODO: sub update_volume_notes {}
+# TODO: sub update_volume_attribute {}
 
 # croak would not include the caller from within this module
 sub __error {
diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index c5f3894..890b378 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -286,13 +286,26 @@ sub check_connection {
     return 1;
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::get_volume_notes($class, @_);
 }
+
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::update_volume_notes($class, @_);
 }
 
+sub get_volume_attribute {
+    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
+}
+
+sub update_volume_attribute {
+    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
+}
+
 1;
diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm
index 3b9a791..1fd4c47 100644
--- a/PVE/Storage/CephFSPlugin.pm
+++ b/PVE/Storage/CephFSPlugin.pm
@@ -232,14 +232,26 @@ sub deactivate_storage {
     }
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::get_volume_notes($class, @_);
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::update_volume_notes($class, @_);
 }
 
+sub get_volume_attribute {
+    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
+}
+
+sub update_volume_attribute {
+    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
+}
+
 1;
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 0ac9b06..b496d06 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -90,6 +90,8 @@ sub parse_is_mountpoint {
     return $is_mp; # contains a path
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
@@ -104,6 +106,8 @@ sub get_volume_notes {
     return '';
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
 
@@ -121,6 +125,26 @@ sub update_volume_notes {
     return;
 }
 
+sub get_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+    if ($attribute eq 'notes') {
+	return $class->get_volume_notes($scfg, $storeid, $volname);
+    }
+
+    return;
+}
+
+sub update_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+    if ($attribute eq 'notes') {
+	return $class->update_volume_notes($scfg, $storeid, $volname, $value);
+    }
+
+    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
+}
+
 sub status {
     my ($class, $storeid, $scfg, $cache) = @_;
 
diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
index 39bf15a..bc57e35 100644
--- a/PVE/Storage/NFSPlugin.pm
+++ b/PVE/Storage/NFSPlugin.pm
@@ -188,13 +188,26 @@ sub check_connection {
     return 1;
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::get_volume_notes($class, @_);
 }
+
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my $class = shift;
     PVE::Storage::DirPlugin::update_volume_notes($class, @_);
 }
 
+sub get_volume_attribute {
+    return PVE::Storage::DirPlugin::get_volume_attribute(@_);
+}
+
+sub update_volume_attribute {
+    return PVE::Storage::DirPlugin::update_volume_attribute(@_);
+}
+
 1;
diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index bb1c382..76699e6 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -782,6 +782,8 @@ sub deactivate_volume {
     return 1;
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
@@ -792,6 +794,8 @@ sub get_volume_notes {
     return $data->{notes};
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
 
@@ -802,6 +806,26 @@ sub update_volume_notes {
     return undef;
 }
 
+sub get_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+    if ($attribute eq 'notes') {
+	return $class->get_volume_notes($scfg, $storeid, $volname);
+    }
+
+    return;
+}
+
+sub update_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+    if ($attribute eq 'notes') {
+	return $class->update_volume_notes($scfg, $storeid, $volname, $value);
+    }
+
+    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
+}
+
 sub volume_size_info {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 417d1fd..1182008 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -851,18 +851,52 @@ sub file_size_info {
     return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use get_volume_attribute instead.
 sub get_volume_notes {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
 
     die "volume notes are not supported for $class";
 }
 
+# FIXME remove on the next APIAGE reset.
+# Deprecated, use update_volume_attribute instead.
 sub update_volume_notes {
     my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_;
 
     die "volume notes are not supported for $class";
 }
 
+# Returns undef if the attribute is not supported for the volume.
+# Should die if there is an error fetching the attribute.
+# Possible attributes:
+# notes     - user-provided comments/notes.
+sub get_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+    if ($attribute eq 'notes') {
+	 my $notes = eval { $class->get_volume_notes($scfg, $storeid, $volname); };
+	 if (my $err = $@) {
+	     return if $err =~ m/^volume notes are not supported/;
+	     die $err;
+	 }
+	 return $notes;
+    }
+
+    return;
+}
+
+# Dies if the attribute is not supported for the volume.
+sub update_volume_attribute {
+    my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+    if ($attribute eq 'notes') {
+	$class->update_volume_notes($scfg, $storeid, $volname, $value);
+    }
+
+    die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
+}
+
 sub volume_size_info {
     my ($class, $scfg, $storeid, $volname, $timeout) = @_;
     my $path = $class->filesystem_path($scfg, $volname);
@@ -1098,6 +1132,8 @@ my $get_subdir_files = sub {
     return $res;
 };
 
+# If attributes are set on a volume, they should be included in the result.
+# See get_volume_attribute for a list of possible attributes.
 sub list_volumes {
     my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
 
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 4/7] prune mark: preserve additional information for the keep-all case
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (2 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 3/7] add generalized functions to manage volume attributes Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 5/7] fix #3307: make it possible to set protection for backups Fabian Ebner
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

Currently, if an entry is already marked as 'protected'.

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

No changes from v1.

This makes adding the test in the next patch much simpler, because
the expected mark of a protected backup can be hardcoded as
'protected'. But even without that, it makes sense to preserve the
more specific mark.

 PVE/Storage.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 729c90e..e7d8e62 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1670,6 +1670,8 @@ sub prune_mark_backup_group {
 
     if ($keep->{'keep-all'} || scalar(@positive_opts) == 0) {
 	foreach my $prune_entry (@{$backup_group}) {
+	    # preserve additional information like 'protected'
+	    next if $prune_entry->{mark} && $prune_entry->{mark} ne 'remove';
 	    $prune_entry->{mark} = 'keep';
 	}
 	return;
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 5/7] fix #3307: make it possible to set protection for backups
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (3 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 4/7] prune mark: preserve additional information for the keep-all case Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 6/7] prune: mark renamed and protected backups differently Fabian Ebner
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

A protected backup is not removed by free_image and ignored when
pruning.

The protection_file_path function is introduced in Storage.pm, so that
it can also be used by vzdump itself and in archive_remove.

For pruning, renamed backups already behaved similiar to how protected
backups will, but there are a few reasons to not just use that for
implementing the new feature:
1. It wouldn't protect against removal.
2. It would make it necessary to rename notes and log files too.
3. It wouldn't naturally extend to other volumes if that's needed.

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

Changes from v1:
    * Rebase, because of newly introduced fall-back to
      {get, update}_volume_notes.
    * Don't die if unlink failed with ENOENT.
    * Add rationale about not re-using "renamed is protected" behavior
      to the commit message.

 PVE/API2/Storage/Content.pm | 37 ++++++++++++++++++++++++++++---------
 PVE/Storage.pm              |  9 +++++++++
 PVE/Storage/DirPlugin.pm    | 30 ++++++++++++++++++++++++++++++
 PVE/Storage/Plugin.pm       |  7 +++++++
 test/prune_backups_test.pm  | 13 +++++++++++++
 5 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index b3dc593..45b8de8 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -113,6 +113,11 @@ __PACKAGE__->register_method ({
 		    },
 		    optional => 1,
 		},
+		protected => {
+		    description => "Protection status. Currently only supported for backups.",
+		    type => 'boolean',
+		    optional => 1,
+		},
 	    },
 	},
 	links => [ { rel => 'child', href => "{volid}" } ],
@@ -295,7 +300,12 @@ __PACKAGE__->register_method ({
 		description => "Optional notes.",
 		optional => 1,
 		type => 'string',
-	    }
+	    },
+	    protected => {
+		description => "Protection status. Currently only supported for backups.",
+		type => 'boolean',
+		optional => 1,
+	    },
 	},
     },
     code => sub {
@@ -321,12 +331,14 @@ __PACKAGE__->register_method ({
 	    format => $format,
 	};
 
-	# keep going if fetching an optional attribute fails
-	eval {
-	    my $notes = PVE::Storage::get_volume_attribute($cfg, $volid, 'notes');
-	    $entry->{notes} = $notes if defined($notes);
-	};
-	warn $@ if $@;
+	for my $attribute (qw(notes protected)) {
+	    # keep going if fetching an optional attribute fails
+	    eval {
+		my $value = PVE::Storage::get_volume_attribute($cfg, $volid, $attribute);
+		$entry->{$attribute} = $value if defined($value);
+	    };
+	    warn $@ if $@;
+	}
 
 	return $entry;
     }});
@@ -356,6 +368,11 @@ __PACKAGE__->register_method ({
 		type => 'string',
 		optional => 1,
 	    },
+	    protected => {
+		description => "Protection status. Currently only supported for backups.",
+		type => 'boolean',
+		optional => 1,
+	    },
 	},
     },
     returns => { type => 'null' },
@@ -371,8 +388,10 @@ __PACKAGE__->register_method ({
 
 	PVE::Storage::check_volume_access($rpcenv, $authuser, $cfg, undef, $volid);
 
-	if (exists $param->{notes}) {
-	    PVE::Storage::update_volume_attribute($cfg, $volid, 'notes', $param->{notes});
+	for my $attr (qw(notes protected)) {
+	    if (exists $param->{$attr}) {
+		PVE::Storage::update_volume_attribute($cfg, $volid, $attr, $param->{$attr});
+	    }
 	}
 
 	return undef;
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index e7d8e62..3beb645 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1463,6 +1463,12 @@ sub decompressor_info {
     return $info;
 }
 
+sub protection_file_path {
+    my ($path) = @_;
+
+    return "${path}.protected";
+}
+
 sub archive_info {
     my ($archive) = shift;
     my $info;
@@ -1494,6 +1500,9 @@ sub archive_info {
 sub archive_remove {
     my ($archive_path) = @_;
 
+    die "cannot remove protected archive '$archive_path'\n"
+	if -e protection_file_path($archive_path);
+
     my $dirname = dirname($archive_path);
     my $archive_info = eval { archive_info($archive_path) } // {};
     my $logfn = $archive_info->{logfilename};
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index b496d06..0d9bf51 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use Cwd;
 use File::Path;
+use IO::File;
 use POSIX;
 
 use PVE::Storage::Plugin;
@@ -132,6 +133,14 @@ sub get_volume_attribute {
 	return $class->get_volume_notes($scfg, $storeid, $volname);
     }
 
+    my ($vtype) = $class->parse_volname($volname);
+    return if $vtype ne 'backup';
+
+    if ($attribute eq 'protected') {
+	my $path = $class->filesystem_path($scfg, $volname);
+	return -e PVE::Storage::protection_file_path($path) ? 1 : 0;
+    }
+
     return;
 }
 
@@ -142,6 +151,27 @@ sub update_volume_attribute {
 	return $class->update_volume_notes($scfg, $storeid, $volname, $value);
     }
 
+    my ($vtype) = $class->parse_volname($volname);
+    die "only backups support attribute '$attribute'\n" if $vtype ne 'backup';
+
+    if ($attribute eq 'protected') {
+	my $path = $class->filesystem_path($scfg, $volname);
+	my $protection_path = PVE::Storage::protection_file_path($path);
+
+	return if !((-e $protection_path) xor $value); # protection status already correct
+
+	if ($value) {
+	    my $fh = IO::File->new($protection_path, O_CREAT, 0644)
+		or die "unable to create protection file '$protection_path' - $!\n";
+	    close($fh);
+	} else {
+	    unlink $protection_path or $! == ENOENT
+		or die "could not delete protection file '$protection_path' - $!\n";
+	}
+
+	return;
+    }
+
     die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
 }
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 1182008..1ebd705 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -782,6 +782,9 @@ sub alloc_image {
 sub free_image {
     my ($class, $storeid, $scfg, $volname, $isBase, $format) = @_;
 
+    die "cannot remove protected volume '$volname' on '$storeid'\n"
+	if $class->get_volume_attribute($scfg, $storeid, $volname, 'protected');
+
     my $path = $class->filesystem_path($scfg, $volname);
 
     if ($isBase) {
@@ -871,6 +874,7 @@ sub update_volume_notes {
 # Should die if there is an error fetching the attribute.
 # Possible attributes:
 # notes     - user-provided comments/notes.
+# protected - not to be removed by free_image, and for backups, ignored when pruning.
 sub get_volume_attribute {
     my ($class, $scfg, $storeid, $volname, $attribute) = @_;
 
@@ -1115,6 +1119,7 @@ my $get_subdir_files = sub {
 		$info->{notes} = $notes if defined($notes);
 	    }
 
+	    $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original);
 	} elsif ($tt eq 'snippets') {
 
 	    $info = {
@@ -1320,6 +1325,8 @@ sub prune_backups {
 	    $prune_entry->{mark} = 'protected';
 	}
 
+	$prune_entry->{mark} = 'protected' if $backup->{protected};
+
 	push @{$prune_list}, $prune_entry;
     }
 
diff --git a/test/prune_backups_test.pm b/test/prune_backups_test.pm
index a87c4b3..8ad6144 100644
--- a/test/prune_backups_test.pm
+++ b/test/prune_backups_test.pm
@@ -29,6 +29,12 @@ foreach my $vmid (@vmids) {
 	    'ctime' => $basetime - 24*60*60 - 60*60,
 	    'vmid'  => $vmid,
 	},
+	{
+	    'volid' => "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_18_51.tar.zst",
+	    'ctime' => $basetime - 24*60*60 - 60*60 + 30,
+	    'vmid'  => $vmid,
+	    'protected' => 1,
+	},
 	{
 	    'volid' => "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_19_21.tar.zst",
 	    'ctime' => $basetime - 24*60*60 - 60*60 + 60,
@@ -140,6 +146,13 @@ sub generate_expected {
 		'mark'  => $marks->[1],
 		'vmid'  => $vmid,
 	    },
+	    {
+		'volid' => "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_18_51.tar.zst",
+		'type'  => 'qemu',
+		'ctime' => $basetime - 24*60*60 - 60*60 + 30,
+		'mark'  => 'protected',
+		'vmid'  => $vmid,
+	    },
 	    {
 		'volid' => "$storeid:backup/vzdump-qemu-$vmid-2019_12_31-11_19_21.tar.zst",
 		'type'  => 'qemu',
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 6/7] prune: mark renamed and protected backups differently
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (4 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 5/7] fix #3307: make it possible to set protection for backups Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 7/7] pbs: integrate support for protected Fabian Ebner
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

While it makes no difference for pruning itself, protected backups are
additionally protected against removal. Avoid the potential to confuse
the two. Also update the description for the API return value and add
an enum constraint.

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

No changes from v1.

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

diff --git a/PVE/API2/Storage/PruneBackups.pm b/PVE/API2/Storage/PruneBackups.pm
index 2509a46..e6ab276 100644
--- a/PVE/API2/Storage/PruneBackups.pm
+++ b/PVE/API2/Storage/PruneBackups.pm
@@ -61,9 +61,10 @@ __PACKAGE__->register_method ({
 		    type => 'integer',
 		},
 		'mark' => {
-		    description => "Whether the backup would be kept or removed. For backups that don't " .
-				   "use the standard naming scheme, it's 'protected'.",
+		    description => "Whether the backup would be kept or removed. Backups that are" .
+			" protected or don't use the standard naming scheme are not removed.",
 		    type => 'string',
+		    enum => ['keep', 'remove', 'protected', 'renamed'],
 		},
 		type => {
 		    description => "One of 'qemu', 'lxc', 'openvz' or 'unknown'.",
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 1ebd705..a2145fa 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1322,7 +1322,7 @@ sub prune_backups {
 	    push @{$backup_groups->{$group}}, $prune_entry;
 	} else {
 	    # ignore backups that don't use the standard naming scheme
-	    $prune_entry->{mark} = 'protected';
+	    $prune_entry->{mark} = 'renamed';
 	}
 
 	$prune_entry->{mark} = 'protected' if $backup->{protected};
diff --git a/test/prune_backups_test.pm b/test/prune_backups_test.pm
index 8ad6144..b57d280 100644
--- a/test/prune_backups_test.pm
+++ b/test/prune_backups_test.pm
@@ -189,7 +189,7 @@ sub generate_expected {
 		'volid' => "$storeid:backup/vzdump-$vmid-renamed.tar.zst",
 		'type'  => 'unknown',
 		'ctime' => 1234,
-		'mark'  => 'protected',
+		'mark'  => 'renamed',
 		'vmid'  => $vmid,
 	    },
 	) if !defined($type);
@@ -375,7 +375,7 @@ my $tests = [
 	    {
 		'volid' => "$storeid:backup/vzdump-lxc-novmid.tar.gz",
 		'ctime' => 1234,
-		'mark'  => 'protected',
+		'mark'  => 'renamed',
 		'type'  => 'lxc',
 	    },
 	],
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 storage 7/7] pbs: integrate support for protected
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (5 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 6/7] prune: mark renamed and protected backups differently Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 1/5] vzdump: skip protected backups for dumpdir pruning Fabian Ebner
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

free_image doesn't need to check for protection, because that will
happen on the server.

Getting/updating notes has also been refactored to re-use the code
for the PBS api calls.

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

Changes from v1:
    * Rebase, because of newly introduced fall-back to
      {get, update}_volume_notes.
    * Set mark 'protected' rather than 'keep' if backup is protected
      in prune_backups().

Needs new external dependency for strptime, because it's not in perl's
POSIX module.

An alternative would be to use perlmod and export the proxmox crate's
function for parsing the timestring.

Also depends on Dominik's patches for PBS:
https://lists.proxmox.com/pipermail/pbs-devel/2021-September/004099.html

 PVE/Storage/PBSPlugin.pm | 66 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index 76699e6..06ebfa7 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -9,7 +9,8 @@ use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
 use IO::File;
 use JSON;
 use MIME::Base64 qw(decode_base64);
-use POSIX qw(strftime ENOENT);
+use POSIX qw(mktime strftime ENOENT);
+use POSIX::strptime;
 
 use PVE::APIClient::LWP;
 use PVE::JSONSchema qw(get_standard_option);
@@ -218,6 +219,36 @@ sub print_volid {
     return "${storeid}:${volname}";
 }
 
+# essentially the inverse of print_volid
+sub api_param_from_volname {
+    my ($class, $volname) = @_;
+
+    my $name = ($class->parse_volname($volname))[1];
+
+    my ($btype, $bid, $timestr) = split('/', $name);
+
+    my @tm = (POSIX::strptime($timestr, "%FT%TZ"));
+    # expect sec, min, hour, mday, mon, year
+    die "error parsing time from '$volname'" if grep { !defined($_) } @tm[0..5];
+
+    my $btime;
+    {
+	local $ENV{TZ} = 'UTC'; # $timestr is UTC
+
+	# Fill in isdst to avoid undef warning. No daylight saving time for UTC.
+	$tm[8] //= 0;
+
+	my $since_epoch = mktime(@tm) or die "error converting time from '$volname'\n";
+	$btime = int($since_epoch);
+    }
+
+    return {
+	'backup-type' => $btype,
+	'backup-id' => $bid,
+	'backup-time' => $btime,
+    };
+}
+
 my $USE_CRYPT_PARAMS = {
     backup => 1,
     restore => 1,
@@ -403,9 +434,12 @@ sub prune_backups {
 		my $vmid = $backup->{'backup-id'};
 		my $volid = print_volid($storeid, $type, $vmid, $ctime);
 
+		my $mark = $backup->{keep} ? 'keep' : 'remove';
+		$mark = 'protected' if $backup->{protected};
+
 		push @{$prune_list}, {
 		    ctime => $ctime,
-		    mark => $backup->{keep} ? 'keep' : 'remove',
+		    mark => $mark,
 		    type => $type eq 'vm' ? 'qemu' : 'lxc',
 		    vmid => $vmid,
 		    volid => $volid,
@@ -658,6 +692,7 @@ sub list_volumes {
 
 	$info->{verification} = $item->{verification} if defined($item->{verification});
 	$info->{notes} = $item->{comment} if defined($item->{comment});
+	$info->{protected} = 1 if $item->{protected};
 	if (defined($item->{fingerprint})) {
 	    $info->{encrypted} = $item->{fingerprint};
 	} elsif (snapshot_files_encrypted($item->{files})) {
@@ -813,6 +848,21 @@ sub get_volume_attribute {
 	return $class->get_volume_notes($scfg, $storeid, $volname);
     }
 
+    if ($attribute eq 'protected') {
+	my $param = $class->api_param_from_volname($volname);
+
+	my $password = pbs_get_password($scfg, $storeid);
+	my $conn = pbs_api_connect($scfg, $password);
+	my $datastore = $scfg->{datastore};
+
+	my $res = eval { $conn->get("/api2/json/admin/datastore/$datastore/$attribute", $param); };
+	if (my $err = $@) {
+	    return if $err->{code} == 404; # not supported
+	    die $err;
+	}
+	return $res;
+    }
+
     return;
 }
 
@@ -823,6 +873,18 @@ sub update_volume_attribute {
 	return $class->update_volume_notes($scfg, $storeid, $volname, $value);
     }
 
+    if ($attribute eq 'protected') {
+	my $param = $class->api_param_from_volname($volname);
+	$param->{$attribute} = $value;
+
+	my $password = pbs_get_password($scfg, $storeid);
+	my $conn = pbs_api_connect($scfg, $password);
+	my $datastore = $scfg->{datastore};
+
+	$conn->put("/api2/json/admin/datastore/$datastore/$attribute", $param);
+	return;
+    }
+
     die "attribute '$attribute' is not supported for storage type '$scfg->{type}'\n";
 }
 
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 manager 1/5] vzdump: skip protected backups for dumpdir pruning
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (6 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 7/7] pbs: integrate support for protected Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 2/5] ui: storage content: avoid redundant options hasNotesColumn and hideColumns Fabian Ebner
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

Keeps the behavior consistent with what happens for storages. It also
is required to not get into conflict with the check in archive_remove,
i.e. pruning here marks a backup as 'remove' and then archive_remove
complains that it's protected.

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

No changes from v1.

Dependency bump for pve-storage needed.

 PVE/VZDump.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index d00be8b2..a5a956c8 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -992,6 +992,13 @@ sub exec_backup_task {
 	    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';
+		    }
+		}
+
 		PVE::Storage::prune_mark_backup_group($bklist, $prune_options);
 
 		foreach my $prune_entry (@{$bklist}) {
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 manager 2/5] ui: storage content: avoid redundant options hasNotesColumn and hideColumns
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (7 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 1/5] vzdump: skip protected backups for dumpdir pruning Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 3/5] ui: backup views: add protected column Fabian Ebner
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

Replace both by a showColumns option instead. As the current use of
hasNotesColumn already indicates, when new content-specific columns
are added, it is more natural for each derived class to specify the
columns it wants, rather than those it doesn't.

For hideColumns, there was no user. For hasNotesColumn, the only user
was the backup view.

Set the column information in the storage.BackupView class itself
rather than the instance (like hasNotesColumn was).

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

New in v2.

 www/manager6/storage/BackupView.js  |  2 ++
 www/manager6/storage/Browser.js     |  1 -
 www/manager6/storage/ContentView.js | 14 ++++++++------
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 0613c94d..5fec3b18 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -3,6 +3,8 @@ Ext.define('PVE.storage.BackupView', {
 
     alias: 'widget.pveStorageBackupView',
 
+    showColumns: ['name', 'notes', 'date', 'format', 'size'],
+
     initComponent: function() {
 	var me = this;
 
diff --git a/www/manager6/storage/Browser.js b/www/manager6/storage/Browser.js
index fe5df3e2..1916ff6a 100644
--- a/www/manager6/storage/Browser.js
+++ b/www/manager6/storage/Browser.js
@@ -63,7 +63,6 @@ Ext.define('PVE.storage.Browser', {
 		    iconCls: 'fa fa-floppy-o',
 		    itemId: 'contentBackup',
 		    pluginType: plugin,
-		    hasNotesColumn: true,
 		});
 	    }
 	    if (contents.includes('images')) {
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index 3f5b686b..9ba2b4ce 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -371,12 +371,14 @@ Ext.define('PVE.storage.ContentView', {
 	    },
 	};
 
-	if (me.hideColumns) {
-	    me.hideColumns.forEach(key => delete availableColumns[key]);
-	}
-	if (!me.hasNotesColumn) {
-	    delete availableColumns.notes;
-	}
+	let showColumns = me.showColumns || ['name', 'date', 'format', 'size'];
+
+	Object.keys(availableColumns).forEach(function(key) {
+	    if (!showColumns.includes(key)) {
+		delete availableColumns[key];
+	    }
+	});
+
 	if (me.extraColumns && typeof me.extraColumns === 'object') {
 	    Object.assign(availableColumns, me.extraColumns);
 	}
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 manager 3/5] ui: backup views: add protected column
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (8 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 2/5] ui: storage content: avoid redundant options hasNotesColumn and hideColumns Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 4/5] ui: backup views: add button to change protection status Fabian Ebner
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

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

New in v2.

 www/manager6/grid/BackupView.js     | 6 ++++++
 www/manager6/storage/BackupView.js  | 2 +-
 www/manager6/storage/ContentView.js | 6 ++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index 8825ed96..3731c985 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -320,6 +320,12 @@ Ext.define('PVE.grid.BackupView', {
 		    flex: 1,
 		    renderer: Ext.htmlEncode,
 		},
+		{
+		    header: gettext('Protected'),
+		    width: 100,
+		    renderer: Proxmox.Utils.format_boolean,
+		    dataIndex: 'protected',
+		},
 		{
 		    header: gettext('Date'),
 		    width: 150,
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 5fec3b18..359f5f18 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -3,7 +3,7 @@ Ext.define('PVE.storage.BackupView', {
 
     alias: 'widget.pveStorageBackupView',
 
-    showColumns: ['name', 'notes', 'date', 'format', 'size'],
+    showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
 
     initComponent: function() {
 	var me = this;
diff --git a/www/manager6/storage/ContentView.js b/www/manager6/storage/ContentView.js
index 9ba2b4ce..01249d1f 100644
--- a/www/manager6/storage/ContentView.js
+++ b/www/manager6/storage/ContentView.js
@@ -353,6 +353,12 @@ Ext.define('PVE.storage.ContentView', {
 		renderer: Ext.htmlEncode,
 		dataIndex: 'notes',
 	    },
+	    'protected': {
+		header: gettext('Protected'),
+		width: 100,
+		renderer: Proxmox.Utils.format_boolean,
+		dataIndex: 'protected',
+	    },
 	    'date': {
 		header: gettext('Date'),
 		width: 150,
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 manager 4/5] ui: backup views: add button to change protection status
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (9 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 3/5] ui: backup views: add protected column Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 5/5] ui: prune: also handle new 'renamed' keep reason Fabian Ebner
  2021-11-09 16:51 ` [pve-devel] applied-series: [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Grünbichler
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

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

New in v2.

 www/manager6/grid/BackupView.js    | 19 +++++++++++++++++++
 www/manager6/storage/BackupView.js | 16 ++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index 3731c985..b8407104 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -297,6 +297,25 @@ Ext.define('PVE.grid.BackupView', {
 			    }).show();
 			},
 		    },
+		    {
+			xtype: 'proxmoxButton',
+			text: gettext('Change Protection'),
+			disabled: true,
+			handler: function(button, event, record) {
+			    const volid = record.data.volid;
+			    const storage = storagesel.getValue();
+			    const url =
+				`/api2/extjs/nodes/${nodename}/storage/${storage}/content/${volid}`;
+			    Proxmox.Utils.API2Request({
+				url: url,
+				method: 'PUT',
+				waitMsgTarget: me,
+				params: { 'protected': record.data.protected ? 0 : 1 },
+				failure: (response) => Ext.Msg.alert('Error', response.htmlStatus),
+				success: (response) => reload(),
+			    });
+			},
+		    },
 		    '-',
 		    delete_btn,
 		    '->',
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 359f5f18..dca140fe 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -171,6 +171,22 @@ Ext.define('PVE.storage.BackupView', {
 		    }).show();
 		},
 	    },
+	    {
+		xtype: 'proxmoxButton',
+		text: gettext('Change Protection'),
+		disabled: true,
+		handler: function(button, event, record) {
+		    const volid = record.data.volid;
+		    Proxmox.Utils.API2Request({
+			url: `/api2/extjs/nodes/${nodename}/storage/${me.storage}/content/${volid}`,
+			method: 'PUT',
+			waitMsgTarget: me,
+			params: { 'protected': record.data.protected ? 0 : 1 },
+			failure: (response) => Ext.Msg.alert('Error', response.htmlStatus),
+			success: (response) => reload(),
+		    });
+		},
+	    },
 	    '-',
 	    pruneButton,
 	);
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] [PATCH v2 manager 5/5] ui: prune: also handle new 'renamed' keep reason
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (10 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 4/5] ui: backup views: add button to change protection status Fabian Ebner
@ 2021-09-30 11:42 ` Fabian Ebner
  2021-11-09 16:51 ` [pve-devel] applied-series: [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Grünbichler
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Ebner @ 2021-09-30 11:42 UTC (permalink / raw)
  To: pve-devel

The back-end no longer returns 'protected' for renamed backups, and
'protected' backups are now their own kind.

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

New in v2.

 www/manager6/window/Prune.js | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/www/manager6/window/Prune.js b/www/manager6/window/Prune.js
index 3056ab20..6156fe03 100644
--- a/www/manager6/window/Prune.js
+++ b/www/manager6/window/Prune.js
@@ -186,6 +186,8 @@ Ext.define('PVE.PruneInputPanel', {
 			    if (record.data.mark === 'keep') {
 				return 'true (' + record.data.keepReason + ')';
 			    } else if (record.data.mark === 'protected') {
+				return 'true (protected)';
+			    } else if (record.data.mark === 'renamed') {
 				return 'true (renamed)';
 			    } else {
 				return 'false';
-- 
2.30.2





^ permalink raw reply	[flat|nested] 14+ messages in thread

* [pve-devel] applied-series: [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected
  2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
                   ` (11 preceding siblings ...)
  2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 5/5] ui: prune: also handle new 'renamed' keep reason Fabian Ebner
@ 2021-11-09 16:51 ` Fabian Grünbichler
  12 siblings, 0 replies; 14+ messages in thread
From: Fabian Grünbichler @ 2021-11-09 16:51 UTC (permalink / raw)
  To: Proxmox VE development discussion

On September 30, 2021 1:42 pm, Fabian Ebner wrote:
> Protected bacukps cannot be removed accidentally and will be ignored
> for pruning. A <backup>.protected file serves as a protection marker
> for file-based storages.
> 
> 
> Changes from v1:
>     * Avoid races when using unlink.
>     * Also add fall-back for {get, update}_volume_notes to other
>       plugin implementations, because external plugins might be
>       derived from those too.
>     * Add UI integration patches.
> 
> 
> For the storage part, an APIAGE+APIVER bump is needed.
> Dependency bump from pve-manager to pve-storage is needed.
> 
> 
> To work, the PBS integration needs Dominik's patches for PBS (seems
> like a rebase is needed for those, I tested on top of v2.0.10):
> https://lists.proxmox.com/pipermail/pbs-devel/2021-September/004099.html
> 
> 
> pve-storage:
> 
> Fabian Ebner (7):
>   dir plugin: update notes: don't fail if file is already removed
>   dir plugin: get notes: return undef if notes are not supported
>   add generalized functions to manage volume attributes
>   prune mark: preserve additional information for the keep-all case
>   fix #3307: make it possible to set protection for backups
>   prune: mark renamed and protected backups differently
>   pbs: integrate support for protected
> 
>  PVE/API2/Storage/Content.pm      | 36 ++++++++++---
>  PVE/API2/Storage/PruneBackups.pm |  5 +-
>  PVE/Storage.pm                   | 23 +++++---
>  PVE/Storage/BTRFSPlugin.pm       |  4 +-
>  PVE/Storage/CIFSPlugin.pm        | 13 +++++
>  PVE/Storage/CephFSPlugin.pm      | 12 +++++
>  PVE/Storage/DirPlugin.pm         | 62 +++++++++++++++++++++-
>  PVE/Storage/NFSPlugin.pm         | 13 +++++
>  PVE/Storage/PBSPlugin.pm         | 90 +++++++++++++++++++++++++++++++-
>  PVE/Storage/Plugin.pm            | 45 +++++++++++++++-
>  test/prune_backups_test.pm       | 17 +++++-
>  11 files changed, 296 insertions(+), 24 deletions(-)
> 
> 
> pve-manager:
> 
> Fabian Ebner (5):
>   vzdump: skip protected backups for dumpdir pruning
>   ui: storage content: avoid redundant options hasNotesColumn and
>     hideColumns
>   ui: backup views: add protected column
>   ui: backup views: add button to change protection status
>   ui: prune: also handle new 'renamed' keep reason
> 
>  PVE/VZDump.pm                       |  7 +++++++
>  www/manager6/grid/BackupView.js     | 25 +++++++++++++++++++++++++
>  www/manager6/storage/BackupView.js  | 18 ++++++++++++++++++
>  www/manager6/storage/Browser.js     |  1 -
>  www/manager6/storage/ContentView.js | 20 ++++++++++++++------
>  www/manager6/window/Prune.js        |  2 ++
>  6 files changed, 66 insertions(+), 7 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 




^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-11-09 16:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 11:42 [pve-devel] [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 1/7] dir plugin: update notes: don't fail if file is already removed Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 2/7] dir plugin: get notes: return undef if notes are not supported Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 3/7] add generalized functions to manage volume attributes Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 4/7] prune mark: preserve additional information for the keep-all case Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 5/7] fix #3307: make it possible to set protection for backups Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 6/7] prune: mark renamed and protected backups differently Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 storage 7/7] pbs: integrate support for protected Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 1/5] vzdump: skip protected backups for dumpdir pruning Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 2/5] ui: storage content: avoid redundant options hasNotesColumn and hideColumns Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 3/5] ui: backup views: add protected column Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 4/5] ui: backup views: add button to change protection status Fabian Ebner
2021-09-30 11:42 ` [pve-devel] [PATCH v2 manager 5/5] ui: prune: also handle new 'renamed' keep reason Fabian Ebner
2021-11-09 16:51 ` [pve-devel] applied-series: [PATCH-SERIES v2 manager/storage] fix #3307: allow backups to be marked as protected Fabian Grünbichler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal