[pbs-devel] [RFC PATCH proxmox-backup{, -qemu}, pve-{qemu, manager}, qemu-server 0/6] fix #4289: Set protected flag on backup creation

[pbs-devel] [RFC PATCH proxmox-backup{, -qemu}, pve-{qemu, manager}, qemu-server 0/6] fix #4289: Set protected flag on backup creation

[Christoph Heiss]

TL;DR
-----
This fixes '#4289: Making protected backups from Web UI with "Verify
New" in datastore enabled fails' by introducing a new API parameter (and
corresponding CLI switches) for setting a backup as protected before
finishing the backup job.

The problem
-----------
When a datastore has the "Verify new" flag set and a backup with the
protected flag set is created (e.g. using `vzdump --protected 1 ..`),
the job might fail in the final stages due to a locking race. The
"Verify new" flag means that backups are immediately locked for
verification as soon as their transfer is finished and the `/finish`
endpoint is called.

If vzdump then tries to set the `protected` flag on that backup, it can
fail due to being currently locked by the verification task.

Prior art
---------
A prior, very naive approach to solve this was to simply wait for the
verification to finish. Due to multiple problems and the following
discussion [0], I started from scratch to implement it properly.

The solution
------------
This adds a new parameter `protected` to the `/finish` endpoint of the
backup API. If this parameter is set to `true`, the backup will be set
as protected before finishing and unlocking it. It defaults to
unprotected as before.

The `proxmox-backup-client` gets a new CLI switch `--protected`, in the
same manner as `vzdump`. This simply determines the value of the
`protected` API parameter.

Further, this also introduces an (unfortunately) *API-breaking* change
to the QEMU side of things (patches 3 & 4). This is needed so that this
works with disk backups of VMs. If there is some better way, I'd be
happy to iterate on it.

Fiona already hinted me at the problem on how to handle these
API-breaking changes, since we do not force a specific QEMU version upon
users.

Regarding patch 6: This check might introduce a time-of-check <=>
time-of-set race - could this ever be a problem?

Notes
-----
I marked this whole series RFC, to generally get feedback on the overall
approach and further guidance on how such API/ABI-breaking things need
to be handled.

Also, although patches 4, 5 & 6 are PVE-specific, I opted to send the
whole series to pbs-devel for this spin, to ease reviewing the whole
series. Later on I can split these out for pve-devel as/if needed.

Patches 1 & 2 are otherwise completely independent of the other patches,
as they only add the API parameter.

[0] https://lists.proxmox.com/pipermail/pve-devel/2023-January/055306.html

---
proxmox-backup:

Christoph Heiss (2):
      api2: Add `protected` parameter to `finish` endpoint
      client: Add `--protected` CLI flag to backup command

 pbs-client/src/backup_writer.rs   |  4 ++--
 proxmox-backup-client/src/main.rs | 11 ++++++++++-
 src/api2/backup/environment.rs    | 11 +++++++++++
 src/api2/backup/mod.rs            | 25 ++++++++++++++++++++-----
 4 files changed, 43 insertions(+), 8 deletions(-)

proxmox-backup-qemu:

Christoph Heiss (1):
      api: Supply `protected` parameter to the `finish` API call

 current-api.h   | 3 ++-
 simpletest.c    | 2 +-
 src/backup.rs   | 3 ++-
 src/commands.rs | 5 ++++-
 src/lib.rs      | 5 ++++-
 5 files changed, 13 insertions(+), 5 deletions(-)

pve-qemu:

Christoph Heiss (1):
      pve: Add patch to support new proxmox-backup-qemu API

 ...Add-support-for-protected-flag-to-proxmox.patch | 150 +++++++++++++++++++++
 debian/patches/series                              |   1 +
 2 files changed, 151 insertions(+)

qemu-server:

Christoph Heiss (1):
      vzdump: Set protected flag on backup start if supported

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

pve-manager:

Christoph Heiss (1):
      vzdump: Only set protected attribute if not already done

 PVE/VZDump.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
2.34.1

[pbs-devel] [RFC PATCH proxmox-backup 1/6] api2: Add `protected` parameter to `finish` endpoint

[Christoph Heiss]

Groundwork for fixing #4289. This way, a backup can be set as protected
without a separate API call and thus avoid locking problems.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 pbs-client/src/backup_writer.rs |  4 ++--
 src/api2/backup/environment.rs  | 11 +++++++++++
 src/api2/backup/mod.rs          | 25 ++++++++++++++++++++-----
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index be6da2a6..3e4b4878 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -165,10 +165,10 @@ impl BackupWriter {
         self.h2.upload("PUT", path, param, content_type, data).await
     }

-    pub async fn finish(self: Arc<Self>) -> Result<(), Error> {
+    pub async fn finish(self: Arc<Self>, param: Option<Value>) -> Result<(), Error> {
         let h2 = self.h2.clone();

-        h2.post("finish", None)
+        h2.post("finish", param)
             .map_ok(move |_| {
                 self.abort.abort();
             })
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 4f07f9b4..05ee12ea 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -590,6 +590,17 @@ impl BackupEnvironment {
         Ok(())
     }

+    /// Sets the backup as protected.
+    pub fn set_protected(&self) -> Result<(), Error> {
+        let state = self.state.lock().unwrap();
+        state.ensure_unfinished()?;
+
+        let protected_path = self.backup_dir.protected_file();
+        std::fs::File::create(protected_path)
+            .map(|_| ())
+            .map_err(|err| format_err!("could not create protection file: {}", err))
+    }
+
     /// Mark backup as finished
     pub fn finish_backup(&self) -> Result<(), Error> {
         let mut state = self.state.lock().unwrap();
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 90e2ea7e..decad084 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -344,10 +344,7 @@ const BACKUP_API_SUBDIRS: SubdirMap = &[
     ),
     (
         "finish",
-        &Router::new().post(&ApiMethod::new(
-            &ApiHandler::Sync(&finish_backup),
-            &ObjectSchema::new("Mark backup as finished.", &[]),
-        )),
+        &Router::new().post(&API_METHOD_FINISH_BACKUP),
     ),
     (
         "fixed_chunk",
@@ -771,12 +768,30 @@ fn close_fixed_index(
     Ok(Value::Null)
 }

+#[sortable]
+const API_METHOD_FINISH_BACKUP: ApiMethod = ApiMethod::new(
+    &ApiHandler::Sync(&finish_backup),
+    &ObjectSchema::new(
+        "Mark backup as finished.",
+        &sorted!([
+            ("protected", true, &BooleanSchema::new("Mark backup as protected").schema()),
+        ]),
+    ),
+);
+
 fn finish_backup(
-    _param: Value,
+    param: Value,
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
     let env: &BackupEnvironment = rpcenv.as_ref();
+    let protected = param["protected"].as_bool().unwrap_or(false);
+
+    if protected {
+        if let Err(err) = env.set_protected() {
+            env.log(format!("failed to set backup protected: {}", err));
+        }
+    }

     env.finish_backup()?;
     env.log("successfully finished backup");
--
2.34.1

[pbs-devel] [RFC PATCH proxmox-backup 2/6] client: Add `--protected` CLI flag to backup command

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-backup-client/src/main.rs | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 55198108..ee09db83 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -663,6 +663,12 @@ fn spawn_catalog_upload(
                optional: true,
                default: false,
            },
+           "protected": {
+               type: Boolean,
+               description: "Set backup as protected after upload.",
+               optional: true,
+               default: false,
+           },
        }
    }
 )]
@@ -716,6 +722,7 @@ async fn create_backup(

     let empty = Vec::new();
     let exclude_args = param["exclude"].as_array().unwrap_or(&empty);
+    let protected = param["protected"].as_bool().unwrap_or(false);

     let mut pattern_list = Vec::with_capacity(exclude_args.len());
     for entry in exclude_args {
@@ -1082,7 +1089,9 @@ async fn create_backup(
         .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
         .await?;

-    client.finish().await?;
+    client.finish(Some(json!({
+        "protected": protected,
+    }))).await?;

     let end_time = std::time::Instant::now();
     let elapsed = end_time.duration_since(start_time);
--
2.34.1

[pbs-devel] [RFC PATCH proxmox-backup-qemu 3/6] api: Supply `protected` parameter to the `finish` API call

[Christoph Heiss]

This can be used to set backups as protected as soon as the transfer is
finished, to e.g. avoid races while trying to set it protected later on,
while the PBS might have locked it already for verification.

! This is a breaking API/ABI change !

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 current-api.h   | 3 ++-
 simpletest.c    | 2 +-
 src/backup.rs   | 3 ++-
 src/commands.rs | 5 ++++-
 src/lib.rs      | 5 ++++-
 5 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/current-api.h b/current-api.h
index f38ad4b..729a3a3 100644
--- a/current-api.h
+++ b/current-api.h
@@ -271,7 +271,7 @@ void proxmox_backup_close_image_async(struct ProxmoxBackupHandle *handle,
 /**
  * Finish the backup (sync)
  */
-int proxmox_backup_finish(struct ProxmoxBackupHandle *handle, char **error);
+int proxmox_backup_finish(struct ProxmoxBackupHandle *handle, bool protected_, char **error);

 /**
  * Finish the backup
@@ -280,6 +280,7 @@ int proxmox_backup_finish(struct ProxmoxBackupHandle *handle, char **error);
  * All registered images have to be closed before calling this.
  */
 void proxmox_backup_finish_async(struct ProxmoxBackupHandle *handle,
+                                 bool protected_,
                                  void (*callback)(void*),
                                  void *callback_data,
                                  int *result,
diff --git a/simpletest.c b/simpletest.c
index ceb5afd..b061bde 100644
--- a/simpletest.c
+++ b/simpletest.c
@@ -77,7 +77,7 @@ void main(int argc, char **argv) {
   }

   printf("finish backup\n");
-  if (proxmox_backup_finish(pbs, &pbs_error) < 0) {
+  if (proxmox_backup_finish(pbs, false, &pbs_error) < 0) {
     fprintf(stderr, "proxmox_backup_finish failed - %s\n", pbs_error);
     proxmox_backup_free_error(pbs_error);
     exit(-1);
diff --git a/src/backup.rs b/src/backup.rs
index bbe4f00..8a7b566 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -286,7 +286,7 @@ impl BackupTask {
         abortable_command(command_future, abort_rx.recv()).await
     }

-    pub async fn finish(&self) -> Result<c_int, Error> {
+    pub async fn finish(&self, protected: bool) -> Result<c_int, Error> {
         self.check_aborted()?;

         let command_future = finish_backup(
@@ -294,6 +294,7 @@ impl BackupTask {
             self.crypt_config.clone(),
             self.rsa_encrypted_key.clone(),
             Arc::clone(&self.manifest),
+            protected,
         );

         let mut abort_rx = self.abort.subscribe();
diff --git a/src/commands.rs b/src/commands.rs
index 37d653c..52cdd8b 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -473,6 +473,7 @@ pub(crate) async fn finish_backup(
     crypt_config: Option<Arc<CryptConfig>>,
     rsa_encrypted_key: Option<Vec<u8>>,
     manifest: Arc<Mutex<BackupManifest>>,
+    protected: bool,
 ) -> Result<c_int, Error> {
     if let Some(rsa_encrypted_key) = rsa_encrypted_key {
         let target = ENCRYPTED_KEY_BLOB_NAME;
@@ -521,7 +522,9 @@ pub(crate) async fn finish_backup(
         .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
         .await?;

-    client.finish().await?;
+    client.finish(Some(json!({
+        "protected": protected,
+    }))).await?;

     Ok(0)
 }
diff --git a/src/lib.rs b/src/lib.rs
index b3c7b85..2fe234b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -703,6 +703,7 @@ pub extern "C" fn proxmox_backup_close_image_async(
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
 pub extern "C" fn proxmox_backup_finish(
     handle: *mut ProxmoxBackupHandle,
+    protected: bool,
     error: *mut *mut c_char,
 ) -> c_int {
     let mut result: c_int = -1;
@@ -713,6 +714,7 @@ pub extern "C" fn proxmox_backup_finish(

     proxmox_backup_finish_async(
         handle,
+        protected,
         callback_info.callback,
         callback_info.callback_data,
         callback_info.result,
@@ -732,6 +734,7 @@ pub extern "C" fn proxmox_backup_finish(
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
 pub extern "C" fn proxmox_backup_finish_async(
     handle: *mut ProxmoxBackupHandle,
+    protected: bool,
     callback: extern "C" fn(*mut c_void),
     callback_data: *mut c_void,
     result: *mut c_int,
@@ -746,7 +749,7 @@ pub extern "C" fn proxmox_backup_finish_async(
     };

     task.runtime().spawn(async move {
-        let result = task.finish().await;
+        let result = task.finish(protected).await;
         callback_info.send_result(result);
     });
 }
--
2.34.1

[pbs-devel] [RFC PATCH pve-qemu 4/6] pve: Add patch to support new proxmox-backup-qemu API

[Christoph Heiss]

Adds a QEMU patch to support the updated API from proxmox-backup-qemu,
which adds a `protected_` parameter to `proxmox_backup_finish{,_async}`.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 ...upport-for-protected-flag-to-proxmox.patch | 150 ++++++++++++++++++
 debian/patches/series                         |   1 +
 2 files changed, 151 insertions(+)
 create mode 100644 debian/patches/pve/0063-PVE-Backup-Add-support-for-protected-flag-to-proxmox.patch

diff --git a/debian/patches/pve/0063-PVE-Backup-Add-support-for-protected-flag-to-proxmox.patch b/debian/patches/pve/0063-PVE-Backup-Add-support-for-protected-flag-to-proxmox.patch
new file mode 100644
index 0000000..4d69abd
--- /dev/null
+++ b/debian/patches/pve/0063-PVE-Backup-Add-support-for-protected-flag-to-proxmox.patch
@@ -0,0 +1,150 @@
+From b40c4fe9cc04c95ddaf405fdb4d57cd000f91944 Mon Sep 17 00:00:00 2001
+From: Christoph Heiss <c.heiss@proxmox.com>
+Date: Wed, 18 Jan 2023 10:35:01 +0100
+Subject: [PATCH] PVE-Backup: Add support for `protected` flag to
+ proxmox-backup-client
+
+Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
+---
+ block/monitor/block-hmp-cmds.c |  1 +
+ proxmox-backup-client.c        |  3 ++-
+ proxmox-backup-client.h        |  1 +
+ pve-backup.c                   |  6 +++++-
+ qapi/block-core.json           | 11 +++++++++--
+ 5 files changed, 18 insertions(+), 4 deletions(-)
+
+diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
+index ab0c988ae9..fef9a25774 100644
+--- a/block/monitor/block-hmp-cmds.c
++++ b/block/monitor/block-hmp-cmds.c
+@@ -1051,6 +1051,7 @@ void coroutine_fn hmp_backup(Monitor *mon, const QDict *qdict)
+         false, NULL, false, NULL, !!devlist,
+         devlist, qdict_haskey(qdict, "speed"), speed,
+         false, 0, // BackupPerf max-workers
++        false, false, // PBS protected
+         &error);
+
+     hmp_handle_error(mon, error);
+diff --git a/proxmox-backup-client.c b/proxmox-backup-client.c
+index 0923037dec..50691e0993 100644
+--- a/proxmox-backup-client.c
++++ b/proxmox-backup-client.c
+@@ -80,6 +80,7 @@ proxmox_backup_co_register_image(
+ int coroutine_fn
+ proxmox_backup_co_finish(
+     ProxmoxBackupHandle *pbs,
++    bool protected_,
+     Error **errp)
+ {
+     Coroutine *co = qemu_coroutine_self();
+@@ -89,7 +90,7 @@ proxmox_backup_co_finish(
+     int pbs_res = -1;
+
+     proxmox_backup_finish_async(
+-        pbs, proxmox_backup_schedule_wake, &waker, &pbs_res, &pbs_err);
++        pbs, protected_, proxmox_backup_schedule_wake, &waker, &pbs_res, &pbs_err);
+     qemu_coroutine_yield();
+     if (pbs_res < 0) {
+         if (errp) error_setg(errp, "backup finish failed: %s", pbs_err ? pbs_err : "unknown error");
+diff --git a/proxmox-backup-client.h b/proxmox-backup-client.h
+index 8cbf645b2c..12c128f37c 100644
+--- a/proxmox-backup-client.h
++++ b/proxmox-backup-client.h
+@@ -39,6 +39,7 @@ proxmox_backup_co_register_image(
+ int coroutine_fn
+ proxmox_backup_co_finish(
+     ProxmoxBackupHandle *pbs,
++    bool protected_,
+     Error **errp);
+
+ int coroutine_fn
+diff --git a/pve-backup.c b/pve-backup.c
+index 3ca4f74cb8..d2469e6065 100644
+--- a/pve-backup.c
++++ b/pve-backup.c
+@@ -55,6 +55,7 @@ static struct PVEBackupState {
+         bool starting;
+     } stat;
+     int64_t speed;
++    bool protected;
+     BackupPerf perf;
+     VmaWriter *vmaw;
+     ProxmoxBackupHandle *pbs;
+@@ -257,7 +258,7 @@ static void coroutine_fn pvebackup_co_cleanup(void)
+     if (backup_state.pbs) {
+         if (!pvebackup_error_or_canceled()) {
+             Error *local_err = NULL;
+-            proxmox_backup_co_finish(backup_state.pbs, &local_err);
++            proxmox_backup_co_finish(backup_state.pbs, backup_state.protected, &local_err);
+             if (local_err != NULL) {
+                 pvebackup_propagate_error(local_err);
+             }
+@@ -585,6 +586,7 @@ UuidInfo coroutine_fn *qmp_backup(
+     bool has_devlist, const char *devlist,
+     bool has_speed, int64_t speed,
+     bool has_max_workers, int64_t max_workers,
++    bool has_protected, bool protected,
+     Error **errp)
+ {
+     assert(qemu_in_coroutine());
+@@ -914,6 +916,7 @@ UuidInfo coroutine_fn *qmp_backup(
+     qemu_mutex_unlock(&backup_state.stat.lock);
+
+     backup_state.speed = (has_speed && speed > 0) ? speed : 0;
++    backup_state.protected = has_protected ? protected : false;
+
+     backup_state.perf = (BackupPerf){ .max_workers = 16 };
+     if (has_max_workers) {
+@@ -1096,5 +1099,6 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp)
+     ret->query_bitmap_info = true;
+     ret->pbs_masterkey = true;
+     ret->backup_max_workers = true;
++    ret->pbs_protected_flag = true;
+     return ret;
+ }
+diff --git a/qapi/block-core.json b/qapi/block-core.json
+index 65795b7204..01d304022b 100644
+--- a/qapi/block-core.json
++++ b/qapi/block-core.json
+@@ -831,6 +831,8 @@
+ #
+ # @max-workers: see @BackupPerf for details. Default 16.
+ #
++# @protected: set backup as protected when finished (only for format 'pbs', defaults to false)
++#
+ # Returns: the uuid of the backup job
+ #
+ ##
+@@ -851,7 +853,8 @@
+                                     '*firewall-file': 'str',
+                                     '*devlist': 'str',
+                                     '*speed': 'int',
+-                                    '*max-workers': 'int' },
++                                    '*max-workers': 'int',
++                                    '*protected': 'bool' },
+   'returns': 'UuidInfo', 'coroutine': true }
+
+ ##
+@@ -899,6 +902,9 @@
+ #
+ # @pbs-library-version: Running version of libproxmox-backup-qemu0 library.
+ #
++# @pbs-protected-flag: True if the QMP backup call supports the
++#                      'protected' parameter.
++#
+ ##
+ { 'struct': 'ProxmoxSupportStatus',
+   'data': { 'pbs-dirty-bitmap': 'bool',
+@@ -907,7 +913,8 @@
+             'pbs-dirty-bitmap-migration': 'bool',
+             'pbs-masterkey': 'bool',
+             'pbs-library-version': 'str',
+-            'backup-max-workers': 'bool' } }
++            'backup-max-workers': 'bool',
++            'pbs-protected-flag': 'bool' } }
+
+ ##
+ # @query-proxmox-support:
+--
+2.34.1
+
diff --git a/debian/patches/series b/debian/patches/series
index f8e3fe8..7c44728 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -72,3 +72,4 @@ pve/0059-vma-create-support-64KiB-unaligned-input-images.patch
 pve/0060-vma-create-avoid-triggering-assertion-in-error-case.patch
 pve/0061-block-alloc-track-avoid-premature-break.patch
 pve/0062-PVE-Backup-allow-passing-max-workers-performance-set.patch
+pve/0063-PVE-Backup-Add-support-for-protected-flag-to-proxmox.patch
--
2.34.1

[pbs-devel] [RFC PATCH qemu-server 5/6] vzdump: Set protected flag on backup start if supported

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 PVE/VZDump/QemuServer.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 0eb5ec6..1d1131c 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -477,6 +477,13 @@ my sub add_backup_performance_options {
     }
 }

+my sub backup_client_supports_protected_flag {
+    my $supported;
+    PVE::Tools::run_command(['/usr/bin/proxmox-backup-client', 'help', 'backup'],
+                            logfunc => sub { $supported = 1 if shift =~ m/--protected/ });
+    return $supported;
+}
+
 sub archive_pbs {
     my ($self, $task, $vmid) = @_;

@@ -515,6 +522,10 @@ sub archive_pbs {
 	    push @$cmd, '--ns', $ns;
 	}

+	if ($opts->{protected} && backup_client_supports_protected_flag()) {
+	    push @$cmd, '--protected', '1';
+	}
+
 	push @$cmd, "qemu-server.conf:$conffile";
 	push @$cmd, "fw.conf:$firewall" if -e $firewall;

@@ -603,6 +614,9 @@ sub archive_pbs {
 	$params->{'use-dirty-bitmap'} = JSON::true
 	    if $qemu_support->{'pbs-dirty-bitmap'} && !$is_template;

+	$params->{'protected'} = JSON::true
+	    if $qemu_support->{'pbs-protected-flag'} && $opts->{protected};
+
 	$params->{timeout} = 125; # give some time to connect to the backup server

 	my $res = eval { mon_cmd($vmid, "backup", %$params) };
--
2.34.1

[pbs-devel] [RFC PATCH pve-manager 6/6] vzdump: Only set protected attribute if not already done

[Christoph Heiss]

This works together with the new mechanism for PBS backups which set the
backup as protected directly on finishing it.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 PVE/VZDump.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index a04837e7..6c9fff7f 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1095,7 +1095,8 @@ sub exec_backup_task {
 		}
 	    }

-	    if ($opts->{protected}) {
+	    my $already_protected = PVE::Storage::get_volume_attribute($cfg, $volid, 'protected');
+	    if ($opts->{protected} && !$already_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 $@;
--
2.34.1

Re: [pbs-devel] [RFC PATCH proxmox-backup 1/6] api2: Add `protected` parameter to `finish` endpoint

[Fabian Grünbichler]

On January 18, 2023 11:48 am, Christoph Heiss wrote:
> Groundwork for fixing #4289. This way, a backup can be set as protected
> without a separate API call and thus avoid locking problems.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  pbs-client/src/backup_writer.rs |  4 ++--
>  src/api2/backup/environment.rs  | 11 +++++++++++
>  src/api2/backup/mod.rs          | 25 ++++++++++++++++++++-----
>  3 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
> index be6da2a6..3e4b4878 100644
> --- a/pbs-client/src/backup_writer.rs
> +++ b/pbs-client/src/backup_writer.rs
> @@ -165,10 +165,10 @@ impl BackupWriter {
>          self.h2.upload("PUT", path, param, content_type, data).await
>      }
> 
> -    pub async fn finish(self: Arc<Self>) -> Result<(), Error> {
> +    pub async fn finish(self: Arc<Self>, param: Option<Value>) -> Result<(), Error> {
>          let h2 = self.h2.clone();
> 
> -        h2.post("finish", None)
> +        h2.post("finish", param)
>              .map_ok(move |_| {
>                  self.abort.abort();
>              })

IMHO this hunk belongs in the next patch (it's not about the endpoint, but about
the client)

> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
> index 4f07f9b4..05ee12ea 100644
> --- a/src/api2/backup/environment.rs
> +++ b/src/api2/backup/environment.rs
> @@ -590,6 +590,17 @@ impl BackupEnvironment {
>          Ok(())
>      }
> 
> +    /// Sets the backup as protected.
> +    pub fn set_protected(&self) -> Result<(), Error> {
> +        let state = self.state.lock().unwrap();
> +        state.ensure_unfinished()?;
> +
> +        let protected_path = self.backup_dir.protected_file();
> +        std::fs::File::create(protected_path)
> +            .map(|_| ())
> +            .map_err(|err| format_err!("could not create protection file: {}", err))
> +    }
> +
>      /// Mark backup as finished
>      pub fn finish_backup(&self) -> Result<(), Error> {
>          let mut state = self.state.lock().unwrap();
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 90e2ea7e..decad084 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -344,10 +344,7 @@ const BACKUP_API_SUBDIRS: SubdirMap = &[
>      ),
>      (
>          "finish",
> -        &Router::new().post(&ApiMethod::new(
> -            &ApiHandler::Sync(&finish_backup),
> -            &ObjectSchema::new("Mark backup as finished.", &[]),
> -        )),
> +        &Router::new().post(&API_METHOD_FINISH_BACKUP),
>      ),
>      (
>          "fixed_chunk",
> @@ -771,12 +768,30 @@ fn close_fixed_index(
>      Ok(Value::Null)
>  }
> 
> +#[sortable]
> +const API_METHOD_FINISH_BACKUP: ApiMethod = ApiMethod::new(
> +    &ApiHandler::Sync(&finish_backup),
> +    &ObjectSchema::new(
> +        "Mark backup as finished.",
> +        &sorted!([
> +            ("protected", true, &BooleanSchema::new("Mark backup as protected").schema()),
> +        ]),
> +    ),
> +);
> +
>  fn finish_backup(
> -    _param: Value,
> +    param: Value,
>      _info: &ApiMethod,
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<Value, Error> {
>      let env: &BackupEnvironment = rpcenv.as_ref();
> +    let protected = param["protected"].as_bool().unwrap_or(false);
> +
> +    if protected {
> +        if let Err(err) = env.set_protected() {
> +            env.log(format!("failed to set backup protected: {}", err));
> +        }
> +    }
> 
>      env.finish_backup()?;
>      env.log("successfully finished backup");
> --
> 2.34.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [RFC PATCH proxmox-backup 2/6] client: Add `--protected` CLI flag to backup command

[Fabian Grünbichler]

On January 18, 2023 11:48 am, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  proxmox-backup-client/src/main.rs | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 55198108..ee09db83 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -663,6 +663,12 @@ fn spawn_catalog_upload(
>                 optional: true,
>                 default: false,
>             },
> +           "protected": {
> +               type: Boolean,
> +               description: "Set backup as protected after upload.",
> +               optional: true,
> +               default: false,
> +           },
>         }
>     }
>  )]
> @@ -716,6 +722,7 @@ async fn create_backup(
> 
>      let empty = Vec::new();
>      let exclude_args = param["exclude"].as_array().unwrap_or(&empty);
> +    let protected = param["protected"].as_bool().unwrap_or(false);
> 
>      let mut pattern_list = Vec::with_capacity(exclude_args.len());
>      for entry in exclude_args {
> @@ -1082,7 +1089,9 @@ async fn create_backup(
>          .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
>          .await?;
> 
> -    client.finish().await?;
> +    client.finish(Some(json!({
> +        "protected": protected,
> +    }))).await?;

this fails the backup with new client + --protected, at the very end, if the
server doesn't have patch #1 of this series.

we need some sort of compat mechanism...

> 
>      let end_time = std::time::Instant::now();
>      let elapsed = end_time.duration_since(start_time);
> --
> 2.34.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [RFC PATCH proxmox-backup 2/6] client: Add `--protected` CLI flag to backup command

[Christoph Heiss]

On Wed, Jan 18, 2023 at 12:13:52PM +0100, Fabian Grünbichler wrote:
> On January 18, 2023 11:48 am, Christoph Heiss wrote:
> > [..]
> > @@ -1082,7 +1089,9 @@ async fn create_backup(
> >          .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
> >          .await?;
> >
> > -    client.finish().await?;
> > +    client.finish(Some(json!({
> > +        "protected": protected,
> > +    }))).await?;
>
> this fails the backup with new client + --protected, at the very end, if the
> server doesn't have patch #1 of this series.
Right, I did not test _that_ combination. Thanks for taking a look, I
will fix it for the next revision!

>
> we need some sort of compat mechanism...
One approach here would be to check the server version using the
`/version` endpoint and decide on that. (And re-reading that, Fiona
actually brought up that exact point in the previous thread ..)

FWIW, a (more) future-proof and cleaner compat mechanism could maybe be
to have `/version` also return a list of features supported by the
server, beside the current fields?

E.g.
  {
    "data": {
      "release": "2",
      "repoid": "07151513fa92899c65b1efa47d9b2d40c5dc5d82",
      "version": "2.3",
      "features": ["finish-protected-param"]                 <--
    },
    "success": true
  }

Would something like that be acceptable? Just an idea.

>
> >
> >      let end_time = std::time::Instant::now();
> >      let elapsed = end_time.duration_since(start_time);
> > --
> > 2.34.1
> >
> >
> >
> > _______________________________________________
> > pbs-devel mailing list
> > pbs-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> >
> >
> >
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>