public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask
@ 2024-01-24 10:34 Gabriel Goller
  2024-01-24 13:17 ` Fiona Ebner
  0 siblings, 1 reply; 5+ messages in thread
From: Gabriel Goller @ 2024-01-24 10:34 UTC (permalink / raw)
  To: pbs-devel

`prune-group` is currently not a real workertask, ie it behaves like one
but doesn't start a thread nor a task to do its work.

Changed it to start a tokio-task, so that we can delete snapshots
asynchronously. The `dry-run` feature still behaves in the same way and
returns early.

This paves the way for the new logging infra (which uses `task_local` to
define a logger) and improves performance of bigger backup-groups.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/admin/datastore.rs | 104 ++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 51 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a95031e7..4801eb1a 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1007,62 +1007,64 @@ pub fn prune(
         }
         return Ok(json!(prune_result));
     }
+    let upid = WorkerTask::spawn(
+        "prune",
+        Some(worker_id),
+        auth_id.to_string(),
+        true,
+        move |worker| async move {
+            if keep_all {
+                task_log!(worker, "No prune selection - keeping all files.");
+            } else {
+                let mut opts = Vec::new();
+                if !ns.is_root() {
+                    opts.push(format!("--ns {ns}"));
+                }
+                crate::server::cli_keep_options(&mut opts, &keep_options);
 
-    // We use a WorkerTask just to have a task log, but run synchrounously
-    let worker = WorkerTask::new("prune", Some(worker_id), auth_id.to_string(), true)?;
-
-    if keep_all {
-        task_log!(worker, "No prune selection - keeping all files.");
-    } else {
-        let mut opts = Vec::new();
-        if !ns.is_root() {
-            opts.push(format!("--ns {ns}"));
-        }
-        crate::server::cli_keep_options(&mut opts, &keep_options);
-
-        task_log!(worker, "retention options: {}", opts.join(" "));
-        task_log!(
-            worker,
-            "Starting prune on {} group \"{}\"",
-            print_store_and_ns(&store, &ns),
-            group.group(),
-        );
-    }
-
-    for (info, mark) in prune_info {
-        let keep = keep_all || mark.keep();
-
-        let backup_time = info.backup_dir.backup_time();
-        let timestamp = info.backup_dir.backup_time_string();
-        let group: &pbs_api_types::BackupGroup = info.backup_dir.as_ref();
-
-        let msg = format!("{}/{}/{} {}", group.ty, group.id, timestamp, mark,);
-
-        task_log!(worker, "{}", msg);
-
-        prune_result.push(json!({
-            "backup-type": group.ty,
-            "backup-id": group.id,
-            "backup-time": backup_time,
-            "keep": keep,
-            "protected": mark.protected(),
-        }));
-
-        if !(dry_run || keep) {
-            if let Err(err) = info.backup_dir.destroy(false) {
-                task_warn!(
+                task_log!(worker, "retention options: {}", opts.join(" "));
+                task_log!(
                     worker,
-                    "failed to remove dir {:?}: {}",
-                    info.backup_dir.relative_path(),
-                    err,
+                    "Starting prune on {} group \"{}\"",
+                    print_store_and_ns(&store, &ns),
+                    group.group(),
                 );
             }
-        }
-    }
 
-    worker.log_result(&Ok(()));
-
-    Ok(json!(prune_result))
+            for (info, mark) in prune_info {
+                let keep = keep_all || mark.keep();
+
+                let backup_time = info.backup_dir.backup_time();
+                let timestamp = info.backup_dir.backup_time_string();
+                let group: &pbs_api_types::BackupGroup = info.backup_dir.as_ref();
+
+                let msg = format!("{}/{}/{} {}", group.ty, group.id, timestamp, mark,);
+
+                task_log!(worker, "{}", msg);
+
+                prune_result.push(json!({
+                    "backup-type": group.ty,
+                    "backup-id": group.id,
+                    "backup-time": backup_time,
+                    "keep": keep,
+                    "protected": mark.protected(),
+                }));
+
+                if !(dry_run || keep) {
+                    if let Err(err) = info.backup_dir.destroy(false) {
+                        task_warn!(
+                            worker,
+                            "failed to remove dir {:?}: {}",
+                            info.backup_dir.relative_path(),
+                            err,
+                        );
+                    }
+                }
+            }
+            Ok(())
+        },
+    )?;
+    Ok(json!(upid))
 }
 
 #[api(
-- 
2.43.0





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

* Re: [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask
  2024-01-24 10:34 [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask Gabriel Goller
@ 2024-01-24 13:17 ` Fiona Ebner
  2024-01-24 13:28   ` Thomas Lamprecht
  0 siblings, 1 reply; 5+ messages in thread
From: Fiona Ebner @ 2024-01-24 13:17 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Gabriel Goller

Am 24.01.24 um 11:34 schrieb Gabriel Goller:
> -    Ok(json!(prune_result))

> +    Ok(json!(upid))

That seems like a breaking API change at a first glance and thus has to
wait for the next major release unfortunately.

AFAIU the same rules as for PVE apply, see:
https://pve.proxmox.com/wiki/Proxmox_VE_API#API_Stability_.26_Breakage




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

* Re: [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask
  2024-01-24 13:17 ` Fiona Ebner
@ 2024-01-24 13:28   ` Thomas Lamprecht
  2024-01-24 13:42     ` Gabriel Goller
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Lamprecht @ 2024-01-24 13:28 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Fiona Ebner,
	Gabriel Goller

Am 24/01/2024 um 14:17 schrieb Fiona Ebner:
> Am 24.01.24 um 11:34 schrieb Gabriel Goller:
>> -    Ok(json!(prune_result))
> 
>> +    Ok(json!(upid))
> 
> That seems like a breaking API change at a first glance and thus has to
> wait for the next major release unfortunately.
> 
> AFAIU the same rules as for PVE apply, see:
> https://pve.proxmox.com/wiki/Proxmox_VE_API#API_Stability_.26_Breakage
> 

Yes, if applied strictly, and this here might be indeed a breakage that
matters in practice.

But you can work around that by making it use a worker task opt-in via
a new parameter boolean flag.

Oh, and isn't prune_result now unused? We're still lacking a simple 
and standardized method to save a structured result alongside a task
UPID, otherwise it could be saved there, but that's another topic.




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

* Re: [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask
  2024-01-24 13:28   ` Thomas Lamprecht
@ 2024-01-24 13:42     ` Gabriel Goller
  2024-01-25 11:49       ` Gabriel Goller
  0 siblings, 1 reply; 5+ messages in thread
From: Gabriel Goller @ 2024-01-24 13:42 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox Backup Server development discussion,
	Fiona Ebner

On 24-01-2024 14:28, Thomas Lamprecht wrote:
> Am 24/01/2024 um 14:17 schrieb Fiona Ebner:
>> Am 24.01.24 um 11:34 schrieb Gabriel Goller:
>>> -    Ok(json!(prune_result))
>>> +    Ok(json!(upid))
>> That seems like a breaking API change at a first glance and thus has to
>> wait for the next major release unfortunately.
>>
>> AFAIU the same rules as for PVE apply, see:
>> https://pve.proxmox.com/wiki/Proxmox_VE_API#API_Stability_.26_Breakage
>>
> Yes, if applied strictly, and this here might be indeed a breakage that
> matters in practice.
>
> But you can work around that by making it use a worker task opt-in via
> a new parameter boolean flag.
Will do that.
> Oh, and isn't prune_result now unused? We're still lacking a simple
> and standardized method to save a structured result alongside a task
> UPID, otherwise it could be saved there, but that's another topic.
Hmm, yeah noticed that... I think I'll convert it to a structure and
pretty-print it to the task-log.





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

* Re: [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask
  2024-01-24 13:42     ` Gabriel Goller
@ 2024-01-25 11:49       ` Gabriel Goller
  0 siblings, 0 replies; 5+ messages in thread
From: Gabriel Goller @ 2024-01-25 11:49 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox Backup Server development discussion,
	Fiona Ebner

Sent a v2!

On 24-01-2024 14:42, Gabriel Goller wrote:
> On 24-01-2024 14:28, Thomas Lamprecht wrote:
>> Am 24/01/2024 um 14:17 schrieb Fiona Ebner:
>>> Am 24.01.24 um 11:34 schrieb Gabriel Goller:
>>>> - Ok(json!(prune_result))
>>>> +    Ok(json!(upid))
>>> That seems like a breaking API change at a first glance and thus has to
>>> wait for the next major release unfortunately.
>>>
>>> AFAIU the same rules as for PVE apply, see:
>>> https://pve.proxmox.com/wiki/Proxmox_VE_API#API_Stability_.26_Breakage
>>>
>> Yes, if applied strictly, and this here might be indeed a breakage that
>> matters in practice.
>>
>> But you can work around that by making it use a worker task opt-in via
>> a new parameter boolean flag.
> Will do that.
>> Oh, and isn't prune_result now unused? We're still lacking a simple
>> and standardized method to save a structured result alongside a task
>> UPID, otherwise it could be saved there, but that's another topic.
> Hmm, yeah noticed that... I think I'll convert it to a structure and
> pretty-print it to the task-log.
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>





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

end of thread, other threads:[~2024-01-25 11:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-24 10:34 [pbs-devel] [PATCH proxmox-backup] api: make prune-group a real workertask Gabriel Goller
2024-01-24 13:17 ` Fiona Ebner
2024-01-24 13:28   ` Thomas Lamprecht
2024-01-24 13:42     ` Gabriel Goller
2024-01-25 11:49       ` Gabriel Goller

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