[pbs-devel] [PATCH proxmox-backup] api2/tape/backup: wait indefinitely for lock in scheduled backup jobs

[pbs-devel] [PATCH proxmox-backup] api2/tape/backup: wait indefinitely for lock in scheduled backup jobs

[Dominik Csapak]

so that a user can schedule multiple backup jobs onto a single
media pool without having to consider timing them apart

this makes sense since we can backup multiple datastores onto
the same media-set but can only specify one datastore per backup job

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/backup.rs | 57 +++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index 0d42c31a..f29eb715 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -5,6 +5,7 @@ use anyhow::{bail, format_err, Error};
 use serde_json::Value;
 
 use proxmox::{
+    try_block,
     api::{
         api,
         RpcEnvironment,
@@ -177,8 +178,15 @@ pub fn do_tape_backup_job(
 
     let (drive_config, _digest) = config::drive::config()?;
 
-    // early check/lock before starting worker
-    let drive_lock = lock_tape_device(&drive_config, &setup.drive)?;
+    // for scheduled jobs we acquire the lock later in the worker
+    let drive_lock = if schedule.is_some() {
+        None
+    } else {
+        Some(lock_tape_device(&drive_config, &setup.drive)?)
+    };
+
+    let notify_user = setup.notify_user.as_ref().unwrap_or_else(|| &Userid::root_userid());
+    let email = lookup_user_email(notify_user);
 
     let upid_str = WorkerTask::new_thread(
         &worker_type,
@@ -186,26 +194,37 @@ pub fn do_tape_backup_job(
         auth_id.clone(),
         false,
         move |worker| {
-            let _drive_lock = drive_lock; // keep lock guard
-
-            set_tape_device_state(&setup.drive, &worker.upid().to_string())?;
             job.start(&worker.upid().to_string())?;
+            let mut drive_lock = drive_lock;
+
+            let (job_result, summary) = match try_block!({
+                if schedule.is_some() {
+                    // for scheduled tape backup jobs, we wait indefinitely for the lock
+                    task_log!(worker, "waiting for drive lock...");
+                    loop {
+                        if let Ok(lock) = lock_tape_device(&drive_config, &setup.drive) {
+                            drive_lock = Some(lock);
+                            break;
+                        } // ignore errors
+
+                        worker.check_abort()?;
+                    }
+                }
+                set_tape_device_state(&setup.drive, &worker.upid().to_string())?;
 
-            task_log!(worker,"Starting tape backup job '{}'", job_id);
-            if let Some(event_str) = schedule {
-                task_log!(worker,"task triggered by schedule '{}'", event_str);
-            }
-
-            let notify_user = setup.notify_user.as_ref().unwrap_or_else(|| &Userid::root_userid());
-            let email = lookup_user_email(notify_user);
+                task_log!(worker,"Starting tape backup job '{}'", job_id);
+                if let Some(event_str) = schedule {
+                    task_log!(worker,"task triggered by schedule '{}'", event_str);
+                }
 
-            let (job_result, summary) = match backup_worker(
-                &worker,
-                datastore,
-                &pool_config,
-                &setup,
-                email.clone(),
-            ) {
+                backup_worker(
+                    &worker,
+                    datastore,
+                    &pool_config,
+                    &setup,
+                    email.clone(),
+                )
+            }) {
                 Ok(summary) => (Ok(()), summary),
                 Err(err) => (Err(err), Default::default()),
             };
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup] api2/tape/backup: wait indefinitely for lock in scheduled backup jobs

[Dietmar Maurer]

> -    // early check/lock before starting worker
> -    let drive_lock = lock_tape_device(&drive_config, &setup.drive)?;
> +    // for scheduled jobs we acquire the lock later in the worker
> +    let drive_lock = if schedule.is_some() {
> +        None
> +    } else {
> +        Some(lock_tape_device(&drive_config, &setup.drive)?)
> +    };

What is the reason for the different locking times? Can't we always lock
later in the worker?

Re: [pbs-devel] [PATCH proxmox-backup] api2/tape/backup: wait indefinitely for lock in scheduled backup jobs

[Dietmar Maurer]

> +            let (job_result, summary) = match try_block!({
> +                if schedule.is_some() {
> +                    // for scheduled tape backup jobs, we wait indefinitely for the lock
> +                    task_log!(worker, "waiting for drive lock...");
> +                    loop {
> +                        if let Ok(lock) = lock_tape_device(&drive_config, &setup.drive) {
> +                            drive_lock = Some(lock);
> +                            break;
> +                        } // ignore errors

I would prefer to add a timeout parameter to lock_tape_device() call.
The question is if the lock call gets interrupted by task abort?

> +
> +                        worker.check_abort()?;
> +                    }
> +                }
> +                set_tape_device_state(&setup.drive, &worker.upid().to_string())?;

Re: [pbs-devel] [PATCH proxmox-backup] api2/tape/backup: wait indefinitely for lock in scheduled backup jobs

[Dominik Csapak]

On 3/18/21 06:53, Dietmar Maurer wrote:
>> -    // early check/lock before starting worker
>> -    let drive_lock = lock_tape_device(&drive_config, &setup.drive)?;
>> +    // for scheduled jobs we acquire the lock later in the worker
>> +    let drive_lock = if schedule.is_some() {
>> +        None
>> +    } else {
>> +        Some(lock_tape_device(&drive_config, &setup.drive)?)
>> +    };
> 
> What is the reason for the different locking times? Can't we always lock
> later in the worker?
> 

yes we can, but for the non-scheduled backup, we only have one try
to lock and i did not want to start a task since that would generate
a task entry that was preventable by checking beforehand

On 3/18/21 07:04, Dietmar Maurer wrote:
 >
 >> +            let (job_result, summary) = match try_block!({
 >> +                if schedule.is_some() {
 >> +                    // for scheduled tape backup jobs, we wait 
indefinitely for the lock
 >> +                    task_log!(worker, "waiting for drive lock...");
 >> +                    loop {
 >> +                        if let Ok(lock) = 
lock_tape_device(&drive_config, &setup.drive) {
 >> +                            drive_lock = Some(lock);
 >> +                            break;
 >> +                        } // ignore errors
 >
 > I would prefer to add a timeout parameter to lock_tape_device() call.
 > The question is if the lock call gets interrupted by task abort?

i did it this way, because an abort would not trigger if we are just 
waiting on a lock, so i decided to try the lock in a loop with
the check_abort()? call below (so one can abort this)

we could ofc add a timeout parameter, but would not change
the code here (and currently has no real upside, since
we probably would not use different timeouts anyway?)

 >
 >> +
 >> +                        worker.check_abort()?;
 >> +                    }
 >> +                }
 >> +                set_tape_device_state(&setup.drive, 
&worker.upid().to_string())?;