[pbs-devel] [PATCH proxmox-backup] proxmox-restore-daemon: keep daemon active during extract api call

[pbs-devel] [PATCH proxmox-backup] proxmox-restore-daemon: keep daemon active during extract api call

[Dominik Csapak]

we only called watchdog_ping on the start of an api call, which meant
that if a single zip/pxar download took longer than 10 minutes
(which can happen on larger vms/slower connections), we shut-down
the vm and cancelled the download.

instead, spawn a second future, that polls both a sleep and the extract
handle, and ping the watchdog every 60 seconds

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/bin/proxmox_restore_daemon/api.rs | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox_restore_daemon/api.rs b/src/bin/proxmox_restore_daemon/api.rs
index f1d601ce..eafff6e2 100644
--- a/src/bin/proxmox_restore_daemon/api.rs
+++ b/src/bin/proxmox_restore_daemon/api.rs
@@ -277,7 +277,7 @@ fn extract(
 
         let (mut writer, reader) = tokio::io::duplex(1024 * 64);
 
-        if pxar {
+        let handle = if pxar {
             tokio::spawn(async move {
                 let result = async move {
                     // pxar always expects a directory as it's root, so to accommodate files as
@@ -333,7 +333,7 @@ fn extract(
                 if let Err(err) = result {
                     error!("pxar streaming task failed - {}", err);
                 }
-            });
+            })
         } else {
             tokio::spawn(async move {
                 let result = async move {
@@ -355,11 +355,22 @@ fn extract(
                 if let Err(err) = result {
                     error!("file or dir streaming task failed - {}", err);
                 }
-            });
-        }
+            })
+        };
 
         let stream = tokio_util::io::ReaderStream::new(reader);
 
+        tokio::spawn(async move {
+            let mut handle = handle.fuse();
+            let interval = std::time::Duration::new(60, 0);
+            loop {
+                futures::select! {
+                    _ = handle => break,
+                    _ = tokio::time::sleep(interval).fuse() => watchdog_ping(),
+                }
+            }
+        });
+
         let body = Body::wrap_stream(stream);
         Ok(Response::builder()
             .status(StatusCode::OK)
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup] proxmox-restore-daemon: keep daemon active during extract api call

[Thomas Lamprecht]

On 14.05.21 10:42, Dominik Csapak wrote:
> we only called watchdog_ping on the start of an api call, which meant
> that if a single zip/pxar download took longer than 10 minutes
> (which can happen on larger vms/slower connections), we shut-down
> the vm and cancelled the download.
> 
> instead, spawn a second future, that polls both a sleep and the extract
> handle, and ping the watchdog every 60 seconds
> 

This is known and Stefan send a patch for this last week[0], which was OK but
delayed due to the tokio DuplexStream bug he also fixed where we did not
decided then if we still use his workaround after upstream already merged
the fix.

IIRC your approach has some drawbacks, especially without the workaround fixed,
but just some fain memory - in any way I'll go for Stefan's series.

[0]: https://lists.proxmox.com/pipermail/pbs-devel/2021-May/003106.html

Re: [pbs-devel] [PATCH proxmox-backup] proxmox-restore-daemon: keep daemon active during extract api call

[Dominik Csapak]

On 5/14/21 10:54, Thomas Lamprecht wrote:
> On 14.05.21 10:42, Dominik Csapak wrote:
>> we only called watchdog_ping on the start of an api call, which meant
>> that if a single zip/pxar download took longer than 10 minutes
>> (which can happen on larger vms/slower connections), we shut-down
>> the vm and cancelled the download.
>>
>> instead, spawn a second future, that polls both a sleep and the extract
>> handle, and ping the watchdog every 60 seconds
>>
> 
> This is known and Stefan send a patch for this last week[0], which was OK but
> delayed due to the tokio DuplexStream bug he also fixed where we did not
> decided then if we still use his workaround after upstream already merged
> the fix.
> 
> IIRC your approach has some drawbacks, especially without the workaround fixed,
> but just some fain memory - in any way I'll go for Stefan's series.
> 
> [0]: https://lists.proxmox.com/pipermail/pbs-devel/2021-May/003106.html
> 

ah yes i see it now, thanks :)