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

[pbs-devel] [RFC PATCH v2 proxmox-backup{, -qemu}, pve-{qemu, manager}, qemu-server 0/7] 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' [0] 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 Snapshots" 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 Snapshots" 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.

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.

To be able to use this new parameter in a backwards-compatible way with
older servers, clients need to know whether this new parameter is
supported by the server or not, to not fail the schema verification.
Thus introduce a "server features" discovery mechanism (patch 1). The
`/version` endpoint gets a new field `features`, which is an array of
strings indicating what extra "features" are supported by the client, as
generally proposed by Fabian in [1]. This is new as of v2.

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, or falling back to the separate API endpoint
for older servers, on a "best-effort" basis (as this can still fail the
exact same way as #4289 describes).

Further, this also introduces an (unfortunately) *API-breaking* change
to the QEMU side of things (patches 4 & 5). 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 of API-breaking changes in [2],
since we do not force a specific QEMU version upon users. What's the
best way to go about this?

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-7 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 soon as the
Rust/API-side is merged.

Patches 1-3 are otherwise completely independent of the other patches,
as they only add the API parameter and client functionality on the Rust
side.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4289
[1] https://lists.proxmox.com/pipermail/pbs-devel/2023-January/005867.html
[2] https://lists.proxmox.com/pipermail/pve-devel/2023-January/055394.html

Series history
--------------
Changes are documented indivdual per patch.

v1: https://lists.proxmox.com/pipermail/pbs-devel/2023-January/005865.html

---
Christoph Heiss (3):
      api2: Introduce server features discovery mechanism
      api2: Add `protected` parameter to `finish` endpoint
      client: Add `--protected` CLI flag to backup command

 examples/upload-speed.rs               |  2 +-
 pbs-client/src/backup_writer.rs        |  6 +--
 pbs-client/src/tools/mod.rs            | 25 +++++++++++-
 proxmox-backup-client/src/benchmark.rs |  2 +-
 proxmox-backup-client/src/main.rs      | 72 +++++++++++++++++++++++++++-------
 5 files changed, 86 insertions(+), 21 deletions(-)

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

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

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(+)

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

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

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

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

--
2.34.1

[pbs-devel] [RFC PATCH v2 proxmox-backup 1/7] api2: Introduce server features discovery mechanism

[Christoph Heiss]

This is vaguely based on the idea to introduce some sort of API
compatibility mechansim, to detect whether e.g. a new API parameter is
supported by the server or not. [0]

Essentially, a new `features` field is added to the `/version` endpoint,
which is an array (of strings) of supported (backwards-incompatible)
features by the server. Clients can retrieve this and act accordingly.
Features are internally described as an enum, with kebab-case'd names
for external consumption through the API.

Some inspriration was taken from how proxmox backup support in QEMU
works, using `query-proxmox-support` command.

This is intended as a first proposal on how this mechanism could work.

[0] https://lists.proxmox.com/pipermail/pbs-devel/2023-January/005867.html ff.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
* New patch

 pbs-api-types/src/lib.rs | 9 +++++++++
 src/api2/version.rs      | 4 +++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 5e043954..dda2bd5b 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -531,3 +531,12 @@ pub struct BasicRealmInfo {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub comment: Option<String>,
 }
+
+#[api]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// List of features a server can supported.
+pub enum ServerFeature {
+    /// Indicates that the `protected` parameter on the /finish endpoint is suppported
+    FinishHasProtectedParam,
+}
diff --git a/src/api2/version.rs b/src/api2/version.rs
index 0e91688b..0d6e66c4 100644
--- a/src/api2/version.rs
+++ b/src/api2/version.rs
@@ -3,6 +3,7 @@
 use anyhow::Error;
 use serde_json::{json, Value};

+use pbs_api_types::ServerFeature;
 use proxmox_router::{ApiHandler, ApiMethod, Permission, Router, RpcEnvironment};
 use proxmox_schema::ObjectSchema;

@@ -14,7 +15,8 @@ fn get_version(
     Ok(json!({
         "version": pbs_buildcfg::PROXMOX_PKG_VERSION,
         "release": pbs_buildcfg::PROXMOX_PKG_RELEASE,
-        "repoid": pbs_buildcfg::PROXMOX_PKG_REPOID
+        "repoid": pbs_buildcfg::PROXMOX_PKG_REPOID,
+        "features": [ServerFeature::FinishHasProtectedParam],
     }))
 }

--
2.34.1

[pbs-devel] [RFC PATCH v2 proxmox-backup 2/7] 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>
---
Changes v1 -> v2:
* Moved BackupWriter::finish adaption to patch 2

 src/api2/backup/environment.rs | 11 +++++++++++
 src/api2/backup/mod.rs         | 30 ++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 8 deletions(-)

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..757364b6 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -342,13 +342,7 @@ const BACKUP_API_SUBDIRS: SubdirMap = &[
             .post(&API_METHOD_CREATE_DYNAMIC_INDEX)
             .put(&API_METHOD_DYNAMIC_APPEND),
     ),
-    (
-        "finish",
-        &Router::new().post(&ApiMethod::new(
-            &ApiHandler::Sync(&finish_backup),
-            &ObjectSchema::new("Mark backup as finished.", &[]),
-        )),
-    ),
+    ("finish", &Router::new().post(&API_METHOD_FINISH_BACKUP)),
     (
         "fixed_chunk",
         &Router::new().upload(&API_METHOD_UPLOAD_FIXED_CHUNK),
@@ -771,12 +765,32 @@ 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 v2 proxmox-backup 3/3] client: Add `--protected` CLI flag to backup command

[Christoph Heiss]

This implements setting the backup as protected after finishing it,
either using the newly introduced API parameter on the `/finish`
endpoint, falling back to the "old" way using the separate endpoint,
thus supporting this in a backwards-compatible way.

The same caveat as in bug #4289 [0] applies here as well: If the
datastore has "Verify New Snapshots" set, the latter way might fail to
set the backup as protected.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4289

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Note: BackupWriter::start() actually does fine with just a &HttpClient,
since HttpClient::start_h2_connection() then .clone()'s it anyway and
not consume it.

Changes v1 -> v2:
* Moved BackupWriter::finish adaption to this patch
* Only set API parameter if available using new server feature mechanism
* Implement best-effort backwards-compatible way for older server

 examples/upload-speed.rs               |  2 +-
 pbs-client/src/backup_writer.rs        |  6 +--
 pbs-client/src/tools/mod.rs            | 25 ++++++++-
 proxmox-backup-client/src/benchmark.rs |  2 +-
 proxmox-backup-client/src/main.rs      | 72 ++++++++++++++++++++------
 5 files changed, 86 insertions(+), 21 deletions(-)

diff --git a/examples/upload-speed.rs b/examples/upload-speed.rs
index f9fc52a8..e4b570ec 100644
--- a/examples/upload-speed.rs
+++ b/examples/upload-speed.rs
@@ -18,7 +18,7 @@ async fn upload_speed() -> Result<f64, Error> {
     let backup_time = proxmox_time::epoch_i64();

     let client = BackupWriter::start(
-        client,
+        &client,
         None,
         datastore,
         &BackupNamespace::root(),
diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index be6da2a6..4ecc6622 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -76,7 +76,7 @@ impl BackupWriter {
     // FIXME: extract into (flattened) parameter struct?
     #[allow(clippy::too_many_arguments)]
     pub async fn start(
-        client: HttpClient,
+        client: &HttpClient,
         crypt_config: Option<Arc<CryptConfig>>,
         datastore: &str,
         ns: &BackupNamespace,
@@ -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/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index fa1d5092..140ef9c7 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -15,7 +15,9 @@ use proxmox_router::cli::{complete_file_name, shellword_split};
 use proxmox_schema::*;
 use proxmox_sys::fs::file_get_json;

-use pbs_api_types::{Authid, BackupNamespace, RateLimitConfig, UserWithTokens, BACKUP_REPO_URL};
+use pbs_api_types::{
+    Authid, BackupNamespace, RateLimitConfig, ServerFeature, UserWithTokens, BACKUP_REPO_URL,
+};

 use crate::{BackupRepository, HttpClient, HttpClientOptions};

@@ -502,6 +504,27 @@ pub async fn complete_namespace_do(
     result
 }

+/// Returns a list of (backwards-incompatible) features supported by the server.
+pub async fn get_supported_server_features(client: &HttpClient) -> Vec<ServerFeature> {
+    match client.get("api2/json/version", None).await {
+        Ok(mut result) if result["data"]["features"].is_array() => {
+            match serde_json::from_value::<Vec<ServerFeature>>(result["data"]["features"].take()) {
+                Ok(features) => features,
+                Err(e) => {
+                    log::warn!("failed to deserialize feature list: {}", e);
+                    vec![]
+                }
+            }
+        }
+        // Old server with no feature advertising yet.
+        Ok(_) => vec![],
+        Err(e) => {
+            log::error!("could not connect to server - {}", e);
+            vec![]
+        }
+    }
+}
+
 pub fn base_directories() -> Result<xdg::BaseDirectories, Error> {
     xdg::BaseDirectories::with_prefix("proxmox-backup").map_err(Error::from)
 }
diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs
index b3047308..1262fb46 100644
--- a/proxmox-backup-client/src/benchmark.rs
+++ b/proxmox-backup-client/src/benchmark.rs
@@ -229,7 +229,7 @@ async fn test_upload_speed(

     log::debug!("Connecting to backup server");
     let client = BackupWriter::start(
-        client,
+        &client,
         crypt_config.clone(),
         repo.store(),
         &BackupNamespace::root(),
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 55198108..bab67207 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -25,15 +25,16 @@ use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 use pbs_api_types::{
     Authid, BackupDir, BackupGroup, BackupNamespace, BackupPart, BackupType, CryptMode,
     Fingerprint, GroupListItem, HumanByte, PruneJobOptions, PruneListItem, RateLimitConfig,
-    SnapshotListItem, StorageStatus, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
-    BACKUP_TYPE_SCHEMA, TRAFFIC_CONTROL_BURST_SCHEMA, TRAFFIC_CONTROL_RATE_SCHEMA,
+    ServerFeature, SnapshotListItem, StorageStatus, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
+    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, TRAFFIC_CONTROL_BURST_SCHEMA,
+    TRAFFIC_CONTROL_RATE_SCHEMA,
 };
 use pbs_client::catalog_shell::Shell;
 use pbs_client::tools::{
     complete_archive_name, complete_auth_id, complete_backup_group, complete_backup_snapshot,
     complete_backup_source, complete_chunk_size, complete_group_or_snapshot,
     complete_img_archive_name, complete_namespace, complete_pxar_archive_name, complete_repository,
-    connect, connect_rate_limited, extract_repository_from_value,
+    connect, connect_rate_limited, extract_repository_from_value, get_supported_server_features,
     key_source::{
         crypto_parameters, format_key_source, get_encryption_key_password, KEYFD_SCHEMA,
         KEYFILE_SCHEMA, MASTER_PUBKEY_FD_SCHEMA, MASTER_PUBKEY_FILE_SCHEMA,
@@ -663,6 +664,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 +723,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 {
@@ -837,6 +845,9 @@ async fn create_backup(

     log::info!("Client name: {}", proxmox_sys::nodename());

+    let server_features = get_supported_server_features(&client).await;
+    log::debug!("Supported server features: {:?}", server_features);
+
     let start_time = std::time::Instant::now();

     log::info!(
@@ -876,8 +887,8 @@ async fn create_backup(
         }
     };

-    let client = BackupWriter::start(
-        client,
+    let writer = BackupWriter::start(
+        &client,
         crypt_config.clone(),
         repo.store(),
         &backup_ns,
@@ -887,7 +898,7 @@ async fn create_backup(
     )
     .await?;

-    let download_previous_manifest = match client.previous_backup_time().await {
+    let download_previous_manifest = match writer.previous_backup_time().await {
         Ok(Some(backup_time)) => {
             log::info!(
                 "Downloading previous manifest ({})",
@@ -906,7 +917,7 @@ async fn create_backup(
     };

     let previous_manifest = if download_previous_manifest {
-        match client.download_previous_manifest().await {
+        match writer.download_previous_manifest().await {
             Ok(previous_manifest) => {
                 match previous_manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref)) {
                     Ok(()) => Some(Arc::new(previous_manifest)),
@@ -951,7 +962,7 @@ async fn create_backup(
                 };

                 log_file("config file", &filename, &target);
-                let stats = client
+                let stats = writer
                     .upload_blob_from_file(&filename, &target, upload_options)
                     .await?;
                 manifest.add_file(target, stats.size, stats.csum, crypto.mode)?;
@@ -965,7 +976,7 @@ async fn create_backup(
                 };

                 log_file("log file", &filename, &target);
-                let stats = client
+                let stats = writer
                     .upload_blob_from_file(&filename, &target, upload_options)
                     .await?;
                 manifest.add_file(target, stats.size, stats.csum, crypto.mode)?;
@@ -974,7 +985,7 @@ async fn create_backup(
                 // start catalog upload on first use
                 if catalog.is_none() {
                     let catalog_upload_res =
-                        spawn_catalog_upload(client.clone(), crypto.mode == CryptMode::Encrypt)?;
+                        spawn_catalog_upload(writer.clone(), crypto.mode == CryptMode::Encrypt)?;
                     catalog = Some(catalog_upload_res.catalog_writer);
                     catalog_result_rx = Some(catalog_upload_res.result);
                 }
@@ -1001,7 +1012,7 @@ async fn create_backup(
                 };

                 let stats = backup_directory(
-                    &client,
+                    &writer,
                     &filename,
                     &target,
                     chunk_size_opt,
@@ -1024,7 +1035,7 @@ async fn create_backup(
                 };

                 let stats =
-                    backup_image(&client, &filename, &target, chunk_size_opt, upload_options)
+                    backup_image(&writer, &filename, &target, chunk_size_opt, upload_options)
                         .await?;
                 manifest.add_file(target, stats.size, stats.csum, crypto.mode)?;
             }
@@ -1060,7 +1071,7 @@ async fn create_backup(
             encrypt: false,
             ..UploadOptions::default()
         };
-        let stats = client
+        let stats = writer
             .upload_blob_from_data(rsa_encrypted_key, target, options)
             .await?;
         manifest.add_file(target.to_string(), stats.size, stats.csum, crypto.mode)?;
@@ -1078,11 +1089,42 @@ async fn create_backup(
         encrypt: false,
         ..UploadOptions::default()
     };
-    client
+    writer
         .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
         .await?;

-    client.finish().await?;
+
+    let mut params = json!({});
+
+    let has_protected_param = server_features.contains(&ServerFeature::FinishHasProtectedParam);
+    if has_protected_param {
+        params["protected"] = json!(protected);
+    }
+
+    writer.finish(Some(params)).await?;
+
+    // In case the server does not support the new `protected` parameter and the user has requested
+    // to set the backup as protected, fall back to the separate API endpoint as a "best-effort"
+    // solution. This might fail for larger backups if the datastore has "Verify New Snapshots"
+    // enabled.
+    if !has_protected_param && protected {
+        let path = format!("api2/json/admin/datastore/{}/protected", repo.store());
+
+        let mut params = json!({
+            "backup-id": backup_id,
+            "backup-time": backup_time,
+            "backup-type": backup_type,
+            "protected": protected,
+        });
+
+        if backup_ns.is_root() {
+            params["ns"] = json!(backup_ns);
+        }
+
+        if let Err(e) = client.put(&path, Some(params)).await {
+            log::warn!("Unable to set backup as protected: {}", e);
+        }
+    }

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

[pbs-devel] [RFC PATCH v2 proxmox-backup-qemu 4/7] 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>
---
Depends on the previous proxmox-backup patches (1-3 in the series).

RFC question: It would probably be sensible to issue a warning in
BackupTask::finish() - but what is the right way for that? A simple
eprintln!()?

Changes v1 -> v2:
* Use new server feature mechanism to detect support for API parameter

 current-api.h   |  3 ++-
 simpletest.c    |  2 +-
 src/backup.rs   | 24 ++++++++++++++++++++----
 src/commands.rs | 10 +++++++++-
 src/lib.rs      |  5 ++++-
 5 files changed, 36 insertions(+), 8 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..60ec453 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -10,8 +10,10 @@ use tokio::runtime::Runtime;
 use proxmox_async::runtime::get_runtime_with_builder;
 use proxmox_sys::fs::file_get_contents;

-use pbs_api_types::{BackupType, CryptMode};
-use pbs_client::{BackupWriter, HttpClient, HttpClientOptions};
+use pbs_api_types::{BackupType, CryptMode, ServerFeature};
+use pbs_client::{
+    tools::get_supported_server_features, BackupWriter, HttpClient, HttpClientOptions,
+};
 use pbs_datastore::BackupManifest;
 use pbs_key_config::{load_and_decrypt_key, rsa_encrypt_key_config, KeyConfig};
 use pbs_tools::crypt_config::CryptConfig;
@@ -35,6 +37,7 @@ pub(crate) struct BackupTask {
     known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
     abort: tokio::sync::broadcast::Sender<()>,
     aborted: OnceCell<String>, // set on abort, conatins abort reason
+    server_features: OnceCell<Vec<ServerFeature>>,
 }

 impl BackupTask {
@@ -92,6 +95,7 @@ impl BackupTask {
             writer: OnceCell::new(),
             last_manifest: OnceCell::new(),
             aborted: OnceCell::new(),
+            server_features: OnceCell::new(),
         })
     }

@@ -141,10 +145,15 @@ impl BackupTask {
                 &self.setup.auth_id,
                 options,
             )?;
+
+            self.server_features
+                .set(get_supported_server_features(&http).await)
+                .map_err(|_| format_err!("already connected!"))?;
+
             let mut backup_dir = self.setup.backup_dir.clone();
             backup_dir.group.ty = BackupType::Vm;
             let writer = BackupWriter::start(
-                http,
+                &http,
                 self.crypt_config.clone(),
                 &self.setup.store,
                 &self.setup.backup_ns,
@@ -286,14 +295,21 @@ 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 protected_supported = self
+            .server_features
+            .get()
+            .map(|sf| sf.contains(&ServerFeature::FinishHasProtectedParam))
+            .unwrap_or(false);
+
         let command_future = finish_backup(
             self.need_writer()?,
             self.crypt_config.clone(),
             self.rsa_encrypted_key.clone(),
             Arc::clone(&self.manifest),
+            protected && protected_supported,
         );

         let mut abort_rx = self.abort.subscribe();
diff --git a/src/commands.rs b/src/commands.rs
index 37d653c..4258711 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -468,11 +468,15 @@ pub(crate) async fn write_data(
     Ok(if reused { 0 } else { size as c_int })
 }

+/// The caller must ensure that all used server features are actually supported by the server.
+/// Currently, this only applies to `protected`. If this is set to true, the server has to support
+/// the `FinishHasProtectedParam` feature flag.
 pub(crate) async fn finish_backup(
     client: Arc<BackupWriter>,
     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 +525,11 @@ pub(crate) async fn finish_backup(
         .upload_blob_from_data(manifest.into_bytes(), MANIFEST_BLOB_NAME, options)
         .await?;

-    client.finish().await?;
+    let mut param = json!({});
+    if protected {
+        param["protected"] = json!(true);
+    }

+    client.finish(Some(param)).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 v2 pve-qemu 5/7] 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}`.
Also exposes a new feature flag from `query-proxmox-support`, indicating
whether this new parameter is supported by the proxmox-backup-client or
not.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
* Added information about new feature flag to commit message

 ...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 v2 qemu-server 6/7] vzdump: Set protected flag on backup start if supported

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
* No changes

 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 v2 pve-manager 7/7] 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>
---
This might introduce a time-of-check <=> time-of-set race - could this
ever be a problem?

Changes v1 -> v2:
* No changes

 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 v2 proxmox-backup 1/7] api2: Introduce server features discovery mechanism

[Thomas Lamprecht]

Am 25/01/2023 um 13:18 schrieb Christoph Heiss:
> This is vaguely based on the idea to introduce some sort of API
> compatibility mechansim, to detect whether e.g. a new API parameter is
> supported by the server or not. [0]
> 
> Essentially, a new `features` field is added to the `/version` endpoint,
> which is an array (of strings) of supported (backwards-incompatible)
> features by the server. Clients can retrieve this and act accordingly.
> Features are internally described as an enum, with kebab-case'd names
> for external consumption through the API.
> 
> Some inspriration was taken from how proxmox backup support in QEMU
> works, using `query-proxmox-support` command.
> 
> This is intended as a first proposal on how this mechanism could work.

you could also just check the version? that's already there, no need for
adding extra complexity.

tbh, I'd be fine without any such check, as new *client* needing newer
server is fine; one just does need to ensure that old clients still work
with new server.

If you want, you could catch the parameter exception in the PVE call
site, either adding a hint about "new server required" or falling back
there (albeit I'm not so liking that).

After all immediate protection of backups is only relevant for manual
trigerring that, not for the scheduled backup jobs, so it won't affect
existing PVE jobs anyway.

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

[Thomas Lamprecht]

Am 25/01/2023 um 13:18 schrieb Christoph Heiss:
> When a datastore has the "Verify New Snapshots" 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 Snapshots" 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.

but the protection flag resides in the "unprotected" part of the manifest,
and IMO it seems very odd that I cannot change a protection flag if verification
is running, which can happen anytime not only on verify-new.

So why not adapt this that protection can be changed independent of verification,
which would require zero client changes and make for a better UX in general -
albeit it certainly needs some well thought out (lock) handling.

Re: [pbs-devel] [RFC PATCH v2 proxmox-backup 1/7] api2: Introduce server features discovery mechanism

[Christoph Heiss]

Thanks for taking a look a this!

On Wed, Jan 25, 2023 at 04:56:26PM +0100, Thomas Lamprecht wrote:
> Am 25/01/2023 um 13:18 schrieb Christoph Heiss:
> > [..]
>
> you could also just check the version? that's already there, no need for
> adding extra complexity.
>
> tbh, I'd be fine without any such check, as new *client* needing newer
> server is fine; one just does need to ensure that old clients still work
> with new server.
I thought about that too at first - seemed a bit "hacky" too me, but if
it is also fine by you, I will happily drop this patch and just check the
server version.

Will do a v3 soon with that incorporated, barring other feedback on that
matter. I'm very happy about better proposals for any of this stuff,
since this grew considerably from what I initally thought should be a
relatively simple fix.

>
> If you want, you could catch the parameter exception in the PVE call
> site, either adding a hint about "new server required" or falling back
> there (albeit I'm not so liking that).
>
> After all immediate protection of backups is only relevant for manual
> trigerring that, not for the scheduled backup jobs, so it won't affect
> existing PVE jobs anyway.

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

[Christoph Heiss]

On Wed, Jan 25, 2023 at 05:00:03PM +0100, Thomas Lamprecht wrote:
> Am 25/01/2023 um 13:18 schrieb Christoph Heiss:
> > When a datastore has the "Verify New Snapshots" 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 Snapshots" 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.
>
> but the protection flag resides in the "unprotected" part of the manifest,
> and IMO it seems very odd that I cannot change a protection flag if verification
> is running, which can happen anytime not only on verify-new.
Hm, I did indeed not consider this case - this really seems a bit quirky
then after all.

>
> So why not adapt this that protection can be changed independent of verification,
> which would require zero client changes and make for a better UX in general -
> albeit it certainly needs some well thought out (lock) handling.
I will look into this, thanks for the hint! I tried messing with the
locking before, which wasn't all that fruitfully.

In any case, definitely seems like the right way for this now, plus it
would make things a lot cleaner. Did not know that the protection flag
was generally a "unprotected" part of the manifest, this of course
changes things.