[pbs-devel] [PATCH v1 proxmox-backup 0/1] serverside benchmark cleanup

[pbs-devel] [PATCH v1 proxmox-backup 0/1] serverside benchmark cleanup

[Hannes Laimer]

Add a flag when runnig a benchmark so everything created by the
benchmark may be cleaned up properly. The worker now also has the type
'benchmark', so benchmark tasks are distinguishable from normal backup
tasks.

Hannes Laimer (1):
  add benchmark flag to backup creation for proper cleanup when running
    a benchmark

 src/api2/backup.rs                         | 18 ++++++++++++++----
 src/bin/proxmox-backup-client.rs           |  1 +
 src/bin/proxmox_backup_client/benchmark.rs |  1 +
 src/client/backup_writer.rs                |  4 +++-
 4 files changed, 19 insertions(+), 5 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH v1 proxmox-backup] add benchmark flag to backup creation for proper cleanup when running a benchmark

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
v1:
	- set worker_type to 'benchmark', if it is one
	- delete whole '<datastore>/host/benchmark'-folder after benchmark
	- add optional boolean parameter 'benchmark' to backup endpoint
	- add backup boolean parameter to BackupWriter start() function

 src/api2/backup.rs                         | 18 ++++++++++++++----
 src/bin/proxmox-backup-client.rs           |  1 +
 src/bin/proxmox_backup_client/benchmark.rs |  1 +
 src/client/backup_writer.rs                |  4 +++-
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/api2/backup.rs b/src/api2/backup.rs
index ad608d85..0edc9240 100644
--- a/src/api2/backup.rs
+++ b/src/api2/backup.rs
@@ -38,6 +38,7 @@ pub const API_METHOD_UPGRADE_BACKUP: ApiMethod = ApiMethod::new(
             ("backup-id", false, &BACKUP_ID_SCHEMA),
             ("backup-time", false, &BACKUP_TIME_SCHEMA),
             ("debug", true, &BooleanSchema::new("Enable verbose debug logging.").schema()),
+            ("benchmark", true, &BooleanSchema::new("Is a benchmark.").schema()),
         ]),
     )
 ).access(
@@ -56,6 +57,7 @@ fn upgrade_to_backup_protocol(
 
 async move {
     let debug = param["debug"].as_bool().unwrap_or(false);
+    let benchmark = param["benchmark"].as_bool().unwrap_or(false);
 
     let userid: Userid = rpcenv.get_user().unwrap().parse()?;
 
@@ -115,15 +117,19 @@ async move {
 
     let (path, is_new, _snap_guard) = datastore.create_locked_backup_dir(&backup_dir)?;
     if !is_new { bail!("backup directory already exists."); }
-
-    WorkerTask::spawn("backup", Some(worker_id), userid.clone(), true, move |worker| {
+    let absolute_backup_dir_path = format!("{}/{}", &datastore.base_path().to_str().unwrap(), backup_group);
+    let mut worker_type = "backup";
+    if benchmark {
+        worker_type = "benchmark";
+    }
+    WorkerTask::spawn(worker_type, Some(worker_id), userid.clone(), true, move |worker| {
         let mut env = BackupEnvironment::new(
             env_type, userid, worker.clone(), datastore, backup_dir);
 
         env.debug = debug;
         env.last_backup = last_backup;
 
-        env.log(format!("starting new backup on datastore '{}': {:?}", store, path));
+        env.log(format!("starting new {} on datastore '{}': {:?}", worker_type, store, path));
 
         let service = H2Service::new(env.clone(), worker.clone(), &BACKUP_API_ROUTER, debug);
 
@@ -160,7 +166,11 @@ async move {
                 req = req_fut => req,
                 abrt = abort_future => abrt,
             };
-
+            if benchmark {
+                env.log("benchmark finished successfully");
+                std::fs::remove_dir_all(absolute_backup_dir_path)?;
+                return Ok(());
+            }
             match (res, env.ensure_finished()) {
                 (Ok(_), Ok(())) => {
                     env.log("backup finished successfully");
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 9a6f309d..1e9cc680 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1026,6 +1026,7 @@ async fn create_backup(
         &backup_id,
         backup_time,
         verbose,
+        false
     ).await?;
 
     let previous_manifest = if let Ok(previous_manifest) = client.download_previous_manifest().await {
diff --git a/src/bin/proxmox_backup_client/benchmark.rs b/src/bin/proxmox_backup_client/benchmark.rs
index 9355d39e..b7c67e7f 100644
--- a/src/bin/proxmox_backup_client/benchmark.rs
+++ b/src/bin/proxmox_backup_client/benchmark.rs
@@ -226,6 +226,7 @@ async fn test_upload_speed(
         "benchmark",
         backup_time,
         false,
+        true
     ).await?;
 
     if verbose { eprintln!("Start TLS speed test"); }
diff --git a/src/client/backup_writer.rs b/src/client/backup_writer.rs
index 35e9c0bd..64c3cf27 100644
--- a/src/client/backup_writer.rs
+++ b/src/client/backup_writer.rs
@@ -53,6 +53,7 @@ impl BackupWriter {
         backup_id: &str,
         backup_time: DateTime<Utc>,
         debug: bool,
+        benchmark: bool
     ) -> Result<Arc<BackupWriter>, Error> {
 
         let param = json!({
@@ -60,7 +61,8 @@ impl BackupWriter {
             "backup-id": backup_id,
             "backup-time": backup_time.timestamp(),
             "store": datastore,
-            "debug": debug
+            "debug": debug,
+            "benchmark": benchmark
         });
 
         let req = HttpClient::request_builder(
-- 
2.20.1

Re: [pbs-devel] [PATCH v1 proxmox-backup] add benchmark flag to backup creation for proper cleanup when running a benchmark

[Dietmar Maurer]

some comment:

@@ -118,10 +118,8 @@ async move {
     let (path, is_new, _snap_guard) = datastore.create_locked_backup_dir(&backup_dir)?;
     if !is_new { bail!("backup directory already exists."); }
     let absolute_backup_dir_path = format!("{}/{}", &datastore.base_path().to_str().unwrap(), backup_group);
-    let mut worker_type = "backup";
-    if benchmark {
-        worker_type = "benchmark";
-    }
+    let worker_type = if benchmark { "benchmark" } else { "backup" };
+
     WorkerTask::spawn(worker_type, Some(worker_id), userid.clone(), true, move |worker| {
         let mut env = BackupEnvironment::new(
             env_type, userid, worker.clone(), datastore, backup_dir);


On 09/02/2020 11:41 AM Hannes Laimer <h.laimer@proxmox.com> wrote:
> 
>  
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> v1:
> 	- set worker_type to 'benchmark', if it is one
> 	- delete whole '<datastore>/host/benchmark'-folder after benchmark

This is super dangerous, because you delete the whole group now, i.e: you
can do the following

run job /vm/12345 --benchmark

which completely removes all you VM backups?!