all lists on 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal